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

feat: secure biometric implementation #89

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

lubbo
Copy link

@lubbo lubbo commented Feb 13, 2023

Problem

Get and Set credentials were not enforcing biometric encryption.

Solution

  • JS interface was not modified.
  • Native implementation now uses biometric native API to get and set credentials as per iOS and Android specifications:

Android

https://medium.com/androiddevelopers/using-biometricprompt-with-cryptoobject-how-and-why-aace500ccdb7

iOS

https://developer.apple.com/documentation/localauthentication/accessing_keychain_items_with_face_id_or_touch_id

  • Credentials are wiped out if device biometry is changed (add or remove biometry factors)
  • Passcode has to be set on the device

Additional changes

  • Android support for multiple credentials has been implemented (based on server parameter)
  • Both Android and iOS code has been refactored to be more aligned with MVVM and MVC patterns:

Android

  • ViewModel for AuthActivity has been created and all non UI logic has been moved there.
  • Credentials class model has been created with from/to JSON utilities.
  • Crypto logics have been moved into a dedicated CryptoManager.
  • Due to potentially insecure plugin response implicit Intent, getCredentials response is passed from Activity to NativeBiometric Plugin using a LocalBroadcastManager.
  • All string parameters keys have been moved into dedicated constants.
  • Gradle and AGP have been updated to 7.5 and 7.4.1
  • Android target and compile sdk versions have been updated to 32.

iOS

  • All crypto logic has been extracted from plugin class and moved into a dedicated CryptoManager class

@kiwi-josh
Copy link

Bump on this 🙏

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.

2 participants