Disallow dangerous permissions

jenkinsci · Jul 9, 2019 · bf50316 · bf50316
1 parent 73956b6
commit bf50316
Show file tree

Hide file tree

Showing 6 changed files with 45 additions and 12 deletions.
diff --git a/src/main/java/io/jenkins/plugins/rolestrategy/FolderAuthorizationStrategyManagementLink.java b/src/main/java/io/jenkins/plugins/rolestrategy/FolderAuthorizationStrategyManagementLink.java
@@ -271,7 +271,7 @@ public void doDeleteFolderRole(@QueryParameter(required = true) String roleName)
         }
     }
 
-    private static Set<Permission> getSafePermissions(Set<PermissionGroup> groups) {
+    static Set<Permission> getSafePermissions(Set<PermissionGroup> groups) {
         HashSet<Permission> safePermissions = new HashSet<>();
         groups.stream().map(PermissionGroup::getPermissions).forEach(safePermissions::addAll);
         safePermissions.removeAll(PermissionHelper.DANGEROUS_PERMISSIONS);

diff --git a/src/main/java/io/jenkins/plugins/rolestrategy/FolderBasedAuthorizationStrategy.java b/src/main/java/io/jenkins/plugins/rolestrategy/FolderBasedAuthorizationStrategy.java
@@ -10,7 +10,6 @@
 import hudson.model.Job;
 import hudson.security.ACL;
 import hudson.security.AuthorizationStrategy;
-import hudson.security.Permission;
 import hudson.security.SidACL;
 import io.jenkins.plugins.rolestrategy.acls.GlobalAclImpl;
 import io.jenkins.plugins.rolestrategy.acls.JobAclImpl;
@@ -30,7 +29,6 @@
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.HashSet;
 import java.util.NoSuchElementException;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
@@ -74,12 +72,9 @@ public FolderBasedAuthorizationStrategy(Set<GlobalRole> globalRoles, Set<FolderR
              *
              * The same thing happens in RoleBasedAuthorizationStrategy. See RoleBasedStrategy.DESCRIPTOR.newInstance()
              */
-            HashSet<PermissionWrapper> adminPermissions = new HashSet<>();
-            RoleBasedAuthorizationStrategy.getPermissionGroups(RoleType.Global)
-                    .forEach(permissionGroup -> permissionGroup.getPermissions().stream()
-                            .map(Permission::getId)
-                            .map(PermissionWrapper::new)
-                            .forEach(adminPermissions::add));
+            Set<PermissionWrapper> adminPermissions = PermissionWrapper.wrapPermissions(
+                    FolderAuthorizationStrategyManagementLink.getSafePermissions(
+                            RoleBasedAuthorizationStrategy.getPermissionGroups(RoleType.Global)));
 
             GlobalRole adminRole = new GlobalRole(ADMIN_ROLE_NAME, adminPermissions);
             adminRole.assignSids(new PrincipalSid(Jenkins.getAuthentication()).getPrincipal());

diff --git a/src/main/java/io/jenkins/plugins/rolestrategy/acls/AbstractAcl.java b/src/main/java/io/jenkins/plugins/rolestrategy/acls/AbstractAcl.java
@@ -3,8 +3,10 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.security.Permission;
 import hudson.security.SidACL;
+import jenkins.model.Jenkins;
 import org.acegisecurity.acls.sid.Sid;
 import org.apache.commons.collections.CollectionUtils;
+import org.jenkinsci.plugins.rolestrategy.permissions.PermissionHelper;
 
 import javax.annotation.Nullable;
 import java.util.HashSet;
@@ -25,6 +27,10 @@ abstract class AbstractAcl extends SidACL {
             justification = "hudson.security.SidACL requires null when unknown")
     @Nullable
     protected Boolean hasPermission(Sid sid, Permission permission) {
+        if (PermissionHelper.DANGEROUS_PERMISSIONS.contains(permission)) {
+            permission = Jenkins.ADMINISTER;
+        }
+
         Set<Permission> permissions = permissionList.get(toString(sid));
         if (permissions != null && CollectionUtils.containsAny(permissions, getImplyingPermissions(permission))) {
             return true;

diff --git a/src/main/java/io/jenkins/plugins/rolestrategy/misc/PermissionWrapper.java b/src/main/java/io/jenkins/plugins/rolestrategy/misc/PermissionWrapper.java
@@ -2,7 +2,10 @@
 
 import hudson.security.Permission;
 import io.jenkins.plugins.rolestrategy.roles.AbstractRole;
+import org.jenkinsci.plugins.rolestrategy.permissions.PermissionHelper;
+import org.kohsuke.stapler.DataBoundConstructor;
 
+import javax.annotation.Nonnull;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Set;
@@ -24,7 +27,8 @@ public final class PermissionWrapper {
      *
      * @param id the id of the permission this {@link PermissionWrapper} contains.
      */
-    public PermissionWrapper(String id) {
+    @DataBoundConstructor
+    public PermissionWrapper(@Nonnull String id) {
         this.id = id;
         permission = Permission.fromId(id);
         checkPermission();
@@ -68,11 +72,13 @@ public int hashCode() {
     /**
      * Checks if the permission for this {@link PermissionWrapper} is valid.
      *
-     * @throws IllegalArgumentException when the permission did not exist or was null
+     * @throws IllegalArgumentException when the permission did not exist, was null or was dangerous.
      */
     private void checkPermission() {
         if (permission == null) {
             throw new IllegalArgumentException("Unable to infer permission from Id: " + id);
+        } else if (PermissionHelper.isDangerous(permission)) {
+            throw new IllegalArgumentException("Dangerous Permissions are not supported.");
         }
     }
 

diff --git a/src/test/java/io/jenkins/plugins/rolestrategy/FolderBasedAuthorizationStrategyTest.java b/src/test/java/io/jenkins/plugins/rolestrategy/FolderBasedAuthorizationStrategyTest.java
@@ -8,6 +8,7 @@
 import hudson.security.ACL;
 import hudson.security.ACLContext;
 import hudson.security.Permission;
+import hudson.security.PermissionGroup;
 import io.jenkins.plugins.rolestrategy.roles.FolderRole;
 import io.jenkins.plugins.rolestrategy.roles.GlobalRole;
 import jenkins.model.Jenkins;
@@ -17,6 +18,7 @@
 import org.jvnet.hudson.test.JenkinsRule;
 
 import java.util.Collections;
+import java.util.HashSet;
 
 import static io.jenkins.plugins.rolestrategy.misc.PermissionWrapper.wrapPermissions;
 import static junit.framework.TestCase.assertFalse;
@@ -50,7 +52,10 @@ public void setUp() throws Exception {
         final String adminRoleName = "adminRole";
         final String overallReadRoleName = "overallRead";
 
-        strategy.addGlobalRole(new GlobalRole(adminRoleName, wrapPermissions(Permission.getAll())));
+        strategy.addGlobalRole(new GlobalRole(adminRoleName,
+                wrapPermissions(FolderAuthorizationStrategyManagementLink.getSafePermissions(
+                        new HashSet<>(PermissionGroup.getAll())))));
+
         strategy.assignSidToGlobalRole(adminRoleName, "admin");
 
         strategy.addGlobalRole(new GlobalRole(overallReadRoleName, wrapPermissions(Permission.READ)));

diff --git a/src/test/java/io/jenkins/plugins/rolestrategy/misc/PermissionWrapperTest.java b/src/test/java/io/jenkins/plugins/rolestrategy/misc/PermissionWrapperTest.java
@@ -0,0 +1,21 @@
+package io.jenkins.plugins.rolestrategy.misc;
+
+import jenkins.model.Jenkins;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+
+public class PermissionWrapperTest {
+    @ClassRule
+    public static JenkinsRule j = new JenkinsRule();
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testDangerousPermission() {
+        new PermissionWrapper(Jenkins.RUN_SCRIPTS.getId());
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void testNullPermission() {
+        new PermissionWrapper("this is not a permission id");
+    }
+}