From c8a672874b8de43e36cad54bd49ba041ebca7185 Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Thu, 16 Jan 2025 10:40:30 -0800 Subject: [PATCH] Fix `useMfa` error handling (#50844) * Fix useMfa cancel logic to avoid duplicate error messages across dialog layers. * Add MfaCanceledError and use promise.reject for mfa cancel. * Address comments. * Fix test. * Remove outdated comment. * Add comment; Simplify who dialog logic. --- .../components/AuthnDialog/AuthnDialog.tsx | 15 +++-- web/packages/teleport/src/lib/useMfa.test.tsx | 11 ++-- web/packages/teleport/src/lib/useMfa.ts | 56 ++++++++++--------- 3 files changed, 45 insertions(+), 37 deletions(-) diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index ced5ff924ed3c..e8a53eb3365a4 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -23,21 +23,26 @@ import { Cross, FingerprintSimple } from 'design/Icon'; import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; -import { MfaState } from 'teleport/lib/useMfa'; +import { MfaCanceledError, MfaState } from 'teleport/lib/useMfa'; import { MFA_OPTION_TOTP } from 'teleport/services/mfa'; export type Props = { mfaState: MfaState; replaceErrorText?: string; + // onClose is an optional function to perform additional operations + // upon closing the dialog. e.g. close a shell session onClose?: () => void; }; export default function AuthnDialog({ - mfaState: { options, challenge, submit, attempt, resetAttempt }, + mfaState: { options, challenge, submit, attempt, cancelAttempt }, replaceErrorText, - onClose, + onClose = () => {}, }: Props) { - if (!challenge && attempt.status !== 'error') return; + const showError = + attempt.status === 'error' && !(attempt.error instanceof MfaCanceledError); + + if (!challenge && !showError) return; // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. const onlyTotpAvailable = @@ -50,7 +55,7 @@ export default function AuthnDialog({ { - resetAttempt(); + cancelAttempt(); onClose(); }} > diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index b3f5599ab1ce2..2420fc0e7a82d 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -27,7 +27,7 @@ import { MfaChallengeResponse, } from 'teleport/services/mfa'; -import { useMfa } from './useMfa'; +import { MfaCanceledError, useMfa } from './useMfa'; const mockChallenge: MfaAuthenticateChallenge = { webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, @@ -232,14 +232,15 @@ describe('useMfa', () => { expect(auth.getMfaChallenge).toHaveBeenCalled(); }); - mfa.current.resetAttempt(); + mfa.current.cancelAttempt(); - await expect(respPromise).rejects.toThrow( - new Error('MFA attempt cancelled by user') - ); + await expect(respPromise).rejects.toThrow(new MfaCanceledError()); await waitFor(() => { expect(mfa.current.attempt.status).toEqual('error'); }); + expect( + mfa.current.attempt.status === 'error' && mfa.current.attempt.error + ).toEqual(new MfaCanceledError()); }); }); diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 4d014f10e23ba..2bb8054665d69 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -43,7 +43,7 @@ export type MfaProps = { type mfaResponsePromiseWithResolvers = { promise: Promise; resolve: (v: MfaChallengeResponse) => void; - reject: (v?: any) => void; + reject: (err: Error) => void; }; /** @@ -57,9 +57,6 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [options, setMfaOptions] = useState(); const [challenge, setMfaChallenge] = useState(); - const mfaResponsePromiseWithResolvers = - useRef(); - useEffect(() => { setMfaRequired(isMfaRequired); }, [isMfaRequired]); @@ -77,7 +74,6 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { // 4. Receive the mfa response through the mfaResponsePromise ref and return it. // // The caller should also display errors seen in attempt. - const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( async (challenge?: MfaAuthenticateChallenge) => { @@ -90,50 +86,50 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { return; } - // Set mfa requirement and options after we get a challenge for the first time. - if (!mfaRequired) setMfaRequired(true); - if (!options) setMfaOptions(getMfaChallengeOptions(challenge)); - // Prepare a new promise to collect the mfa response retrieved // through the submit function. - let resolve, reject; + let resolve: (value: MfaChallengeResponse) => void; + let reject: (err: Error) => void; const promise = new Promise((res, rej) => { resolve = res; reject = rej; }); - mfaResponsePromiseWithResolvers.current = { + mfaResponseRef.current = { promise, resolve, reject, }; + // Set mfa requirement and options after we get a challenge for the first time. + setMfaRequired(true); + setMfaOptions(getMfaChallengeOptions(challenge)); + setMfaChallenge(challenge); try { return await promise; } finally { - mfaResponsePromiseWithResolvers.current = null; setMfaChallenge(null); } }, - [req, mfaResponsePromiseWithResolvers, options, mfaRequired] + [req, mfaRequired] ) ); - const resetAttempt = () => { - if (mfaResponsePromiseWithResolvers.current) - mfaResponsePromiseWithResolvers.current.reject( - new Error('MFA attempt cancelled by user') - ); - mfaResponsePromiseWithResolvers.current = null; - setMfaChallenge(null); - setMfaAttempt(makeEmptyAttempt()); + const mfaResponseRef = useRef(); + + const cancelAttempt = () => { + if (mfaResponseRef.current) { + mfaResponseRef.current.reject(new MfaCanceledError()); + } }; const getChallengeResponse = useCallback( async (challenge?: MfaAuthenticateChallenge) => { const [resp, err] = await getResponse(challenge); + if (err) throw err; + return resp; }, [getResponse] @@ -141,12 +137,12 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const submit = useCallback( async (mfaType?: DeviceType, totpCode?: string) => { - if (!mfaResponsePromiseWithResolvers.current) { + if (!mfaResponseRef.current) { throw new Error('submit called without an in flight MFA attempt'); } try { - await mfaResponsePromiseWithResolvers.current.resolve( + await mfaResponseRef.current.resolve( await auth.getMfaChallengeResponse(challenge, mfaType, totpCode) ); } catch (err) { @@ -158,7 +154,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { }); } }, - [challenge, mfaResponsePromiseWithResolvers, setMfaAttempt] + [challenge, setMfaAttempt] ); return { @@ -168,7 +164,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { getChallengeResponse, submit, attempt, - resetAttempt, + cancelAttempt, }; } @@ -207,7 +203,7 @@ export type MfaState = { ) => Promise; submit: (mfaType?: DeviceType, totpCode?: string) => Promise; attempt: Attempt; - resetAttempt: () => void; + cancelAttempt: () => void; }; // used for testing @@ -219,6 +215,12 @@ export function makeDefaultMfaState(): MfaState { getChallengeResponse: async () => null, submit: () => null, attempt: makeEmptyAttempt(), - resetAttempt: () => null, + cancelAttempt: () => null, }; } + +export class MfaCanceledError extends Error { + constructor() { + super('User canceled MFA attempt'); + } +}