Skip to content

Allow limited oauth scopes#45

Merged
samrocketman merged 1 commit into
masterfrom
allow_limited_scopes
Jul 24, 2015
Merged

Allow limited oauth scopes#45
samrocketman merged 1 commit into
masterfrom
allow_limited_scopes

Conversation

@samrocketman
Copy link
Copy Markdown
Member

@samrocketman samrocketman commented Jul 19, 2015

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.

cc @s0undt3ch, @sirosen, @cloudbeesci

@samrocketman samrocketman force-pushed the allow_limited_scopes branch from 1ced0b7 to 67eb146 Compare July 19, 2015 16:58
@samrocketman
Copy link
Copy Markdown
Member Author

samrocketman commented Jul 19, 2015

I amended the commit message for grammar.

@jenkinsadmin
Copy link
Copy Markdown
Member

jenkinsadmin commented Jul 19, 2015

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@samrocketman
Copy link
Copy Markdown
Member Author

samrocketman commented Jul 19, 2015

Peer review is welcome.

@s0undt3ch
Copy link
Copy Markdown

s0undt3ch commented Jul 19, 2015

👍 Thanks!

@samrocketman samrocketman force-pushed the allow_limited_scopes branch from 67eb146 to 9ff2882 Compare July 20, 2015 05:44
sirosen
sirosen reviewed Jul 20, 2015
View reviewed changes
Comment thread src/main/java/org/jenkinsci/plugins/GithubAuthenticationToken.java Outdated
Copy link
Copy Markdown
Contributor

@sirosen sirosen Jul 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional needs a comment.
If I were to come across this line while working on the code, I wouldn't have any idea what's being checked here.
To figure it out, I'd have to reach for the GitHub API docs and work backwards from "What are the common properties of these scopes?"

Probably something like "Check to see if we can read organizational and team memberships" would do the trick

@samrocketman samrocketman force-pushed the allow_limited_scopes branch from 9ff2882 to 65eecb0 Compare July 20, 2015 19:23
@samrocketman
Copy link
Copy Markdown
Member Author

samrocketman commented Jul 20, 2015

@sirosen I've updated the pull request to contain an additional scope as well as comments for what it is doing.

@sirosen
Copy link
Copy Markdown
Contributor

sirosen commented Jul 20, 2015

Awesome, that's exactly what I wanted. 👍
Other than that, this looks good up to my ability to read the code.

@samrocketman
Copy link
Copy Markdown
Member Author

samrocketman commented Jul 21, 2015

This should be merged after #47 to make use of the hasScope() function in GithubSecurityRealm.

@samrocketman samrocketman mentioned this pull request Jul 21, 2015
Merged
@samrocketman samrocketman modified the milestone: github-oauth-0.22 Jul 21, 2015
@samrocketman
Copy link
Copy Markdown
Member Author

samrocketman commented Jul 21, 2015

Perhaps make myRealm a private final variable of the GithubAuthenticationToken class. Check for null and set it before using hasScope().

@samrocketman samrocketman force-pushed the allow_limited_scopes branch from 65eecb0 to f51842d Compare July 23, 2015 23:55
@samrocketman
Allow limited oauth scopes
6ed061a 
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.
@samrocketman samrocketman force-pushed the allow_limited_scopes branch from f51842d to 6ed061a Compare July 24, 2015 01:36
@samrocketman samrocketman merged commit 6ed061a into master Jul 24, 2015
samrocketman added a commit that referenced this pull request Jul 24, 2015
@samrocketman
Merge PR #45 Allow limited oauth scopes
674ec3a
@samrocketman samrocketman deleted the allow_limited_scopes branch July 24, 2015 01:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

github-oauth-0.22

Development

Successfully merging this pull request may close these issues.

4 participants

@samrocketman @jenkinsadmin @s0undt3ch @sirosen