Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

iodeslykos
Copy link

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
Author

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

@mcmathews
Copy link

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

@KostyaSha
Copy link
Member

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

@iodeslykos
Copy link
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
Copy link
Member

KostyaSha commented Feb 14, 2024

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
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?

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