JENKINS-61206 - Support System Read / Extended read permissions for agent configurations #4531

timja · 2020-02-29T12:08:20Z

Adds system read / extended read support for agents / clouds.

The rule I applied was anything that was previously checking for administer for viewing is now system read, anything checking for connect or configure is Agent/ExtendedRead

Manual testing notes

When using the core-pr-tester (docker run --rm -ti -p 8080:8080 -e ID=4531 jenkins/core-pr-tester), you can use script console to enable the permissions:

Jenkins.SYSTEM_READ.enabled = true
Computer.EXTENDED_READ.enabled = true

Proposed changelog entries

Entry 1: JENKINS-61206, Add system read support for 'Node Monitoring Configuration' and configuring clouds
Entry 2: JENKINS-61206, Add Agent/ExtendedRead support for viewing agent configuration, system information and logs
Entry 3: JENKINS-61206, Add support for the permissions attribute to task.jelly
Entry 4: JENKINS-61206, Add hasAnyPermissions API to Functions to allow it to be called by views
...

Proposed upgrade guidelines

N/A

Submitter checklist

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 should exist and be labeled as lts-candidate

daniel-beck · 2020-02-29T12:58:11Z

core/src/main/java/hudson/Functions.java

+    /**
+     * This version is so that the 'hasAnyPermission'
+     * degrades gracefully if "it" is not an {@link AccessControlled} object.
+     * Otherwise it will perform no check and that problem is hard to notice.


Just throw in that case? The assumption that object is an ancestor isn't necessarily true. For example, in upstream cause related UI, you would show information about another job / build.

It’s consistent with hasPermission and wasn’t working without this for the l:task it was just silently doing nothing

core/src/main/java/hudson/Functions.java

Otherwise there was a white page and it looked weird

daniel-beck · 2020-04-24T13:56:45Z

f:password has specific support to hide the password when it's shown in the context of an Item. This implemented a security fix (users without Configure/with Extended Read could see plain text or encrypted passwords on the config form).

jenkins/core/src/main/java/hudson/Functions.java

Lines 2019 to 2024 in ed80d24

    
           if (req != null) { 
        
               Item item = req.findAncestorObject(Item.class); 
        
               if (item != null && !item.hasPermission(Item.CONFIGURE)) { 
        
                   return "********"; 
        
               } 
        
           }

While this is now pretty irrelevant for "read only" forms, it's not inconceivable that other views are added by a plugin that are available to users with Computer/ExtendedRead but are not marked read-only.

So I would like to see that method amended to mask f:password when neither Computer/Configure permission is granted, nor readOnlyMode set.

timja · 2020-04-24T14:26:51Z

f:password has specific support to hide the password when it's shown in the context of an Item. This implemented a security fix (users without Configure/with Extended Read could see plain text or encrypted passwords on the config form).

jenkins/core/src/main/java/hudson/Functions.java

Lines 2019 to 2024 in ed80d24

if (req != null) {

Item item = req.findAncestorObject(Item.class);

if (item != null && !item.hasPermission(Item.CONFIGURE)) {

return "********";

}

}

While this is now pretty irrelevant for "read only" forms, it's not inconceivable that other views are added by a plugin that are available to users with Computer/ExtendedRead but are not marked read-only.

So I would like to see that method amended to mask f:password when neither Computer/Configure permission is granted, nor readOnlyMode set.

The only usages of Computer.EXTENDED_READ are currently in jenkins core, this feature has never been documented, and it not enabled by the extended read permission plugin, I will fix that as part of this work.

I see it as pretty inconceivable really.

Do you still think it's worth doing that?

daniel-beck · 2020-04-24T14:42:18Z

@timja I do, any plugin adding support for Computer/Extended Read without also ensuring their custom form is readOnlyMode would enable this problem.

To clarify, it is not currently a problem for the reasons you mention, but may become one with this PR.

timja · 2020-04-24T14:57:54Z

Hmm k, will take a look

timja · 2020-04-25T08:48:22Z

@timja I do, any plugin adding support for Computer/Extended Read without also ensuring their custom form is readOnlyMode would enable this problem.

To clarify, it is not currently a problem for the reasons you mention, but may become one with this PR.

Pushed, will look at a test later on, I've manually tested it

timja · 2020-04-25T13:24:00Z

@timja I do, any plugin adding support for Computer/Extended Read without also ensuring their custom form is readOnlyMode would enable this problem.

To clarify, it is not currently a problem for the reasons you mention, but may become one with this PR.

For testing, any suggestion on how to add a form with a Computer in it's ancestor but not in what's already got readOnlyMode as part of this PR?

I tested it by commenting out the readOnlyMode and adding a secret field to JNLPLauncher

daniel-beck · 2020-04-25T14:14:37Z

For testing, any suggestion on how to add a form with a Computer in it's ancestor but not in what's already got readOnlyMode as part of this PR?

@timja Attach an Action to the Computer (or Slave I never remember which) with a view that has an f:password.

timja · 2020-04-28T20:42:06Z

For testing, any suggestion on how to add a form with a Computer in it's ancestor but not in what's already got readOnlyMode as part of this PR?

@timja Attach an Action to the Computer (or Slave I never remember which) with a view that has an f:password.

resolved in 393ad7f

daniel-beck · 2020-05-08T10:16:08Z

core/src/main/resources/hudson/model/Computer/sidepanel.jelly

-      <l:task href="${rootURL}/${it.url}configure" icon="icon-setting icon-md" permission="${it.EXTENDED_READ}" title="${%Configure}"/>
+
+      <j:choose>
+        <j:when test="${app.hasPermission(it.CONFIGURE)}">


Looks like the same mistake that made all job configurations read-only?

Suggested change

<j:when test="${app.hasPermission(it.CONFIGURE)}">

<j:when test="${it.hasPermission(it.CONFIGURE)}">

seems to work though, =/

I've committed it but both seem to work in this case from what I can see

Don't grant the permission globally, use project-specific matrix auth and grant it on the specific agent only.

Hmm ? That’s what’s there now isn’t it?

Ah you mean when I was testing before sure

core/src/main/resources/hudson/model/Computer/configure.jelly

Co-authored-by: Daniel Beck <[email protected]>

…temRead-agents

amuniz · 2020-05-13T13:33:34Z

I've created #4724 which will generate some merge conflicts with this. I'm happy to resolve conflicts there is this is merged first (or help to resolve conflicts here if needed).

timja · 2020-05-15T18:51:30Z

@daniel-beck would you be able to take another look please?

daniel-beck

Didn't fit as a line comment, so mentioning it here: It would be good if users with Overall/SystemRead were able to see the global cloud configuration that now only says

The cloud configuration has moved to a separate configuration page.

I see nothing wrong with this PR, even in manual testing. Thanks for adding proper Computer/ExtendedRead support.

core/src/main/resources/hudson/model/ComputerSet/sidepanel.jelly

core/src/main/resources/jenkins/model/GlobalCloudConfiguration/index.groovy

timja · 2020-05-19T15:24:04Z

Anyone else want to take a look at this?

daniel-beck

Re-:+1:

core/src/main/resources/hudson/model/ComputerSet/index.jelly

core/src/main/resources/jenkins/model/Jenkins/configure.jelly

daniel-beck

Re-:+1: assuming latest changes were manually tested.

oleg-nenashev · 2020-05-20T08:30:11Z

@oleg-nenashev to retest it

oleg-nenashev

Yesterday I spent some time to verify the PR, and it looks good to me. On my test instance the forms worked pretty well with Extended Read. I am +1 for merging and early release so that we could do crowd testing during the hackfest.

Cc @jenkinsci/core , is everybody fine with release on Monday with this fix?

JENKINS-61206 System read / Extended read for agents

cf86609

timja added web-ui The PR includes WebUI changes which may need special expertise developer Changes which impact plugin developers labels Feb 29, 2020

daniel-beck self-requested a review February 29, 2020 12:55

daniel-beck reviewed Feb 29, 2020

View reviewed changes

timja commented Mar 1, 2020

View reviewed changes

core/src/main/java/hudson/Functions.java Show resolved Hide resolved

Update core/src/main/java/hudson/Functions.java

35dba6f

timja closed this Mar 1, 2020

timja reopened this Mar 1, 2020

timja closed this Mar 5, 2020

timja reopened this Mar 5, 2020

timja added 3 commits March 6, 2020 21:04

Set permission to false explicitly

e00fa4f

Merge branch 'master' into systemRead-agents

b0a574d

Add message when no configured clouds

0ffa5fc

Otherwise there was a white page and it looked weird

daniel-beck self-requested a review March 6, 2020 23:06

timja added 2 commits April 25, 2020 08:29

Merge branch 'master' into systemRead-agents

7e8bb9b

Hide password for Computer.EXTENDED_READ

b7dd142

timja added 2 commits April 28, 2020 19:50

Merge branch 'master' into systemRead-agents

6e1eeaf

Add test

393ad7f

timja requested review from jvz and jeffret-b April 28, 2020 20:43

daniel-beck reviewed May 8, 2020

View reviewed changes

Use it instead of app

86de15b

daniel-beck reviewed May 8, 2020

View reviewed changes

core/src/main/resources/hudson/model/Computer/configure.jelly Outdated Show resolved Hide resolved

Update core/src/main/resources/hudson/model/Computer/configure.jelly

06d8455

Co-authored-by: Daniel Beck <[email protected]>

daniel-beck self-requested a review May 8, 2020 17:32

timja added 3 commits May 11, 2020 21:09

Merge branch 'master' into systemRead-agents

c0783af

Merge branch 'systemRead-agents' of github.com:timja/jenkins into sys…

47e31f5

…temRead-agents

Code simplification

31c35d6

This was referenced May 15, 2020

Read only jenkins announcement jenkins-infra/jenkins.io#3279

Merged

[JENKINS-61206] - Enable the _Agent/ExtendedRead_ permissions jenkinsci/extended-read-permission-plugin#13

Merged

daniel-beck approved these changes May 19, 2020

View reviewed changes

core/src/main/resources/hudson/model/ComputerSet/sidepanel.jelly Outdated Show resolved Hide resolved

core/src/main/resources/jenkins/model/GlobalCloudConfiguration/index.groovy Show resolved Hide resolved

Inline jelly text customisation

bbe611c

timja requested a review from a team May 19, 2020 15:23

Allow system read to see cloud move blurb

37c712b

daniel-beck approved these changes May 19, 2020

View reviewed changes

core/src/main/resources/hudson/model/ComputerSet/index.jelly Outdated Show resolved Hide resolved

core/src/main/resources/jenkins/model/Jenkins/configure.jelly Show resolved Hide resolved

Tooltip based on permission

1beda15

daniel-beck approved these changes May 19, 2020

View reviewed changes

oleg-nenashev self-requested a review May 20, 2020 08:30

oleg-nenashev approved these changes May 22, 2020

View reviewed changes

oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 22, 2020

timja merged commit 634004a into jenkinsci:master May 24, 2020

daniel-beck mentioned this pull request May 24, 2020

[JENKINS-62264] User with MANAGE permissions can access node monitoring #4724

Merged

10 tasks

oleg-nenashev changed the title ~~JENKINS-61206 System read / Extended read for agents~~ JENKINS-61206 - Support System Read / Extended read permissions for agent configurations May 25, 2020

daniel-beck mentioned this pull request Jul 27, 2020

Update at-since Javadoc #4877

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JENKINS-61206 - Support System Read / Extended read permissions for agent configurations #4531

JENKINS-61206 - Support System Read / Extended read permissions for agent configurations #4531

timja commented Feb 29, 2020

daniel-beck Feb 29, 2020

timja Feb 29, 2020

daniel-beck commented Apr 24, 2020

timja commented Apr 24, 2020

daniel-beck commented Apr 24, 2020

timja commented Apr 24, 2020

timja commented Apr 25, 2020

timja commented Apr 25, 2020 •

edited

Loading

daniel-beck commented Apr 25, 2020 •

edited

Loading

timja commented Apr 28, 2020

daniel-beck May 8, 2020 •

edited

Loading

timja May 8, 2020

timja May 8, 2020

daniel-beck May 8, 2020

timja May 8, 2020

timja May 8, 2020

amuniz commented May 13, 2020

timja commented May 15, 2020

daniel-beck left a comment

timja commented May 19, 2020

daniel-beck left a comment

daniel-beck left a comment

oleg-nenashev commented May 20, 2020

oleg-nenashev left a comment

	<j:when test="${app.hasPermission(it.CONFIGURE)}">
	<j:when test="${it.hasPermission(it.CONFIGURE)}">

JENKINS-61206 - Support System Read / Extended read permissions for agent configurations #4531

JENKINS-61206 - Support System Read / Extended read permissions for agent configurations #4531

Conversation

timja commented Feb 29, 2020

Manual testing notes

Proposed changelog entries

Proposed upgrade guidelines

Submitter checklist

Desired reviewers

Maintainer checklist

daniel-beck Feb 29, 2020

Choose a reason for hiding this comment

timja Feb 29, 2020

Choose a reason for hiding this comment

daniel-beck commented Apr 24, 2020

timja commented Apr 24, 2020

daniel-beck commented Apr 24, 2020

timja commented Apr 24, 2020

timja commented Apr 25, 2020

timja commented Apr 25, 2020 • edited Loading

daniel-beck commented Apr 25, 2020 • edited Loading

timja commented Apr 28, 2020

daniel-beck May 8, 2020 • edited Loading

Choose a reason for hiding this comment

timja May 8, 2020

Choose a reason for hiding this comment

timja May 8, 2020

Choose a reason for hiding this comment

daniel-beck May 8, 2020

Choose a reason for hiding this comment

timja May 8, 2020

Choose a reason for hiding this comment

timja May 8, 2020

Choose a reason for hiding this comment

amuniz commented May 13, 2020

timja commented May 15, 2020

daniel-beck left a comment

Choose a reason for hiding this comment

timja commented May 19, 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

oleg-nenashev commented May 20, 2020

oleg-nenashev left a comment

Choose a reason for hiding this comment

timja commented Apr 25, 2020 •

edited

Loading

daniel-beck commented Apr 25, 2020 •

edited

Loading

daniel-beck May 8, 2020 •

edited

Loading