-
Notifications
You must be signed in to change notification settings - Fork 132
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
Add CredentialsManager and generic Storage #96
Conversation
dcff045
to
36a4faa
Compare
storage.store(KEY_ID_TOKEN, credentials.getIdToken()); | ||
storage.store(KEY_TOKEN_TYPE, credentials.getType()); | ||
|
||
long expiresIn = credentials.getExpiresIn() == null ? 0 : credentials.getExpiresIn(); |
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.
you throw an exception if credentials.getExpiresIn()
is null, so there's no need to check again
String expirationTimeValue = storage.retrieve(KEY_EXPIRATION_TIME); | ||
Long expiresIn = Long.parseLong(expiresInValue); | ||
|
||
boolean invalidTokens = isEmpty(accessToken) && isEmpty(idToken); |
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.
only a suggestion: I don't think a variable is necessary here..
this condition is way simpler than the if in https://github.com/auth0/Auth0.Android/pull/96/files#diff-2651976c12b43b2192543b92b10ed2e4R46
|
||
boolean invalidTokens = isEmpty(accessToken) && isEmpty(idToken); | ||
if (invalidTokens) { | ||
callback.onFailure(new CredentialsManagerException("No Credentials where previously set.")); |
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.
"where" -> "were"
callback.onFailure(new CredentialsManagerException("No Credentials where previously set.")); | ||
return; | ||
} | ||
long expirationTime = expirationTimeValue == null ? 0 : Long.parseLong(expirationTimeValue); |
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.
expirationTimeValue
should never be null from what I can see, but I guess it's almost the same, this will fail below
* Getter for the amount of seconds since the tokens were issued in which they will be deemed invalid. | ||
* To know precisely when this is going to happen you need to save the time every time you request a new pair of tokens. | ||
* | ||
* @return the seconds since the tokens where issued in which they will be deemed invalid. |
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.
minor comment, but this is like a TTL, maybe the method and return description is easier to understand if you just say that.
or have a native english speaker help here, this doesn't sound that well
if (expirationTime > getCurrentTimeInMillis()) { | ||
Long expiresIn = Long.parseLong(expiresInValue); |
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.
expiresIn
value is not right anymore in this case. you can use the expirationTimeValue - currentTime if necessary.
} | ||
long expirationTime = Long.parseLong(expirationTimeValue); | ||
if (expirationTime > getCurrentTimeInMillis()) { | ||
Long expiresIn = Long.parseLong(expiresInValue); |
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.
the comment should be here ... sorry I commented with the link on the email and it comments on the commit not in the general PR
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.
LGTM, only a comment/suggestion about the usefulness of a fixed "expires_in" without knowing the issuing time.
* Getter for the token lifetime in seconds. | ||
* Once expired, the token can no longer be used to access an API and a new token needs to be obtained. | ||
* | ||
* @return the token lifetime in seconds. |
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.
"since the time it was returned".
but is this any useful? I know the spec uses 'expires in' but maybe it's better if it's calculated? you save the calculated expiration time when the token is created, and then return "exp minus now" ?
or directly add a method to return the approximate expiration time and delete this one?
README.md
Outdated
```java | ||
AuthenticationAPIClient authClient = //create auth client | ||
Storage storage = //create new storage | ||
CredentialsManager manager = new CredentialsManager(authClient, storage); |
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.
Show how to use the default storage with the implementation warning:
The storage implementation is up to you. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application directory with Context.MODE_PRIVATE mode.
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.
Also mention if this needs to be singleton or not, maybe a better example here on in QS
README.md
Outdated
|
||
```java | ||
Credentials credentials = //login to Auth0, i.e. by using the authClient | ||
manager.setCredentials(credentials); |
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.
store
or save
might be better
* Represents a Storage of key-value data. | ||
*/ | ||
@SuppressWarnings("WeakerAccess") | ||
public interface Storage { |
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.
The Storage should handle the types to store and its conversion so if it needs to store String and Long it should have methods to set and get them
05e9c9f
to
4c7dbe0
Compare
A Credentials Manager will save credentials and retrieve them checking for valid values, expiration and even refreshing them if needed. If an error arises the user receives a
CredentialsManagerException
.Usage
access_token
orid_token
value is needed along with a validexpires_in
value, if not it will raiseCredentialsManagerException
.access_token
orid_token
was not set it will raiseCredentialsManagerException
. If the saved credentials haven't expired they will be returned. If the credentials have expired and therefresh_token
is null aCredentialsManagerException
will be raised; else it will try to renew them by using the given authClient.TODO
Storage
with the library that usesSharedPreferences
.