JENKINS-61208 Allow system read to view more admin monitors #4685

timja · 2020-04-23T21:27:02Z

See JENKINS-61208.

Screenshots

Proposed changelog entries

Entry 1: JENKINS-61208 Allow system read to view more admin monitors
...

Proposed upgrade guidelines

N/A

Submitter checklist

(If applicable) Jira issue is well described
Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
- Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
Appropriate autotests or explanation to why this change has no tests
For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

There are at least 2 approvals for the pull request and no outstanding requests for change
Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
Changelog entries in the PR title and/or Proposed changelog entries are correct
Proper changelog labels are set so that the changelog can be generated automatically
If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java

core/src/main/java/jenkins/security/UpdateSiteWarningsMonitor.java

timja · 2020-04-29T21:52:18Z

Test failures are super weird, they pass locally, merged in master and will see what happens on CI

daniel-beck

Approval conditional on TooManyJobsButNoView comment being addressed.

core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java

daniel-beck

Thanks!

test/src/test/java/hudson/diagnosis/TooManyJobsButNoViewTest.java

varyvol · 2020-04-30T09:07:56Z

core/src/main/resources/hudson/diagnosis/ReverseProxySetupMonitor/message.jelly

@@ -22,12 +22,14 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 THE SOFTWARE.
 -->
 <?jelly escape-by-default='true'?>
-<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:f="/lib/form">
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:f="/lib/form" xmlns:l="/lib/layout">
  <div id="redirect-error" class="alert alert-danger reverse-proxy__hidden"
       data-url="${rootURL}/${it.url}/test" data-context="${rootURL}">
    <form method="post" action="${rootURL}/${it.url}/act" name="${it.id}">
      <f:submit name="yes" value="${%More Info}"/>


Why does this one allow to submit the yes? SYSTEM_READ does have access to view the information the button is redirecting to?

I don't see any problem if a non-admin user can see the More Info button. It's not configuring anything if IIRC

it's a server side redirect to a jenkins.io redirect, https://github.com/timja/jenkins/blob/system-read-more-admin-monitors/core/src/main/java/hudson/diagnosis/ReverseProxySetupMonitor.java#L118-L126

timja · 2020-04-30T10:19:27Z

Let's merge this in ~24 hours if there's no negative feedback

jeffret-b

These look good. I'm glad to see the new tests.

oleg-nenashev

Looks good to me. The open comment about lack of Job access is an unlikely edge case IMHO, so I am fine with the PR as is

oleg-nenashev · 2020-04-30T20:29:14Z

core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java

+        if (j.hasPermission(Jenkins.ADMINISTER)) {
+            return j.getViews().size() == 1 && j.getItemMap().size() > THRESHOLD;
+        }
+        // SystemRead


or "Manage" soon, I suspect

core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java

timja · 2020-04-30T20:42:15Z

Looks good to me. The open comment about lack of Job access is an unlikely edge case IMHO, so I am fine with the PR as is

the edge case was handled in e2d5b08

JENKINS-61208 Allow system read to view more admin monitors

4998156

timja requested review from a team, jeffret-b and oleg-nenashev April 23, 2020 21:27

daniel-beck reviewed Apr 23, 2020

View reviewed changes

core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java Show resolved Hide resolved

core/src/main/java/jenkins/security/UpdateSiteWarningsMonitor.java Show resolved Hide resolved

Add permission check

c82a692

daniel-beck self-requested a review April 29, 2020 10:41

timja closed this Apr 29, 2020

timja reopened this Apr 29, 2020

Merge branch 'master' into system-read-more-admin-monitors

cc7efb8

Fix merge

1ecd8b6

daniel-beck approved these changes Apr 30, 2020

View reviewed changes

core/src/main/java/hudson/diagnosis/TooManyJobsButNoView.java Show resolved Hide resolved

Don't show views monitor if no access

e2d5b08

daniel-beck approved these changes Apr 30, 2020

View reviewed changes

test/src/test/java/hudson/diagnosis/TooManyJobsButNoViewTest.java Outdated Show resolved Hide resolved

Simplify check

c17a27a

varyvol reviewed Apr 30, 2020

View reviewed changes

fcojfernandez approved these changes Apr 30, 2020

View reviewed changes

varyvol approved these changes Apr 30, 2020

View reviewed changes

timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback labels Apr 30, 2020

daniel-beck approved these changes Apr 30, 2020

View reviewed changes

jeffret-b approved these changes Apr 30, 2020

View reviewed changes

oleg-nenashev approved these changes Apr 30, 2020

View reviewed changes

timja merged commit 1837293 into jenkinsci:master May 1, 2020

timja deleted the system-read-more-admin-monitors branch May 1, 2020 11:01

daniel-beck mentioned this pull request Nov 15, 2024

[JENKINS-73908] Do not show button to upgrade to SystemRead users #9981

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-61208 Allow system read to view more admin monitors #4685

JENKINS-61208 Allow system read to view more admin monitors #4685

timja commented Apr 23, 2020

timja commented Apr 29, 2020

daniel-beck left a comment

daniel-beck left a comment

varyvol Apr 30, 2020

fcojfernandez Apr 30, 2020

timja Apr 30, 2020

timja commented Apr 30, 2020

jeffret-b left a comment

oleg-nenashev left a comment

oleg-nenashev Apr 30, 2020

timja commented Apr 30, 2020

JENKINS-61208 Allow system read to view more admin monitors #4685

JENKINS-61208 Allow system read to view more admin monitors #4685

Conversation

timja commented Apr 23, 2020

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

timja commented Apr 29, 2020

daniel-beck left a comment

Choose a reason for hiding this comment

daniel-beck left a comment

Choose a reason for hiding this comment

varyvol Apr 30, 2020

Choose a reason for hiding this comment

fcojfernandez Apr 30, 2020

Choose a reason for hiding this comment

timja Apr 30, 2020

Choose a reason for hiding this comment

timja commented Apr 30, 2020

jeffret-b left a comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev Apr 30, 2020

Choose a reason for hiding this comment

timja commented Apr 30, 2020