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

Use current context for validation functions #1608

Merged

Conversation

Dohbedoh
Copy link
Contributor

@Dohbedoh Dohbedoh commented Sep 24, 2024

Following #746. Using the context is useful in CloudBees plugin that allow managing Kubernetes Cloud within folders.

cc @Vlatombe @jglick

Testing done

  • Tested creation of new cloud under Manage Jenkins > Cloud
  • Tested configuration of new cloud under Manage Jenkins > Cloud
  • Tested creation of new Kubernetes Shared Cloud in a folder (CloudBees instance)
  • Tested configuration of new Kubernetes Shared Cloud in a folder (CloudBees instance)

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@Dohbedoh Dohbedoh requested a review from a team as a code owner September 24, 2024 03:29
@@ -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);
Copy link
Contributor Author

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 ?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jglick @Vlatombe after digging a bit about this, the remaining Overall/Administer checks were added in #1580 but apparently we ought to allow users with Overall/Manage since #1546.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Contributor Author

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

@Vlatombe Vlatombe added the enhancement Improvements label Oct 10, 2024
@Vlatombe Vlatombe merged commit 11898cf into jenkinsci:master Oct 10, 2024
8 checks passed
@Dohbedoh Dohbedoh deleted the improvement/testconnection-context branch October 25, 2024 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants