Skip to content
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

Refresh OAuth2 access tokens by default in the OAuthSwiftClient #217

Closed
FGoessler opened this issue Apr 4, 2016 · 7 comments · Fixed by #596
Closed

Refresh OAuth2 access tokens by default in the OAuthSwiftClient #217

FGoessler opened this issue Apr 4, 2016 · 7 comments · Fixed by #596

Comments

@FGoessler
Copy link
Contributor

@fabiomassimo created a convenience method on OAuth2Swift to automatically refresh the access token in case of a failure for a normal request in #209 which is nice 👍.

I expected this to be the default behaviour of every request made via the OAuthSwiftClient, which seems not to be the case right now.

My suggestion:

  • Put the logic for the refresh in the OAuthSwiftClient and only execute it for OAuth2 requests.
  • Which OAuth version is used can be easily extracted from the OAuthSwiftCredential of the client.
  • Since the convenience method currently requires some other methods which are implemented on OAuth2Swift I would either move them as well, move them to an extension of OAuthSwiftClient in the OAuth2Swift file or transform them to class methods. What is the preferred solution?
  • Mark the convenience on OAuth2Swift as deprecated and let it just call through to the client. Or even remove the convenience method since no release was made since the introduction of the method and therefore no consumer of this library should be affected.

Any thoughts, comments, considerations, suggestions, ...?

I would start working on a PR then. 😉

FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 4, 2016
…n NSError to detect whether it was caused by an invalid access token
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 4, 2016
… OAuthSwiftClient

Created a tokenExpirationHandler on OAuthSwiftClient to enable the OAuth2Swift class to configure the client to handle expired access tokens.
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 4, 2016
…roperty on OAuthSwiftClient to get informed about access token renewals

Build as a replacement for the optional callback parameter on the deprecated convenience method in OAuth2Swift.
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
…n NSError to detect whether it was caused by an invalid access token
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
… OAuthSwiftClient

Created a tokenExpirationHandler on OAuthSwiftClient to enable the OAuth2Swift class to configure the client to handle expired access tokens.
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
…roperty on OAuthSwiftClient to get informed about access token renewals

Build as a replacement for the optional callback parameter on the deprecated convenience method in OAuth2Swift.
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 5, 2016
…tionHandler which now has an assert to not use it for OAuth1
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
…n NSError to detect whether it was caused by an invalid access token
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
… OAuthSwiftClient

Created a tokenExpirationHandler on OAuthSwiftClient to enable the OAuth2Swift class to configure the client to handle expired access tokens.
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
…roperty on OAuthSwiftClient to get informed about access token renewals

Build as a replacement for the optional callback parameter on the deprecated convenience method in OAuth2Swift.
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
FGoessler pushed a commit to FGoessler/OAuthSwift that referenced this issue Apr 6, 2016
…tionHandler which now has an assert to not use it for OAuth1
@antwerpenR
Copy link

I am trying to get this to work with Meetup.com and it seems that the access token is correctly set in line 83 of authorizeWithCallbackURL but you are not storing the "expires_in" responseParameter. Also, I can find no trace of any possible use of "grant type=refresh-token" and requestOAuthAccessTokenWithParameters on line 160 is never being called. For reference, the Meetup Spec is here: http://www.meetup.com/meetup_api/auth/#oauth2server-auth-success

Am I doing something wrong or is this a bug? It does seem that the token is correct and signing of requests works properly....just no handling of the refresh

@antwerpenR
Copy link

I think the solution may just be to store the "expires_in" at the same time that you store the token. Inserting these lines: 85, 86 and 87.
if let expiresIn:String = responseParameters["expires_in"], offset = Double(expiresIn) {
self.client.credential.oauth_token_expires_at = NSDate(timeInterval: offset, sinceDate: NSDate())
}

@FGoessler
Copy link
Contributor Author

I assume you're using the current version on master, right?

The expires in value is actually stored in the requestOAuthAccessTokenWithParameters method.

To have this automatic refresh token renewal you need to use the special method startAuthorizedRequest(...) on the OAuth2Swift object for every request which should check for token renewal. It's currently not handled "transparently" for all request (e.g. if you do them via the GET(...) convenience methods on the OAuthSwiftClient) - this is what this issue and my PR aim for, but the work there is not finished yet and I wouldn't rely on it yet.

@antwerpenR
Copy link

antwerpenR commented May 4, 2016

Yes - I am on Master latest version.

I use oauthswift.authorizeWithCallbackURL(

  NSURL(string: redirectURL)!,...

(successfully) to get an access token which works....but I see that
"expires in" is not stored.

I then use

oauthswift.startAuthorizedRequest(requiredURL, method: .GET, parameters:...

to (successfully) sign requests and fetch data...and this works for one
hour until the token expires. At that time I get a return saying "invalid
credentials". If I do modify the code to store "expires in" then I get
and error return "expired token" when it expires.....what should I do then
to do a refresh without needing authorisation again?

Thanks for your help...!

Roger Price

On Wed, May 4, 2016 at 7:54 PM, Florian Gößler notifications@github.com
wrote:

I assume you're using the current version on master, right?

The expires in value is actually stored in the
requestOAuthAccessTokenWithParameters method.

To have this automatic refresh token renewal you need to use the special
method startAuthorizedRequest(...) on the OAuth2Swift object for every
request which should check for token renewal. It's currently not handled
"transparently" for all request (e.g. if you do them via the GET(...)
convenience methods on the OAuthSwiftClient) - this is what this issue and
my PR aim for, but the work there is not finished yet and I wouldn't rely
on it yet.


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#217 (comment)

@antwerpenR
Copy link

antwerpenR commented May 5, 2016 via email

@Timac
Copy link
Contributor

Timac commented May 12, 2020

The PR #596 proposes a solution:

  • that makes OAuthSwift automatically refresh access tokens when they expire
  • which is opt-in by providing one simple convenient method
  • containing the minimum amount of changes needed to avoid to break existing apps

@phatblat phatblat linked a pull request May 18, 2021 that will close this issue
@phatblat
Copy link
Collaborator

Added in #596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants