Fix for defect #695: permission check for multibranch pipelines #1544
Conversation
…r multibranch pipelines
mlynnyk
changed the title
|
Thanks @mlynnyk for you code contribution! I will review your PR over the next few days and merge once it is approved and we are ready to do so. |
| (GitLabConnectionConfig) Jenkins.get().getDescriptor(GitLabConnectionConfig.class); | ||
| if (gitlabConfig != null) { | ||
| if (gitlabConfig.isUseAuthenticatedEndpoint()) { | ||
| if (!project.getACL().hasPermission(authentication, permission)) { |
There was a problem hiding this comment.
'hasPermission(org.acegisecurity.Authentication, hudson.security.Permission)' is deprecated, suggest using https://javadoc.jenkins-ci.org/hudson/security/AccessControlled.html#hasPermission2(org.springframework.security.core.Authentication,hudson.security.Permission) instead.
| Assert.assertThrows( | ||
| HttpResponses.HttpResponseException.class, | ||
| () -> new PushBuildAction(item, getJson("PushEvent.json"), null).execute(response)); | ||
| item.onSCMSourceUpdated(source); |
There was a problem hiding this comment.
'onSCMSourceUpdated(jenkins.scm.api.SCMSource)' is deprecated, implementations of SCMSourceOwner would prefer the SCMEventListener extension point which allows for more fine-grained response to events, so prefer delivering event notification through SCMHeadEvent.fireNow(SCMHeadEvent), SCMSourceEvent.fireNow(SCMSourceEvent) or SCMNavigatorEvent.fireNow(SCMNavigatorEvent) as appropriate.
| throw HttpResponses.ok(); | ||
| } | ||
| throw HttpResponses.errorWithoutStack(409, "Push Hook is not supported for this project"); | ||
| } | ||
|
|
||
| private class SCMSourceOwnerNotifier implements Runnable { | ||
| private final Authentication authentication; |
There was a problem hiding this comment.
'org.acegisecurity.Authentication' is deprecated.
| throw HttpResponses.ok(); | ||
| } | ||
| throw HttpResponses.errorWithoutStack(409, "Push Hook is not supported for this project"); | ||
| } | ||
|
|
||
| private class SCMSourceOwnerNotifier implements Runnable { | ||
| private final Authentication authentication; | ||
|
|
||
| public SCMSourceOwnerNotifier(Authentication authentication) { |
There was a problem hiding this comment.
'org.acegisecurity.Authentication' is deprecated.
|
@mlynnyk Would you like to address the deprecations, or are you happy with the code as is? |
|
Hi @krisstern , thank you for reviewing this and providing your feedback. I would say that the matter of addressing these deprecation issues appears to be somewhat wider than this particular code change. For example, when replacing "org.acegisecurity.Authentication" with "org.springframework.security.core.Authentication" (I believe that's the correct one) it would make sense to update other classes using it and eliminate its usage completely. When I added this PR, I merely wanted to fix the defect while keeping the code more or less consistent with the rest of the implementation. I don't really want to get involved with a wider refactoring, this appears to be a whole separate task. Also, I am currently running the plugin with this code change on a production environment and it works perfectly well, so it has been tested. Because of these reasons I would prefer to leave this code as it is now. |
|
Hi @mlynnyk Sure, let me check things interactively one more time before merging this. Again, thank you for your contribution! |
|
Hi there - as a follow up to this PR, does the existing Multibranch documentation still apply? It seems like tokens won't be used going forward (nor were they ever used?) and the official doc should request use of Basic Auth parameters. https://github.com/jenkinsci/gitlab-plugin?tab=readme-ov-file#pipeline-multibranch-jobs-1 |
This pull request addresses this issue: #695 . The discussion does provide a detailed description of the defect.
While it is not a critical security problem, the issue does, in fact, results in allowing a user to perform branch indexing action, which is normally restring by the "Job/Build" permission.
Having the plugin with the code change installed, and trying to trigger a webhook against a multibranch pipeline would return not 200 (which is current behavior in all cases) but the correct "Error 403 anonymous is missing the Job/Build permission" response. Likewise, having a Webhook configured with basic auth ( http://username:apitoken@my_jenkins.com:8080/project/pipeline_test/ ) when user "username" does not have sufficient permissions, would result in "Error 403 username is missing the Job/Build permission", which, again, should be the correct behavior.