From dc36857468f76e74ea7686bd863b2943d0107223 Mon Sep 17 00:00:00 2001
From: Vincent Latombe <vincent@latombe.net>
Date: Mon, 8 Jul 2024 09:53:43 +0200
Subject: [PATCH] Cover new logic and fix check

---
 .../hudson/model/AdministrativeMonitor.java   | 18 +++++++-
 core/src/main/java/jenkins/model/Jenkins.java |  2 +-
 .../AdministrativeMonitorsDecoratorTest.java  | 42 +++++++++++++++++++
 3 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/core/src/main/java/hudson/model/AdministrativeMonitor.java b/core/src/main/java/hudson/model/AdministrativeMonitor.java
index 30008d4ba0ad..e2f69d654ca8 100644
--- 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
index a4640680006b..a3376f99d75e 100644
--- 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
index edbf5f27e48f..691b9d2c8868 100644
--- 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;
+        }
+    }
 }