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

[JENKINS-28975] Added support for View permissions at the View level rather than only at global level #21

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@pskumar448
Copy link

pskumar448 commented Aug 14, 2016

Added possibility to setup permissions at view level rather giving permissions at global level.

This will hide the View from top tab bar, but if the user try to access via direct URL it will be accessible.

New feature PR (jenkinsci/jenkins#2499) to has been submitted to Jenkins Core to handle direct URL authorization of the user on view.

https://issues.jenkins-ci.org/browse/JENKINS-28975

<!--
- The MIT License
-
- Copyright (c) 2013, Oleg Nenashev, Synopsys Inc.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 14, 2016

Member

Wrong license header

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 14, 2016

Member

Imho even the slave file needs some rework

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 14, 2016

OK. I will update the license header and push the changes.

@pskumar448 pskumar448 force-pushed the pskumar448:master branch from d8aa4d9 to ef8f4ed Aug 14, 2016

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 14, 2016

@oleg-nenashev
License headers for the files manage-view-roles.jelly and assign-view-roles.jelly updated and pushed changes.

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 15, 2016

@oleg-nenashev
Got any chance to look this PR.

@@ -592,6 +599,10 @@ else if (type.equals(SLAVE)) {
groups.remove(PermissionGroup.get(SCM.class));
groups.remove(PermissionGroup.get(Run.class));
}
else if (type.equals(VIEW)) {
groups = new ArrayList<PermissionGroup>();
groups.add(PermissionGroup.get(View.class));

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 15, 2016

Member

Blacklisting of groups above is made for a reason. If somebody adds a permission for view outside this group (e.g. MANAGE_OWNERSHIP.View), it won't appear in the table. For old plugins there is no way to add permissions to existing groups IIRC

This comment has been minimized.

Copy link
@pskumar448

pskumar448 Aug 15, 2016

Author

If I get all the PermissionGroups then removing other except the View.class but Credentials section is coming in table so to eliminate that section which is not having any importance created a new list with just View PermissionGroup.

This comment has been minimized.

Copy link
@pskumar448

pskumar448 Aug 16, 2016

Author

@oleg-nenashev
Shall I do it like the previous implementation for VIEW too as you said that it has been implemented for a reason.
Or else is this OK to add View.class only.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2016

Member

Let's keep it as is by now

This comment has been minimized.

Copy link
@pskumar448
@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 15, 2016

Generally looks good to me. Need to perform some testing

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 16, 2016

@oleg-nenashev
After testing this PR let me know any changes required.

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 19, 2016

@oleg-nenashev
Any update on this.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 19, 2016

I rarely do testing during the working week :(
Hope to spend some time on the weekend

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 20, 2016

The current permission check approach does not work correctly for Views in Folders Plugin. Seems you may have to emulate a kind of getFullName() method for views. And same for user custom views, for which the current behavior seems to be may be a security risk (needs core code review).

Also, I see the strange layout tabbing in Safari, which didn't use to happen before the patch (needs to be confirmed).

screen shot 2016-08-20 at 23 43 13

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 21, 2016

@oleg-nenashev
Ok. I will update with view full name for folder level views.

@pskumar448 pskumar448 force-pushed the pskumar448:master branch from ef8f4ed to 079b05c Aug 23, 2016

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 23, 2016

@oleg-nenashev
Emulated view full name to add support for folder level views. Let me know feedback on this after testing.

return getACL(VIEW, getViewFullName(view), RoleType.View, view);
}

String getViewFullName(View view) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 23, 2016

Member

This code will be invoked VERY frequently. It really makes sense to replace recursive call by a loop with StringBuilder

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 23, 2016

Ok. I will modify the according to your feedback.

@pskumar448 pskumar448 force-pushed the pskumar448:master branch from 079b05c to 97b90bf Aug 23, 2016

return view.getViewName();
}
else {
sb.append("/" + view.getViewName());

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 23, 2016

Member

The approach is not very safe in the case of slashes in the view name, which were allowed in old Jenkins versions, but it's a corner case we can ignore

This comment has been minimized.

Copy link
@pskumar448

pskumar448 Aug 23, 2016

Author

We are using slash to separate the view name from the parent name.
This functionality I have taken from the Jenkins core, referred some model classes how emulation of full name happening.


String getViewFullName(View view) {
StringBuilder sb = new StringBuilder();
sb.append(view.getOwnerItemGroup().getFullName());

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 23, 2016

Member

I'm not exactly sure what is going to happen for User private views here. Maybe it worth to create a couple of unit tests to address such cases. I can help with Role Strategy initialization framework for it

This comment has been minimized.

Copy link
@pskumar448

pskumar448 Aug 23, 2016

Author

Ok. I will try to understand how User private views works as a functionality. Do you have any information regarding the crucial functionality of the User private views in Jenkins.

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 27, 2016

Hi @oleg-nenashev
Could you let me know regarding the User private views and any foresee issues with these changes in role-strategy plugin. I am clueless here regarding the User private views.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Aug 29, 2016

@pskumar448
The issue is that one user with Jenkins.READ permission may be able to access personal views of another user if the role does not take users into account. E.g. "Foo" pattern may apply to "/Foo" and to "/user/oleg-nenashev/Foo" if you do not somehow resolve users

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Aug 30, 2016

OK. @oleg-nenashev
Thanks for giving me the scenario, I will try to fix this issue.

@pskumar448

This comment has been minimized.

Copy link
Author

pskumar448 commented Sep 29, 2016

@oleg-nenashev
I am working on this to fix the Views related to MyViews.

@remm

This comment has been minimized.

Copy link

remm commented Nov 15, 2016

Hi guys, could you help me?
Looks like this bug should be fixed with this pull request.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

oleg-nenashev commented Nov 18, 2016

@remm Responded in the ticket

@oleg-nenashev oleg-nenashev changed the title Added support for View permissions at the View level rather than only at global level [JENKINS-28975] Added support for View permissions at the View level rather than only at global level Jun 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.