Implement nicer property DSL for Job DSL and Pipeline (#144)

* Implement nicer property DSL for Job DSL and Pipeline * Nicer output * Test fixes * Test fix * Simplify * Thanks Spotless * Add compatibility warning for DSL change * Restrictions, Javadoc, add support for 'userOrGroup' * Resolve TODOs, use SCM internal ID to allow loading * Fix typo * Adapt test --------- Co-authored-by: Daniel Beck <[email protected]>
jenkinsci · Aug 22, 2023 · f9e6080 · f9e6080
1 parent 47a3280
commit f9e6080
Show file tree

Hide file tree

Showing 8 changed files with 248 additions and 42 deletions.
diff --git a/pom.xml b/pom.xml
@@ -27,7 +27,7 @@
     <revision>3.1.11</revision>
     <changelist>-SNAPSHOT</changelist>
     <gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
-    <hpi.compatibleSinceVersion>3.0</hpi.compatibleSinceVersion>
+    <hpi.compatibleSinceVersion>3.2</hpi.compatibleSinceVersion>
     <jenkins.version>2.387.3</jenkins.version>
     <spotless.check.skip>false</spotless.check.skip>
   </properties>

diff --git a/...main/java/com/cloudbees/hudson/plugins/folder/properties/AuthorizationMatrixProperty.java b/...main/java/com/cloudbees/hudson/plugins/folder/properties/AuthorizationMatrixProperty.java
@@ -109,12 +109,13 @@ public AuthorizationMatrixProperty(Map<Permission, ? extends Set<String>> grante
         }
     }
 
-    @DataBoundConstructor // JENKINS-49199: Used for job-dsl
-    @Restricted(NoExternalUse.class)
-    public AuthorizationMatrixProperty(List<String> permissions) {
-        for (String permission : permissions) {
-            add(permission);
-        }
+    /**
+     * Exists for reflective Job DSL / Pipeline use only.
+     */
+    @DataBoundConstructor
+    @Restricted(DoNotUse.class)
+    public AuthorizationMatrixProperty(List<DslEntry> entries) {
+        setEntries(entries);
     }
 
     @Override
@@ -143,6 +144,13 @@ protected void setOwner(@NonNull AbstractFolder<?> owner) {
         FolderContributor.record(owner);
     }
 
+    @Override
+    @Restricted(DoNotUse.class)
+    public List<DslEntry> getEntries() {
+        // ReflectionUtils#getPublicProperty / PropertyDescriptor#getPropertyDescriptor can't find default methods
+        return AuthorizationProperty.super.getEntries();
+    }
+
     @Extension(optional = true)
     @Symbol("authorizationMatrix")
     public static class DescriptorImpl extends AbstractFolderPropertyDescriptor

diff --git a/src/main/java/hudson/security/AuthorizationMatrixProperty.java b/src/main/java/hudson/security/AuthorizationMatrixProperty.java
@@ -134,13 +134,13 @@ public AuthorizationMatrixProperty(Map<Permission, Set<String>> grantedPermissio
         }
     }
 
+    /**
+     * Exists for reflective Job DSL / Pipeline use only.
+     */
+    @Restricted(DoNotUse.class)
     @DataBoundConstructor
