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

Fix incorrect permission checks in KubernetesComputer views #834

Merged

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Sep 1, 2020

#367 added views for KubernetesComputer. The backends methods need administer role however the views are not protected. They don't display stacktraces since #430 however, the links shouldn't even be there.

Noting #657 is proposing to require EXTENDED_READ only. Will likely be affected by this PR.

@Vlatombe Vlatombe added the bug Bug Fixes label Sep 1, 2020
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Theoretically these could be gated by Computer.CONFIGURE, in case some non-admin user has permission to manage nodes, though practically that would not be useful for Cloud-provisioned nodes since typical AuthorizationStrategys would not let you set up distinct permissions on those anyway. So I think this is fine. These are basically all r/o views anyway right?

@Vlatombe
Copy link
Member Author

Vlatombe commented Sep 1, 2020

Theoretically these could be gated by Computer.CONFIGURE

FTR, Configure is explicitly denied on these ephemeral nodes to prevent access to the configuration page.

@Vlatombe
Copy link
Member Author

Vlatombe commented Sep 1, 2020

These are basically all r/o views anyway right?

Yes, I was reluctant to #657 because of the possible secret exposure.

@Vlatombe Vlatombe merged commit c78197f into jenkinsci:master Sep 1, 2020
@Vlatombe Vlatombe deleted the missing-permission-checks-computer branch September 1, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants