Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-2715

Field validators in configure screen require admin permission

    • Icon: Patch Patch
    • Resolution: Fixed
    • Icon: Critical Critical
    • _unsorted
    • None
    • Platform: All, OS: All

      I found 8 cases when a field validator in the project configure screen required
      admin permission:
      1-2) Project to build before/after this one
      3-4) Schedule for build times or poll SCM times
      5) Subversion remote url check
      6-7) FishEye cvs/svn
      8) Sventon

      My log filled with many long exceptions as soon as I visited a configure page
      with a user having all permissions except admin.
      I resolved this issue in 3 ways:
      a) In items 1-4 the checkURL now includes project=${it.name}. If a valid name
      is given for this parameter, the check requires CONFIGURE permission on that
      project; otherwise it checks for admin permission.
      b) Items 5-7 start with the same check as (a) for some basic checks of the field
      syntax.. however, these actually connect to URLs and check the content. This
      portion is done only if the user has admin permission, otherwise that part is
      just skipped.
      c) The Sventon validator had only a retrieve-URL check (no simple validation),
      so for that one I simply omitted the checkURL attribute in the jelly file for
      non admins.

      Patch attached.

          [JENKINS-2715] Field validators in configure screen require admin permission

          Alan Harder added a comment -

          Created an attachment (id=465)
          Permission checks for field validators on job configure screen

          Alan Harder added a comment - Created an attachment (id=465) Permission checks for field validators on job configure screen

          Alan Harder added a comment -

          Attaching updated patch with these changes:

          • Added "jsUrlEscape" in Functions and Util classes, call this from all the
            applicable config.jelly files. I found two characters allowed in job names that
            cause problems in these checkURL attributes: single quote (') causes javascript
            error if not escape with backslash, and plus sign gets converted into a
            space when received on the server unless urlencoded (%2B).
          • Renamed "project" request param to "job" and use $it.fullName as value instead
            of $it.name, following the pattern used in a few other validators.
          • Fixed my change in Sventon config.jelly
          • New fix in Hudson.java for permission check on validator in "newJob" page

          Note that an alternative to the whole jsUrlEscape thing is to add ' and +
          characters as "unsafe" and rejected by checkGoodName.

          Additional notes:

          • I have now tested all these validators as: (a) admin, (b) user with all
            permissions except admin, and (c) user with only global view permission, but
            granted configure permission on a particular project.
          • I noticed a comment in Hudson.doCheckLocalFSRoot ("so needs to be
            protected").. just wanted to point out that a 3rd param of "true" to the
            FormFieldValidator constructor would add an admin permission check, but 3rd
            param to FormFieldValidator.WorkspaceDirectory does NOT add a permission check..
            so whoever wrote that comment wasn't getting what they expected with that "true"
            param.
          • I noticed that a project using permissions just for that project really
            override all global permissions.. i.e., a user with global configure permission
            (even an admin user with every permission) can no longer configure the job
            unless granted permission directly on it. Just curious if that is intended..
            seems odd that admins can't configure every job.

          Alan Harder added a comment - Attaching updated patch with these changes: Added "jsUrlEscape" in Functions and Util classes, call this from all the applicable config.jelly files. I found two characters allowed in job names that cause problems in these checkURL attributes: single quote (') causes javascript error if not escape with backslash, and plus sign gets converted into a space when received on the server unless urlencoded (%2B). Renamed "project" request param to "job" and use $it.fullName as value instead of $it.name, following the pattern used in a few other validators. Fixed my change in Sventon config.jelly New fix in Hudson.java for permission check on validator in "newJob" page Note that an alternative to the whole jsUrlEscape thing is to add ' and + characters as "unsafe" and rejected by checkGoodName. Additional notes: I have now tested all these validators as: (a) admin, (b) user with all permissions except admin, and (c) user with only global view permission, but granted configure permission on a particular project. I noticed a comment in Hudson.doCheckLocalFSRoot ("so needs to be protected").. just wanted to point out that a 3rd param of "true" to the FormFieldValidator constructor would add an admin permission check, but 3rd param to FormFieldValidator.WorkspaceDirectory does NOT add a permission check.. so whoever wrote that comment wasn't getting what they expected with that "true" param. I noticed that a project using permissions just for that project really override all global permissions.. i.e., a user with global configure permission (even an admin user with every permission) can no longer configure the job unless granted permission directly on it. Just curious if that is intended.. seems odd that admins can't configure every job.

          Alan Harder added a comment -

          Created an attachment (id=466)
          Updated patch for validator permission checks

          Alan Harder added a comment - Created an attachment (id=466) Updated patch for validator permission checks

          Alan Harder added a comment -

          The 3rd item under "Additional notes" above is issue #2186.

          Alan Harder added a comment - The 3rd item under "Additional notes" above is issue #2186.

          Thank you for the patch.

          I've been thinking about this change, but it seems cleaner to do this by
          exposing descriptors under the URL space of the item (like
          /job/foo/descriptor/foo.bar.Zot/checkXYZ), so that the check method can
          internally obtain the current job context and use that to guide the operation.

          This also enables better context sensitive check, and it keeps the config.jelly
          files of the publishers smaller and less error prone.

          Kohsuke Kawaguchi added a comment - Thank you for the patch. I've been thinking about this change, but it seems cleaner to do this by exposing descriptors under the URL space of the item (like /job/foo/descriptor/foo.bar.Zot/checkXYZ), so that the check method can internally obtain the current job context and use that to guide the operation. This also enables better context sensitive check, and it keeps the config.jelly files of the publishers smaller and less error prone.

          Alan Harder added a comment -

          I will work on a new patch that puts the job name into the URL path instead of
          as a job=... query parameter. However, the whole checkURL is still a javascript
          string so an unescaped ' character still causes an error. Should I keep a
          ${h.jsEscape(it.fullName)} type of call for each of these checks, or do you have
          another idea?

          Alan Harder added a comment - I will work on a new patch that puts the job name into the URL path instead of as a job=... query parameter. However, the whole checkURL is still a javascript string so an unescaped ' character still causes an error. Should I keep a ${h.jsEscape(it.fullName)} type of call for each of these checks, or do you have another idea?

          Alan Harder added a comment -

          Working on this.

          Alan Harder added a comment - Working on this.

          Alan Harder added a comment -

          Created an attachment (id=485)
          Patch for validator permission checks

          Alan Harder added a comment - Created an attachment (id=485) Patch for validator permission checks

          Alan Harder added a comment -

          Latest patch has these changes:

          • FormFieldValidator.getProjectFromURL, new protected method.. now locates
            project using request.getRestOfPath() instead of "job" query param. Use this
            method from my new permission-checking constructor, and also in a couple of the
            inner classes that were already loading a project.
          • Add permission check in the Workspace* inner classes.. was missing here (I
            filed separate issue #2755 for this, but fixed here).
          • Util.jsEscape now just escapes ' and \ with a \. Don't need urlencode of +
            now that job name is in the URL path (I guess stapler doesn't urldecode the
            path, so + stays as +).
          • Relaxed permissions on validators that just check syntax (cron syntax, url
            syntax).. no permission needed for these.
          • Still requiring admin permission for connecting to URLs, and project-configure
            permission to check filesystem paths or valid job names.
          • Added a comment at the start of each validator about what permission it requires

          Alan Harder added a comment - Latest patch has these changes: FormFieldValidator.getProjectFromURL, new protected method.. now locates project using request.getRestOfPath() instead of "job" query param. Use this method from my new permission-checking constructor, and also in a couple of the inner classes that were already loading a project. Add permission check in the Workspace* inner classes.. was missing here (I filed separate issue #2755 for this, but fixed here). Util.jsEscape now just escapes ' and \ with a \. Don't need urlencode of + now that job name is in the URL path (I guess stapler doesn't urldecode the path, so + stays as +). Relaxed permissions on validators that just check syntax (cron syntax, url syntax).. no permission needed for these. Still requiring admin permission for connecting to URLs, and project-configure permission to check filesystem paths or valid job names. Added a comment at the start of each validator about what permission it requires

          Code changed in hudson
          User: : kohsuke
          Path:
          trunk/hudson/main/core/pom.xml
          trunk/hudson/main/core/src/main/java/hudson/Util.java
          trunk/hudson/main/core/src/main/java/hudson/model/AbstractItem.java
          trunk/hudson/main/core/src/main/java/hudson/model/Hudson.java
          trunk/hudson/main/core/src/main/java/hudson/scm/SubversionSCM.java
          trunk/hudson/main/core/src/main/java/hudson/scm/browsers/FishEyeCVS.java
          trunk/hudson/main/core/src/main/java/hudson/scm/browsers/FishEyeSVN.java
          trunk/hudson/main/core/src/main/java/hudson/scm/browsers/Sventon.java
          trunk/hudson/main/core/src/main/java/hudson/security/AccessControlled.java
          trunk/hudson/main/core/src/main/java/hudson/tasks/BuildTrigger.java
          trunk/hudson/main/core/src/main/java/hudson/tasks/JavadocArchiver.java
          trunk/hudson/main/core/src/main/java/hudson/tasks/Shell.java
          trunk/hudson/main/core/src/main/java/hudson/tasks/test/AggregatedTestResultPublisher.java
          trunk/hudson/main/core/src/main/java/hudson/triggers/TimerTrigger.java
          trunk/hudson/main/core/src/main/java/hudson/util/FormFieldValidator.java
          trunk/hudson/main/core/src/main/resources/hudson/scm/browsers/Sventon/config.jelly
          trunk/hudson/main/core/src/main/resources/hudson/tasks/ArtifactArchiver/config.jelly
          trunk/hudson/main/core/src/main/resources/hudson/tasks/BuildTrigger/config.jelly
          trunk/hudson/main/core/src/main/resources/hudson/tasks/Fingerprinter/config.jelly
          trunk/hudson/main/core/src/main/resources/hudson/tasks/JavadocArchiver/config.jelly
          trunk/hudson/main/core/src/main/resources/hudson/tasks/junit/JUnitResultArchiver/config.jelly
          trunk/hudson/main/core/src/main/resources/hudson/tasks/test/AggregatedTestResultPublisher/config.jelly
          trunk/hudson/main/core/src/main/resources/lib/hudson/project/config-upstream-pseudo-trigger.jelly
          trunk/www/changelog.html
          http://fisheye4.cenqua.com/changelog/hudson/?cs=13838
          Log:
          [FIXED JENKINS-2715] Field validators in configure screen shouldn't require the admin permission where possible.

          SCM/JIRA link daemon added a comment - Code changed in hudson User: : kohsuke Path: trunk/hudson/main/core/pom.xml trunk/hudson/main/core/src/main/java/hudson/Util.java trunk/hudson/main/core/src/main/java/hudson/model/AbstractItem.java trunk/hudson/main/core/src/main/java/hudson/model/Hudson.java trunk/hudson/main/core/src/main/java/hudson/scm/SubversionSCM.java trunk/hudson/main/core/src/main/java/hudson/scm/browsers/FishEyeCVS.java trunk/hudson/main/core/src/main/java/hudson/scm/browsers/FishEyeSVN.java trunk/hudson/main/core/src/main/java/hudson/scm/browsers/Sventon.java trunk/hudson/main/core/src/main/java/hudson/security/AccessControlled.java trunk/hudson/main/core/src/main/java/hudson/tasks/BuildTrigger.java trunk/hudson/main/core/src/main/java/hudson/tasks/JavadocArchiver.java trunk/hudson/main/core/src/main/java/hudson/tasks/Shell.java trunk/hudson/main/core/src/main/java/hudson/tasks/test/AggregatedTestResultPublisher.java trunk/hudson/main/core/src/main/java/hudson/triggers/TimerTrigger.java trunk/hudson/main/core/src/main/java/hudson/util/FormFieldValidator.java trunk/hudson/main/core/src/main/resources/hudson/scm/browsers/Sventon/config.jelly trunk/hudson/main/core/src/main/resources/hudson/tasks/ArtifactArchiver/config.jelly trunk/hudson/main/core/src/main/resources/hudson/tasks/BuildTrigger/config.jelly trunk/hudson/main/core/src/main/resources/hudson/tasks/Fingerprinter/config.jelly trunk/hudson/main/core/src/main/resources/hudson/tasks/JavadocArchiver/config.jelly trunk/hudson/main/core/src/main/resources/hudson/tasks/junit/JUnitResultArchiver/config.jelly trunk/hudson/main/core/src/main/resources/hudson/tasks/test/AggregatedTestResultPublisher/config.jelly trunk/hudson/main/core/src/main/resources/lib/hudson/project/config-upstream-pseudo-trigger.jelly trunk/www/changelog.html http://fisheye4.cenqua.com/changelog/hudson/?cs=13838 Log: [FIXED JENKINS-2715] Field validators in configure screen shouldn't require the admin permission where possible.

          Alan Harder added a comment -
              • Issue 2703 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 2703 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 2629 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 2629 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 1855 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 1855 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 1718 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 1718 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 2030 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 2030 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 1684 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 1684 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 2168 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 2168 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment -
              • Issue 2434 has been marked as a duplicate of this issue. ***

          Alan Harder added a comment - Issue 2434 has been marked as a duplicate of this issue. ***

            mindless Alan Harder
            mindless Alan Harder
            Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

              Created:
              Updated:
              Resolved: