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

Automatically refresh access tokens in OAuthSwiftClient #596

Merged
merged 2 commits into from May 24, 2020
Merged

Automatically refresh access tokens in OAuthSwiftClient #596

merged 2 commits into from May 24, 2020

Conversation

Timac
Copy link
Contributor

@Timac Timac commented May 12, 2020

Summary

Access tokens have an expiration date but OAuthSwift doesn't automatically refresh access tokens by default.

This PR 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

Related links:

Changes:

  • Add a new function requestWithAutomaticAccessTokenRenewal in OAuthSwiftClient. When used, it automatically refreshes the access token if it expired and restart the query.
  • Move the implementation for renewAccessToken and requestOAuthAccessToken from OAuth2Swift to OAuthSwiftClient. The methods renewAccessToken and requestOAuthAccessToken are still available in OAuth2Swift but they simply forward the call to OAuthSwiftClient to ensure that old apps don't need to be changed.

Why?

I developed an application, called Clatters, that needs to access various services relying on OAuth. Rather that creating my own OAuth library, I decided to use OAuthSwift as it perfectly fit my needs.

However I believe that OAuthSwift should automatically refresh access tokens when they expire. The app itself shouldn't have to implement the logic to handle expired token.

Clatters has been available in the iOS App Store since February 2020 and relies on the code written in this PR.

Usage

You can call the convenient method requestWithAutomaticAccessTokenRenewal in OAuthSwiftClient with the correct parameters. If the access token expired, requestWithAutomaticAccessTokenRenewal will automatically renew the access token and retry the query. Here is an example to perform a query on Reddit:

static func requestIdentity(client: OAuthSwiftClient) {
		client.requestWithAutomaticAccessTokenRenewal(url: URL(string: "https://oauth.reddit.com/api/v1/me")!, method: .GET, headers: nil, contentType: nil, accessTokenBasicAuthentification: true, accessTokenUrl: "https://www.reddit.com/api/v1/access_token", onTokenRenewal: nil) { (result) in
			switch result {
				case .success(let response):
					// Do something
					debugPrint("Received \(response)")
					break
				case .failure:
					// Fail nicely
					break
			}
		}
    }

Notes

I wanted to keep the changes proposed in this PR really simple. As previously mentioned:

  • this new feature is opt-in by using a convenient method.
  • apps already using OAuthSwift shouldn't be affected and don't need to adopt the new API.
  • no code has been removed or deprecated.

Access tokens have an expiration date but OAuthSwift doesn't automatically refresh access tokens by default.

This change 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

Changes:
- Add a new function requestWithAutomaticAccessTokenRenewal in OAuthSwiftClient. When used, it automatically refreshes the access token if it expired and restart the query.
- Move the implementation for renewAccessToken and requestOAuthAccessToken from OAuth2Swift to OAuthSwiftClient. The methods renewAccessToken and requestOAuthAccessToken are still available in OAuth2Swift but they simply forward the call to OAuthSwiftClient to ensure that old apps don't need to be changed.

You can call the convenient method requestWithAutomaticAccessTokenRenewal in OAuthSwiftClient with the correct parameters. If the access token expired, requestWithAutomaticAccessTokenRenewal will automatically renew the access token and retry the query. Here is an example to perform a query on Reddit:

static func requestIdentity(client: OAuthSwiftClient) {
		client.requestWithAutomaticAccessTokenRenewal(url: URL(string: "https://oauth.reddit.com/api/v1/me")!, method: .GET, headers: nil, contentType: nil, accessTokenBasicAuthentification: true, accessTokenUrl: "https://www.reddit.com/api/v1/access_token", onTokenRenewal: nil) { (result) in
			switch result {
				case .success(let response):
					// Do something
					debugPrint("Received \(response)")
					break
				case .failure:
					// Fail nicely
					break
			}
		}
    }
@phimage
Copy link
Member

phimage commented May 13, 2020

Thanks I will look at that closely

I see some issue with code formatting

