Details

    • Similar Issues:

      Description

      Currently workflow-multibranch uses WorkflowJobProperty.decorateACL to customize ACL behavior but this is a hack. Discussed with Stephen Connolly:

      Suggest a new core interface

      package jenkins.security;
      /** Permits {@link ACL} decisions to be overridden by extensions outside the control of either an {@link AccessControlled} or an {@link AuthorizationStrategy}. */
      public abstract class ACLDecorator implements ExtensionPoint {
          /**
           * May be used to override {@link ACL#hasPermission(Authentication, Permission)} decisions.
           * @param object the object of a permission check
           * @param a some authentication (not {@link ACL#SYSTEM}, which is always granted everything)
           * @param permission some permission
           * @return true to grant the permission, false to deny it, or null to allow another extension and finally the base logic to apply
           */
          public abstract Boolean hasPermission(@Nonnull AccessControlled object, @Nonnull Authentication a, @Nonnull Permission permission);
          public abstract Boolean hasCreatePermission(@Nonnull AccessControlled object, @Nonnull Authentication a, @Nonnull ItemGroup c, @Nonnull TopLevelItemDescriptor d);
      }
      

      to be called from new methods in ACL, e.g.:

      public static boolean hasPermission(@Nonnull AccessControlled object, @Nonnull Authentication a, @Nonnull Permission p) {
          if (a == SYSTEM) {
              return true;
          }
          for (ACLDecorator d : ExtensionList.lookup(ACLDecorator.class)) {
              Boolean b = d.hasPermission(object, a, p);
              if (b != null) {
                  return b;
              }
          }
          return object.getACL().hasPermission(a, p);
      }
      /** @deprecated use {@link #hasPermission(AccessControlled, Authentication, Permission)} instead */
      public final boolean hasPermission(@Nonnull Permission p) {
          // no backlink to AccessControlled, cannot retrofit
          return hasPermission(Jenkins.getAuthentication(), p);
      }
      // similarly for checkPermission, hasCreatePermission, checkCreatePermission
      

      in turn called from implementations of AccessControlled.hasPermission and checkPermission such as in AbstractItem:

      @Override
      public boolean hasPermission(Permission p) {
          return ACL.hasPermission(this, Jenkins.getAuthentication(), p);
      }
      

      This would allow WorkflowJobProperty to be deprecated in favor of:

      @Extension
      public static MakeBranchProjectsReadOnly extends ACLDecorator {
          @Override
          public Boolean hasPermission(@Nonnull AccessControlled object, @Nonnull Authentication a, @Nonnull Permission permission) {
              if (object instanceof WorkflowJob &&
                      (permission == Item.CONFIGURE || permission == Item.DELETE) &&
                      ((WorkflowJob) object).getProperty(BranchJobProperty.class) != null) {
                  return false;
              }
              return null;
          }
      }
      

      or more generally, to also cover organization scanning:

      @Extension
      public static MakeComputedFolderChildrenReadOnly extends ACLDecorator {
          @Override
          public Boolean hasPermission(@Nonnull AccessControlled object, @Nonnull Authentication a, @Nonnull Permission permission) {
              if (object instanceof TopLevelItem &&
                      (permission == Item.CONFIGURE || permission == Item.DELETE) &&
                      ((Item) object).getParent() instanceof ComputedFolder) {
                  return false;
              }
              return null;
          }
      }
      

      Such an API would also allow a solution to the longstanding problem of making parts of AuthorizationStrategy (but not SecurityRealm) pluggable. That would be useful for creating delegating principals for CLI/REST authentication, authorize-project stored authentications, etc.

      It could also potentially allow GithubAuthorizationStrategy (with its hacky list of adminUserNames) to be replaced by a decorator. Thus a generic AuthorizationStrategy could grant administrative permission to some users, and deal with miscellaneous projects, whereas this decorator would grant job permissions to GithubAuthenticationToken (again punting on the question of delegating SecurityRealm!).

      The main questions are

      • Timing: this would need to go into some LTS and not be available for a while. Does it matter much?
      • Performance: can the overhead be kept minimal, assuming implementations are carefully written to quickly return null in most cases?
      • Expressiveness: does this simple API suffice for future use cases such as delegating principals?

      Another question is whether a user seeing the multibranch project should be able to see every branch. In general, they should not: some branch sources might be from a secure private fork, for example. More importantly, a user should not necessarily see every repository in an organization. How about a UserProperty that is a GitHub identity?

      Do we need a decorator that runs "around" the AuthorizationStrategy, rather than only before? Not really needed merely to remove permissions, but...if we need it to be additive, then need direct support in the strategy.

        Attachments

          Issue Links

            Activity

            Hide
            stephenconnolly Stephen Connolly added a comment -

            I don't remember discussing this with you, but the design sounds like something I would discuss...

            Show
            stephenconnolly Stephen Connolly added a comment - I don't remember discussing this with you, but the design sounds like something I would discuss...
            Hide
            mjdetullio Matthew DeTullio added a comment -

            I like this

            Show
            mjdetullio Matthew DeTullio added a comment - I like this

              People

              • Assignee:
                Unassigned
                Reporter:
                jglick Jesse Glick
              • Votes:
                1 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated: