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.

Sign up for GitHub

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

Jump to bottom

Fix permission denied on test connection for non admin #1439

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Fix permission denied on test connection for non admin #1439

merged 1 commit into from
Mar 29, 2023

Conversation

mat1e
Copy link
Member

@mat1e mat1e commented Mar 29, 2023

See #1198
Fix "Access Denied" error message when non admin user click on "Test Connection" button for the Gitlab connection on a Job.

@mat1e mat1e requested a review from a team as a code owner March 29, 2023 10:36
krisstern
krisstern approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@krisstern krisstern left a comment

Choose a reason for hiding this comment

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

Thanks @mat1e! Looks good.

@krisstern krisstern merged commit 3438d18 into jenkinsci:master Mar 29, 2023
@krisstern krisstern added the bug For changelog: Minor bug. Will be listed after features label Mar 29, 2023
@krisstern krisstern changed the title fix permission denied on test connection for non admin Fix permission denied on test connection for non admin Mar 29, 2023
@daniel-beck
Copy link
Member

This change makes no sense and should not be released. Explicitly checking for Overall/Read is extremely rarely needed (basically only in UnprotectedRootAction), since Jenkins takes care of this, and checking for this permission on an Item is never correct. I expect this will not work with non-trivial authorization strategies like matrix-auth, at least in some configurations.

Instead, check item.hasPermission(Item.CONFIGURE). See https://www.jenkins.io/doc/developer/security/form-validation/#checking-permissions

krisstern added a commit that referenced this pull request Mar 29, 2023
@krisstern
Revert "fix permission denied on test connection for non admin (#1439)"
e0c1ced 
This reverts commit 3438d18.
@krisstern krisstern mentioned this pull request Mar 29, 2023
Merged
@krisstern
Copy link
Member

Thanks @daniel-beck for the info, I have opened #1440 to correct this behavior.

MarkEWaite pushed a commit that referenced this pull request Mar 29, 2023
@krisstern
Correct "Fix permission denied on test connection for non admin" PR (#…
f1ecd8d 
…1440)

* Revert "fix permission denied on test connection for non admin (#1439)"

This reverts commit 3438d18.

* Update src/main/java/com/dabsquared/gitlabjenkins/connection/GitLabConnectionProperty.java
@mat1e
Copy link
Member Author

mat1e commented Mar 30, 2023

Thanks for your review !

@krisstern
Copy link
Member

You are welcome @mat1e

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krisstern krisstern krisstern approved these changes

Assignees
No one assigned
Labels
bug For changelog: Minor bug. Will be listed after features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mat1e @daniel-beck @krisstern