Fix for [JENKINS-54031] GitHub OAuth plugin fails with Jenkins 2.146

[sgtcoolguy]

JIRA: https://issues.jenkins-ci.org/browse/JENKINS-54031

This is an attempt to fix permissions issues introduced in the last LTS patch release of Jenkins. According to comments from @daniel-beck it appears this plugin isn't handling Item.DISCOVER permissions properly as a sub-set of READ.

I tried to clean up the code a little to make it easier to follow; add some tests around anonymous reads of the github-webhook and cc tray; make use of the Permission.impliedBy property to iterate up the hierarchy.

The notable change in behavior here is that this now treats DISCOVER permissions request like READ; users with write access to a repo can still cancel builds, but cannot view configuration anymore.

I'm not entirely sure if this changes actual behavior/rights for users when "allowing authenticated user to create jobs" - I think it now limits permissions solely to Item.CREATE when there's no repository name (whereas before it allowed read/configure/delete/view config/cancel). I'm not sure what's the right permission set to allow in this case, maybe if an authenticated user can create jobs they should have rights to CREATE/CONFIGURE/EXTENDED_READ/DELETE for all jobs too?

[samrocketman]

Thanks, I’ll take a look this week. Currently traveling.

[sgtcoolguy]

FYI, tried this out on my team's Jenkins at version 2.138.2 and seems to work for us.

[alock]

Any update on this?

[samrocketman]

Haven’t had a chance to run through the motions of upgrade, test, and release. When I do you’ll see this merged. I also typically comment my test results and process.

[kfarnung]

There's definitely work to do here, I have "allow anonymous read" enabled on my instance and with this fix the "Credentials" view is available without any authentication. Not an issue with the published version of the plugin.

I'd recommend taking a step back and making a smaller change, there's a lot going on here and I think a more targeted change would be better given the critical nature of authentication.

[sgtcoolguy]

I pushed an update that did not try to resolve permissions up to Permission.READ, but to the specific Item.READ/Item.BUILD/Item.CREATE/Item.WORKSPACE and any sub-permissions.

The general issue here is that this plugin should be "aware" of the impliedBy hierarchy of permissions. I was checking too high-level for Permission.READ, which meant it allowed for any permission that extended from that global one (like reading credentials!). So I think the move to the specific Item level permissions and any extending those should be correct. (I did verify locally that this change no longer exposed the Credentials link at all).

[kfarnung]

Can we add a new test that covers the scenario? I agree that this is the better longterm fix, I just worry about the scale of the changes with existing test collateral.

[joshpollara]

Is there an update here? Thanks.

[samrocketman]

Can you rebase this PR on the latest master and build your refactor on top of it?

[sgtcoolguy]

I'd prefer to close this one, as the immediate fix for the Jenkins 2.46 issue was already merged in a simpler way; and my PR #106 includes the updated test suite and a more comprehensive refactoring to try and streamline the auth flow to reduce API calls and cache as much as possible.