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 created issue -

          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.
          Kohsuke Kawaguchi made changes -
          Status Original: Open [ 1 ] New: In Progress [ 3 ]

          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 made changes -
          Status Original: In Progress [ 3 ] New: Open [ 1 ]

          Alan Harder added a comment -

          Working on this.

          Alan Harder added a comment - Working on this.
          Alan Harder made changes -
          Status Original: Open [ 1 ] New: In Progress [ 3 ]

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

              Created:
              Updated:
              Resolved: