Cover new logic and fix check

jenkinsci · Jul 8, 2024 · dc36857 · dc36857
1 parent 4152c57
commit dc36857
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 3 deletions.
diff --git a/core/src/main/java/hudson/model/AdministrativeMonitor.java b/core/src/main/java/hudson/model/AdministrativeMonitor.java
@@ -192,7 +192,7 @@ public void doDisable(StaplerRequest req, StaplerResponse rsp) throws IOExceptio
      * </p>
      * <p>
      *     This method only allows for a single permission to be returned. If more complex permission checks are required,
-     *     override {@link #checkRequiredPermission()} instead.
+     *     override {@link #checkRequiredPermission()} and {@link #hasRequiredPermission()} instead.
      * </p>
      * <p>
      *     Implementers need to ensure that {@code doAct} and other web methods perform necessary permission checks:
@@ -208,15 +208,29 @@ public Permission getRequiredPermission() {
     /**
      * Checks if the current user has the minimum required permission to view this administrative monitor.
      * <p>
-     * Subclasses may override this method instead of {@link #getRequiredPermission()} to perform more complex permission checks,
+     * Subclasses may override this method and {@link #hasRequiredPermission()} instead of {@link #getRequiredPermission()} to perform more complex permission checks,
      * for example, checking either {@link Jenkins#MANAGE} or {@link Jenkins#SYSTEM_READ}.
      * </p>
      * @see #getRequiredPermission()
+     * @see #hasRequiredPermission()
      */
     public void checkRequiredPermission() {
         Jenkins.get().checkPermission(getRequiredPermission());
     }
 
+    /**
+     * Checks if the current user has the minimum required permission to view this administrative monitor.
+     * <p>
+     * Subclasses may override this method and {@link #checkRequiredPermission} instead of {@link #getRequiredPermission()} to perform more complex permission checks,
+     * for example, checking either {@link Jenkins#MANAGE} or {@link Jenkins#SYSTEM_READ}.
+     * </p>
+     * @see #getRequiredPermission()
+     * @see #checkRequiredPermission()
+     */
+    public boolean hasRequiredPermission() {
+        return Jenkins.get().hasPermission(getRequiredPermission());
+    }
+
     /**
      * Checks if the current user has the minimum required permission to view any administrative monitor.
      *

diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java
@@ -2360,7 +2360,7 @@ public List<AdministrativeMonitor> getActiveAdministrativeMonitors() {
         }
         return administrativeMonitors.stream().filter(m -> {
             try {
-                return Jenkins.get().hasPermission(m.getRequiredPermission()) && m.isEnabled() && m.isActivated();
+                return m.hasRequiredPermission() && m.isEnabled() && m.isActivated();
             } catch (Throwable x) {
                 LOGGER.log(Level.WARNING, null, x);
                 return false;

diff --git a/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java b/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java
@@ -175,4 +175,46 @@ public boolean isActivated() {
             return true;
         }
     }
+
+    @Test
+    public void ensureAdminMonitorsCanBeSeenByManagersOrSystemReaders() {
+        j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
+        var managerLogin = "manager";
+        var systemReadLogin = "system-reader";
+        var managerUser = User.getById(managerLogin, true);
+        var systemReadUser = User.getById(systemReadLogin, true);
+
+        j.jenkins.setAuthorizationStrategy(new MockAuthorizationStrategy()
+                .grant(Jenkins.MANAGE, Jenkins.READ).everywhere().to(managerLogin)
+                .grant(Jenkins.READ, Jenkins.SYSTEM_READ).everywhere().to(systemReadLogin)
+        );
+
+        try (var ignored = ACL.as2(managerUser.impersonate2())) {
+            assertThat(Jenkins.get().getActiveAdministrativeMonitors(), hasItem(instanceOf(ManagerOrSystemReaderAdministrativeMonitor.class)));
+        }
+        try (var ignored = ACL.as2(systemReadUser.impersonate2())) {
+            assertThat(Jenkins.get().getActiveAdministrativeMonitors(), hasItem(instanceOf(ManagerOrSystemReaderAdministrativeMonitor.class)));
+        }
+    }
+
+    @TestExtension("ensureAdminMonitorsCanBeSeenByManagersOrSystemReaders")
+    public static class ManagerOrSystemReaderAdministrativeMonitor extends AdministrativeMonitor {
+
+        private static final Permission[] REQUIRED_ANY_PERMISSIONS = {Jenkins.MANAGE, Jenkins.SYSTEM_READ};
+
+        @Override
+        public void checkRequiredPermission() {
+            Jenkins.get().checkAnyPermission(REQUIRED_ANY_PERMISSIONS);
+        }
+
+        @Override
+        public boolean hasRequiredPermission() {
+            return Jenkins.get().hasAnyPermission(REQUIRED_ANY_PERMISSIONS);
+        }
+
+        @Override
+        public boolean isActivated() {
+            return true;
+        }
+    }
 }