-
Notifications
You must be signed in to change notification settings - Fork 247
Add support for disabling credential auto-renewal #980
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
base: master
Are you sure you want to change the base?
Conversation
95cbae6
to
0734b2b
Compare
0734b2b
to
2fab17b
Compare
@@ -240,12 +244,13 @@ public struct CredentialsManager { | |||
/// - Returns: If there are credentials stored containing a refresh token. | |||
public func canRenew() -> Bool { | |||
guard let credentials = self.retrieveCredentials() else { return false } | |||
return credentials.refreshToken != nil | |||
return self.allowsAutoRefreshing && credentials.refreshToken != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check allowsAutoRefreshing
before self.retrieveCredentials()
?
@@ -652,6 +657,11 @@ public struct CredentialsManager { | |||
dispatchGroup.leave() | |||
return callback(.success(credentials)) | |||
} | |||
|
|||
guard self.allowsAutoRefreshing else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is right. This will prevent refreshing, but that's not the only thing the code below does, it also tries to read the credentials from the store/keychain
Hi @NachoSoto, thanks for raising this.
Would configuring the Rotation Overlap Period work for this purpose? Given two concurrent renewal requests (one from the app and one from the extension), once the first succeeds, if the second one completes within the rotation overlap period, it will succeed as well, and no token rotation error will be returned. ![]() See https://auth0.com/docs/secure/tokens/refresh-tokens/configure-refresh-token-rotation#configure-in-the-dashboard for more information. You can also configure a lower timeout value to ensure that the second request either succeeds or fails within the rotation overlap period. To do so, you need to use a custom var configuration = URLSessionConfiguration.default
// configure it...
let customURLSession = URLSession(configuration: configuration)
let auth = Auth0.authentication(session: customURLSession)
let credentialsManager = CredentialsManager(authentication: auth) |
Are there any guarantees that the two can safely complete concurrently? And after that, there could be a race condition where the first one that completed is the one that ends up in the keychain, leading to a broken token, right? |
The first one that completes will be automatically saved in the Keychain by the Credentials Manager. But, if the rotation overlap period is configured, then the second request will still succeed, and will also be automatically saved in the Keychain. I just tried it out, and exchanging the same refresh token concurrently yielded the same access token, so there should be no issues on that front. |
@NachoSoto did setting the rotation overlap period work for your use case? |
I haven't had a chance yet. But even if it handles race conditions correctly with 2 processes potentially writing in the keychain simultaneously, we would still prefer to run Auth0 from an app extension in a read-only configuration (which is what we'd accomplish with this PR). |
I'd suggest trying out the rotation overlap period first and seeing if you run into any issues. |
FYI I won't be working as a maintainer of this library anymore. In the future please work with @NandanPrabhu. |
How can you guarantee there can’t be race conditions with 2 processes writing into the keychain though? |
It seems I did not explain the purpose of the Rotation Overlap Period setting clearly. My bad. Let's try again. Configuring this value (e.g., to 30 seconds), along with a shorter timeout value (e.g., 5 seconds), means:
In other words, this will not prevent concurrent renewal requests; it will make the concurrency irrelevant. |
If not, please feel free to create a reproduction (e.g., a minimal app with an extension, both writing to the Keychain in a loop) and share it with us. |
📋 Changes
CredentialsManager
constructor (not changing default behavior) to allow disabling credential renewals.