Add option to automatically present biometric opt-in and lock #4041
Conversation
Clang Static Analysis Issues
Generated by 🚫 Danger |
|
||||||||||||||||||||||
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #4041 +/- ##
==========================================
+ Coverage 65.73% 68.24% +2.51%
==========================================
Files 245 245
Lines 21458 21468 +10
==========================================
+ Hits 14105 14651 +546
+ Misses 7353 6817 -536
🚀 New features to boost your workflow:
|
|
||||||||||||||||||||
|
||||||||||||||
|
Just double checking, there wasn't any issue with having the new logic on top of the logic here in SFLoginViewController? Do they both get called but one ends up being a no-op? |
|
@bbirman good catch. Both can fire: The new logic in lock() fires first (synchronously after lock), before SFLoginViewController is presented (since login is async). That said, the existing logic at line 95 ( |
That sounds good! The only downside is that whatever the default is would be a change for one of the flows but 14.0 would be a good window for it, @brandonpage @wmathurin any thoughts on making automatic presentation default to |
|
@bbirman @wmathurin @brandonpage Is it okay to automatically present the biometric the first time? |
Yes, and I would honestly prefer if it was opt-out. That was my original intention, but the client who we built this for (and never used it) wanted to show their own dialog. UI styling are very consistent on iOS so I really don't see an issue with showing our prompt. Perhaps on Android we may want to allow the app to override the dialog. |
|
|
||
| if hasBiometricOptedIn() && automaticPresentation { | ||
| SFApplicationHelper.sharedApplication()?.connectedScenes.forEach() { scene in | ||
| presentBiometric(scene: scene) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why is this needed? The Login view controller should automatically present biometric upon lock if the user is opted in?
There was a problem hiding this comment.
This is for the native login path. When useNativeLogin is enabled, SFLoginViewController isn't used — the app's custom VC is presented instead, so the existing auto-present at line 95 of SFLoginViewController.m never fires. This ensures native login apps get the same auto-present behavior when automaticPresentation is enabled.
That said, for the standard (webview) login path, you're right — SFLoginViewController already handles it. Should we gate line 95 behind automaticPresentation too for consistency, or leave it as-is since it's existing behavior?
|
Should the code in SFLoginViewController be removed now that there's presentation logic for both paths in BiometricAuthenticationManagerInternal? |
@bbirman The SFLoginViewController code predates this and doesn't check automaticPresentation, so they're not fully equivalent. Consolidating them would be a good follow-up (we'd want SFLoginViewController to respect automaticPresentation too), but it's a behavioral change for existing webview-login apps that should be its own PR. Want me to file a story for it? |
|
Yes, that was the gap identified from the original proof of concept work on the internal branch, now that "automaticPresentation" defaults to true there shouldn't be a behavioral difference to the consuming app/user |
|
@bbirman I'll push another PR. |
4 participants
Summary
automaticPresentationproperty toBiometricAuthenticationManagerprotocol that, when enabled:falseto preserve existing behavior — apps must explicitly opt inChanges
automaticPresentationproperty to the public protocolfalse) and added auto-present logic at the end oflock()that iterates connected scenes and callspresentBiometric(scene:)when conditions are met!hasBiometricOptedIn && automaticPresentation)automaticPresentation×hasBiometricOptedInfor both lock and opt-in dialog scenariosautomaticPresentationto mock for protocol conformanceTest Matrix
Scenario A: Auto opt-in dialog after login
Scenario B: Auto biometric prompt on lock
Scenario C: Interaction with native login
Scenario D: Existing behavior (regression tests)
automaticPresentation = false(default)automaticPresentationautomaticPresentationpresentOptInDialogTest plan
automaticPresentation = false(default) → no change in existing behaviorResolves W-21981313