OAuth2PasswordGrant should parse refresh token

[damienrambout]

Hi,

I'm using OAuth2PasswordGrant in my project and I don't think the "password" flow is fully implemented.

First, OAuth2PasswordGrant will try to use the authorization_uri instead of the token_uri to request and access_token using the username and password. (This can be hacked by using the a token uri as the authorization_uri value).

Second, I believe all grant types allow to refresh access_token using the refresh_token. I just see this implemented in OAuth2CodeGrant. I think all the refresh logic (refreshToken, doRefreshToken(), ...) should be shared with OAuth2PasswordGrant.

Cheers.

[p2]

Yes, you're correct with both observations. Thanks!

On 14 Sep 2015, at 23:40, Damien Rambout notifications@github.com wrote:

Hi,

I'm using OAuth2PasswordGrant in my project and I don't think the "password" flow is fully implemented.

First, OAuth2PasswordGrant will try to use the authorization_uri instead of the token_uri to request and access_token using the username and password. (This can be hacked by using the a token uri as the authorization_uri value).

Second, I believe all grant types allow to refresh access_token using the refresh_token. I just see this implemented in OAuth2CodeGrant. I think all the refresh logic (refreshToken, doRefreshToken(), ...) should be shared with OAuth2PasswordGrant.

Cheers.

—
Reply to this email directly or view it on GitHub.

[p2]

So for your first issue, this is merely a question of wording. The password grant (and the OAuth2 base class in general) only has an authURL property, because the implicit and password grants only perform one round trip. This is usually called the authorization server, hence authorize_uri is factually correct. The token_uri is only available on the code grant flow.

[damienrambout]

@p2 after re-reading the IETF specs, you are absolutely right about the authorize_uri, sorry about that :)

[p2]

No worries, I was confused as well since it's the endpoint that gives you a token.

[damienrambout]

@p2, any news (ETA) about the refresh token issue?

[p2]

I have unfinished work on another machine, was sidetracked by a release that happened yesterday. Hope to resume soon.

[damienrambout]

Ok nice ! 👍 Thanks

[glennschmidt]

Not to be argumentative, but I disagree that it should use the parameter name authorize_uri to describe the location where the token is retrieved from. I think this will be very confusing for people.

The RFC says that an OAuth 2.0 server has to implement two endpoints - one is called the 'authorization' endpoint, and the other the 'token' endpoint.

The 'authorization' endpoint is only used for the 'authorization code' and 'implicit' grant flows. It presents a login form to the user, then redirects back to an application.

The 'token' endpoint is used for all other grant types, and it is a REST/JSON endpoint.

So the endpoints have quite different functions. It makes sense for an OAuth client library to accept configuration of both endpoint URLs, then pick the appropriate one automatically for the grant type that is being used. When you have config parameters named authorize_uri and token_uri, naturally people will assume these correspond to the two endpoints.

https://tools.ietf.org/html/rfc6749#section-3

Authorization endpoint - used by the client to obtain authorization from the resource owner via user-agent redirection

https://tools.ietf.org/html/rfc6749#section-3.2

The token endpoint is used with every authorization grant except for the implicit grant type

Just my two cents.

[p2]

Yes, that's great reasoning, agreed. Damien can rename the issue back to the original title, then. :)

Migrating the refresh token mechanism requires a bit of a rewrite to be clean and I'm stuck halfway through, still busy with other projects, so it will be a while.

[glennschmidt]

I've committed a fix for this for my own use, thought i would send a PR in case it helps. Not sure if you want to refactor further though.

[p2]

Yes, that's about what I have so far as well. I'm not quite happy with the bloat it adds to the base class, but since I won't get to refactoring too soon that's good enough, thanks!

[p2]

Thanks to @glennschmidt this has been fixed in #52 I presume. Can you confirm, @damienrambout ?

[damienrambout]

Sorry for the late answer. I guess this should solve my problem :)
Unfortunately, I have not been able to try it because when I try to install it using pod with a specific commit SHA, the whole SwiftKeychain part is not loaded. So the pod does not compile anymore.
Maybe I'll try cloning the whole project to try it.

Anyway, I think you could release a new version including this pull request.

[p2]

I'm still doing some rewriting on the develop branch which also incorporates this merge, just don't have the time ATM to properly finish everything.

As for the Pod issue, would it work if you added SwiftKeychain as a separate Pod?

[damienrambout]

It won't work adding SwiftKeychain as a separate pod because Keychain and ArchiveKey are directly referenced inside p2.Oauth2, without importing SwiftKeychain.

You might want to add a dependency to SwiftKeychain in your podspec, and then add import SwiftKeychain statements where needed. This will also prevent future conflict issues when people will want to add SwiftKeychain as a separate pod as well.

