[FIXED JENKINS-18884] Access check to jobs in People pages. #1102
Conversation
…e page. People page of AllView no longer lists all users. That feature should be provided by security realms.
|
core » jenkins-core #86 SUCCESS |
| } | ||
| } | ||
|
|
||
| // for testing purpose | ||
| /*package*/ Set<User> getModified() { |
There was a problem hiding this comment.
Maybe use @Restricted(NoExternalUse.class) here?
There was a problem hiding this comment.
I don't know much about access-modifier. Is it right to understand like this?:
NoExternalUsestands for "do not use from packages other thancore"- That is put to prevent access to
AsynchPeople#getModifiedfrom plugins. - Though
testpackage accesses toAsynchPeople#getModified, that doesn't matter foraccess-modifier-checkeris not configured fortestpackage.
|
core » jenkins-core #88 SUCCESS |
…r testing purpose.
| /*package*/ Set<User> getModified() { | ||
| return modified; | ||
| } | ||
|
|
There was a problem hiding this comment.
Added @Restricted(NoExternalUse.class) to prevent access from other packages (especially plugins).
Functions#printLogRecordHtmlandRun#previousBuildare also annotated with@Restricted(NoExternalUse.class)and accessed from test package.- test package does not fail even access-modifier-checker is added to pom.xml. It seems that access-modifier-checker does not check accesses from src/test directories.
There was a problem hiding this comment.
to prevent access from other packages
Doesn't work that way – there's already the default visibility for that. AFAIK it works like this: @Restricted(NoExternalUse.class) prevents use by e.g. plugins (that need to consider this annotation by using the access modifier checker that is part of the same component), even if they have classes in the same package. Nobody would really care about its visibility, as it can always be changed, including code that uses it – except, due to plugins, once it's accessible and released, someone might use it and removal will break that plugin. See Javadoc here, and the general documentation for this.
There was a problem hiding this comment.
Hmm, so it prevents access not from other packages, but from other modules. Is it right?
|
core » jenkins-core #117 SUCCESS |
|
In this approach the mapping of the user ID to the user name is still visible (for all the jobs a user has read permission). This should be actually prevented in JENKINS-18884. Can we additionally bind the visibility of the user ID property to the ADMINISTRATOR permission? Then in the people view only the user names of the "friend" projects are shown but not the IDs. |
|
How about introducing a new permission to access http://JENKINS/user/XXXX ? |
|
Yes, that would be a another good (and simple?) approach. Didn't you already implement that in your other pull request? (I'm still wondering though why it is required to see a user ID on any UI page, the user name should be sufficient.) |
|
No, I didn't. I also wonder those features are really required (including People page itself). |
|
You are right, it might be to risky to remove the user ID. |
|
People page could alternatively be extracted into a plugin, then users could disable it or (more easily) customize. And IMO it's easier to justify new permissions introduced in plugins. I see no reason to keep this in core. |
|
I filed a card for that idea. It is true that in a lot of installations the People view is undesirable; and I have had to do major work on it to make it perform acceptably. |
|
Why isn't it still merged? |
|
Still recommend this be pulled out into an optional plugin, which could then additionally define a new permission if there is a need for some users but not others to view the list of all users. |
|
My alternate recommendation now filed as JENKINS-26469. |
|
I'd rather agree with @jglick , but IMO it makes sense even in the current state |
oleg-nenashev
added
needs-more-reviews
|
What about the conflict @ikedam |
|
I don't plan to work for this issue anymore. |
|
Give new PR a try: #9052 |
People page displays users extracted from change logs of jobs belonging to that view.
And People page of All view also displays all users registered in Jenkins.
They do no permission check.
This patch does followings:
HudsonPrivateSecurityRealm.ManageUserLinks). If needed, I would implement a default user listing page for administrators.This is another approach of #1094 .
First I planned to add a new permission to restrict access to People page in All view, but I get to think that the root problem is the difference of behaviors of People pages between All view and other views.