Added credential manager methods clear and hasValid
#133
Conversation
Auth0/CredentialsManager.swift
Outdated
| @@ -58,6 +58,26 @@ public struct CredentialsManager { | |||
| return self.storage.setData(NSKeyedArchiver.archivedData(withRootObject: credentials), forKey: storeKey) | |||
| } | |||
|
|
|||
| /// Clear credentials stored in keychain | |||
| /// | |||
| /// - Returns: Bool outcome of removal success | |||
There was a problem hiding this comment.
no need to state the Bool type, just say true if credentials were removed, false if no credentials were found or it failed to remove`
Auth0/CredentialsManager.swift
Outdated
|
|
||
| /// Checks if valid credentials are available | ||
| /// | ||
| /// - Returns: Bool outcome of result |
There was a problem hiding this comment.
no need to state the Bool type, just say if there are valid and non-expired credentials stored`
Auth0/CredentialsManager.swift
Outdated
| let data = self.storage.data(forKey:self.storeKey), | ||
| let credentials = NSKeyedUnarchiver.unarchiveObject(with: data) as? Credentials, | ||
| credentials.accessToken != nil, | ||
| let expiresIn = credentials.expiresIn, expiresIn > Date() |
There was a problem hiding this comment.
I think it will be clear to have
let expiresIn = credentials.expiresIn else { return false }
return expiresIn > Date()
|
Also I might be inclined to have just |
|
@hzalaz These were my original thoughts on naming conventions as it's implied by the context of the class. However the argument is consistency with Android although the Swift API guidelines would say Omit Needless Words 😄 |
Added `clearCredentials` method Added relevant tests
Added more tests
cocojoe
force-pushed
the
added-credential-manager-extras
branch
from
bd75222
to
bbf5ed4
Compare
cocojoe
changed the title
clearCredentials and hasCredentialsclear and hasValid
There was a problem hiding this comment.
Just a small missing check. I'm also considering that storing either access_token or id_token should give "valid credentials". Is there a reason to limit it just to access_token? cc @hzalaz
| @@ -53,11 +53,31 @@ public struct CredentialsManager { | |||
| /// Store credentials instance in keychain | |||
| /// | |||
| /// - Parameter credentials: credentials instance to store | |||
| /// - Returns: Bool outcome of success | |||
| /// - Returns: if credentials were stored | |||
There was a problem hiding this comment.
The underlying method can fail, it's just unlikely to ever happen :)
| /// | ||
| /// - Returns: if credentials were removed | ||
| public func clear() -> Bool { | ||
| return self.storage.deleteEntry(forKey: storeKey) |
There was a problem hiding this comment.
The underlying method can fail, it's just unlikely to ever happen :)
Auth0/CredentialsManager.swift
Outdated
| credentials.accessToken != nil, | ||
| let expiresIn = credentials.expiresIn | ||
| else { return false } | ||
| return expiresIn > Date() |
There was a problem hiding this comment.
This is one of the conditions. But if expiresIn <= Date() && credentials.refreshToken != nil you will be able to refresh it, so that's another case that returns "valid credentials". No network call required here.
There was a problem hiding this comment.
I don't know if the refreshToken is valid or not, could be revoked. This check tells me if the current accessToken has expired or not, if it has expired it is invalid and the user should request fresh credentials with retrieveCredentials.
There was a problem hiding this comment.
Having the refresh_token revoked is an edge case. Admin has to do that either manually in the user's dashboard or calling the MGMT API with a well-scoped token. I rather pay the price of a 401 and make the user clear the credentials than saying "your token is no longer valid" when I know that I can refresh it. That it's also a valid credentials scenario IMO.
There was a problem hiding this comment.
This is just checking the AT (credentials) not if the RT is valid or not, which as @cocojoe said it will imply that the app should call to obtain fresh credentials. In the case it fails the app could prompt to re-login
There was a problem hiding this comment.
If it were me I'd have the following props:
- empty: if there is an AT stored
- expired: if the AT is expired
- canRefresh: if there is a RT
Additional methods added to
CredentialsManagerclear()-> Delete credentials from keychain storagehasValid()-> Quick check that valid credentials are available & not expired