Merge pull request #3177 from jglick/SYSTEM-JENKINS-20474

[JENKINS-20474] Optimize away ACL construction and AuthorizationStrategy calls when checking permissions for SYSTEM
jenkinsci · Dec 9, 2017 · 969ed92 · 969ed92
2 parents 041e9fe + 7948104
commit 969ed92
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 1 deletion.
diff --git a/core/src/main/java/hudson/security/ACL.java b/core/src/main/java/hudson/security/ACL.java
@@ -64,6 +64,9 @@ public abstract class ACL {
      */
     public final void checkPermission(@Nonnull Permission p) {
         Authentication a = Jenkins.getAuthentication();
+        if (a == SYSTEM) {
+            return;
+        }
         if(!hasPermission(a,p))
             throw new AccessDeniedException2(a,p);
     }
@@ -75,7 +78,11 @@ public final void checkPermission(@Nonnull Permission p) {
      *      if the user doesn't have the permission.
      */
     public final boolean hasPermission(@Nonnull Permission p) {
-        return hasPermission(Jenkins.getAuthentication(),p);
+        Authentication a = Jenkins.getAuthentication();
+        if (a == SYSTEM) {
+            return true;
+        }
+        return hasPermission(a, p);
     }
 
     /**
@@ -101,6 +108,9 @@ public final boolean hasPermission(@Nonnull Permission p) {
     public final void checkCreatePermission(@Nonnull ItemGroup c,
                                             @Nonnull TopLevelItemDescriptor d) {
         Authentication a = Jenkins.getAuthentication();
+        if (a == SYSTEM) {
+            return;
+        }
         if (!hasCreatePermission(a, c, d)) {
             throw new AccessDeniedException(Messages.AccessDeniedException2_MissingPermission(a.getName(),
                     Item.CREATE.group.title+"/"+Item.CREATE.name + Item.CREATE + "/" + d.getDisplayName()));
@@ -136,6 +146,9 @@ public boolean hasCreatePermission(@Nonnull Authentication a, @Nonnull ItemGroup
     public final void checkCreatePermission(@Nonnull ViewGroup c,
                                             @Nonnull ViewDescriptor d) {
         Authentication a = Jenkins.getAuthentication();
+        if (a == SYSTEM) {
+            return;
+        }
         if (!hasCreatePermission(a, c, d)) {
             throw new AccessDeniedException(Messages.AccessDeniedException2_MissingPermission(a.getName(),
                     View.CREATE.group.title + "/" + View.CREATE.name + View.CREATE + "/" + d.getDisplayName()));

diff --git a/core/src/main/java/hudson/security/AccessControlled.java b/core/src/main/java/hudson/security/AccessControlled.java
@@ -59,6 +59,9 @@ default boolean hasPermission(@Nonnull Permission permission) {
      * @since FIXME
      */
     default boolean hasPermission(@Nonnull Authentication a, @Nonnull Permission permission) {
+        if (a == ACL.SYSTEM) {
+            return true;
+        }
         return getACL().hasPermission(a, permission);
     }
 

diff --git a/test/src/test/java/hudson/security/ACLTest.java b/test/src/test/java/hudson/security/ACLTest.java
@@ -0,0 +1,75 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2017 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package hudson.security;
+
+import hudson.model.FreeStyleProject;
+import hudson.model.Item;
+import java.util.Collection;
+import java.util.Collections;
+import org.acegisecurity.Authentication;
+import org.junit.Test;
+import static org.junit.Assert.*;
+import org.junit.Rule;
+import org.jvnet.hudson.test.Issue;
+import org.jvnet.hudson.test.JenkinsRule;
+
+public class ACLTest {
+
+    @Rule
+    public JenkinsRule r = new JenkinsRule();
+
+    @Issue("JENKINS-20474")
+    @Test
+    public void bypassStrategyOnSystem() throws Exception {
+        FreeStyleProject p = r.createFreeStyleProject();
+        r.jenkins.setSecurityRealm(r.createDummySecurityRealm());
+        r.jenkins.setAuthorizationStrategy(new DoNotBotherMe());
+        assertTrue(p.hasPermission(Item.CONFIGURE));
+        assertTrue(p.hasPermission(ACL.SYSTEM, Item.CONFIGURE));
+        p.checkPermission(Item.CONFIGURE);
+        p.checkAbortPermission();
+        assertEquals(Collections.singletonList(p), r.jenkins.getAllItems());
+    }
+
+    private static class DoNotBotherMe extends AuthorizationStrategy {
+
+        @Override
+        public ACL getRootACL() {
+            return new ACL() {
+                @Override
+                public boolean hasPermission(Authentication a, Permission permission) {
+                    throw new AssertionError("should not have needed to check " + permission + " for " + a);
+                }
+            };
+        }
+
+        @Override
+        public Collection<String> getGroups() {
+            return Collections.emptySet();
+        }
+
+    }
+
+}