search

jenkinsci / gitlab-plugin

A Jenkins plugin for interfacing with GitLab
https://plugins.jenkins.io/gitlab-plugin/
GNU General Public License v2.0
1.43k stars 613 forks source link

Fix for defect #695: permission check for multibranch pipelines #1544

Closed mlynnyk closed 1 year ago

mlynnyk commented 1 year ago

This pull request addresses this issue: https://github.com/jenkinsci/gitlab-plugin/issues/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.

krisstern commented 1 year ago

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.

krisstern commented 1 year ago

@mlynnyk Would you like to address the deprecations, or are you happy with the code as is?

mlynnyk commented 1 year ago

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.

krisstern commented 1 year ago

Hi @mlynnyk Sure, let me check things interactively one more time before merging this. Again, thank you for your contribution!

MattLud commented 1 month ago

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