Renew tokens in Credentials Manager if either token has expired [SDK-999] #319
Conversation
Credentials Manager will now refresh upon either token expiring
| @@ -150,11 +150,11 @@ class SafariWebAuth: WebAuth { | |||
| func newSafari(_ authorizeURL: URL, callback: @escaping (Result<Credentials>) -> Void) -> (SFSafariViewController, (Result<Credentials>) -> Void) { | |||
| let controller = SFSafariViewController(url: authorizeURL) | |||
| controller.modalPresentationStyle = safariPresentationStyle | |||
|
|
|||
There was a problem hiding this comment.
The SwiftLinter picked these up.
Updated Test Dependencies
|
@joshcanhelp turns out the logic was actually a little broken, I cleaned up naming a little. |
| @@ -193,4 +192,44 @@ public struct CredentialsManager { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func hasExpired(_ credentials: Credentials) -> Bool { | |||
There was a problem hiding this comment.
Check logic here please, basically if either token has expired then it should return true
There was a problem hiding this comment.
Current logic looks correct (and tests should verify) ... but we could be much more clear here if we returned on the expired condition instead of setting variables:
if credentials.expiresIn <= Date() {
return true
}
var tokenDecoded = decode(jwt: credentials.idToken);
var exp = tokenDecoded["exp"] as? Double;
if Date(timeIntervalSince1970: exp) < Date() {
return true
}
return falseThat's unlikely to be correct (let alone idiomatic) Swift 😆 but hopefully the message is clear.
There was a problem hiding this comment.
@cocojoe The hasExpired logic is correct, as josh pointed out. But I think the performance impact could be reduced a lot if instead of checking the exp date of each token and decode the id token every time you call this method, you just save the exp date most close to expire at "save credentials" step, and then just have a single date comparison check on this function.
There was a problem hiding this comment.
Yeah I had similar logic to above but I think a mix up in tests sent me down the wrong path, have corrected now.
I don't mind the idea of a single expiry time based upon lowest. Will leave it for a future internal refactor though.
| guard let expiresIn = credentials.expiresIn else { return callback(.noCredentials, nil) } | ||
| guard expiresIn < Date() else { return callback(nil, credentials) } | ||
| guard credentials.expiresIn != nil else { return callback(.noCredentials, nil) } | ||
| guard self.hasExpired(credentials) else { return callback(nil, credentials) } |
There was a problem hiding this comment.
if it has expired continue executing else return credentials we have stored
| else { return false } | ||
| return expiresIn > Date() || credentials.refreshToken != nil | ||
| return !self.hasExpired(credentials) || credentials.refreshToken != nil |
There was a problem hiding this comment.
if neither AT or IDT have expired OR we have a refresh token as we can use that to gain new tokens so technically we have valid credentials. Although the RT could be invalid, not possible to know until calling renew.
There was a problem hiding this comment.
Why would we be using a refresh token for a new ID token? Particularly in the case of a native app.
There was a problem hiding this comment.
@joshcanhelp To obtain a fresh copy of the user profile, I imagine. Either that or use the access token to call /userinfo and obtain it that way.
|
@lbalmaceda please check logic aligns with your PR thanks |
…999] (auth0#319) * Added private token expiry validation method Credentials Manager will now refresh upon either token expiring * Moved decode method as unaccessible for tvOS/macOS * Fixed Logic on token expiry added more tests Updated Test Dependencies
Changes
There are no changes to public API, changes are internal.
Existing behaviour was to validate only the
expires_inas the use case was for renewing the AT. However, this has been expanded now to include the ID Tokenexpas part of the validation.So if either the AT or IDT are expired then the credentials will be invalid and have to be renewed.
References
#309
Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. If this library has unit and/or integration testing, tests should be added for new functionality and existing tests should complete without errors.
Checklist