github-oauth-plugin icon indicating copy to clipboard operation
github-oauth-plugin copied to clipboard

JENKINS-50122 Fix rights management for workflow jobs

Open jpigree opened this issue 6 years ago • 11 comments

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 https://github.com/jenkinsci/github-oauth-plugin/pull/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.

jpigree avatar May 15 '19 06:05 jpigree

@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

jpigree avatar May 15 '19 20:05 jpigree

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?

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.

jpigree avatar Aug 07 '19 02:08 jpigree

Is the wildcard import the only blocker? I'm mostly into python, but if this is the only blocker I can try to setup a java environment here to fix it.

I would really love to see this fixed 😀

emoreth avatar Aug 21 '19 17:08 emoreth

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.

samrocketman avatar Aug 21 '19 18:08 samrocketman

Is there something I can do to fix this issue? This is a big issue for me.

emoreth avatar Aug 27 '19 19:08 emoreth

Is this still missing something? Can I help with something do get this merged?

emoreth avatar Oct 04 '19 11:10 emoreth

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: github-oauth.hpi.TXT

jpigree avatar Oct 04 '19 18:10 jpigree

Well, that's bad! 😞

Thank you for your help @jpigree !

emoreth avatar Oct 08 '19 16:10 emoreth

Hello, I can take a look at this again. Apologies for the delays I know you worked hard on this.

samrocketman avatar Oct 10 '19 13:10 samrocketman

Any updates @samrocketman ? I do prefer an official release but I can use the suggested alternative if you don't have the time.

emoreth avatar Oct 22 '19 19:10 emoreth

Hey @jpigree

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.

Is this true? We have multibranch pipeline and non admin users can't cancel builds

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.

Could you point out how can I install custom plugin in my jenkins instance?

TIA

adis-io avatar Dec 07 '23 15:12 adis-io