-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use current context for validation functions #1608
Use current context for validation functions #1608
Conversation
@@ -1120,7 +1133,7 @@ public FormValidation doCheckDirectConnection( | |||
private static boolean hasPermission(AccessControlled owner) { | |||
if (owner instanceof Jenkins) { | |||
// Regular cloud | |||
return owner.hasPermission(Jenkins.ADMINISTER); | |||
return owner.hasPermission(Jenkins.MANAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Vlatombe I could not find the reason for a requirement for Jenkins.ADMINISTER
instead of Jenkins.MANAGE
here. Seems to me that Jenkins.MANAGE
is restrictive enough ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MANAGE
does not give you permission to configure clouds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this line should be reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1546 is about letting Managers manage pod templates, but we don't let them edit other Kubernetes cloud configurations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, users with only Manage cannot configure Jenkins.clouds
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But per #1546 it looks like we only check on the Manage permissions. The only reason this is still guarded by Overall/Administer
is because core does not allow non admins to access the Cloud configuration page.
In anyway, I am following your recommendation here, and changing to Overall/Administer
Following #746. Using the context is useful in CloudBees plugin that allow managing Kubernetes Cloud within folders.
cc @Vlatombe @jglick
Testing done
Submitter checklist