From 7df974eea1f8c9be48ae88a66e90a96a80d4c721 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Thu, 4 Jul 2024 13:00:32 +0200 Subject: [PATCH 1/3] Allow administrative monitors to be displayed for users with `Overall/MANAGE` permission This allows users with `Overall/MANAGE` permission but not `Overall/SYSTEM_READ` to display administrative monitors if the administrative monitor has been implemented to allow it. --- .../hudson/model/AdministrativeMonitor.java | 13 ++++- .../AdministrativeMonitorsDecorator.java | 2 +- core/src/main/java/jenkins/model/Jenkins.java | 2 +- .../AdministrativeMonitorsDecoratorTest.java | 52 +++++++++++++++++++ 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/model/AdministrativeMonitor.java b/core/src/main/java/hudson/model/AdministrativeMonitor.java index 7a9a0dde970c..f02d98811861 100644 --- a/core/src/main/java/hudson/model/AdministrativeMonitor.java +++ b/core/src/main/java/hudson/model/AdministrativeMonitor.java @@ -183,7 +183,7 @@ public void doDisable(StaplerRequest req, StaplerResponse rsp) throws IOExceptio /** * Required permission to view this admin monitor. - * By default {@link Jenkins#ADMINISTER}, but {@link Jenkins#SYSTEM_READ} is also supported. + * By default {@link Jenkins#ADMINISTER}, but {@link Jenkins#SYSTEM_READ} or {@link Jenkins#MANAGE} are also supported. *

* Changing this permission check to return {@link Jenkins#SYSTEM_READ} will make the active * administrative monitor appear on {@code manage.jelly} and on the globally visible @@ -201,6 +201,17 @@ public Permission getRequiredPermission() { return Jenkins.ADMINISTER; } + /** + * Checks if the current user has the minimum required permission to view any administrative monitor. + * + * @return true if the current user has the minimum required permission to view any administrative monitor. + * + * @since TODO + */ + public static boolean hasPermissionToDisplay() { + return Jenkins.get().hasAnyPermission(Jenkins.SYSTEM_READ, Jenkins.MANAGE); + } + /** * Ensure that URLs in this administrative monitor are only accessible to users with {@link #getRequiredPermission()}. */ diff --git a/core/src/main/java/jenkins/management/AdministrativeMonitorsDecorator.java b/core/src/main/java/jenkins/management/AdministrativeMonitorsDecorator.java index 21cc0ba1df0c..5b94090f4e3b 100644 --- a/core/src/main/java/jenkins/management/AdministrativeMonitorsDecorator.java +++ b/core/src/main/java/jenkins/management/AdministrativeMonitorsDecorator.java @@ -139,7 +139,7 @@ private Collection getAllActiveAdministrativeMonitors() { * @return the list of active monitors if we should display them, otherwise null. */ public Collection getMonitorsToDisplay() { - if (!Jenkins.get().hasPermission(Jenkins.SYSTEM_READ)) { + if (!(AdministrativeMonitor.hasPermissionToDisplay())) { return null; } diff --git a/core/src/main/java/jenkins/model/Jenkins.java b/core/src/main/java/jenkins/model/Jenkins.java index 4782e4b321f2..a4640680006b 100644 --- a/core/src/main/java/jenkins/model/Jenkins.java +++ b/core/src/main/java/jenkins/model/Jenkins.java @@ -2355,7 +2355,7 @@ public AdministrativeMonitor getAdministrativeMonitor(String id) { * @since 2.64 */ public List getActiveAdministrativeMonitors() { - if (!Jenkins.get().hasPermission(SYSTEM_READ)) { + if (!AdministrativeMonitor.hasPermissionToDisplay()) { return Collections.emptyList(); } return administrativeMonitors.stream().filter(m -> { diff --git a/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java b/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java index 37d28638e65b..c67cc2f6529c 100644 --- a/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java +++ b/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java @@ -24,13 +24,21 @@ package jenkins.management; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.instanceOf; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import hudson.ExtensionList; import hudson.model.AdministrativeMonitor; import hudson.model.User; +import hudson.security.ACL; +import hudson.security.Permission; import jenkins.model.Jenkins; import org.jenkinsci.Symbol; +import org.junit.AfterClass; +import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.Issue; @@ -43,6 +51,16 @@ public class AdministrativeMonitorsDecoratorTest { @Rule public JenkinsRule j = new JenkinsRule(); + @BeforeClass + public static void enablePermissions() { + System.setProperty("jenkins.security.ManagePermission", "true"); + } + + @AfterClass + public static void disablePermissions() { + System.clearProperty("jenkins.security.ManagePermission"); + } + @Test public void ensureAdminMonitorsAreNotRunPerNonAdminPage() throws Exception { j.jenkins.setSecurityRealm(j.createDummySecurityRealm()); @@ -131,4 +149,38 @@ public boolean isSecurity() { return true; } } + + @Test + public void ensureAdminMonitorsCanBeSeenByManagers() { + 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(ManagerAdministrativeMonitor.class))); + } + try (var ignored = ACL.as2(systemReadUser.impersonate2())) { + assertThat(Jenkins.get().getActiveAdministrativeMonitors(), not(hasItem(instanceOf(ManagerAdministrativeMonitor.class)))); + } + } + + @TestExtension("ensureAdminMonitorsCanBeSeenByManagers") + public static class ManagerAdministrativeMonitor extends AdministrativeMonitor { + @Override + public Permission getRequiredPermission() { + return Jenkins.MANAGE; + } + + @Override + public boolean isActivated() { + return true; + } + } } From 4152c5726b272a1fd5bb1ccef08a39f5d8921980 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Mon, 8 Jul 2024 09:35:49 +0200 Subject: [PATCH 2/3] Add a method to check for permission instead --- .../hudson/model/AdministrativeMonitor.java | 18 +++++++++++++++++- .../AdministrativeMonitorsDecoratorTest.java | 14 +++----------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/hudson/model/AdministrativeMonitor.java b/core/src/main/java/hudson/model/AdministrativeMonitor.java index f02d98811861..30008d4ba0ad 100644 --- a/core/src/main/java/hudson/model/AdministrativeMonitor.java +++ b/core/src/main/java/hudson/model/AdministrativeMonitor.java @@ -191,6 +191,10 @@ public void doDisable(StaplerRequest req, StaplerResponse rsp) throws IOExceptio * {@link #doDisable(StaplerRequest, StaplerResponse)} will still always require Administer permission. *

*

+ * This method only allows for a single permission to be returned. If more complex permission checks are required, + * override {@link #checkRequiredPermission()} instead. + *

+ *

* Implementers need to ensure that {@code doAct} and other web methods perform necessary permission checks: * Users with System Read permissions are expected to be limited to read-only access. * Form UI elements that change system state, e.g. toggling a feature on or off, need to be hidden from users @@ -201,6 +205,18 @@ public Permission getRequiredPermission() { return Jenkins.ADMINISTER; } + /** + * Checks if the current user has the minimum required permission to view this administrative monitor. + *

+ * Subclasses may override this method instead of {@link #getRequiredPermission()} to perform more complex permission checks, + * for example, checking either {@link Jenkins#MANAGE} or {@link Jenkins#SYSTEM_READ}. + *

+ * @see #getRequiredPermission() + */ + public void checkRequiredPermission() { + Jenkins.get().checkPermission(getRequiredPermission()); + } + /** * Checks if the current user has the minimum required permission to view any administrative monitor. * @@ -218,7 +234,7 @@ public static boolean hasPermissionToDisplay() { @Override @Restricted(NoExternalUse.class) public Object getTarget() { - Jenkins.get().checkPermission(getRequiredPermission()); + checkRequiredPermission(); return this; } diff --git a/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java b/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java index c67cc2f6529c..edbf5f27e48f 100644 --- a/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java +++ b/test/src/test/java/jenkins/management/AdministrativeMonitorsDecoratorTest.java @@ -37,10 +37,9 @@ import hudson.security.Permission; import jenkins.model.Jenkins; import org.jenkinsci.Symbol; -import org.junit.AfterClass; -import org.junit.BeforeClass; import org.junit.Rule; import org.junit.Test; +import org.jvnet.hudson.test.FlagRule; import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.MockAuthorizationStrategy; @@ -51,15 +50,8 @@ public class AdministrativeMonitorsDecoratorTest { @Rule public JenkinsRule j = new JenkinsRule(); - @BeforeClass - public static void enablePermissions() { - System.setProperty("jenkins.security.ManagePermission", "true"); - } - - @AfterClass - public static void disablePermissions() { - System.clearProperty("jenkins.security.ManagePermission"); - } + @Rule + public final FlagRule managePermissionRule = FlagRule.systemProperty("jenkins.security.ManagePermission", "true"); @Test public void ensureAdminMonitorsAreNotRunPerNonAdminPage() throws Exception { From dc36857468f76e74ea7686bd863b2943d0107223 Mon Sep 17 00:00:00 2001 From: Vincent Latombe Date: Mon, 8 Jul 2024 09:53:43 +0200 Subject: [PATCH 3/3] 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 *

*

* 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. *

*

* 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. *

- * 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}. *

* @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. + *

+ * 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}. + *

+ * @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 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; + } + } }