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

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Jul 5, 2024

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.

Testing done

Verified with a custom administrative monitor allowing Overall/MANAGE that it can be displayed in the corresponding UI area by a user with this permission.

Proposed changelog entries

  • Allow some administrative monitors to be displayed for users with Overall/MANAGE permission

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

…/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.
@Vlatombe Vlatombe added the developer Changes which impact plugin developers label Jul 5, 2024
@Vlatombe Vlatombe requested review from a team, jglick, amuniz and mikecirioli July 5, 2024 14:28
@@ -201,13 +205,36 @@ 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.

@Vlatombe Vlatombe requested a review from jglick July 8, 2024 12:50
* <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.

@jglick
Copy link
Member

jglick commented Jul 8, 2024

What about AdministrativeMonitorsConfiguration and

public void doDisable(StaplerRequest req, StaplerResponse rsp) throws IOException {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);
? With this change, IIUC, users with only MANAGE could see monitors and agree to fix the issue the monitor warns about, but they could not dismiss monitors they found irrelevant.

@Vlatombe Vlatombe added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jul 10, 2024
@Vlatombe Vlatombe merged commit ea07383 into jenkinsci:master Jul 11, 2024
16 checks passed
@Vlatombe Vlatombe deleted the administrative-monitors-for-managers branch July 11, 2024 07:00
* 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants