Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[expo-local-authentication] Give proper error when device is unsecured #6962

Merged
merged 1 commit into from Feb 4, 2020
Merged

[expo-local-authentication] Give proper error when device is unsecured #6962

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions ...ion/android/src/main/java/expo/modules/localauthentication/LocalAuthenticationModule.java
View file
Open in desktop
Expand Up @@ -3,6 +3,7 @@
package expo.modules.localauthentication;

import android.app.Activity;
import android.app.KeyguardManager;
import android.content.Context;
import android.os.Build;
import android.os.Bundle;
Expand Down Expand Up @@ -101,6 +102,15 @@ public void authenticateAsync(final Promise promise) {
return;
}

if (getKeyguardManager().isDeviceSecure() == false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we could use the same implementation as in isEnrolledAsync but this is fine too as it probably covers more cases 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that "isEnrolled" means "does the user have biometric data enrolled", but maybe that's just my take 🤔

Maybe we should improve the documentation around this to be more clear on this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, looking on the documentation for isEnrolledAsync:

Determine whether the device has saved fingerprints or facial data to use for authentication.

getKeyguardManager().isDeviceSecure() returns true if there is any form of security, e.g. passcode, pattern, password, fingerprint, etc. So we should probably not use that in isEnrolledAsync 🤔

Maybe we should refactor to something like:

  • "hardware supported level" -> 0=None, 1=Secret, 2=Biometric
  • "enrolled level" -> 0=None, 1=Secret, 2=Biometric

That way the programmer can easy decide which level is acceptable for their app ☺️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this discussion followed up anywhere? I'm very interested in the multiple levels thing @LinusU talks about 👀

Copy link
Contributor Author

@LinusU LinusU Feb 12, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't followed up, but I've been keeping this tab open for eight days now to remind myself to do that 😄

I'll open up a dedicated issue now 👌

here 👉 #7032

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, thanks for your work on this @LinusU! ❤️

Bundle errorResult = new Bundle();
errorResult.putBoolean("success", false);
errorResult.putString("error", "not_enrolled");
errorResult.putString("message", "KeyguardManager#isDeviceSecure() returned false");
promise.resolve(errorResult);
return;
}

// BiometricPrompt callbacks are invoked on the main thread so also run this there to avoid
// having to do locking.
mUIManager.runOnUiQueueThread(new Runnable() {
Expand Down Expand Up @@ -181,6 +191,10 @@ private static String convertErrorCode(int code) {
}
}

private KeyguardManager getKeyguardManager() {
return (KeyguardManager) getCurrentActivity().getApplicationContext().getSystemService(Context.KEYGUARD_SERVICE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to mention that we should check whether getCurrentActivity() is not null in most cases in the repo. There is no guarantee that an instance of the module is connected to any activity - for example when you run RN app instance in the background - in that case we don't have an activity and view hierarchy as well. Anyway, I'm ok with this code because authentication won't work without an activity either way, so that's fine here to just throw an error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can submit a follow up PR that checks getCurrentActivity() and bails with an error 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here we go 👉 #6971

}

private Activity getCurrentActivity() {
ActivityProvider activityProvider = mModuleRegistry.getModule(ActivityProvider.class);
return activityProvider != null ? activityProvider.getCurrentActivity() : null;
Expand Down