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

Avoid crashes from unknown exceptions on lockscreen key migration. #6780

Merged
merged 1 commit into from
Aug 9, 2022

Conversation

jmartinesp
Copy link
Member

@jmartinesp jmartinesp commented Aug 9, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Fixes #6769 .

Motivation and context

During lockscreen biometric key migrations, both UserNotAuthenticatedException and KeyPermanentlyInvalidatedException were caught, but there are many other possible exceptions that can be thrown. If any exceptions are thrown, we'll catch them, disable biometric authentication, then show pin code authentication.

Screenshots / GIFs

Tests

  • Install a version < 1.4.31 (1.4.27 maybe?).
  • Make sure you have biometric authentication enabled on your device.
  • Enable biometric auth in Settings > Security and Privacy > Protect access. You'll need to enable pin code auth, then biometric one.
  • Close the app.
  • Disable biometric authentication on your device (delete all fingerprints, disable face unlock, etc.).
  • Upgrade to this version and launch the app.

If the app doesn't crash, everything is fine. Biometric authentication should be disabled in this case too.

Tested devices

  • Physical
  • Emulator
  • OS version(s): 12, 11.

Checklist

@jmartinesp jmartinesp requested a review from a team August 9, 2022 08:33
@jmartinesp jmartinesp self-assigned this Aug 9, 2022
@jmartinesp jmartinesp requested review from fedrunov and removed request for a team August 9, 2022 08:33
@ouchadam ouchadam added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Aug 9, 2022
}.onFailure { e ->
Timber.e(e, "Could not automatically create biometric key. Biometric authentication will be disabled.")
systemKeyStoreCrypto.deleteKey()
vectorPreferences.setUseBiometricToUnlock(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this mean we've fallen back to prompting for a pin?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It's not a great solution, but I can see no alternative. If the migration fails, that's because the key is no longer valid and biometric auth won't work anyway even if the key exists.

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!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 9, 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

77.8% 77.8% Coverage
0.0% 0.0% Duplication

@jmartinesp jmartinesp merged commit 6e1e31b into develop Aug 9, 2022
@jmartinesp jmartinesp deleted the fix/jorgem/lockscreen-key-migration-crashes branch August 9, 2022 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Pin code Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash - KeyStoreCrypto.ensureKey - RuntimeException
2 participants