Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow administrative monitors to be displayed for users with Overall/MANAGE permission #9437

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions core/src/main/java/hudson/model/AdministrativeMonitor.java
Original file line number Diff line number Diff line change
Expand Up @@ -183,14 +183,18 @@ 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.
jglick marked this conversation as resolved.
Show resolved Hide resolved
* <p>
* 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
* {@link jenkins.management.AdministrativeMonitorsDecorator} to users without Administer permission.
* {@link #doDisable(StaplerRequest, StaplerResponse)} will still always require Administer permission.
* </p>
* <p>
* This method only allows for a single permission to be returned. If more complex permission checks are required,
* override {@link #checkRequiredPermission()} and {@link #hasRequiredPermission()} instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that these methods are very similar to AccessControlled, but creating a custom ACL seems more laborious than implementing them as written.

* </p>
* <p>
* 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
Expand All @@ -201,13 +205,50 @@ public Permission getRequiredPermission() {
return Jenkins.ADMINISTER;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hesitating to deprecate this method, but it still matches simple use cases so went for a javadoc instead.

}

/**
* Checks if the current user has the minimum required permission to view this administrative monitor.
* <p>
* 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}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javadoc is misleading as it implies that you could grant permission to see admin monitors based on some other criteria. But in fact due to hasPermissionToDisplay these are really the only permissions you can check.

* </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.
*
* @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()}.
*/
@Override
@Restricted(NoExternalUse.class)
public Object getTarget() {
Jenkins.get().checkPermission(getRequiredPermission());
checkRequiredPermission();
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ private Collection<AdministrativeMonitor> getAllActiveAdministrativeMonitors() {
* @return the list of active monitors if we should display them, otherwise null.
*/
public Collection<AdministrativeMonitor> getMonitorsToDisplay() {
if (!Jenkins.get().hasPermission(Jenkins.SYSTEM_READ)) {
if (!(AdministrativeMonitor.hasPermissionToDisplay())) {
return null;
}

Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/jenkins/model/Jenkins.java
Original file line number Diff line number Diff line change
Expand Up @@ -2355,12 +2355,12 @@
* @since 2.64
*/
public List<AdministrativeMonitor> getActiveAdministrativeMonitors() {
if (!Jenkins.get().hasPermission(SYSTEM_READ)) {
if (!AdministrativeMonitor.hasPermissionToDisplay()) {

Check warning on line 2358 in core/src/main/java/jenkins/model/Jenkins.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line

Line 2358 is only partially covered, one branch is missing
return Collections.emptyList();
}
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,22 @@

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.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;
Expand All @@ -43,6 +50,9 @@ public class AdministrativeMonitorsDecoratorTest {
@Rule
public JenkinsRule j = new JenkinsRule();

@Rule
public final FlagRule<String> managePermissionRule = FlagRule.systemProperty("jenkins.security.ManagePermission", "true");

@Test
public void ensureAdminMonitorsAreNotRunPerNonAdminPage() throws Exception {
j.jenkins.setSecurityRealm(j.createDummySecurityRealm());
Expand Down Expand Up @@ -131,4 +141,80 @@ 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;
}
}

@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;
}
}
}
Loading