For #18264: add biometric prompt to credit card settings #19505

eliserichards · 2021-05-13T22:25:28Z

Followup to use prompt helper in logins auth fragment: #19547

In this PR:

Mirror the authentication flow for saved logins
Create BiometricPromptPreferenceFragment.kt to be used with CC, addresses (future), and logins (Use BiometricPromptPreferenceFragment for saved logins #19547)
Management fragment is secure
Management fragment pops you back to reauth when the fragment is paused
CC Editor is secure
Editor pops back to reauth when the fragment is paused

Screenshots

secure on pause

Whenever the app is paused (backgrounded, etc) after we authenticate, we shouldn't be able to see what's on the screen. This is the same functionality as logins.

authenticate to see cards

When navigating to the list of credit cards from settings, we prompt the user to authenticate (or prompt them to make a device pin/password if they don't have one)

reauth on pause - list of cards

If we have authenticated and see the list of credit cards, if the user pauses the app we pop them back to the CC settings so they have to reauth to see the list again.

reauth on pause - editing card

If we are in the credit card editor, if the user pauses the app we pop them back to the CC settings so they have to reauth to see the cards again.

Pull Request checklist

Tests: This PR includes thorough tests or an explanation of why it does not
Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

To download an APK when reviewing a PR:

click on Show All Checks,
click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
click on the "Fenix - assemble" task, then click "Run Artifacts".
the APK links should be on the left side of the screen, named for each CPU architecture

codecov-commenter · 2021-05-18T18:10:41Z

Codecov Report

Merging #19505 (b5bff93) into master (bd8facb) will decrease coverage by 0.11%.
The diff coverage is 14.89%.

@@             Coverage Diff              @@
##             master   #19505      +/-   ##
============================================
- Coverage     35.52%   35.41%   -0.12%     
+ Complexity     1649     1648       -1     
============================================
  Files           544      545       +1     
  Lines         21949    22022      +73     
  Branches       3252     3260       +8     
============================================
  Hits           7798     7798              
- Misses        13254    13325      +71     
- Partials        897      899       +2

Impacted Files	Coverage Δ
...pp/src/main/java/org/mozilla/fenix/ext/Fragment.kt	`13.79% <0.00%> (-1.03%)`	⬇️
...x/settings/creditcards/CreditCardEditorFragment.kt	`0.00% <0.00%> (ø)`
...tings/creditcards/CreditCardsManagementFragment.kt	`0.00% <0.00%> (ø)`
...enix/settings/logins/fragment/EditLoginFragment.kt	`0.00% <0.00%> (ø)`
...ix/settings/logins/fragment/LoginDetailFragment.kt	`0.00% <0.00%> (ø)`
...ettings/logins/fragment/SavedLoginsAuthFragment.kt	`0.00% <0.00%> (ø)`
...ix/settings/logins/fragment/SavedLoginsFragment.kt	`0.00% <0.00%> (ø)`
...ngs/biometric/BiometricPromptPreferenceFragment.kt	`7.69% <7.69%> (ø)`
...settings/creditcards/CreditCardsSettingFragment.kt	`30.23% <20.00%> (-12.88%)`	⬇️
.../src/main/java/org/mozilla/fenix/utils/Settings.kt	`72.77% <100.00%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd8facb...b5bff93. Read the comment docs.

mergify · 2021-05-19T16:09:33Z

This pull request has conflicts when rebasing. Could you fix it @eliserichards? 🙏

app/src/main/res/values/strings.xml

app/src/main/java/org/mozilla/fenix/ext/Fragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardEditorFragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsManagementFragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

app/src/main/java/org/mozilla/fenix/settings/biometric/BiometricPromptPreferenceFragment.kt

Addressed comments :)

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsManagementFragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

app/src/main/java/org/mozilla/fenix/settings/biometric/BiometricPromptFeature.kt

app/src/main/java/org/mozilla/fenix/ext/Fragment.kt

app/src/main/java/org/mozilla/fenix/settings/biometric/BiometricPromptPreferenceFragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardEditorFragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

grigoryk · 2021-05-21T08:06:20Z

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

-                    CreditCardsSettingFragmentDirections
-                        .actionCreditCardsSettingFragmentToCreditCardsManagementFragment()
-                findNavController().navigateBlockingForAsyncNavGraph(directions)
+                verifyCredentialsOrShowSetupWarning(preference.context, creditCardPreferences)


Just a little drive-by comment, but I figured I'll chime in...

There's no reason to require authentication to display a list of cards, or even to open a specific card. Sensitive information (card number) should not be displayed right away. We can't actually display it - it's encrypted! So, that's what I imagined would be guarded by an auth prompt - the action to display a specific credit card number.

To put it in other words - there's a cryptographic key that we need to fetch in order to decrypt or encrypt sensitive information related to a credit card number. The act of obtaining a key, while not technically necessary, should be protected by some authentication mechanism (e.g. a biometrics prompt).

What I think is happening in this PR is more akin to how we're treating logins currently. That's not wrong, of course. However, even for logins we'll be soon switching to a per-field authentication model (vs per-db, as it is currently), which would mean that only username/password fields will be encrypted, while the rest will be in plaintext. That'll allow us to follow the same pattern for both credit cards and logins at the product UX level (e.g. require auth only when accessing sensitive information).

Thanks for the context! We should probably check in with UX on this because AFAIK this is the user authentication flow that they want (for now)

mergify · 2021-05-24T08:10:51Z

This pull request has conflicts when rebasing. Could you fix it @eliserichards? 🙏

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

mergify · 2021-05-26T16:39:00Z

This pull request has conflicts when rebasing. Could you fix it @eliserichards? 🙏

eliserichards · 2021-05-26T18:57:11Z

Confirmed with @topotropic that we should be securing CCs the same way we secure logins, pending discussion with Grisha and GL.

… is paused

…ture

Cleanup Rename biometric prompt helper to BiometricPromptPreferenceFragment. Add authentication for eeditor fragment. Add newline between unrelated strings Cleanup nits Remove secure flags now that we are using the secure fragment Move navigation inline for cc settings fragment. Improve kdocs in biometric prompt pref fragment. Improve function name for toggling prefs. Make fragment redirect to reauth generic for logins and cc. Increment security warning when we show pin on credit card settings

app/src/main/java/org/mozilla/fenix/settings/biometric/BiometricPromptPreferenceFragment.kt

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

gabrielluong · 2021-05-27T02:36:30Z

Seeing this in the failure

java.lang.IllegalStateException: Fragment CreditCardsSettingFragment{3765b2a8} (1aeee736-e127-4a19-a7a2-770054a54a3b) CreditCardsSettingFragmentTest} does not have a NavController set
	at androidx.navigation.fragment.NavHostFragment.findNavController(NavHostFragment.java:131)
	at androidx.navigation.fragment.FragmentKt.findNavController(Fragment.kt:29)
	at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragment$updateCardManagementPreference$1.onPreferenceClick(CreditCardsSettingFragment.kt:171)
	at androidx.preference.Preference.performClick(Preference.java:1184)
	at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragmentTest.GIVEN the list of credit cards is empty, WHEN fragment is displayed THEN the manage credit cards pref is 'Add card'(CreditCardsSettingFragmentTest.kt:99)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:546)
	at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:252)
	at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:89)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

gabrielluong · 2021-05-27T06:21:02Z

java.lang.IllegalStateException: Can't access the Fragment View's LifecycleOwner when getView() is null i.e., before onCreateView() or after onDestroyView()
	at androidx.fragment.app.Fragment.getViewLifecycleOwner(Fragment.java:328)
	at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragment.navigateOnSuccess(CreditCardsSettingFragment.kt:68)
	at org.mozilla.fenix.settings.biometric.BiometricPromptPreferenceFragment.verifyCredentialsOrShowSetupWarning(BiometricPromptPreferenceFragment.kt:106)
	at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragment$updateCardManagementPreference$1.onPreferenceClick(CreditCardsSettingFragment.kt:170)
	at androidx.preference.Preference.performClick(Preference.java:1184)
	at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragmentTest.GIVEN the list of credit cards is not empty, WHEN fragment is displayed THEN the manage credit cards pref is 'Manage saved cards'(CreditCardsSettingFragmentTest.kt:73)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
	at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:546)
	at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:252)
	at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:89)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

gabrielluong · 2021-05-27T04:54:57Z

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

@@ -69,8 +103,11 @@ class CreditCardsSettingFragment : PreferenceFragmentCompat() {
    override fun onViewCreated(view: View, savedInstanceState: Bundle?) {
        super.onViewCreated(view, savedInstanceState)
        consumeFrom(creditCardsStore) { state ->
-            updateCardManagementPreference(state.creditCards.isNotEmpty(), findNavController())
+            hasCreditCards = state.creditCards.isNotEmpty()
+            updateCardManagementPreference(state.creditCards.isNotEmpty())


I always get this comment whenever I did this, but it seems fair that I would repeat my learning that hasCreditCarrdscan be used here

gabrielluong · 2021-05-27T04:58:00Z

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt

    }

    /**
     * Updates preferences visibility depending on credit cards being already saved or not.
     */
    @VisibleForTesting
    internal fun updateCardManagementPreference(
-        hasCreditCards: Boolean,
-        navController: NavController


I think passing the navController was intentional here looking at the CI error.

java.lang.IllegalStateException: Fragment CreditCardsSettingFragment{3765b2a8} (1aeee736-e127-4a19-a7a2-770054a54a3b) CreditCardsSettingFragmentTest} does not have a NavController set at androidx.navigation.fragment.NavHostFragment.findNavController(NavHostFragment.java:131) at androidx.navigation.fragment.FragmentKt.findNavController(Fragment.kt:29) at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragment$updateCardManagementPreference$1.onPreferenceClick(CreditCardsSettingFragment.kt:171) at androidx.preference.Preference.performClick(Preference.java:1184) at org.mozilla.fenix.settings.creditcards.CreditCardsSettingFragmentTest.GIVEN the list of credit cards is empty, WHEN fragment is displayed THEN the manage credit cards pref is 'Add card'(CreditCardsSettingFragmentTest.kt:99) at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50) at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12) at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47) at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17) at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26) at org.robolectric.RobolectricTestRunner$HelperTestRunner$1.evaluate(RobolectricTestRunner.java:546) at org.robolectric.internal.SandboxTestRunner$2.lambda$evaluate$0(SandboxTestRunner.java:252) at org.robolectric.internal.bytecode.Sandbox.lambda$runOnMainThread$0(Sandbox.java:89) at java.util.concurrent.FutureTask.run(FutureTask.java:266) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:748)

gabrielluong · 2021-05-27T05:12:41Z

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt


    private lateinit var creditCardsStore: CreditCardsFragmentStore
+    private var hasCreditCards: Boolean = false


This one is a bit weird since we have a variable here, but I don't think this is ever being used since we are always passing this into updateCardManagementPreference.

eliserichards requested review from a team as code owners May 13, 2021 22:25

eliserichards force-pushed the 18264-cc-biometrics branch from ae9a184 to 700f429 Compare May 13, 2021 22:31

gabrielluong added the pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on label May 14, 2021

gabrielluong self-requested a review May 14, 2021 01:16

gabrielluong self-assigned this May 14, 2021

eliserichards force-pushed the 18264-cc-biometrics branch from 700f429 to e0b3df0 Compare May 18, 2021 17:52

eliserichards added needs:review PRs that need to be reviewed and removed pr:work-in-progress PRs that are not ready to be reviewed yet and are actively being worked on labels May 18, 2021

eliserichards force-pushed the 18264-cc-biometrics branch 3 times, most recently from a82bd08 to e65b0bf Compare May 18, 2021 18:52

eliserichards mentioned this pull request May 18, 2021

[Bug] Edit card section is not displayed in landscape mode after resuming Fenix #19494

Closed

eliserichards force-pushed the 18264-cc-biometrics branch from e65b0bf to 299ab14 Compare May 19, 2021 18:28

gabrielluong reviewed May 20, 2021

View reviewed changes

app/src/main/res/values/strings.xml Show resolved Hide resolved

gabrielluong previously requested changes May 20, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/ext/Fragment.kt Show resolved Hide resolved

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardEditorFragment.kt Outdated Show resolved Hide resolved

gabrielluong reviewed May 20, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsManagementFragment.kt Outdated Show resolved Hide resolved

gabrielluong reviewed May 20, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsManagementFragment.kt Show resolved Hide resolved

gabrielluong reviewed May 20, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt Outdated Show resolved Hide resolved

gabrielluong reviewed May 20, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/biometric/BiometricPromptPreferenceFragment.kt Show resolved Hide resolved

gabrielluong suggested changes May 21, 2021

View reviewed changes

grigoryk reviewed May 21, 2021

View reviewed changes

mcarare reviewed May 24, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt Show resolved Hide resolved

eliserichards force-pushed the 18264-cc-biometrics branch 2 times, most recently from 669db86 to b59780b Compare May 24, 2021 18:35

eliserichards requested a review from gabrielluong May 25, 2021 18:27

eliserichards force-pushed the 18264-cc-biometrics branch from b59780b to e8631e8 Compare May 25, 2021 19:58

eliserichards added 3 commits May 26, 2021 13:57

Set secure flags in credit card list and reauthenticate when fragment…

b754bea

… is paused

Create helper class for biometric prompts to build and manage the fea…

780d262

…ture

Create authentication for credit cards using the biometric prompt helper

254d307

eliserichards force-pushed the 18264-cc-biometrics branch from e8631e8 to 4db61e2 Compare May 26, 2021 18:59

eliserichards force-pushed the 18264-cc-biometrics branch from 4db61e2 to 7889a73 Compare May 26, 2021 19:03

gabrielluong suggested changes May 26, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/biometric/BiometricPromptPreferenceFragment.kt Show resolved Hide resolved

gabrielluong reviewed May 26, 2021

View reviewed changes

app/src/main/java/org/mozilla/fenix/settings/creditcards/CreditCardsSettingFragment.kt Show resolved Hide resolved

Update hasCards state and verify navigation when manage ccs is selected

67303bf

gabrielluong added 2 commits May 27, 2021 01:18

Fix test errors

7267fb1

Minor rewording

26c5d21

gabrielluong added 3 commits May 27, 2021 02:31

Fix tests

9705a40

Prevent the "Saved cards" toolbar title from being hidden

ee398e1

Remove unused imports

b5bff93

gabrielluong reviewed May 27, 2021

View reviewed changes

gabrielluong approved these changes May 27, 2021

View reviewed changes

gabrielluong merged commit 279d598 into mozilla-mobile:master May 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #18264: add biometric prompt to credit card settings #19505

For #18264: add biometric prompt to credit card settings #19505

eliserichards commented May 13, 2021 •

edited

codecov-commenter commented May 18, 2021 •

edited

mergify bot commented May 19, 2021

grigoryk May 21, 2021

eliserichards May 21, 2021

mergify bot commented May 24, 2021

mergify bot commented May 26, 2021

eliserichards commented May 26, 2021

gabrielluong commented May 27, 2021

gabrielluong commented May 27, 2021

gabrielluong May 27, 2021

gabrielluong May 27, 2021

gabrielluong May 27, 2021


		private lateinit var creditCardsStore: CreditCardsFragmentStore
		private var hasCreditCards: Boolean = false

For #18264: add biometric prompt to credit card settings #19505

For #18264: add biometric prompt to credit card settings #19505

Conversation

eliserichards commented May 13, 2021 • edited

Screenshots

secure on pause

authenticate to see cards

reauth on pause - list of cards

reauth on pause - editing card

Pull Request checklist

To download an APK when reviewing a PR:

codecov-commenter commented May 18, 2021 • edited

Codecov Report

mergify bot commented May 19, 2021

grigoryk May 21, 2021

Choose a reason for hiding this comment

eliserichards May 21, 2021

Choose a reason for hiding this comment

mergify bot commented May 24, 2021

mergify bot commented May 26, 2021

eliserichards commented May 26, 2021

gabrielluong commented May 27, 2021

gabrielluong commented May 27, 2021

gabrielluong May 27, 2021

Choose a reason for hiding this comment

gabrielluong May 27, 2021

Choose a reason for hiding this comment

gabrielluong May 27, 2021

Choose a reason for hiding this comment

eliserichards commented May 13, 2021 •

edited

codecov-commenter commented May 18, 2021 •

edited