From 6de6710b2b102310c6e61c351d58c73abe47be17 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 2 Jan 2025 19:56:17 -0800 Subject: [PATCH 1/6] 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 | 9 +- web/packages/teleport/src/lib/useMfa.ts | 84 +++++++++---------- 3 files changed, 51 insertions(+), 48 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..dad6b2645b70b 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -232,14 +232,17 @@ describe('useMfa', () => { expect(auth.getMfaChallenge).toHaveBeenCalled(); }); - mfa.current.resetAttempt(); + mfa.current.cancelAttempt(); await expect(respPromise).rejects.toThrow( - new Error('MFA attempt cancelled by user') + new Error('User canceled MFA attempt') ); + // If the user cancels the MFA attempt and closes the dialog, the mfa status + // should be 'success', or else the dialog would remain open to display the error. + // This error is meant to be handled by the caller. await waitFor(() => { - expect(mfa.current.attempt.status).toEqual('error'); + expect(mfa.current.attempt.status).toEqual('success'); }); }); }); diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 54d1299c65648..af1e3d531330e 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,63 @@ 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 canceled 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; + + if (resp instanceof Error) { + throw resp; + } + + 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 +158,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { }); } }, - [challenge, mfaResponsePromiseWithResolvers, setMfaAttempt] + [challenge, setMfaAttempt] ); return { @@ -168,7 +168,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { getChallengeResponse, submit, attempt, - resetAttempt, + cancelAttempt, }; } @@ -207,7 +207,7 @@ export type MfaState = { ) => Promise; submit: (mfaType?: DeviceType, totpCode?: string) => Promise; attempt: Attempt; - resetAttempt: () => void; + cancelAttempt: () => void; }; // used for testing @@ -219,6 +219,6 @@ export function makeDefaultMfaState(): MfaState { getChallengeResponse: async () => null, submit: () => null, attempt: makeEmptyAttempt(), - resetAttempt: () => null, + cancelAttempt: () => null, }; } From bce38211b4c3b4f3a89ad0d7535911797782d274 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 13 Jan 2025 10:39:44 -0800 Subject: [PATCH 2/6] Add MfaCanceledError and use promise.reject for mfa cancel. --- .../components/AuthnDialog/AuthnDialog.tsx | 9 +++++-- web/packages/teleport/src/lib/useMfa.ts | 24 ++++++++++++------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index c03af9be9c4e0..9ef08d48cdf30 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -23,7 +23,7 @@ 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 = { @@ -37,7 +37,12 @@ export default function AuthnDialog({ replaceErrorText, onClose = () => {}, }: Props) { - if (!challenge && attempt.status !== 'error') return; + const showMfaDialog = + !!challenge || + (attempt.status === 'error' && + !(attempt.error instanceof MfaCanceledError)); + + if (!showMfaDialog) return; // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. const onlyTotpAvailable = diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index af1e3d531330e..dfb2c437064d0 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -41,8 +41,9 @@ export type MfaProps = { }; type mfaResponsePromiseWithResolvers = { - promise: Promise; - resolve: (v: MfaChallengeResponse | Error) => void; + promise: Promise; + resolve: (v: MfaChallengeResponse) => void; + reject: (err: Error) => void; }; /** @@ -108,7 +109,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const cancelAttempt = () => { if (mfaResponseRef.current) { - mfaResponseRef.current.resolve(new Error('User canceled MFA attempt')); + mfaResponseRef.current.reject(new MfaCanceledError()); } }; @@ -116,24 +117,23 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { async (challenge?: MfaAuthenticateChallenge) => { // 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 => { + let resolve: (value: MfaChallengeResponse) => void; + let reject: (err: Error) => void; + const promise = new Promise((res, rej) => { resolve = res; + reject = rej; }); mfaResponseRef.current = { promise, resolve, + reject, }; const [resp, err] = await getResponse(promise, challenge); if (err) throw err; - if (resp instanceof Error) { - throw resp; - } - return resp as MfaChallengeResponse; }, [getResponse] @@ -222,3 +222,9 @@ export function makeDefaultMfaState(): MfaState { cancelAttempt: () => null, }; } + +export class MfaCanceledError extends Error { + constructor() { + super('User canceled MFA attempt'); + } +} From c3ad8367e8ad75e83f02353ff298afaf419df6d5 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 14 Jan 2025 10:15:02 -0800 Subject: [PATCH 3/6] Address comments. --- web/packages/teleport/src/lib/useMfa.ts | 42 +++++++++++-------------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index dfb2c437064d0..7f05e106dc408 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -74,13 +74,9 @@ 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 ( - responsePromise: Promise, - challenge?: MfaAuthenticateChallenge - ) => { + async (challenge?: MfaAuthenticateChallenge) => { // If a previous call determined that MFA is not required, this is a noop. if (mfaRequired === false) return; @@ -90,13 +86,28 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { return; } + // Prepare a new promise to collect the mfa response retrieved + // through the submit function. + let resolve: (value: MfaChallengeResponse) => void; + let reject: (err: Error) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + + 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 responsePromise; + return await promise; } finally { setMfaChallenge(null); } @@ -115,26 +126,11 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const getChallengeResponse = useCallback( async (challenge?: MfaAuthenticateChallenge) => { - // Prepare a new promise to collect the mfa response retrieved - // through the submit function. - let resolve: (value: MfaChallengeResponse) => void; - let reject: (err: Error) => void; - const promise = new Promise((res, rej) => { - resolve = res; - reject = rej; - }); - - mfaResponseRef.current = { - promise, - resolve, - reject, - }; - - const [resp, err] = await getResponse(promise, challenge); + const [resp, err] = await getResponse(challenge); if (err) throw err; - return resp as MfaChallengeResponse; + return resp; }, [getResponse] ); From 740e2ac85dfb746b07d35d35232c2f565bc3c4f4 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 14 Jan 2025 11:48:19 -0800 Subject: [PATCH 4/6] Fix test. --- web/packages/teleport/src/lib/useMfa.test.tsx | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index dad6b2645b70b..afea424c1b8a4 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, @@ -234,15 +234,16 @@ describe('useMfa', () => { mfa.current.cancelAttempt(); - await expect(respPromise).rejects.toThrow( - new Error('User canceled MFA attempt') - ); + await expect(respPromise).rejects.toThrow(new MfaCanceledError()); // If the user cancels the MFA attempt and closes the dialog, the mfa status // should be 'success', or else the dialog would remain open to display the error. // This error is meant to be handled by the caller. await waitFor(() => { - expect(mfa.current.attempt.status).toEqual('success'); + expect(mfa.current.attempt.status).toEqual('error'); }); + expect( + mfa.current.attempt.status === 'error' && mfa.current.attempt.error + ).toEqual(new MfaCanceledError()); }); }); From 2041cb159a6f893a5cfdc6f95efdd4ceed2f3147 Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 15 Jan 2025 10:26:28 -0800 Subject: [PATCH 5/6] Remove outdated comment. --- web/packages/teleport/src/lib/useMfa.test.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index afea424c1b8a4..2420fc0e7a82d 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -236,9 +236,6 @@ describe('useMfa', () => { await expect(respPromise).rejects.toThrow(new MfaCanceledError()); - // If the user cancels the MFA attempt and closes the dialog, the mfa status - // should be 'success', or else the dialog would remain open to display the error. - // This error is meant to be handled by the caller. await waitFor(() => { expect(mfa.current.attempt.status).toEqual('error'); }); From 454a2d265abbc56709b9123c91efb23fae2c4b54 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 16 Jan 2025 10:34:59 -0800 Subject: [PATCH 6/6] Add comment; Simplify who dialog logic. --- .../src/components/AuthnDialog/AuthnDialog.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index 9ef08d48cdf30..e8a53eb3365a4 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -29,6 +29,8 @@ 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; }; @@ -37,12 +39,10 @@ export default function AuthnDialog({ replaceErrorText, onClose = () => {}, }: Props) { - const showMfaDialog = - !!challenge || - (attempt.status === 'error' && - !(attempt.error instanceof MfaCanceledError)); + const showError = + attempt.status === 'error' && !(attempt.error instanceof MfaCanceledError); - if (!showMfaDialog) return; + if (!challenge && !showError) return; // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. const onlyTotpAvailable =