Improve authenticated flow of the Credentials Manager #519
Conversation
lbalmaceda
force-pushed
the
feat-credman-results
branch
from
c458588
to
98442fa
Compare
lbalmaceda
force-pushed
the
feat-credman-results
branch
from
98442fa
to
0895e13
Compare
| @@ -63,12 +68,13 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT | |||
|
|
|||
| /** | |||
| * Require the user to authenticate using the configured LockScreen before accessing the credentials. | |||
| * This feature is disabled by default and will only work if the device is running on Android version 21 or up and if the user | |||
| * has configured a secure LockScreen (PIN, Pattern, Password or Fingerprint). | |||
| * This method MUST be called in [Activity.onCreate]. This feature is disabled by default and will | |||
There was a problem hiding this comment.
It's OK if they don't. I'm defaulting to the old scenario anyways, but it's better to lead them to use the new way for future compatibility & easier deprecation reasons.
| @@ -84,25 +90,42 @@ public class SecureCredentialsManager @VisibleForTesting(otherwise = VisibleForT | |||
| ): Boolean { | |||
| require(!(requestCode < 1 || requestCode > 255)) { "Request code must be a value between 1 and 255." } | |||
| val kManager = activity.getSystemService(Context.KEYGUARD_SERVICE) as KeyguardManager | |||
| authIntent = | |||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) kManager.createConfirmDeviceCredentialIntent( | |||
There was a problem hiding this comment.
the min SDK is already 21, no reason to keep these checks around
| * prevent that exception while still falling back to the old "startActivityForResult" flow. That's in | ||
| * case devs are invoking this method in places other than the Activity's "OnCreate" method. | ||
| */ | ||
| if (activity is ComponentActivity && !activity.lifecycle.currentState.isAtLeast( |
There was a problem hiding this comment.
If the context is a subclass of ComponentActivity and not yet STARTED, the new path will be used. Otherwise, legacy scenario.
| // Activity is "created" | ||
| val activityController = Robolectric.buildActivity( | ||
| ComponentActivity::class.java | ||
| ).create() |
There was a problem hiding this comment.
activity is so far "created"
| ) | ||
|
|
||
| // Activity is "started" so pending ActivityResults are dispatched | ||
| activityController.start() |
There was a problem hiding this comment.
we then instruct it to move to the "started" state, that releases the activity results that we assert
| activityController.start() | ||
| // Trigger the prompt for credentials | ||
| manager.getCredentials(callback) | ||
| verify(activity, never()).startActivityForResult(any(), anyInt()) |
There was a problem hiding this comment.
we test that legacy path is not used
| ComponentActivity::class.java | ||
| ).create() | ||
| val activity = Mockito.spy(activityController.get()) | ||
| val canceledResult = ActivityResult(Activity.RESULT_CANCELED, null) |
There was a problem hiding this comment.
similar to the above test, but now we test that the user "canceled" the auth request
| ComponentActivity::class.java | ||
| ).create() | ||
| val activity = Mockito.spy(activityController.get()) | ||
| val successfulResult = ActivityResult(Activity.RESULT_OK, null) |
There was a problem hiding this comment.
user will succeed to prove their identity
| Activity::class.java | ||
| ).create().start().resume().get() | ||
| ComponentActivity::class.java | ||
| ).create().resume().start().get() |
There was a problem hiding this comment.
proves that if using a ComponentActivity that is already started, it doesn't throw the exception because we are handling that and falling back to the legacy use case (which is tested below).
| val activity = Mockito.spy( | ||
| Robolectric.buildActivity( | ||
| Activity::class.java | ||
| ).create().get() |
There was a problem hiding this comment.
The rest of the tests onwards change this line. From starting from an activity that was "resumed" to start with an activity that is "created".
lbalmaceda
force-pushed
the
feat-credman-results
branch
from
d75c789
to
71b5c68
Compare
Changes
The
requireAuthenticationmethod takes anActivityinstance to use as context to launch the "lock screen" intent. When this is a subclass ofComponentActivity, we can make use of the new Activity Results API to launch that intent instead. This is the recommended way now thatstartActivityForResulthas been deprecated.When that's the case, the launch and result handling happen internally without the developer having to invoke the
checkAuthenticationResultmethod to "complete" the flow.This PR does NOT include breaking changes.
References
See #513
Testing
Added unit tests and manually tested the new flow.
Existing implementations would still receive a call to their activities'
onActivityResultmethod, but with a request code value different than the one requested by our SDK, passed by the dev in therequireAuthenticationcall. Developers should ignore it because the request code is not a match. Yet, we were already handling this case internally and checking for the request code in these lines anyway, so distracted developers are covered.I did notice that if the
requireAuthenticationmethod was called after the activity is resumed/started, then the following runtime exception is raised:So I added a check (and tests) to prevent using this use case with Activity Results if the activity passed as context is already started.