From 01d553dd41b49144f909073dcdf19d54846eaa33 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 2 Jan 2025 19:56:17 -0800 Subject: [PATCH] Fix useMfa cancel logic to avoid duplicate error messages across dialog layers. --- .../components/AuthnDialog/AuthnDialog.tsx | 6 +- web/packages/teleport/src/lib/useMfa.test.tsx | 2 +- web/packages/teleport/src/lib/useMfa.ts | 83 +++++++++---------- 3 files changed, 45 insertions(+), 46 deletions(-) diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index ced5ff924ed3c..c03af9be9c4e0 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -33,9 +33,9 @@ export type Props = { }; 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; @@ -50,7 +50,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..fff8970eb3112 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -232,7 +232,7 @@ describe('useMfa', () => { expect(auth.getMfaChallenge).toHaveBeenCalled(); }); - mfa.current.resetAttempt(); + mfa.current.cancelAttempt(); await expect(respPromise).rejects.toThrow( new Error('MFA attempt cancelled by user') diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 54d1299c65648..352b394322026 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -41,9 +41,8 @@ export type MfaProps = { }; type mfaResponsePromiseWithResolvers = { - promise: Promise; - resolve: (v: MfaChallengeResponse) => void; - reject: (v?: any) => void; + promise: Promise; + resolve: (v: MfaChallengeResponse | Error) => void; }; /** @@ -57,9 +56,6 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [options, setMfaOptions] = useState(); const [challenge, setMfaChallenge] = useState(); - const mfaResponsePromiseWithResolvers = - useRef(); - useEffect(() => { setMfaRequired(isMfaRequired); }, [isMfaRequired]); @@ -80,7 +76,10 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( - async (challenge?: MfaAuthenticateChallenge) => { + async ( + responsePromise: Promise, + challenge?: MfaAuthenticateChallenge + ) => { // If a previous call determined that MFA is not required, this is a noop. if (mfaRequired === false) return; @@ -91,62 +90,62 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { } // 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; - const promise = new Promise((res, rej) => { - resolve = res; - reject = rej; - }); - - mfaResponsePromiseWithResolvers.current = { - promise, - resolve, - reject, - }; + setMfaRequired(true); + setMfaOptions(getMfaChallengeOptions(challenge)); setMfaChallenge(challenge); try { - return await promise; + return await responsePromise; } 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.resolve(new Error('User cancelled MFA attempt')); + } }; const getChallengeResponse = useCallback( async (challenge?: MfaAuthenticateChallenge) => { - const [resp, err] = await getResponse(challenge); + // Prepare a new promise to collect the mfa response retrieved + // through the submit function. + let resolve: (value: MfaChallengeResponse | Error) => void; + const promise = new Promise(res => { + resolve = res; + }); + + mfaResponseRef.current = { + promise, + resolve, + }; + + const [resp, err] = await getResponse(promise, challenge); + if (err) throw err; - return resp; + + const respErr = resp as Error; + if (respErr) throw respErr; + + return resp as MfaChallengeResponse; }, [getResponse] ); 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 +157,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { }); } }, - [challenge, mfaResponsePromiseWithResolvers, setMfaAttempt] + [challenge, setMfaAttempt] ); return { @@ -168,7 +167,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { getChallengeResponse, submit, attempt, - resetAttempt, + cancelAttempt, }; } @@ -207,7 +206,7 @@ export type MfaState = { ) => Promise; submit: (mfaType?: DeviceType, totpCode?: string) => Promise; attempt: Attempt; - resetAttempt: () => void; + cancelAttempt: () => void; }; // used for testing @@ -219,6 +218,6 @@ export function makeDefaultMfaState(): MfaState { getChallengeResponse: async () => null, submit: () => null, attempt: makeEmptyAttempt(), - resetAttempt: () => null, + cancelAttempt: () => null, }; }