@Timac
Copy link
Contributor Author

Timac commented May 13, 2020

@phimage Let me know if I can help in any way or if you need some clarifications.

@phimage
Copy link
Member

phimage commented May 14, 2020

I need that you fix the indentation/fomatting, I make a review about this
then I have time I will check the code closely (maybe this weekend)

@Timac
Copy link
Contributor Author

Timac commented May 14, 2020

@phimage I pushed a first change to fix the swiftlint warnings introduced in 94f98f7

That's a change I didn't want to include in this PR, but I think it will help for you to review.

@phimage phimage merged commit fcd6b5f into OAuthSwift:master May 24, 2020
@phimage
Copy link
Member

phimage commented May 24, 2020

thanks, I will check code format later

@Timac Timac deleted the feature/AutomaticallyRefreshAccessTokens branch May 25, 2020 07:20
@dutennakoon
Copy link

Hi, when can we expect this to be released?

@Timac
Copy link
Contributor Author

Timac commented Sep 29, 2020

Hi, when can we expect this to be released?

This PR has been merged so you can easily adopt the code today by pulling the code from the master branch.

@dutennakoon
Copy link

Thanks @Timac. I was using the release number as the pod OAuthSwift version. pod 'OAuthSwift', '~> 2.0.0'

@dutennakoon
Copy link

@Timac When can we expect a release tag for this? As we cannot control the versions from pulling the code from the master branch always.

@phimage
Copy link
Member

phimage commented Sep 29, 2020

@dutennakoon
You could specify a specific commit hash instead of master head (yes its better for security reason)

I use SwiftPM now (and sometimes carthage) so I will not be fast with cocoadpod (no more installed and connected to their specs)
But because of Xcode 12 I want to make a new release, publish older tag too
I have just loose all my id/token to test many services, it take time to recreate it....

@dutennakoon
Copy link

Thanks for the update @phimage

@vijaywargiya
Copy link

Hey @Timac, in your example you are passing onTokenRenewal as nil. But in the method you have the following code on line number 289. So, the renewal does not happen if you pass this as nil. Can you let me know if I'm doing something wrong here?

if let onTokenRenewal = onTokenRenewal {

@vijaywargiya
Copy link

I have made the changes here and now this works correctly. vijaywargiya@67b441a

@Timac
Copy link
Contributor Author

Timac commented Oct 4, 2020

@vijaywargiya The example could indeed be improved to demonstrate how to pass a onTokenRenewal.
The idea is that you have to pass a non-nil onTokenRenewal if you want the renewal to happen. If you pass nil, the renewal should not happen.

The change you proposed could led to more issues than it helps. I believe that after this change, requestWithAutomaticAccessTokenRenewal could potentially call itself with the same parameters over and over. This would led to an infinite loop and the app to crash.

I would suggest to instead make sure that you pass a non-nil onTokenRenewal if you want to renewal feature to happen.

@vijaywargiya
Copy link

I think in that case we should make the parameter not accept nil. Since it would not work with a nil param anyways.
Also, I suppose this onTokenRenewal would not be useful everytime and we should accept and handle nil onTokenRenewal. We can add handling so that requestWithAutomaticAccessTokenRenewal is not called in a loop again and again.

@gadget-man
Copy link

I'm looking for exactly this functionality, but can't work out how to use it!! Currently I call oauthswift.authorize which returns the access_token, refresh_token and expiry, which I save to the keychain. What I can't work out is how to replace oauthswift.authorize with requestWithAutomactiAccesstokenRenewal - can somebody point me in the right direction?

@phatblat phatblat added this to the 2.1.3 milestone May 17, 2021
@phatblat phatblat linked an issue May 18, 2021 that may be closed by this pull request
@phatblat
Copy link
Collaborator

This introduced a new API and thus warrants a new minor version. Going to change the version from 2.1.3 to 2.2.0.

@phatblat phatblat mentioned this pull request May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refresh OAuth2 access tokens by default in the OAuthSwiftClient
6 participants