[damienrambout]

Right now, I should be able to test it using a local pod based on a git submodule.

[p2]

Pod dependency would be good, but I have to see how that plays with compiling the module where the Keychain classes are compiled within (which I prefer since I don't use Pods myself).

Would be cool if you could report your testing.

[damienrambout]

I've been testing the refresh feature (i.e. calling authorize()) on a OAuth2PasswordGrant value and it fails throwing OAuth2IncompleteSetup.NoUsername.

I've been investigating and basically it occurs because tryToObtainAccessTokenIfNeeded() fails and the fallback is obtainAccessToken(). So why is tryToObtainAccessTokenIfNeeded() failing?
The error thrown by tryToObtainAccessTokenIfNeeded() (inside OAuth2.swift) state that no redirect_uri has been defined (see authorizeURLWithBase(base:, redirect:, scope:, responseType:, params:)). Which is true, but should not be a problem since OAuth2PasswordGrant does not require a redirect_uri.

Basically, this can be solved either by removing the lines checking for redirect_uri. Or an overridable property requiresRedirectURI (or the name you want) could be defined on OAuth2 (true by default, and false for OAuth2PasswordGrant), and then check this property at runtime before throwing an error. You could also perform a check at initialization time.

[p2]

Thanks for testing! Yes, possible that these things happen on the latest develop branch, I've moved quite some things and haven't tested all scenarios. Will take a look.

On Nov 24, 2015, at 06:52, Damien Rambout notifications@github.com wrote:

I've been testing the refresh feature (i.e. calling authorize()) on a OAuth2PasswordGrant value and it fails throwing OAuth2IncompleteSetup.NoUsername.

I've been investigating and basically it occurs because tryToObtainAccessTokenIfNeeded() fails and the fallback is obtainAccessToken(). So why is tryToObtainAccessTokenIfNeeded() failing?
The error thrown by tryToObtainAccessTokenIfNeeded() (inside OAuth2.swift) state that no redirect_uri has been defined (see authorizeURLWithBase(base:, redirect:, scope:, responseType:, params:)). Which is true, but should not be a problem since OAuth2PasswordGrant does not require a redirect_uri.

Basically, this can be solved either by removing the lines checking for redirect_uri. Or an overridable property requiresRedirectURI (or the name you want) could be defined on OAuth2 (true by default, and false for OAuth2PasswordGrant), and then check this property at runtime before throwing an error. You could also perform a check at initialization time.

—
Reply to this email directly or view it on GitHub.

[p2]

I've pushed some changes, one is to make SwiftKeychain a dependency in CoocaPods, the other one to not require redirect_url when refreshing a token. I'm not sure how to test the CocoaPods update without pushing, and I don't have a password grant endpoint to test the refresh. If you get a chance let me know how it works!

[p2]

It would work if changed as follows, but SwiftKeychain currently doesn't define an OS X target, hence I cannot use the approach of a dependent Pod.

s.pod_target_xcconfig = { 'OTHER_SWIFT_FLAGS' => '-DIMPORT_SWIFT_KEYCHAIN' }

[tompson]

just wanted to say that I am also looking forward for the refreshToken support in OAuth2PasswordGrant

[p2]

No worries about the duplicate. ;) You can try the develop branch, it's already implemented there but I'll do more testing.

On Dec 2, 2015, at 07:29, Thomas Einwaller notifications@github.com wrote:

just wanted to say that I am also looking forward for the refreshToken support in OAuth2PasswordGrant

—
Reply to this email directly or view it on GitHub.

[tompson]

will try the develop branch, thanks!

[tompson]

what would be the best/easiest way to switch from cocoapod to the develop branch?

[p2]

Not a CocoaPods guy myself, but looking at the docs it seems something like this could work:

pod 'p2.OAuth2', :git => 'https://github.com/p2/OAuth2.git', :branch => 'develop'

[tompson]

thanks again - did not know that is is possible to select the branches in cocoapods - should have RTFM ;-)

[p2]

Haven't tested, so I don't know how well it works.

[tompson]

has problem because the SwiftKeychain stuff is not correctly added to the project

[p2]

Ah right; I've just pushed a change to make it a dependency on iOS, can you try again?

[tompson]

ok, that works, got it running again

now I run into the problem that assureMatchesState fails because no state is sent in my token response

[p2]

Thanks a lot for testing. Let me fix that...

[p2]

Alright, should be fixed.

[tompson]

👍 works!

[p2]

Great!

@damienrambout This should now also work for you, via CocoaPods. Thanks for testing!

[p2]

Finalized in version 2.1

[tompson]

🎉