Restrict default OAuthScope to "read:org" #39
Conversation
The default scope for OAuth should arguably be as restricted as possible. The older default of "repo,read:org" requested push access to all of a user's repositories. For many users of the plugin, this was considered a deal-breaker before configurable OAuth Scopes. For admins who prefer setting these values through the web UI, it's better to start with a more restricted scope, as it means that the admin doesn't need to share push privileges with Jenkins at any point during server configuration. Plus, this is a better default for novice users who are not familiar with the GitHub OAuth docs and OAuth scoping.
|
That makes sense to me |
|
cc @jhoblitt |
|
Is there a way to not even have org in there? By disabling certain parts of the plugin which require it? I'm just using the plugin for authentication, I really don't want nothing to do with my users org's... |
|
@s0undt3ch github will issue an oauth token without a scope so in theory it should work -- have you tried it? |
|
That's what I was using and got a traceback because of the missing read:org, my guess is that this plugin always tries to pull org info? Because of its org based permissions feature? I'm not using that though hence me asking about not eve trying to fetch that data.... |
|
@s0undt3ch I confirm that at a minimum For now, |
|
Can't the plugin extend the required codes to include read:org only if org based permissions are enabled? Yes this can be considered a feature requiesy, unless, GitHub imposes that perm just for authentication? |
|
It's not because of a limitation on GitHub. It's how the OAuth plugin is getting the access token. Here's the actual stack trace. Notice the last two lines in that stack trace: |
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
@s0undt3ch the patch for
It needs to be something along the lines of: import jenkins.model.Jenkins;
...
if(Jenkins.instance.getSecurityRealm() instanceof GithubSecurityRealm
&& (Jenkins.instance.getSecurityRealm().oauthScopes.indexOf("read:org") >= 0
|| Jenkins.instance.getSecurityRealm().oauthScopes.indexOf("admin:org") >= 0)) {
for (String name : gh.getMyOrganizations().keySet()) {
authorities.add(new GrantedAuthorityImpl(name));
}
}I tried writing my own patch but I'm still too new to plugin development to write something that could compile. I'm not sure how to access the current security realm instance to pull out |
|
@sirosen: as @jhoblitt mentioned you should update |
|
the less permissions to operate the better, asking for a lot is a real roadblock for some people, so 👍 to concept (not sure of limitations or compatibility problems though). |
|
As a data point, I'm using |
|
Per #39 (comment), I've opened PR #40 to fix a wide range of whitespace and formatting issues. |
The GitHub default OAuth Scope lives in two places -- in the GithubSecurityRealm definition, and in the jelly file used to build the web UI for setting scope. In addition to restricting it to read:org in the class definition, it needs to be restricted as such in the jelly.
|
I'm also using "read:org" on a live server, so I know that it works. To me, the inclusion of "repo" is a clear and obvious mistake, while whether or not "read:org" should be the default (which could be removed by the admin after the fact, of course) is up for debate. |
|
I'm mostly after the required support to just have |
|
@s0undt3ch I believe the |
|
That's my point, I'm not using it, so, I should not be forced to use If an admin chooses to use the GH authorization strategy, and he does not have that perm, he'll see the traceback, or, that component adds that perm to the list. Basically, it needs to be configurable... |
|
@s0undt3ch it's already configurable with #35. The sane default should be Nobody said anything about patching every release. Patch it and create a pull request. |
|
@s0undt3ch there's two issues that I want to clarify. I think they're both distinct.
|
|
Agreed on both. |
|
I want to make sure I'm not blocking this getting merged. As far as I can tell, the conclusion of the discussion here between @s0undt3ch and @samrocketman is that supporting more restrictive scopes is an issue, but not to be addressed here. Anything I've missed? |
|
@sirosen nope, I think you're good. I'll merge it the next chance I get. |
If using the GitHub OAuth plugin for authentication only, then it doesn't make sense to query GitHub for Organization membership since it will not be using GitHub OAuth for authorization. This stems from a discussion in pull request #39.
The default scope for OAuth should arguably be as restricted as possible. The older default of "repo,read:org" requested push access to all of a user's repositories. For many users of the plugin, this was considered a deal-breaker before configurable OAuth Scopes, so there's no reason to assume that most users would want this moving forward.
For admins who prefer setting these values through the web UI, it's better to start with a more restricted scope, as it means that the admin doesn't need to share push privileges with Jenkins at any point during server configuration. This is also a better default for novice users who are not familiar with the GitHub OAuth docs and OAuth scoping.
This is, of course, a follow-up to discussions that we had with #35