-    public AuthorizationMatrixProperty(List<String> permissions) {
-        for (String str : permissions) {
-            if (str != null) {
-                this.add(str);
-            }
-        }
+    public AuthorizationMatrixProperty(List<DslEntry> entries) {
+        setEntries(entries);
     }
 
     /**
@@ -182,6 +182,13 @@ public Permission getEditingPermission() {
         return Item.CONFIGURE;
     }
 
+    @Override
+    @Restricted(DoNotUse.class)
+    public List<DslEntry> getEntries() {
+        // ReflectionUtils#getPublicProperty / PropertyDescriptor#getPropertyDescriptor can't find default methods
+        return AuthorizationProperty.super.getEntries();
+    }
+
     @Extension
     @Symbol("authorizationMatrix")
     public static class DescriptorImpl extends JobPropertyDescriptor

diff --git a/src/main/java/org/jenkinsci/plugins/matrixauth/AuthorizationProperty.java b/src/main/java/org/jenkinsci/plugins/matrixauth/AuthorizationProperty.java
@@ -23,11 +23,29 @@
  */
 package org.jenkinsci.plugins.matrixauth;
 
+import edu.umd.cs.findbugs.annotations.NonNull;
+import hudson.Extension;
+import hudson.model.Describable;
+import hudson.model.Descriptor;
+import hudson.security.Permission;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import jenkins.model.Jenkins;
+import org.jenkinsci.Symbol;
 import org.jenkinsci.plugins.matrixauth.inheritance.InheritGlobalStrategy;
 import org.jenkinsci.plugins.matrixauth.inheritance.InheritanceStrategy;
 import org.jenkinsci.plugins.matrixauth.inheritance.NonInheritingStrategy;
+import org.jenkinsci.plugins.matrixauth.integrations.PermissionFinder;
 import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.DoNotUse;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
+import org.kohsuke.stapler.DataBoundConstructor;
 
 @Restricted(NoExternalUse.class)
 public interface AuthorizationProperty extends AuthorizationContainer {
@@ -72,4 +90,181 @@ default void setBlocksInheritance(boolean blocksInheritance) {
     default boolean isBlocksInheritance() {
         return getInheritanceStrategy() instanceof NonInheritingStrategy;
     }
+
+    /**
+     * Set entries from DSL in Job DSL or Pipeline plugins.
+     *
+     * @param entries list of entries to use for permission assignment
+     */
+    @Restricted(DoNotUse.class)
+    default void setEntries(List<DslEntry> entries) {
+        for (DslEntry entry : entries) {
+            entry.addPermission(this);
+        }
+    }
+
+    /**
+     * Getter supporting nicer DSL syntax for Job DSL and Pipeline job property definitions.
+     * @return a list of {@link DslEntry}
+     */
+    default List<DslEntry> getEntries() {
+        final Map<PermissionEntry, SortedSet<String>> mapping = new HashMap<>();
+        getGrantedPermissionEntries()
+                .forEach((permission, value) ->
+                        value.forEach(sid -> mapping.computeIfAbsent(sid, unused -> new TreeSet<>())
+                                .add(permission.group.getId() + "/" + permission.name)));
+        return mapping.entrySet().stream()
+                .map(e -> {
+                    final PermissionEntry key = e.getKey();
+                    if (key.getType() == AuthorizationType.USER) {
+                        return new DslUser(key.getSid(), new ArrayList<>(e.getValue()));
+                    }
+                    if (key.getType() == AuthorizationType.GROUP) {
+                        return new DslGroup(key.getSid(), new ArrayList<>(e.getValue()));
+                    }
+                    if (key.getType() == AuthorizationType.EITHER) {
+                        return new DslUserOrGroup(key.getSid(), new ArrayList<>(e.getValue()));
+                    }
+                    throw new IllegalStateException("Got unexpected key type in: " + key);
+                })
+                .distinct()
+                .sorted()
+                .collect(Collectors.toList());
+    }
+
+    /**
+     * Common superclass for {@link DslUser}, {@link DslGroup}, and {@link DslUserOrGroup}, supporting nicer DSLs
+     * for Job DSL and Pipeline Job definitions/reconfigurations.
+     * Job DSL and Pipeline use this for {@link hudson.security.AuthorizationMatrixProperty}.
+     * Job DSL additionally uses this for {@link com.cloudbees.hudson.plugins.folder.properties.AuthorizationMatrixProperty}.
+     */
+    @Restricted(NoExternalUse.class)
+    abstract class DslEntry implements Describable<DslEntry>, Comparable<DslEntry> {
+        private final String name;
+        private final List<String> permissions;
+
+        /**
+         *
+         * @param name the sid of the DSL entity
+         * @param permissions the list of string-typed permissions
+         */
+        public DslEntry(String name, List<String> permissions) {
+            this.name = name;
+            this.permissions = permissions;
+        }
+
+        public String getName() {
+            return name;
+        }
+
+        public List<String> getPermissions() {
+            return permissions;
+        }
+
+        @Override
+        public Descriptor<DslEntry> getDescriptor() {
+            return Jenkins.get().getDescriptorOrDie(getClass());
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) return true;
+            if (o == null || getClass() != o.getClass()) return false;
+            DslEntry that = (DslEntry) o;
+            return Objects.equals(name, that.name) && Objects.equals(permissions, that.permissions);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(name, permissions, getClass());
+        }
+
+        @Override
+        public int compareTo(@NonNull DslEntry that) {
+            if (this.getClass() != that.getClass()) {
+                return this.getClass().getName().compareTo(that.getClass().getName());
+            }
+            if (!this.name.equals(that.name)) {
+                return this.name.compareTo(that.name);
+            }
+            return this.permissions.size() - that.permissions.size();
+        }
+
+        public abstract void addPermission(AuthorizationProperty authorizationProperty);
+
+        protected static Permission findPermission(String value) {
+            final Permission permission = Permission.fromId(value);
+            if (permission != null) {
+                return permission;
+            }
+            return PermissionFinder.findPermission(value);
+        }
+    }
+
+    /**
+     * Represents a user being assigned permissions.
+     */
+    @Restricted(NoExternalUse.class)
+    class DslUser extends DslEntry {
+        @DataBoundConstructor
+        public DslUser(String name, List<String> permissions) {
+            super(name, permissions);
+        }
+
+        @Override
+        public void addPermission(AuthorizationProperty authorizationProperty) {
+            getPermissions()
+                    .forEach(permission ->
+                            authorizationProperty.add(findPermission(permission), PermissionEntry.user(getName())));
+        }
+
+        @Extension
+        @Symbol("user")
+        public static class DescriptorImpl extends Descriptor<DslEntry> {}
+    }
+
+    /**
+     * Represents a group being assigned permissions.
+     */
+    @Restricted(NoExternalUse.class)
+    class DslGroup extends DslEntry {
+        @DataBoundConstructor
+        public DslGroup(String name, List<String> permissions) {
+            super(name, permissions);
+        }
+
+        @Override
+        public void addPermission(AuthorizationProperty authorizationProperty) {
+            getPermissions()
+                    .forEach(permission ->
+                            authorizationProperty.add(findPermission(permission), PermissionEntry.group(getName())));
+        }
+
+        @Extension
+        @Symbol("group")
+        public static class DescriptorImpl extends Descriptor<DslEntry> {}
+    }
+
+    /**
+     * Represents a user or group being assigned permissions.
+     * Generally discouraged "ambiguous" permission assignments.
+     */
+    @Restricted(NoExternalUse.class)
+    class DslUserOrGroup extends DslEntry {
+        @DataBoundConstructor
+        public DslUserOrGroup(String name, List<String> permissions) {
+            super(name, permissions);
+        }
+
+        @Override
+        public void addPermission(AuthorizationProperty authorizationProperty) {
+            getPermissions()
+                    .forEach(permission -> authorizationProperty.add(
+                            findPermission(permission), new PermissionEntry(AuthorizationType.EITHER, getName())));
+        }
+
+        @Extension
+        @Symbol("userOrGroup")
+        public static class DescriptorImpl extends Descriptor<DslEntry> {}
+    }
 }
diff --git a/src/test/java/hudson/security/AuthorizationMatrixPropertyTest.java b/src/test/java/hudson/security/AuthorizationMatrixPropertyTest.java
@@ -40,8 +40,7 @@ public void testSnippetizer() throws Exception {
         SnippetizerTester tester = new SnippetizerTester(j);
         tester.assertRoundTrip(
                 new JobPropertyStep(Collections.singletonList(property)),
-                "properties([authorizationMatrix(inheritanceStrategy: nonInheriting(), "
-                        + "permissions: ['hudson.model.Item.Configure:alice', 'hudson.model.Item.Read:alice', 'hudson.model.Item.Read:bob', 'hudson.scm.SCM.Tag:bob'])])");
+                "properties([authorizationMatrix(entries: [userOrGroup(name: 'alice', permissions: ['Job/Configure', 'Job/Read']), userOrGroup(name: 'bob', permissions: ['Job/Read', 'SCM/Tag'])], inheritanceStrategy: nonInheriting())])");
     }
 
     @Test
