Feature/fga/biometric unlock #1702

ganfra · 2023-10-31T10:41:03Z

This PR branches the biometric unlock logic.

ganfra · 2023-10-31T10:41:49Z

app/src/main/kotlin/io/element/android/x/MainActivity.kt

@@ -42,7 +42,7 @@ import timber.log.Timber

 private val loggerTag = LoggerTag("MainActivity")

-class MainActivity : NodeComponentActivity() {
+class MainActivity : NodeActivity() {


Necessary to have FragmentActivity for Biometric api

ElementBot · 2023-10-31T10:42:48Z

	Warnings
⚠️	gradle/libs.versions.toml#L64 - A newer version of com.google.android.material:material than 1.9.0 is available: 1.10.0
⚠️	gradle/libs.versions.toml#L102 - A newer version of androidx.compose.material3:material3 than 1.2.0-alpha09 is available: 1.2.0-alpha10

Generated by 🚫 dangerJS against c01ed83

github-actions · 2023-10-31T11:08:16Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/P3chEA

jmartinesp

LGTM thanks! I just have a few nits and one thing we might either want to implement now or as soon as we get some errors we saw back in EA (which we might avoid if we're lucky).

Also, if biometric auth is enabled, shouldn't it be prompted by default, instead of the PIN code?

Feel free to ignore the nits or implement them in another PR.

jmartinesp · 2023-10-31T12:47:16Z

appconfig/src/main/kotlin/io/element/android/appconfig/LockScreenConfig.kt

@@ -48,7 +48,16 @@ data class LockScreenConfig(
    /**
     * Time period before locking the app once backgrounded.
     */
-    val gracePeriodInMillis: Long
+    val gracePeriodInMillis: Long,


Nit: maybe this could be a Duration instead?

jmartinesp · 2023-10-31T13:07:42Z

...mpl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricUnlock.kt

+    override fun setup() {
+        try {
+            val secretKey = ensureKey()
+            val cipher = encryptionDecryptionService.createEncryptionCipher(secretKey)


While this will work in general, on EA we had some nasty issues with 'user not authenticated' errors. To avoid this, one possible workaround may be to create the Cipher on demand and have 2 internal authentication methods, since it seems like this is a possible workaround:

authenticateWithCryptoObject, should be called by default, it will do what's inside authenticate right now. If the createEncryptionCipher inside this authenticate method fails with the 'user not authenticated error', however, we go to the 2nd method.

authenticateWithoutCryptoObject, will call the same prompt.authenticate method without a CryptoObject, which will authenticate the user and get rid of the error, then you should be able to initialize the Cipher and use it as you'd usually do.

I'm not saying you should implement this now, but we definitely should if we see those kind of issues coming back on EXA.

user not authenticated error is something different than KeyPermanentlyInvalidatedException?

Yes, it's completely different. This error should only appear if you tried to use the Cipher before using the biometric prompt I think, but for some reason it sometimes will trigger as soon as you initialize the Cipher.

Ok I see, otherwise I'll remove the flag for now...

If you do that we might have to handle key migrations in the future, so be careful.

Will handle that later if we encounter the issue, no time for now :/

No worries, given the deadlines, I think that's the right call too

jmartinesp · 2023-10-31T13:09:32Z

...otlin/io/element/android/features/lockscreen/impl/biometric/DefaultBiometricUnlockManager.kt

+    /**
+     * Returns true if any biometric method (weak or strong) can be used.
+     */
+    private val canUseBiometricAuth: Boolean


Maybe we should rename this to 'isAuthenticatorAvailable'?

jmartinesp · 2023-10-31T13:15:16Z

.../impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/LockScreenStore.kt

+    /**
+     * Returns whether the biometric unlock is allowed or not.
+     */
+    fun isBiometricUnlockAllowed(): Flow<Boolean>


Nit: maybe use enabled here instead of allowed? Allowed sounds like something coming from the OS to me, rather than from a user preference.

It was to reflects what we show on the ui...

ganfra · 2023-10-31T13:33:10Z

LGTM thanks! I just have a few nits and one thing we might either want to implement now or as soon as we get some errors we saw back in EA (which we might avoid if we're lucky).

Also, if biometric auth is enabled, shouldn't it be prompted by default, instead of the PIN code?

This is the case, it automatically opens the Biometric bottom sheet, on top of the PinUnlockView

Feel free to ignore the nits or implement them in another PR.

jmartinesp · 2023-10-31T14:14:57Z

This is the case, it automatically opens the Biometric bottom sheet, on top of the PinUnlockView

Ah, true, it does by default. However, for me it doesn't anymore after:

I dismiss the biometric prompt.
Unlock using pin code.
Move the app to background.
After the grace period, open the app again.

It won't work automatically, I need to click on 'use biometric'. It does work fine again after restarting the app.

ganfra · 2023-10-31T14:22:42Z

This is the case, it automatically opens the Biometric bottom sheet, on top of the PinUnlockView

Ah, true, it does by default. However, for me it doesn't anymore after:

I dismiss the biometric prompt.

Unlock using pin code.

Move the app to background.

After the grace period, open the app again.

It won't work automatically, I need to click on 'use biometric'. It does work fine again after restarting the app.

Got it, will fix, thanks!

codecov · 2023-10-31T17:15:45Z

Codecov Report

Attention: 187 lines in your changes are missing coverage. Please review.

Comparison is base (5f85707) 63.64% compared to head (35410c4) 63.40%.
Report is 12 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1702      +/-   ##
===========================================
- Coverage    63.64%   63.40%   -0.25%     
===========================================
  Files         1267     1277      +10     
  Lines        32881    33129     +248     
  Branches      6809     6854      +45     
===========================================
+ Hits         20928    21006      +78     
- Misses        8805     8959     +154     
- Partials      3148     3164      +16

Files	Coverage Δ
...ockscreen/impl/settings/LockScreenSettingsState.kt	`100.00% <100.00%> (ø)`
...n/impl/settings/LockScreenSettingsStateProvider.kt	`93.33% <100.00%> (+1.02%)`	⬆️
...creen/impl/setup/biometric/SetupBiometricEvents.kt	`100.00% <100.00%> (ø)`
...screen/impl/setup/biometric/SetupBiometricState.kt	`100.00% <100.00%> (ø)`
...atures/lockscreen/impl/setup/pin/SetupPinEvents.kt	`100.00% <ø> (ø)`
...res/lockscreen/impl/setup/pin/SetupPinPresenter.kt	`86.84% <ø> (ø)`
...eatures/lockscreen/impl/setup/pin/SetupPinState.kt	`100.00% <ø> (ø)`
...lockscreen/impl/setup/pin/SetupPinStateProvider.kt	`96.29% <ø> (ø)`
...features/lockscreen/impl/setup/pin/SetupPinView.kt	`60.00% <ø> (ø)`
...ckscreen/impl/setup/pin/validation/PinValidator.kt	`100.00% <ø> (ø)`
... and 25 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonarcloud · 2023-10-31T19:26:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

ganfra added 8 commits October 30, 2023 18:02

Biometric unlock : refactor a bit existing classes

1a9c513

Biometric unlock : first implementation

7ee5bdb

Biometric: add ui for biometric setup

3d7b68d

Biometric unlock : use localazy strings

7106faa

Biometric unlock setup : branch skip/allow events

9280465

Biometric : fix tests

2aa2208

Biometrics: add small test

2997d36

Biometric : clean up

f51231d

ganfra commented Oct 31, 2023

View reviewed changes

Update screenshots

419e8c1

ganfra marked this pull request as ready for review October 31, 2023 11:06

ganfra requested a review from a team as a code owner October 31, 2023 11:06

ganfra requested review from jmartinesp and removed request for a team October 31, 2023 11:06

jmartinesp approved these changes Oct 31, 2023

View reviewed changes

ganfra added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Oct 31, 2023

Merge branch 'develop' into feature/fga/biometric_unlock

86c2786

ganfra mentioned this pull request Oct 31, 2023

Feature/fga/unlock settings #1707

Closed

ganfra added 2 commits October 31, 2023 17:19

Biometric unlock : address some PR review

d5ce245

Biometric : improve a bit edge cases

35410c4

ganfra enabled auto-merge October 31, 2023 17:19

Fix lint issues

c01ed83

jmartinesp disabled auto-merge October 31, 2023 18:22

jmartinesp merged commit 6832b1f into develop Oct 31, 2023
10 of 11 checks passed

jmartinesp deleted the feature/fga/biometric_unlock branch October 31, 2023 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/fga/biometric unlock #1702

Feature/fga/biometric unlock #1702

ganfra commented Oct 31, 2023

ganfra Oct 31, 2023

ElementBot commented Oct 31, 2023 •

edited

github-actions bot commented Oct 31, 2023 •

edited

jmartinesp left a comment

jmartinesp Oct 31, 2023

jmartinesp Oct 31, 2023

ganfra Oct 31, 2023

jmartinesp Oct 31, 2023 •

edited

ganfra Oct 31, 2023

jmartinesp Oct 31, 2023

ganfra Oct 31, 2023

jmartinesp Oct 31, 2023

jmartinesp Oct 31, 2023

jmartinesp Oct 31, 2023

ganfra Oct 31, 2023

ganfra commented Oct 31, 2023

jmartinesp commented Oct 31, 2023

ganfra commented Oct 31, 2023

codecov bot commented Oct 31, 2023 •

edited

sonarcloud bot commented Oct 31, 2023

Feature/fga/biometric unlock #1702

Feature/fga/biometric unlock #1702

Conversation

ganfra commented Oct 31, 2023

Choose a reason for hiding this comment

ElementBot commented Oct 31, 2023 • edited

github-actions bot commented Oct 31, 2023 • edited

jmartinesp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinesp Oct 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganfra commented Oct 31, 2023

jmartinesp commented Oct 31, 2023

ganfra commented Oct 31, 2023

codecov bot commented Oct 31, 2023 • edited

Codecov Report

sonarcloud bot commented Oct 31, 2023

ElementBot commented Oct 31, 2023 •

edited

github-actions bot commented Oct 31, 2023 •

edited

jmartinesp Oct 31, 2023 •

edited

codecov bot commented Oct 31, 2023 •

edited