search

mozilla-mobile / focus-android

⚠️ Firefox Focus (Android) moved to a new repository. It is now developed and maintained as part of: https://github.com/mozilla-mobile/firefox-android
https://github.com/mozilla-mobile/firefox-android
Mozilla Public License 2.0
2.11k stars 711 forks source link

Fingerprint auth prompt cuts off 'New session' on some screens. #6167

Closed jonalmeida closed 2 years ago

jonalmeida commented 2 years ago

Steps to reproduce

  1. Enable finger print auth.
  2. Open a site and leave app.
  3. Return to the app.

Expected behavior

Actual behavior

Device information

jonalmeida commented 2 years ago

Maybe we should just rely on the native fingerprint prompt? When this was added, the API was still new. The OS now has been biometric prompt support that we use in Fenix as well.

See BiometricPromptFeature.

lobontiumira commented 2 years ago

I was able to reproduce this issue on Beta 97.0.0-beta.1 with Oppo Find X3 Lite (Android 11). Although it is cut, it works if tapped on "New session".

lobontiumira commented 2 years ago

We were able to reproduce on Oppo Find X3 Lite (Android 11), and Google Pixel 6 (Android 12), on the latest Focus Nightly from 1/20. When in landscape mode, the "New session" option is visible, not cut off. Not reproducible on Google Pixel (Android 10).

iorgamgabriel commented 2 years ago

Now Focus is using FingerprintManagerCompat , this is deprecated now . To fix this bug and refactor all the FingerprintManagerCompat part we need to use , like @jonalmeida suggested : BiometricPromptFeature. This is how will look like . @jeffreygee It's this ok for you ?

iorgamgabriel commented 2 years ago
jonalmeida commented 2 years ago

Now Focus is using FingerprintManagerCompat , this is deprecated now . To fix this bug and refactor all the FingerprintManagerCompat part we need to use , like @jonalmeida suggested : BiometricPromptFeature. This is how will look like . @jeffreygee It's this ok for you ?

@iorgamgabriel please upstream the BiometricPromptFeature to android components instead of copying the code over. 🙂

jeffreygee commented 2 years ago

@iorgamgabriel Is all of the content customizable on this popup - more specifically the CTA text of 'New Session'?

jeffreygee commented 2 years ago

Question about the current flow too - so the way this currently works is that if a users taps 'New Session' it technically restarts the session. But doesn't this also mean that an outside user can technically come in and hit that and see the user's saved shortcuts without the fingerprint? So i'm wondering if the action here should either be a Cancel (would go back to homescreen) or the fallback method (Use Password)

iorgamgabriel commented 2 years ago

Yes all the texts can be custom . "But doesn't this also mean that an outside user can technically come in and hit that and see the user's saved shortcuts without the fingerprint? " for these I made a fix https://github.com/mozilla-mobile/focus-android/issues/6166 . @jeffreygee

iorgamgabriel commented 2 years ago

I think this is how it should look like and behave with the refactoring from FingerPrint to Biometric .

Acceptance Criteria :

1.As a user when I have a browsing session open or at least one shortcut on the home screen and I close the app then the lockscreen with Biometric will appear.

2.As a user if I click onside the biometric Prompt (the grey pop-up with the fingerprint icon ) this will appear.

3.As a user if I choose to enter in the app with pin or the fingerprint is not recognise this screen will appear.

  1. If the fingerprint is not recognise and I don't know the pin I can't open the app . The only option will be clear cache data from phone apps settings after this the app will be in the initial state.

Note : The user can't have cancel and login with pin in Biometric Prompt , only one option is available .

The option with Cancel Button :

The user can have a screen with two buttons one for the Login and one for cancel . If the user presses Login button the Biometric Prompt will appear the same flow like before . If the user presses Cancel button he can enter in the app without FingerPrint or Pin and the shortcuts will be there .

@jeffreygee @amedyne @jonalmeida please tell me your opinions . Thank you !

jeffreygee commented 2 years ago

@iorgamgabriel I think this flow makes sense to me. With scenario 4, if the user closes the app and reopens it again, will it trigger step 1 all over again?

iorgamgabriel commented 2 years ago

It will not trigger again step 1 . The app will be in the initial state without FingerPrint @jeffreygee

jeffreygee commented 2 years ago

@iorgamgabriel If (before the user clears cache data), the user fails fingerprint and passcode and just hard closes the app, but then reopens the app again, it'll start from Step 1 though right?

iorgamgabriel commented 2 years ago

@jeffreygee Yes , if the user doesn't clean cache data .

amedyne commented 2 years ago

@jeffreygee We need UX designs for some of the screens on the flow here. Thanks!

jeffreygee commented 2 years ago

@iorgamgabriel what's your rationale on #2 (As a user if I click onside the biometric Prompt (the grey pop-up with the fingerprint icon) this will appear)?

I was thinking that the natural reaction to getting the prompt for the fingerprint would be either closing the app via hitting home/back button, or putting in a PIN. This hitting outside the prompt seems unnecessary and I think we should limit the flow a bit here. But if this is a necessary fallback, we can proceed.

iorgamgabriel commented 2 years ago

This is the behaviour of BiometricPrompt . I can't force it to stay on the screen if the user clicks outside .But if the user presses again on Login button BiometricPrompt will appear. @jeffreygee

jeffreygee commented 2 years ago

@iorgamgabriel I mocked up the similar flow (on dark mode) here

iorgamgabriel commented 2 years ago

So when BiometricPrompt is visible will be the Logo and "Firefox Focus" and when it's not will be only the logo? This is correct ? Will not be the two buttons one for Login and the oder for Cancel? @jeffreygee

jeffreygee commented 2 years ago

Adding a note here that updates to the flow have been discussed with @iorgamgabriel and updated on the Figma

lobontiumira commented 2 years ago

Hi all! I've verified on the latest Focus Nightly 104.0a1 (build 361811708 with GV 104.0a1-20220630095519) with Oppo Reno 6 (Android 12), Google Pixel 6 (Android 12), and Oppo Find X3 Lite (Android 11), and here are the results:

fingerprint

Screenshot_5

@jeffreygee @iorgamgabriel I do have a concern. In Figma designs the "Use PIN" option is available to the user from the moment the app is accessed. I did not see this option until the device did not recognized the fingerprint.

Screenshot_4

iorgamgabriel commented 2 years ago

From what I saw in fenix they have another flow if the user doesn't have a fingerprint enrolled in the app . If the user have a pin or a password he can see the credit cards with that . In focus in the old implementation and I kept the same flow in the new one , the user can't unlock the app if he doesn't have a fingerprint enrolled. Should we support this ? I think this should be another feature. @amedyne @jeffreygee @jonalmeida

iorgamgabriel commented 2 years ago

"@jeffreygee @iorgamgabriel I do have a concern. In Figma designs the "Use PIN" option is available to the user from the moment the app is accessed. I did not see this option until the device did not recognized the fingerprint." This depends on Biometric implementation from android . It is different from a phone to another. @lobontiumira

lobontiumira commented 2 years ago

Thank you for the clarification. Closing this ticket as complete and verified.

lobontiumira commented 2 years ago

This is now implemented in Beta 104.0b1 also. Tested with Samsung Galaxy Note 8 (Android 9).