Skip to content

Add oauth 2.0 state field as a parameter to requestAccessUri #58

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

Merged
merged 1 commit into from
Oct 3, 2016
Merged

Add oauth 2.0 state field as a parameter to requestAccessUri #58

merged 1 commit into from
Oct 3, 2016

Conversation

bschwind
Copy link
Contributor

@shunjikonishi please review

In order for an application to properly check the oauth state parameter, it needs to generate the field by itself and pass it in to the OAuth URL generation function. This is similar to how facebook4j handles it: http://facebook4j.org/javadoc/facebook4j/auth/OAuthSupport.html#getOAuthAuthorizationURL(java.lang.String, java.lang.String)

sbt test was failing for me before I made any changes, so I'm not sure if this breaks any tests or not.

@shunjikonishi
Copy link
Contributor

@bschwind fix me

Sorry. sbt test doesn't work now. Because some tests depends on GitHub account status.
I was aware it at my previous fixing, but not investigated well.

About this PR, please modify method signature to use Option.

def requestAccessUri(state: Option[String] = None, scope: String*) = {

because your implementation might cause dangerous situation.
For example, basic use case of requestAccessUri is like this.

     val url = oauth.requestAccessUri("user", "repo")

Above code specifies "user" and "repo" for scope.

However, after your modification, above code means state="user", scope="repo"!!!

and to make matter worse, there is no chance for developers to be aware of this bug.

To use Option and default parameter, I think we can avoid this situation.

@bschwind
Copy link
Contributor Author

bschwind commented Oct 3, 2016

@shunjikonishi Unfortunately I don't think Scala allows that kind of method signature

a parameter section with a `*'-parameter is not allowed to have default arguments
def requestAccessUri(state: Option[String] = None, scope: String*) = {

Do you think I should just make an overloaded method that takes a state parameter instead?

@bschwind
Copy link
Contributor Author

bschwind commented Oct 3, 2016

Actually, overloading will also be confusing, so I think a new method with a different name is probably the best way to approach this.

@shunjikonishi
Copy link
Contributor

Ah, sorry. please remove default parameter.

def requestAccessUri(state: Option[String], scope: String*) = {

We can detect the problem by compile error, when we update lib.

@bschwind
Copy link
Contributor Author

bschwind commented Oct 3, 2016

@shunjikonishi does anyone outside of givery use this library?

@shunjikonishi
Copy link
Contributor

I don't know. But probably not. (This library is not published to maven-central or typesafe)

@bschwind
Copy link
Contributor Author

bschwind commented Oct 3, 2016

@shunjikonishi I think it's better to not touch the original method and just add an overloaded version. That way we won't break any older users of this library, and we can start using the state parameter.

@shunjikonishi
Copy link
Contributor

I'll merge this.

LGTM

@shunjikonishi shunjikonishi merged commit 4d1a4c8 into code-check:master Oct 3, 2016
@bschwind bschwind deleted the oauth-state branch October 3, 2016 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants