Add hasValidCredentials and clearCredentials to CredentialsManager #102
Conversation
| String idToken = storage.retrieveString(KEY_ID_TOKEN); | ||
| Long expiresAt = storage.retrieveLong(KEY_EXPIRES_AT); | ||
|
|
||
| return !(isEmpty(accessToken) && isEmpty(idToken) || expiresAt == null || expiresAt <= getCurrentTimeInMillis() && refreshToken == null); |
There was a problem hiding this comment.
the last || and && combination needs parenthesis I think. at least to read it easier
and maybe split this in two lines, or even better, one line for each "OR" condition
There was a problem hiding this comment.
parenthesis is not needed because the || separates the conditions. I will add them anyway to make it easier to read. 👍
README.md
Outdated
| @@ -449,6 +455,13 @@ manager.getCredentials(new BaseCallback<Credentials, CredentialsManagerException | |||
| ``` | |||
|
|
|||
|
|
|||
| 5. **Clear credentials** | |||
| Whenever you need to remove the credentials from the storage and make sure that the next call to `getCredentials` fails, you must clear them from the manager: | |||
There was a problem hiding this comment.
this is too technical, maybe it's simpler to just say "when you want to log the user out".
and make sure that the next call to
getCredentialsfails
saying it like this sounds weird. my goal is not to have the call fail, that it fails is just a side effect for being logged out
| verify(storage).remove("com.auth0.refresh_token"); | ||
| verify(storage).remove("com.auth0.token_type"); | ||
| verify(storage).remove("com.auth0.expires_at"); | ||
| verify(storage).remove("com.auth0.scope"); |
There was a problem hiding this comment.
just to avoid forgetting checks in the future (supposed you add a new key) you can also verify for "no more interactions" in storage
|
|
||
| when(storage.retrieveString("com.auth0.id_token")).thenReturn(null); | ||
| when(storage.retrieveString("com.auth0.access_token")).thenReturn("accessToken"); | ||
| assertTrue(manager.hasCredentials()); |
There was a problem hiding this comment.
any reason for the change on the way you assert things?
There was a problem hiding this comment.
you mean assertTrue(something) instead of asserThat(something, is(true))?
README.md
Outdated
| 3. **Retrieve credentials** | ||
| Existing credentials will be returned if they are still valid, otherwise the `refresh_token` will be used to attempt to renew them. If the `expires_in` or both the `access_token` and `id_token` values are missing, the method will throw a `CredentialsManagerException`. The same will happen if the credentials have expired and there's no `refresh_token` available. | ||
| 3. **Check credentials existence** | ||
| There are cases were you just want to check if a user session is still valid without the need to use the user credentials right away (i.e. to know if you should present the login screen or the main screen). You could achieve a similar result by calling `getCredentials` and checking the success/failure call, but for convenience we include a `hasCredentials` method that can know in advance if a valid token is available or if it has expired but it could be refreshed, without making an additional network call. The same rules of the `getCredentials` method apply: |
There was a problem hiding this comment.
There are cases were you just want to check if a user session is still valid without the need to use the user credentials right away
I think "without the need to use the user credentials right away" can be omitted. It's simple and the example is more than enough.
You could achieve a similar result
It's not a similar result, this is not a second option. You haven't mentioned the "better" option yet. But I think you just don't even have to mention the "previous workaround". At least not before the simpler/official way.
for convenience we include a
hasCredentialsmethod that can know in advance
that can let you know
lbalmaceda
force-pushed
the
add-credential-manager-methods
branch
from
c0416f2 to
cfc40a3
Compare
| /** | ||
| * Checks if a valid pair of credentials can be obtained from this manager. | ||
| */ | ||
| public boolean hasCredentials() { |
There was a problem hiding this comment.
I'd mention that they are valid credentials not only he existence of one. Also I'd reflect that in the name
lbalmaceda
changed the title
No description provided.