OAuth2PasswordGrant should parse refresh token about oauth2 HOT 37 CLOSED

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

On 14 Sep 2015, at 23:40, Damien Rambout [email protected] 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.

from oauth2.

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.

from oauth2.

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

from oauth2.

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

from oauth2.

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

from oauth2.

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

from oauth2.

Ok nice ! 👍 Thanks

from oauth2.

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.

from oauth2.

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.

from oauth2.

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.

from oauth2.

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!

from oauth2.

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

from oauth2.

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.

from oauth2.

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?

from oauth2.

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.

from oauth2.

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

from oauth2.

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.

from oauth2.

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.

from oauth2.

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 [email protected] 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.

from oauth2.

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!

from oauth2.

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' }

from oauth2.

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

from oauth2.

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 [email protected] 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.

from oauth2.

will try the develop branch, thanks!

from oauth2.

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

from oauth2.

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'

from oauth2.

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

from oauth2.

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

from oauth2.

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

from oauth2.

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

from oauth2.

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

from oauth2.

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

from oauth2.

Alright, should be fixed.

from oauth2.

👍 works!

from oauth2.

Great!

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

from oauth2.

Finalized in version 2.1

from oauth2.

🎉

from oauth2.