-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Bug fix: first biometric auth after log out/in sees unnecessary soft prompt #86291
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
Changes from all commits
c3dd7be
357f0fc
9c2c124
768afc1
676ccff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,10 @@ import type DotLottieAnimation from '@components/LottieAnimations/types'; | |
| import useBiometrics from '@components/MultifactorAuthentication/biometrics/useBiometrics'; | ||
| import {MULTIFACTOR_AUTHENTICATION_PROMPT_UI} from '@components/MultifactorAuthentication/config'; | ||
| import type {MultifactorAuthenticationPromptType} from '@components/MultifactorAuthentication/config/types'; | ||
| import useCurrentUserPersonalDetails from '@hooks/useCurrentUserPersonalDetails'; | ||
| import useOnyx from '@hooks/useOnyx'; | ||
| import {getDeviceBiometricsOnyxKey} from '@libs/actions/MultifactorAuthentication'; | ||
| import type {TranslationPaths} from '@src/languages/types'; | ||
| import ONYXKEYS from '@src/ONYXKEYS'; | ||
| import type IconAsset from '@src/types/utils/IconAsset'; | ||
| import {useMultifactorAuthenticationState} from './State'; | ||
|
|
||
|
|
@@ -29,8 +30,9 @@ type PromptContent = { | |
| function usePromptContent(promptType: MultifactorAuthenticationPromptType): PromptContent { | ||
| const state = useMultifactorAuthenticationState(); | ||
| const {areLocalCredentialsKnownToServer} = useBiometrics(); | ||
| const {accountID} = useCurrentUserPersonalDetails(); | ||
| const [serverHasCredentials, setServerHasCredentials] = useState(false); | ||
| const [deviceBiometricsState] = useOnyx(ONYXKEYS.DEVICE_BIOMETRICS); | ||
| const [deviceBiometricsState] = useOnyx(getDeviceBiometricsOnyxKey(accountID)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This read now only checks Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is known and acceptable - this is not a commonly used feature, as the main use for it is behind a beta, and the UX cost of re-registration is low. I don't think it's worth the complexity to add a migration. |
||
| const hasEverAcceptedSoftPrompt = deviceBiometricsState?.hasAcceptedSoftPrompt ?? false; | ||
|
|
||
| // We need to know if server has this device's credentials specifically | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
Onyx.connectWithoutViewin components is a massive anti-pattern, and if it’s possible for this code to work withuseOnyx, it would definitely be better to use that instead.In
useEffect, thedepsarray isn’t fully populated anyway, so omitting one additional dependency shouldn’t be a problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I don’t think useOnyx would cause process() to trigger too frequently. Could you clarify in more detail why we shouldn’t use useOnyx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Experimentally, it did though 🙈. The
processfunction relies on being called precisely when it should be and never more, and adding a subscription to this flag caused one extra call when we set the flag.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it feels icky, but we've already talked about refactoring this file and agreed it's not a short term priority (connectWithoutView was the pattern already established in this file)