@@ -56,8 +55,7 @@ public void testSnippetizer2() throws Exception {
         SnippetizerTester tester = new SnippetizerTester(j);
         tester.assertRoundTrip(
                 new JobPropertyStep(Collections.singletonList(property)),
-                "properties([authorizationMatrix(inheritanceStrategy: nonInheriting(), "
-                        + "permissions: ['USER:hudson.model.Item.Configure:alice', 'USER:hudson.model.Item.Read:alice', 'USER:hudson.model.Item.Read:bob', 'USER:hudson.scm.SCM.Tag:bob'])])");
+                "properties([authorizationMatrix(entries: [user(name: 'alice', permissions: ['Job/Configure', 'Job/Read']), user(name: 'bob', permissions: ['Job/Read', 'SCM/Tag'])], inheritanceStrategy: nonInheriting())])");
     }
 
     @Test
@@ -79,8 +77,7 @@ public void testSnippetizerInapplicablePermission() throws Exception {
         SnippetizerTester tester = new SnippetizerTester(j);
         tester.assertRoundTrip(
                 new JobPropertyStep(Collections.singletonList(property)),
-                "properties([authorizationMatrix(inheritanceStrategy: nonInheriting(), "
-                        + "permissions: ['hudson.model.Item.Configure:alice', 'hudson.model.Item.Read:alice', 'hudson.model.Item.Read:bob', 'hudson.scm.SCM.Tag:bob'])])");
+                "properties([authorizationMatrix(entries: [userOrGroup(name: 'alice', permissions: ['Job/Configure', 'Job/Read']), userOrGroup(name: 'bob', permissions: ['Job/Read', 'SCM/Tag'])], inheritanceStrategy: nonInheriting())])");
 
         Assert.assertTrue(l.getMessages().stream()
                 .anyMatch(m -> m.contains("Tried to add inapplicable permission")
@@ -126,16 +123,16 @@ public void testPipelineReconfiguration() throws Exception {
 
         project.setDefinition(new CpsFlowDefinition(
                 "properties([authorizationMatrix(inheritanceStrategy: nonInheriting(), "
-                        + "permissions: ['hudson.model.Item.Read:bob', 'hudson.model.Item.Configure:bob', 'hudson.scm.SCM.Tag:bob'])])",
+                        + "entries: [user(name: 'bob', permissions: ['Job/Read', 'Job/Configure', 'hudson.scm.SCM.Tag'])])])",
                 true));
         j.buildAndAssertSuccess(project);
 
         // let's look ast the property
         AuthorizationMatrixProperty property = project.getProperty(AuthorizationMatrixProperty.class);
         Assert.assertTrue(property.getInheritanceStrategy() instanceof NonInheritingStrategy);
-        Assert.assertEquals(3, property.getGrantedPermissions().size());
+        Assert.assertEquals(0, property.getGrantedPermissions().size()); // Unambiguous permissions are 0
         Assert.assertEquals(3, property.getGrantedPermissionEntries().size());
-        Assert.assertEquals("bob", property.getGroups().toArray()[0]);
+        Assert.assertEquals(0, property.getGroups().size());
 
         // now bob has access, including configure
         j.createWebClient().login("bob").goTo(project.getUrl());
@@ -179,7 +176,7 @@ public void testPipelineReconfiguration2() throws Exception {
 
         project.setDefinition(new CpsFlowDefinition(
                 "properties([authorizationMatrix(inheritanceStrategy: nonInheriting(), "
-                        + "permissions: ['USER:hudson.model.Item.Read:bob', 'USER:hudson.model.Item.Configure:bob', 'USER:hudson.scm.SCM.Tag:bob'])])",
+                        + "entries: [user(name: 'bob', permissions: ['Job/Read', 'Job/Configure', 'hudson.scm.SCM.Tag'])])])",
                 true));
         j.buildAndAssertSuccess(project);
 

diff --git a/...org/jenkinsci/plugins/matrixauth/integrations/casc/configuration-as-code-v2-ambiguous.yml b/...org/jenkinsci/plugins/matrixauth/integrations/casc/configuration-as-code-v2-ambiguous.yml
@@ -62,12 +62,12 @@ jobs:
             inheritanceStrategy {
                 nonInheriting()
             }
-            permissions([
-                'Job/Build:authenticated',
-                'Job/Configure:authenticated',
-                'Job/Delete:authenticated',
-                'Job/Read:authenticated',
-            ])
+            entries {
+              group {
+                name('authenticated')
+                permissions([ 'Job/Build', 'Job/Configure', 'Job/Delete', 'Job/Read' ])
+              }
+            }
           }
         }
       }
diff --git a/...resources/org/jenkinsci/plugins/matrixauth/integrations/casc/configuration-as-code-v2.yml b/...resources/org/jenkinsci/plugins/matrixauth/integrations/casc/configuration-as-code-v2.yml
@@ -62,12 +62,12 @@ jobs:
           inheritanceStrategy {
               nonInheriting()
           }
-          permissions([
-              'GROUP:Job/Build:authenticated',
-              'GROUP:Job/Configure:authenticated',
-              'GROUP:Job/Delete:authenticated',
-              'GROUP:Job/Read:authenticated',
-          ])
+          entries {
+            group {
+              name('authenticated')
+              permissions([ 'Job/Build', 'Job/Configure', 'Job/Delete', 'Job/Read' ])
+            }
+          }
         }
       }
     }