From 549728066b9e88c97a510f61ac6c03a243654b1c Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 12:55:54 -0800 Subject: [PATCH] Make useMfa and AuthnDialog more reusable and error proof. --- .../DocumentKubeExec/DocumentKubeExec.tsx | 14 +- .../src/Console/DocumentSsh/DocumentSsh.tsx | 39 +++-- .../Console/DocumentSsh/useFileTransfer.ts | 76 +------- .../src/DesktopSession/DesktopSession.tsx | 13 +- .../src/DesktopSession/useDesktopSession.tsx | 4 +- .../AuthnDialog/AuthnDialog.test.tsx | 18 +- .../components/AuthnDialog/AuthnDialog.tsx | 103 ++++++----- web/packages/teleport/src/lib/useMfa.ts | 164 ++++++++++++++---- 8 files changed, 248 insertions(+), 183 deletions(-) diff --git a/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx b/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx index 9db31e4936592..acdf66da843a1 100644 --- a/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx +++ b/web/packages/teleport/src/Console/DocumentKubeExec/DocumentKubeExec.tsx @@ -22,7 +22,7 @@ import { Box, Indicator } from 'design'; import * as stores from 'teleport/Console/stores/types'; import { Terminal, TerminalRef } from 'teleport/Console/DocumentSsh/Terminal'; -import { useMfa } from 'teleport/lib/useMfa'; +import { useMfaTty } from 'teleport/lib/useMfa'; import useKubeExecSession from 'teleport/Console/DocumentKubeExec/useKubeExecSession'; import Document from 'teleport/Console/Document'; @@ -39,11 +39,11 @@ export default function DocumentKubeExec({ doc, visible }: Props) { const terminalRef = useRef(); const { tty, status, closeDocument, sendKubeExecData } = useKubeExecSession(doc); - const mfa = useMfa(tty); + const mfa = useMfaTty(tty); useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.mfaChallenge]); + }, [visible, mfa.challenge]); const theme = useTheme(); const terminal = ( @@ -63,7 +63,13 @@ export default function DocumentKubeExec({ doc, visible }: Props) { )} - {mfa.mfaChallenge && } + { + mfa.resetAttempt(); + closeDocument(); + }} + /> {status === 'waiting-for-exec-data' && ( diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index 4ea1b934e076c..4ea493ecfea42 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -32,7 +32,8 @@ import { TerminalSearch } from 'shared/components/TerminalSearch'; import * as stores from 'teleport/Console/stores'; import AuthnDialog from 'teleport/components/AuthnDialog'; -import { useMfa } from 'teleport/lib/useMfa'; +import { useMfa, useMfaTty } from 'teleport/lib/useMfa'; +import { MfaChallengeScope } from 'teleport/services/auth/auth'; import Document from '../Document'; @@ -56,8 +57,15 @@ function DocumentSsh({ doc, visible }: PropTypes) { const terminalRef = useRef(); const { tty, status, closeDocument, session } = useSshSession(doc); const [showSearch, setShowSearch] = useState(false); - const mfa = useMfa(tty); - const ft = useFileTransfer(tty, session, doc, mfa.mfaRequired); + + const ttyMfa = useMfaTty(tty); + const ftMfa = useMfa({ + isMfaRequired: ttyMfa.required, + req: { + scope: MfaChallengeScope.USER_SESSION, + }, + }); + const ft = useFileTransfer(tty, session, doc, ftMfa); const theme = useTheme(); function handleCloseFileTransfer() { @@ -71,7 +79,7 @@ function DocumentSsh({ doc, visible }: PropTypes) { useEffect(() => { // when switching tabs or closing tabs, focus on visible terminal terminalRef.current?.focus(); - }, [visible, mfa.mfaChallenge]); + }, [visible, ftMfa.challenge]); const onSearchClose = useCallback(() => { setShowSearch(false); @@ -111,13 +119,9 @@ function DocumentSsh({ doc, visible }: PropTypes) { beforeClose={() => window.confirm('Are you sure you want to cancel file transfers?') } - errorText={ - ft.mfaAttempt.status === 'error' ? ft.mfaAttempt.statusText : null - } afterClose={handleCloseFileTransfer} transferHandlers={{ - getDownloader: ft.getDownloader, - getUploader: ft.getUploader, + ...ft, }} /> @@ -136,15 +140,14 @@ function DocumentSsh({ doc, visible }: PropTypes) { )} - {mfa.mfaChallenge && } - {ft.mfaChallenge && ( - - )} + { + ttyMfa.resetAttempt(); + closeDocument(); + }} + /> + {status === 'initialized' && terminal} ); diff --git a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts index 0439574200501..92c4c9976a198 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts +++ b/web/packages/teleport/src/Console/DocumentSsh/useFileTransfer.ts @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -import { useCallback, useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useState } from 'react'; import { useFileTransferContext } from 'shared/components/FileTransfer'; import { DocumentSsh } from 'teleport/Console/stores'; @@ -24,14 +24,9 @@ import { EventType } from 'teleport/lib/term/enums'; import Tty from 'teleport/lib/term/tty'; import { Session } from 'teleport/services/session'; -import { useAsync } from 'shared/hooks/useAsync'; import cfg from 'teleport/config'; -import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; -import { - DeviceType, - MfaAuthenticateChallenge, - MfaChallengeResponse, -} from 'teleport/services/mfa'; + +import { MfaState } from 'teleport/lib/useMfa'; import { useConsoleContext } from '../consoleContextProvider'; @@ -59,7 +54,7 @@ export const useFileTransfer = ( tty: Tty, session: Session, currentDoc: DocumentSsh, - addMfaToScpUrls: boolean + mfa: MfaState ) => { const { filesStore } = useFileTransferContext(); const startTransfer = filesStore.start; @@ -70,60 +65,13 @@ export const useFileTransfer = ( >([]); const { clusterId, serverId, login } = currentDoc; - const [mfaChallenge, setMfaChallenge] = useState(); - const mfaResponsePromise = - useRef>(); - - const clearMfaChallenge = () => { - setMfaChallenge(null); - }; - - const [mfaAttempt, getMfaResponse] = useAsync( - useCallback(async () => { - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.USER_SESSION, - isMfaRequiredRequest: { - node: { - node_name: serverId, - login: login, - }, - }, - }); - if (!challenge) return; - - setMfaChallenge(challenge); - mfaResponsePromise.current = Promise.withResolvers(); - const mfaResponse = await mfaResponsePromise.current.promise; - return mfaResponse; - }, [setMfaChallenge, clearMfaChallenge, mfaResponsePromise]) - ); - - const [submitMfaAttempt, submitMfa] = useAsync( - useCallback( - async (mfaType?: DeviceType) => { - try { - const resp = auth.getMfaChallengeResponse(mfaChallenge, mfaType); - mfaResponsePromise.current.resolve(resp); - } catch (err) { - mfaResponsePromise.current.reject(err); - throw err; - } - }, - [mfaChallenge, mfaResponsePromise] - ) - ); - const download = useCallback( async ( location: string, abortController: AbortController, moderatedSessionParams?: ModeratedSessionParams ) => { - let mfaResponse: MfaChallengeResponse; - if (addMfaToScpUrls) { - [mfaResponse] = await getMfaResponse(); - } - + const mfaResponse = await mfa.getChallengeResponse(); const url = cfg.getScpUrl({ location, clusterId, @@ -144,7 +92,7 @@ export const useFileTransfer = ( } return getHttpFileTransferHandlers().download(url, abortController); }, - [clusterId, login, serverId, addMfaToScpUrls] + [clusterId, login, serverId, mfa] ); const upload = useCallback( @@ -154,10 +102,7 @@ export const useFileTransfer = ( abortController: AbortController, moderatedSessionParams?: ModeratedSessionParams ) => { - let mfaResponse: MfaChallengeResponse; - if (addMfaToScpUrls) { - [mfaResponse] = await getMfaResponse(); - } + const mfaResponse = await mfa.getChallengeResponse(); const url = cfg.getScpUrl({ location, @@ -178,7 +123,7 @@ export const useFileTransfer = ( } return getHttpFileTransferHandlers().upload(url, file, abortController); }, - [clusterId, serverId, login, addMfaToScpUrls] + [clusterId, serverId, login, mfa] ); /* @@ -317,11 +262,6 @@ export const useFileTransfer = ( } return { - mfaAttempt, - mfaChallenge, - clearMfaChallenge, - submitMfa, - submitMfaAttempt, fileTransferRequests, getUploader, getDownloader, diff --git a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx index 90ff3bc1be5fa..2f68ac3541d63 100644 --- a/web/packages/teleport/src/DesktopSession/DesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/DesktopSession.tsx @@ -184,11 +184,12 @@ export function DesktopSession(props: State) { const MfaDialog = ({ mfa }: { mfa: MfaState }) => { return ( { - mfa.submitAttempt.status = 'failed'; - mfa.submitAttempt.statusText = - 'This session requires multi factor authentication to continue. Please hit "Retry" and follow the prompts given by your browser to complete authentication.'; + {...mfa} + replaceErrorText={ + 'This session requires multi factor authentication to continue. Please hit try again and follow the prompts given by your browser to complete authentication.' + } + resetAttempt={() => { + mfa.resetAttempt(); }} /> ); @@ -294,7 +295,7 @@ const nextScreenState = ( // Otherwise, calculate a new screen state. const showAnotherSessionActive = showAnotherSessionActiveDialog; - const showMfa = webauthn.mfaChallenge; + const showMfa = webauthn.challenge; const showAlert = fetchAttempt.status === 'failed' || // Fetch attempt failed tdpConnection.status === 'failed' || // TDP connection failed diff --git a/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx b/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx index 1f642d38d8d96..f14482669f471 100644 --- a/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx +++ b/web/packages/teleport/src/DesktopSession/useDesktopSession.tsx @@ -22,7 +22,7 @@ import { useParams } from 'react-router'; import useAttempt from 'shared/hooks/useAttemptNext'; import { ButtonState } from 'teleport/lib/tdp'; -import { useMfa } from 'teleport/lib/useMfa'; +import { useMfaTty } from 'teleport/lib/useMfa'; import desktopService from 'teleport/services/desktops'; import userService from 'teleport/services/user'; @@ -130,7 +130,7 @@ export default function useDesktopSession() { }); const tdpClient = clientCanvasProps.tdpClient; - const mfa = useMfa(tdpClient); + const mfa = useMfaTty(tdpClient); const onShareDirectory = () => { try { diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx index d7577b2af823f..3d146dd9fd96a 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.test.tsx @@ -52,7 +52,7 @@ describe('AuthnDialog', () => { test('renders single option dialog', () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { ssoChallenge: mockSsoChallenge, }, }); @@ -68,7 +68,7 @@ describe('AuthnDialog', () => { test('renders multi option dialog', () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { ssoChallenge: mockSsoChallenge, webauthnPublicKey: { challenge: new ArrayBuffer(1), @@ -90,7 +90,7 @@ describe('AuthnDialog', () => { test('displays error text when provided', () => { const errorText = 'Authentication failed'; const mfa = makeMockState({ - mfaChallenge: {}, + challenge: {}, submitAttempt: { status: 'error', statusText: errorText, @@ -106,27 +106,27 @@ describe('AuthnDialog', () => { test('sso button renders with callback', async () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { ssoChallenge: mockSsoChallenge, }, - submitMfa: jest.fn(), + submit: jest.fn(), }); render(); const ssoButton = screen.getByText('Okta'); fireEvent.click(ssoButton); - expect(mfa.submitMfa).toHaveBeenCalledTimes(1); + expect(mfa.submit).toHaveBeenCalledTimes(1); }); test('webauthn button renders with callback', async () => { const mfa = makeMockState({ - mfaChallenge: { + challenge: { webauthnPublicKey: { challenge: new ArrayBuffer(0) }, }, - submitMfa: jest.fn(), + submit: jest.fn(), }); render(); const webauthn = screen.getByText('Passkey or MFA Device'); fireEvent.click(webauthn); - expect(mfa.submitMfa).toHaveBeenCalledTimes(1); + expect(mfa.submit).toHaveBeenCalledTimes(1); }); }); diff --git a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx index b67a5a7f5788b..3bb5ea4442838 100644 --- a/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx +++ b/web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx @@ -26,68 +26,87 @@ import { guessProviderType } from 'shared/components/ButtonSso'; import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso'; import { Attempt } from 'shared/hooks/useAsync'; +import { MfaState } from 'teleport/lib/useMfa'; import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; +type AuthnDialogProps = MfaState & { + replaceErrorText?: string; +}; + export default function AuthnDialog({ - mfaChallenge, - submitMfa, - submitAttempt, - onCancel, -}: Props) { - const hasMultipleOptions = - mfaChallenge.ssoChallenge && mfaChallenge.webauthnPublicKey; + options, + challenge, + submit, + attempt, + resetAttempt, + replaceErrorText, +}: AuthnDialogProps) { + // Only show the dialog when we are in a processing or error state. + if (attempt.status === '' || attempt.status === 'success') return; + + // TODO(Joerger): TOTP should be pretty easy to support here with a small button -> form flow. + const onlyTotpAvailable = options?.length === 1 && challenge?.totpChallenge; return ( ({ width: '400px' })} open={true}>

Verify Your Identity

- + - {submitAttempt.status === 'error' && ( + {onlyTotpAvailable && ( - {submitAttempt.statusText} + { + 'totp is not currently supported for this action, please register a security key to continue' + } + + )} + {attempt.status === 'error' && ( + + {replaceErrorText || attempt.statusText} )} - {hasMultipleOptions + {options?.length > 1 ? 'Select one of the following methods to verify your identity:' : 'Select the method below to verify your identity:'} - - {mfaChallenge.ssoChallenge && ( - submitMfa('sso')} - gap={2} - block - > - - {mfaChallenge.ssoChallenge.device.displayName || - mfaChallenge.ssoChallenge.device.connectorId} - - )} - {mfaChallenge.webauthnPublicKey && ( - submitMfa('webauthn')} - gap={2} - block - > - - Passkey or MFA Device - - )} - + {challenge && ( + + {challenge.ssoChallenge && ( + submit('sso')} + gap={2} + block + > + + {challenge.ssoChallenge.device.displayName || + challenge.ssoChallenge.device.connectorId} + + )} + {challenge.webauthnPublicKey && ( + submit('webauthn')} + gap={2} + block + > + + Passkey or MFA Device + + )} + + )} ); } diff --git a/web/packages/teleport/src/lib/useMfa.ts b/web/packages/teleport/src/lib/useMfa.ts index 71e65f219f18c..704767d81f10d 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -16,68 +16,164 @@ * along with this program. If not, see . */ -import { useCallback, useEffect, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { Attempt, makeEmptyAttempt, useAsync } from 'shared/hooks/useAsync'; import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender'; import { TermEvent } from 'teleport/lib/term/enums'; +import { + CreateAuthenticateChallengeRequest, + parseMfaChallengeJson, +} from 'teleport/services/auth'; import auth from 'teleport/services/auth/auth'; -import { DeviceType, MfaAuthenticateChallenge } from 'teleport/services/mfa'; -import { parseMfaChallengeJson as parseMfaChallenge } from 'teleport/services/mfa/makeMfa'; +import { + DeviceType, + getMfaChallengeOptions, + MfaAuthenticateChallenge, + MfaChallengeResponse, + MfaOption, +} from 'teleport/services/mfa'; -export function useMfa(emitterSender: EventEmitterMfaSender): MfaState { - const [mfaChallenge, setMfaChallenge] = useState(); - const [mfaRequired, setMfaRequired] = useState(false); +export type MfaProps = { + req?: CreateAuthenticateChallengeRequest; + isMfaRequired?: boolean | null; +}; + +// useMfa prepares an MFA state designed for use with AuthnDialog. +// 1. Call getMfaChallengeResponse() where MFA may be required. +// If MFA is not required, it should be a noop, returning null. +// 2. Setup AuthnDialog using the MFA state, to show a dialog when +// mfaAttempt.state === 'processing'. +export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { + const [required, setMfaRequired] = useState(null); + const [options, setMfaOptions] = useState(); - const [submitAttempt, submitMfa] = useAsync( + const [challenge, setMfaChallenge] = useState(); + const mfaResponsePromise = + useRef>(); + + const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( - async (mfaType: DeviceType) => { - if (!mfaChallenge) - throw new Error('expected non empty mfa challenge in state'); - - return auth - .getMfaChallengeResponse(mfaChallenge, mfaType) - .then(res => emitterSender.sendChallengeResponse(res)) - .then(() => setMfaChallenge(null)); + async (challenge?: MfaAuthenticateChallenge) => { + if (!challenge) { + if (isMfaRequired === false || required === false) return; + + challenge = await auth.getMfaChallenge(req); + if (!challenge) { + setMfaRequired(false); + return; + } + } + + // Set mfa requirement and options after we get a challenge for the first time. + if (!required) setMfaRequired(true); + if (!options) setMfaOptions(getMfaChallengeOptions(challenge)); + + mfaResponsePromise.current = Promise.withResolvers(); + setMfaChallenge(challenge); + try { + return await mfaResponsePromise.current.promise; + } finally { + mfaResponsePromise.current = null; + setMfaChallenge(null); + } }, - [mfaChallenge] + [req, mfaResponsePromise, options, isMfaRequired, required] ) ); + const resetAttempt = () => { + if (mfaResponsePromise.current) mfaResponsePromise.current.reject(); + mfaResponsePromise.current = null; + setMfaChallenge(null); + setMfaAttempt(makeEmptyAttempt()); + }; + + const getChallengeResponse = async (challenge?: MfaAuthenticateChallenge) => { + const [resp, err] = await getResponse(challenge); + if (err) throw err; + return resp; + }; + + const submit = useCallback( + async (mfaType?: DeviceType, totpCode?: string) => { + try { + const resp = await auth.getMfaChallengeResponse( + challenge, + mfaType, + totpCode + ); + mfaResponsePromise.current.resolve(resp); + } catch (err) { + setMfaAttempt({ + data: null, + status: 'error', + statusText: err.message, + error: err, + }); + } + }, + [challenge, mfaResponsePromise, setMfaAttempt] + ); + + return { + required, + options, + challenge, + getChallengeResponse, + submit, + attempt, + resetAttempt, + }; +} + +export function useMfaTty(emitterSender: EventEmitterMfaSender): MfaState { + const [mfaRequired, setMfaRequired] = useState(false); + + const mfa = useMfa({ isMfaRequired: mfaRequired }); + useEffect(() => { - const challengeHandler = (challengeJson: string) => { - const challenge = JSON.parse(challengeJson); - setMfaChallenge(parseMfaChallenge(challenge)); + const challengeHandler = async (challengeJson: string) => { + // set Mfa required for other uses of this MfaState (e.g. file transfers) setMfaRequired(true); + + const challenge = parseMfaChallengeJson(JSON.parse(challengeJson)); + const resp = await mfa.getChallengeResponse(challenge); + emitterSender.sendChallengeResponse(resp); }; emitterSender?.on(TermEvent.MFA_CHALLENGE, challengeHandler); return () => { emitterSender?.removeListener(TermEvent.MFA_CHALLENGE, challengeHandler); }; - }, [emitterSender]); + }, [mfa, emitterSender]); - return { - mfaChallenge, - submitAttempt, - submitMfa, - mfaRequired, - }; + return mfa; } export type MfaState = { - mfaChallenge: MfaAuthenticateChallenge; - submitMfa: (mfaType?: DeviceType) => Promise; - submitAttempt: Attempt; - mfaRequired: boolean; + required: boolean; + options: MfaOption[]; + challenge: MfaAuthenticateChallenge; + // Generally you wouldn't pass in a challenge, unless you already + // have one handy, e.g. from a terminal websocket message. + getChallengeResponse: ( + challenge?: MfaAuthenticateChallenge + ) => Promise; + submit: (mfaType?: DeviceType, totpCode?: string) => Promise; + attempt: Attempt; + resetAttempt: () => void; }; // used for testing export function makeDefaultMfaState(): MfaState { return { - mfaChallenge: null, - submitAttempt: makeEmptyAttempt(), - submitMfa: () => null, - mfaRequired: true, + required: true, + options: null, + challenge: null, + getChallengeResponse: async () => null, + submit: () => null, + attempt: makeEmptyAttempt(), + resetAttempt: () => null, }; }