Skip to content

[JENKINS-70548] Allow GitHub Webhooks to be created by users with custom roles - #375

Closed
iodeslykos wants to merge 3 commits into
jenkinsci:masterfrom
iodeslykos:fix/JENKINS-70458/remove-admin-requirement-for-webhooks
Closed

[JENKINS-70548] Allow GitHub Webhooks to be created by users with custom roles#375
iodeslykos wants to merge 3 commits into
jenkinsci:masterfrom
iodeslykos:fix/JENKINS-70458/remove-admin-requirement-for-webhooks

Conversation

@iodeslykos

Copy link
Copy Markdown

Would really like for custom roles to be used in management of Webhooks, because otherwise Jenkins needs to have admin permissions on every repository where managed Webhooks are desired. This is my attempt at making this possible.

  • Remove admin check, allowedToManageHooks() is enough.

Intended to solve https://issues.jenkins.io/browse/JENKINS-70548.

Testing done

Passing tests and building successfully. A change in tests does not appear to be required.

[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  04:14 min
[INFO] Finished at: 2024-01-24T16:22:27-07:00
[INFO] ------------------------------------------------------------------------

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@iodeslykos

Copy link
Copy Markdown
Author

@lanwen Any chance of getting this reviewed and (hopefully) merged?

@mcmathews

Copy link
Copy Markdown

@KostyaSha would you mind taking a look at this? This feature would be very convenient in my team's permission structure.

@KostyaSha

Copy link
Copy Markdown
Member

We can try, in case of issues could be reverted back

@iodeslykos

Copy link
Copy Markdown
Author

@KostyaSha Any ETA on when it could be tried? This is a major factor for our effort to ensure principal of least privilege.

@KostyaSha

KostyaSha commented Feb 14, 2024

Copy link
Copy Markdown
Member

It would be better to move it as option. Admin check was added to identify that returned object will be able to manage hooks. Now it can return connection that will lead to errors. Also AFAIR it was impossible to make an github API check whether user can manage hook and i sent request to github support. AFAIR permissions were listed in http headers and github-api library didn't support it (now it already supports afair).

@iodeslykos

Copy link
Copy Markdown
Author

@KostyaSha I'm not certain how you'd want this to be implemented.

For instance, it doesn't look like the library being used for the GitHub API supports custom roles (would return UNKNOWN): https://github.com/hub4j/github-api/blob/51b3a06fe364a9667d8c4e0b64abc780cff8d371/src/main/java/org/kohsuke/github/GHPermissionType.java#L9C1-L20C17

Are you saying that the admin check should still happen after the webhook permission is confirmed and simply not cause failure?

@iodeslykos

Copy link
Copy Markdown
Author

I can't imagine this will ever be reviewed, so closing.

@iodeslykos iodeslykos closed this Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants