JENKINS-50122 Fix rights management for workflow jobs #114
Conversation
Workflow jobs does not have the BranchJobProperty set but multibranchproject has it. The previous code filtered all workflow jobs not having this property making the plugin GitHub Authorization Settings not working for those workflow jobs (all simple pipelines).
So in order to make multibranch build cancellable we also need to take those jobs into account. Those jobs don't have SCMs setup so we need to get the scm from the BranchJobProperty like it was done before. So, now we check if the BranchJobProperty is set and do the adequate commands to get the scm. Also, added tests for CANCEL permission which was never tested.
|
@samrocketman can you take a look on this when you have time? From my point of view, this fix isn't very risky since it only take care of a specific case. Most of the code modified are unit tests. Thanks |
There was a problem hiding this comment.
I haven't had a chance to fully look at this but I notice this duplicates #99
Have you tested this in your own Jenkins instance or a test instance?
src/test/java/org/jenkinsci/plugins/GithubRequireOrganizationMembershipACLTest.java
Outdated
Show resolved
Hide resolved
I actually use it on our production jenkins instance (instance I recreated a few times from scratch with helm + JCasC). I installed the fix before creating this PR, so it has been a few months now. It is mostly a rewrite of the old #99 PR because it wasn't working with the last changes from current master and I also took into account the reviews. |
|
Is the wildcard import the only blocker? I would really love to see this fixed 😀 |
|
I’m not as familiar with these parts of the code base so I need to test it more. I also need to dig into the blame logs to figure out why these conditionals were added to begin with. From my perspective the change makes sense I just don’t understand why they had the conditionals. Edit: The last plugin release was basically “make this plugin no longer completely broken for everyone” so I tried limiting what went out. Now that it’s stable I can look at this again. |
|
Is there something I can do to fix this issue? This is a big issue for me. |
|
Is this still missing something? Can I help with something do get this merged? |
|
I fixed what @samrocketman wanted I believe. @emoreth, I don't think you should wait for this to be merged. You will be disappointed. I made the fix the 14 may then I went to the jenkins mailing lists to ask for it to be merged without success. One year ago, @masterzen already proposed this fix in a PR but it was never merged either. What I ended up doing is building the hpi with the patch and install it directly on my jenkins. You can do it by dropping it in the plugins directory and restart your jenkins or using the UI (Manage jenkins => Manage plugins => advanced tab => upload plugin). Here is the HPI(remove the .TXT extension) with the patch if you don't want to build it: |
|
Well, that's bad! 😞 Thank you for your help @jpigree ! |
|
Hello, I can take a look at this again. Apologies for the delays I know you worked hard on this. |
|
Any updates @samrocketman ? |
|
Hey @jpigree
Is this true? We have multibranch pipeline and non admin users can't cancel builds
Could you point out how can I install custom plugin in my jenkins instance? TIA |
Normal workflow jobs does not have the BranchJobProperty set. Only those created by multibranch pipelines have it.
However, the previous code filtered all workflow jobs which hadn't this property when searching for their declared repository (in order to match the github repository permissions to the job).
The result is the plugin GitHub Authorization Settings denies 'CANCEL' permission to non admin users for all normal workflow jobs since it can't find their repository names.
This PR is a simplified version of the (very) old PR #99 by @masterzen. If people are interested, I can create another PR to support permission checks on every repositories declared in pipelines like he did in his PR. But I prefered to keep things as simple as possible to increase chances of a quick merge.
I just deployed it on my jenkins instance and it works fine but I would prefer to install an official version.