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

"Project-based Matrix Authorization Strategy" - weird behavior

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

      Global authorization (set up in in "Configure System") is ignored when a project
      has defined its own local authorization matrix. That seems to be wrong behavior,
      shouldn't the local authorization extend the global instead of overriding it?

          [JENKINS-2186] "Project-based Matrix Authorization Strategy" - weird behavior

          adphillips added a comment -

          I have been working on adding READ permission to jobs (see issue #2324). As I
          was working on this, I did encounter this "wierdness". I'm proceeding with the
          fix mentioned in this forum thread:
          http://www.nabble.com/Read-permission-on-Jobs-td20650539.html. The basic idea
          is permissions are additive, so if you have permission to operate on a job
          defined either globally or at the job level, you will be granted this
          permission. The cost you pay for this approach is there will be no way to
          mask-out a permission at the job level if it is granted globally. If anyone has
          objections, please raise them.

          adphillips added a comment - I have been working on adding READ permission to jobs (see issue #2324). As I was working on this, I did encounter this "wierdness". I'm proceeding with the fix mentioned in this forum thread: http://www.nabble.com/Read-permission-on-Jobs-td20650539.html . The basic idea is permissions are additive, so if you have permission to operate on a job defined either globally or at the job level, you will be granted this permission. The cost you pay for this approach is there will be no way to mask-out a permission at the job level if it is granted globally. If anyone has objections, please raise them.

          adphillips added a comment -

          Reassigning to myself

          adphillips added a comment - Reassigning to myself

          adphillips added a comment -

          fix complete will submit patch soon

          adphillips added a comment - fix complete will submit patch soon

          adphillips added a comment -

          Patch is as follows:

          Index: src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java
          ===================================================================
          — src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java
          (revision 13369)
          +++ src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java
          (working copy)
          @@ -4,10 +4,11 @@
          import hudson.model.Descriptor;

          import hudson.model.Jobs;

          import hudson.util.RobustReflectionConverter;

          +import org.acegisecurity.Authentication;

          +import com.thoughtworks.xstream.converters.UnmarshallingContext;

          +import com.thoughtworks.xstream.core.JVM;

          import com.thoughtworks.xstream.io.HierarchicalStreamReader;

          -import com.thoughtworks.xstream.converters.UnmarshallingContext;

          import com.thoughtworks.xstream.mapper.Mapper;

          -import com.thoughtworks.xstream.core.JVM;

          /**

          • {@link GlobalMatrixAuthorizationStrategy}

            plus per-project ACL.

          @@ -22,7 +23,16 @@
          public ACL getACL(AbstractProject<?,?> project) {

          AuthorizationMatrixProperty amp =
          project.getProperty(AuthorizationMatrixProperty.class);

          if (amp != null && amp.isUseProjectSecurity()) {

          • return amp.getACL();

          + final ACL projectAcl = amp.getACL();

          +

          + //The effective ACL is the union of the project and root ACLs.
          Optimistic permissions apply

          + //meaning if either allows access, then grant it.

          + return new ACL() {

          + public boolean hasPermission(Authentication a, Permission
          permission)

          { + return projectAcl.hasPermission(a, permission) || getRootACL().hasPermission(a, permission); + }

          + };

          +

          } else

          { return getRootACL(); }

          adphillips added a comment - Patch is as follows: Index: src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java =================================================================== — src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java (revision 13369) +++ src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java (working copy) @@ -4,10 +4,11 @@ import hudson.model.Descriptor; import hudson.model.Jobs; import hudson.util.RobustReflectionConverter; +import org.acegisecurity.Authentication; +import com.thoughtworks.xstream.converters.UnmarshallingContext; +import com.thoughtworks.xstream.core.JVM; import com.thoughtworks.xstream.io.HierarchicalStreamReader; -import com.thoughtworks.xstream.converters.UnmarshallingContext; import com.thoughtworks.xstream.mapper.Mapper; -import com.thoughtworks.xstream.core.JVM; /** {@link GlobalMatrixAuthorizationStrategy} plus per-project ACL. @@ -22,7 +23,16 @@ public ACL getACL(AbstractProject<?,?> project) { AuthorizationMatrixProperty amp = project.getProperty(AuthorizationMatrixProperty.class); if (amp != null && amp.isUseProjectSecurity()) { return amp.getACL(); + final ACL projectAcl = amp.getACL(); + + //The effective ACL is the union of the project and root ACLs. Optimistic permissions apply + //meaning if either allows access, then grant it. + return new ACL() { + public boolean hasPermission(Authentication a, Permission permission) { + return projectAcl.hasPermission(a, permission) || getRootACL().hasPermission(a, permission); + } + }; + } else { return getRootACL(); }

          adphillips added a comment -

          Created an attachment (id=468)
          patch that implements addiditve permissions

          adphillips added a comment - Created an attachment (id=468) patch that implements addiditve permissions

          Alan Harder added a comment -

          Marking as PATCH.

          Alan Harder added a comment - Marking as PATCH.

          Code changed in hudson
          User: : kohsuke
          Path:
          trunk/hudson/main/core/src/main/java/hudson/security/AuthorizationMatrixProperty.java
          trunk/hudson/main/core/src/main/java/hudson/security/GlobalMatrixAuthorizationStrategy.java
          trunk/hudson/main/core/src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java
          trunk/hudson/main/core/src/main/java/hudson/security/SidACL.java
          trunk/www/changelog.html
          http://fisheye4.cenqua.com/changelog/hudson/?cs=13654
          Log:
          [FIXED JENKINS-2186] In project-based matrix security, global setting should be inherited to per-job setting.
          IN 1.265.

          SCM/JIRA link daemon added a comment - Code changed in hudson User: : kohsuke Path: trunk/hudson/main/core/src/main/java/hudson/security/AuthorizationMatrixProperty.java trunk/hudson/main/core/src/main/java/hudson/security/GlobalMatrixAuthorizationStrategy.java trunk/hudson/main/core/src/main/java/hudson/security/ProjectMatrixAuthorizationStrategy.java trunk/hudson/main/core/src/main/java/hudson/security/SidACL.java trunk/www/changelog.html http://fisheye4.cenqua.com/changelog/hudson/?cs=13654 Log: [FIXED JENKINS-2186] In project-based matrix security, global setting should be inherited to per-job setting. IN 1.265.

          Alan Harder added a comment -

          Comment/question on the implementation in r13654:

          The inner class in SidACL.newInheritingACL calls child and
          parent.hasPermission(Sid,Permission) directly, so it bypasses
          _hasPermission(Authentication,Permission) in those SidImpl classes.

          Will it miss checking ANONYMOUS?

          Alan Harder added a comment - Comment/question on the implementation in r13654: The inner class in SidACL.newInheritingACL calls child and parent.hasPermission(Sid,Permission) directly, so it bypasses _hasPermission(Authentication,Permission) in those SidImpl classes. Will it miss checking ANONYMOUS?

          Alan Harder added a comment -

          Here are the steps to see the ANONYMOUS problem:

          1. Global perms:
          Anonymous does not have Workspace permission
          UserX is either not listed, or does not have Workspace permission
          2. ProjectX perms:
          Anonymous does have Workspace permission
          UserX is listed and does not have Workspace permission

          When UserX logs in and visits some project that does not have any
          project-specific permission, he can see the workspace (it will use only root
          ACL, so anonymous is checked). But when UserX visits ProjectX it does not show
          the Workspace. He can logout and see the workspace as anonymous, however (since
          anonymous is the actual user, that row IS checked).

          Is this a bug? Seems a bit inconsistent to skip the extra ANONYMOUS check in
          the projects with project-specific permissions. Then again, if you grant
          something to Anonymous in a project, you can just check that same box in every
          other row..

          Alan Harder added a comment - Here are the steps to see the ANONYMOUS problem: 1. Global perms: Anonymous does not have Workspace permission UserX is either not listed, or does not have Workspace permission 2. ProjectX perms: Anonymous does have Workspace permission UserX is listed and does not have Workspace permission When UserX logs in and visits some project that does not have any project-specific permission, he can see the workspace (it will use only root ACL, so anonymous is checked). But when UserX visits ProjectX it does not show the Workspace. He can logout and see the workspace as anonymous, however (since anonymous is the actual user, that row IS checked). Is this a bug? Seems a bit inconsistent to skip the extra ANONYMOUS check in the projects with project-specific permissions. Then again, if you grant something to Anonymous in a project, you can just check that same box in every other row..

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

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

            adphillips adphillips
            mast76 mast76
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated:
              Resolved: