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.

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

Disable 'Enable biometrics' option if there are not biometric authenticators enrolled. #6714

Merged

Conversation

jmartinesp
Copy link
Member

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes #6713 .

Motivation and context

Enabling biometric auth in the app is not restricted to users who have biometric authenticators enrolled in the device. This caused UserNotAuthenticatedException crashes for users who enabled this option without having those authenticators.

Screenshots / GIFs

Tests

  • Make sure you have no biometric authenticators enrolled in your device (no fingerprint, no face unlock, etc.).
  • Open the app, go to Settings > Security & Privacy > Protect Access.
  • Enable PIN code.
  • Check 'Enable biometrics' can't be toggled because it's disabled.
  • Now add a fingerprint to your device.
  • Go back to the app, in 'Protect Access'.
  • Check that 'Enable biometrics' can be accessed now.

Note: I had some issues while testing this on emulators, it seems like an emulator bug. Having several fingerprints enrolled and enabling biometrics in the app caused the same UserNotAuthenticatedException crash mentioned above. Apparently, BiometricManager detects the valid authenticators but BiometricPrompt doesn't.

Tested devices

  • Physical
  • Emulator
  • OS version(s): 12

Checklist

@jmartinesp jmartinesp requested a review from a team August 2, 2022 10:15
@jmartinesp jmartinesp self-assigned this Aug 2, 2022
@jmartinesp jmartinesp requested review from ouchadam and removed request for a team August 2, 2022 10:15
@@ -59,7 +59,7 @@ class LockScreenFragment : VectorBaseFragment<FragmentLockScreenBinding>() {
if (state.lockScreenConfiguration.mode == LockScreenMode.CREATE) return@withState

viewLifecycleOwner.lifecycleScope.launchWhenResumed {
if (state.isBiometricKeyInvalidated) {
if (state.canUseBiometricAuth && state.isBiometricKeyInvalidated) {
lockScreenListener?.onBiometricKeyInvalidated()
Copy link
Contributor

Choose a reason for hiding this comment

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

out of interest, what happens in this onBiometricKeyInvalidated flow?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 2 points where the state. isBiometricKeyInvalidated can change and trigger this code:

  1. LockScreenViewModel.showBiometricPrompt, this will call BiometricHelper.authenticate that will fail before showing any UI if the system key is not valid and instead it:
  • Catches any exceptions.
  • Posts a LockScreenViewEvent.AuthError to handle it in the UI.
  • If the error was an instance of KeyPermanentlyInvalidatedException, it will call removeBiometricAuthentication() which will disable the 'biometric enabled' option, delete the broken system key and update the current state to display an alert saying that the key has just been invalidated.
  1. LockScreenViewModel.updateStateWithBiometricInfo is automatically called when the LockScreenConfiguration changes:
  • It calls biometricHelper.isSystemKeyValid.
  • This will reach KeyStoreCrypto.hasValidKey(), which tries to retrieve the current system key and either return true or catch both KeyPermanentlyInvalidatedException and UserNotAuthenticatedException, which are thrown when the internal Cipher.init fails, and return false.
  • state.isBiometricKeyInvalidated will be updated with this returned value and used to display the same alert as in the case above.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for explaining 👍


runCatching { keyStoreCrypto.ensureKey() }
val userNotAuthenticatedException = UserNotAuthenticatedException()
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding the test case 💯

override fun onResume() {
super.onResume()

useBiometricPref.isEnabled = usePinCodePref.isChecked
useBiometricPref.isChecked = shouldCheckBiometricPref(usePinCodePref.isChecked)
useBiometricPref.isEnabled = shouldEnableBiometricPref(isPinCodeChecked = usePinCodePref.isChecked)
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think it's worth extracting the useBiometricPref.isEnabled and useBiometricPref.isChecked setting to a reusable function?

Copy link
Contributor

@ouchadam ouchadam left a comment

Choose a reason for hiding this comment

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

LGTM 👍 minor extraction comment, will leave up to you!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 2, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

57.1% 57.1% Coverage
0.0% 0.0% Duplication

@jmartinesp jmartinesp merged commit c848615 into develop Aug 2, 2022
@jmartinesp jmartinesp deleted the fix/jorgem/disable-biometric-auth-with-no-authenticators branch August 2, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users with no biometric features enrolled can enable biometric authentication
2 participants