Make Credential Managers save the refreshed value #118

lbalmaceda · 2017-09-27T23:01:24Z

My only concern in this PR is that when serializing the Credentials object, Gson (our Json library) will serialize Java Dates to a string value (not long timestamps), thus losing the precision (milis). Should I make the serializer serialize Longs instead? (I've to code that logic) or is it an edge case and we're ok having a 1 second expired Credentials?
EDIT: Fixed. Now Gson serializes dates in a correct format including Timezone and Millis.

Depends on this PR getting merged: #115. Once merged, a rebase will make diff clearer here.

Closes #109

cocojoe · 2017-09-28T09:03:13Z

@lbalmaceda is it possible to improve your code-coverage, is this an issue due to the test env only covering a specific API version so other tests can't be executed?

I guess question is, what's not being covered?

cocojoe · 2017-09-28T09:29:51Z

Regarding your question on date storage, I think ~1 second is fine, given that the expiry time is say 12 or 24 hours?

cocojoe

First pass

cocojoe · 2017-09-28T09:09:36Z

README.md

-1. **Instantiate the manager**
-You'll need an `AuthenticationAPIClient` instance used to renew the credentials when they expire and a `Storage`. The Storage implementation is up to you. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application's directory with Context.MODE_PRIVATE mode. This implementation is thread safe and can either be obtained through a Singleton like method or be created every time it's needed.
+1. **Instantiate the manager:**
+You'll need an `AuthenticationAPIClient` instance to renew the credentials when they expire and a `Storage`. We provide a `SharedPreferencesStorage` that uses `SharedPreferences` to create a file in the application's directory with **Context.MODE_PRIVATE** mode. This implementation is thread safe and can either be obtained through a shared method or on demand.


~~and a Storage~~ -> and a Storage object.

~~We provide a SharedPreferencesStorage that uses SharedPreferences~~ -> We provide a SharedPreferencesStorage class that implements SharedPreferences

Hmm. But SharedPreferences is a KeyValue store class rather than an interface that you could implement.

I wasn't sure tbh just mentioned it

cocojoe · 2017-09-28T09:10:38Z

README.md

 When you want to log the user out:

 ```java
 manager.clearCredentials();
 ```

+
+### Encryption enforced (Min API 21)


Is the language used typically min API X or API X+ ?

real example: Retrofit requires at minimum Java 7 or Android 2.3.

cocojoe · 2017-09-28T09:15:58Z

README.md

+
+#### Requiring Authentication
+
+You can require the user authentication to obtain credentials. This will make the manager prompt the user with the device's configured LockScreen, which they must pass correctly in order to obtain the credentials. **This feature is only available on devices where the user has setup a secured LockScreen** (PIN, Pattern, Password or Fingerprint).


Seems wordy, do we need to explain what a user has to do to pass a LockScreen?

I think the interesting point here is letting them know that no extra PIN/Pwd needs to bet setup/remembered by them, as we use the one they already use for their phone. And of course, if the phone didn't have any secure lockscreen setup, this won't work.

cocojoe · 2017-09-28T09:18:46Z

auth0/src/main/java/com/auth0/android/authentication/storage/CredentialsManager.java

@@ -65,7 +65,7 @@ public void saveCredentials(@NonNull Credentials credentials) {
 */
 public void getCredentials(@NonNull final BaseCallback<Credentials, CredentialsManagerException> callback) {
 String accessToken = storage.retrieveString(KEY_ACCESS_TOKEN);
- String refreshToken = storage.retrieveString(KEY_REFRESH_TOKEN);
+ final String refreshToken = storage.retrieveString(KEY_REFRESH_TOKEN);


How come this one is now final?

Because I use it on an anonymous function right below and needs any variable accessed from outside its scope to be final. -> https://github.com/auth0/Auth0.Android/pull/118/files#diff-2651976c12b43b2192543b92b10ed2e4R91

cocojoe · 2017-09-28T09:22:00Z

auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.java

+ }
+ KeyguardManager kManager = (KeyguardManager) activity.getSystemService(Context.KEYGUARD_SERVICE);
+ this.authIntent = Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP ? kManager.createConfirmDeviceCredentialIntent(title, description) : null;
+ this.authenticateBeforeDecrypt = (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M && kManager.isDeviceSecure()


Readability would be better with parenthesis. Think of parentheses like comments in code: they aren't strictly necessary, and in theory a competent developer should be able to figure out code without them. And yet, these cues are exceedingly helpful.

I will add them but TBH it would almost look the same, as the new line is already doing that thing you mention 😛

lbalmaceda · 2017-10-02T19:40:15Z

README.md

@@ -434,20 +434,26 @@ users
 > In all the cases, the `User ID` parameter is the unique identifier of the auth0 account instance. i.e. in `google-oauth2|123456789081523216417` it would be the part after the '|' pipe: `123456789081523216417`.


-### Credentials Manager
-This library ships with a `CredentialsManager` class to easily store and retrieve fresh Credentials from a given `Storage`.


🤔 I think you're commenting on the deleted line.

lbalmaceda requested review from cocojoe and aaguiarz September 27, 2017 23:01

cocojoe reviewed Sep 28, 2017

View reviewed changes

lbalmaceda force-pushed the save-refreshed-credentials branch 4 times, most recently from 78f33ab to 79642f2 Compare October 2, 2017 18:54

lbalmaceda commented Oct 2, 2017

View reviewed changes

lbalmaceda mentioned this pull request Oct 2, 2017

Proposal: CredentialsManager.getCredentials() should save renewed Credentials #109

Closed

lbalmaceda added this to the v1-Next milestone Oct 5, 2017

lbalmaceda added the CH: Changed label Oct 5, 2017

lbalmaceda added 4 commits October 5, 2017 14:35

save the refreshed credentials.

c02683b

fix java Date serialization

575a1ae

second attempt to fix Date serialization

9d3d32b

chore martin comments.

348ba26

lbalmaceda force-pushed the save-refreshed-credentials branch from 9b0da89 to 348ba26 Compare October 5, 2017 17:35

cocojoe approved these changes Oct 6, 2017

View reviewed changes

lbalmaceda merged commit d7f97b3 into master Oct 11, 2017

lbalmaceda deleted the save-refreshed-credentials branch October 11, 2017 13:00

lbalmaceda modified the milestones: v1-Next, 1.11.0 Oct 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Credential Managers save the refreshed value #118

Make Credential Managers save the refreshed value #118

lbalmaceda commented Sep 27, 2017 •

edited

Loading

cocojoe commented Sep 28, 2017 •

edited

Loading

cocojoe commented Sep 28, 2017 •

edited

Loading

cocojoe left a comment

cocojoe Sep 28, 2017

lbalmaceda Oct 2, 2017

cocojoe Oct 2, 2017

cocojoe Sep 28, 2017

lbalmaceda Oct 2, 2017 •

edited

Loading

cocojoe Sep 28, 2017

lbalmaceda Oct 2, 2017

cocojoe Sep 28, 2017

lbalmaceda Oct 2, 2017

cocojoe Sep 28, 2017

lbalmaceda Oct 2, 2017

lbalmaceda Oct 2, 2017


		#### Requiring Authentication

		You can require the user authentication to obtain credentials. This will make the manager prompt the user with the device's configured LockScreen, which they must pass correctly in order to obtain the credentials. This feature is only available on devices where the user has setup a secured LockScreen (PIN, Pattern, Password or Fingerprint).

Make Credential Managers save the refreshed value #118

Make Credential Managers save the refreshed value #118

Conversation

lbalmaceda commented Sep 27, 2017 • edited Loading

cocojoe commented Sep 28, 2017 • edited Loading

cocojoe commented Sep 28, 2017 • edited Loading

cocojoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented Sep 27, 2017 •

edited

Loading

cocojoe commented Sep 28, 2017 •

edited

Loading

cocojoe commented Sep 28, 2017 •

edited

Loading

lbalmaceda Oct 2, 2017 •

edited

Loading