diff --git a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx index b7a2b93534f84..1c41801500e6a 100644 --- a/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx +++ b/web/packages/teleport/src/Console/DocumentSsh/DocumentSsh.tsx @@ -25,12 +25,13 @@ import { FileTransferActionBar, FileTransferContextProvider, FileTransferRequests, + useFileTransferContext, } from 'shared/components/FileTransfer'; import { TerminalSearch } from 'shared/components/TerminalSearch'; import AuthnDialog from 'teleport/components/AuthnDialog'; import * as stores from 'teleport/Console/stores'; -import { useMfa, useMfaEmitter } from 'teleport/lib/useMfa'; +import { useMfaEmitter } from 'teleport/lib/useMfa'; import { MfaChallengeScope } from 'teleport/services/auth/auth'; import { useConsoleContext } from '../consoleContextProvider'; @@ -54,14 +55,17 @@ function DocumentSsh({ doc, visible }: PropTypes) { const { tty, status, closeDocument, session } = useSshSession(doc); const [showSearch, setShowSearch] = useState(false); - const ttyMfa = useMfaEmitter(tty); - const ftMfa = useMfa({ - isMfaRequired: ttyMfa.required, + const mfa = useMfaEmitter(tty, { + // The MFA requirement will be determined by whether we do/don't get + // an mfa challenge over the event emitter at session start. + isMfaRequired: false, req: { scope: MfaChallengeScope.USER_SESSION, }, }); - const ft = useFileTransfer(tty, session, doc, ftMfa); + const ft = useFileTransfer(tty, session, doc, mfa); + const { openedDialog: ftOpenedDialog } = useFileTransferContext(); + const theme = useTheme(); function handleCloseFileTransfer() { @@ -73,14 +77,12 @@ function DocumentSsh({ doc, visible }: PropTypes) { } useEffect(() => { - // when switching tabs or closing tabs, focus on visible terminal - if ( - ttyMfa.attempt.status === 'processing' || - ftMfa.attempt.status === 'processing' - ) { + // If an MFA attempt starts while switching tabs or closing tabs, + // automatically focus on visible terminal. + if (mfa.challenge) { terminalRef.current?.focus(); } - }, [visible, ttyMfa.attempt.status, ftMfa.attempt.status]); + }, [visible, mfa.challenge]); const onSearchClose = useCallback(() => { setShowSearch(false); @@ -141,8 +143,15 @@ function DocumentSsh({ doc, visible }: PropTypes) { )} - - + { + // Don't close the ssh doc if this is just a file transfer request. + if (!ftOpenedDialog) { + closeDocument(); + } + }} + /> {status === 'initialized' && terminal} ); diff --git a/web/packages/teleport/src/lib/useMfa.test.tsx b/web/packages/teleport/src/lib/useMfa.test.tsx index 2420fc0e7a82d..38e8aafdfe139 100644 --- a/web/packages/teleport/src/lib/useMfa.test.tsx +++ b/web/packages/teleport/src/lib/useMfa.test.tsx @@ -17,7 +17,7 @@ */ import { renderHook, waitFor } from '@testing-library/react'; -import { useState } from 'react'; +import { act } from 'react'; import { CreateAuthenticateChallengeRequest } from 'teleport/services/auth'; import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; @@ -61,10 +61,6 @@ const mockChallengeReq: CreateAuthenticateChallengeRequest = { }; describe('useMfa', () => { - beforeEach(() => { - jest.spyOn(console, 'error').mockImplementation(); - }); - afterEach(() => { jest.clearAllMocks(); }); @@ -80,27 +76,25 @@ describe('useMfa', () => { }) ); - const respPromise = mfa.current.getChallengeResponse(); - await waitFor(() => { - expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); + let resp; + await act(async () => { + resp = mfa.current.getChallengeResponse(); }); + expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); expect(mfa.current.options).toEqual([MFA_OPTION_WEBAUTHN]); - expect(mfa.current.required).toEqual(true); + expect(mfa.current.mfaRequired).toEqual(true); expect(mfa.current.challenge).toEqual(mockChallenge); expect(mfa.current.attempt.status).toEqual('processing'); - await mfa.current.submit('webauthn'); - await waitFor(() => { - expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( - mockChallenge, - 'webauthn', - undefined - ); - }); + await act(async () => mfa.current.submit('webauthn')); + expect(auth.getMfaChallengeResponse).toHaveBeenCalledWith( + mockChallenge, + 'webauthn', + undefined + ); - const resp = await respPromise; - expect(resp).toEqual(mockResponse); + expect(await resp).toEqual(mockResponse); expect(mfa.current.challenge).toEqual(null); expect(mfa.current.attempt.status).toEqual('success'); }); @@ -116,51 +110,63 @@ describe('useMfa', () => { // If a challenge is not returned, an empty mfa response should be returned // early and the requirement changed to false for future calls. - const resp = await mfa.current.getChallengeResponse(); + const resp = await act(async () => { + return mfa.current.getChallengeResponse(); + }); expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); expect(resp).toEqual(undefined); - await waitFor(() => expect(mfa.current.required).toEqual(false)); + await waitFor(() => expect(mfa.current.mfaRequired).toEqual(false)); }); - test('adaptable mfa requirement state', async () => { - jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue(null); - - let isMfaRequired: boolean; - let setMfaRequired: (b: boolean) => void; + test('adaptable mfa requirement', async () => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(mockChallenge); + jest + .spyOn(auth, 'getMfaChallengeResponse') + .mockResolvedValueOnce(mockResponse); - let req: CreateAuthenticateChallengeRequest; - let setReq: (r: CreateAuthenticateChallengeRequest) => void; + const { result: mfa } = renderHook(() => + useMfa({ + isMfaRequired: false, + req: mockChallengeReq, + }) + ); - const { result: mfa } = renderHook(() => { - [isMfaRequired, setMfaRequired] = useState(null); - [req, setReq] = - useState(mockChallengeReq); + // The mfa requirement can be initialized to a non-undefined value to skip + // the mfa check when it isn't needed, e.g. the requirement was predetermined. + expect(mfa.current.mfaRequired).toEqual(false); - return useMfa({ - req: req, - isMfaRequired: isMfaRequired, - }); + // The mfa required state can be changed directly, in case the requirement + // need to be updated by the caller. + await act(async () => { + mfa.current.setMfaRequired(true); + }); + await waitFor(() => { + expect(mfa.current.mfaRequired).toEqual(true); }); - // mfaRequired should change when the isMfaRequired arg changes, allowing - // callers to propagate mfa required late (e.g. per-session MFA for file transfers) - setMfaRequired(false); - await waitFor(() => expect(mfa.current.required).toEqual(false)); - - setMfaRequired(true); - await waitFor(() => expect(mfa.current.required).toEqual(true)); + // The mfa required state can be changed back to undefined (default) or null to force + // the next mfa attempt to re-check mfa required / attempt to get a challenge. + await act(async () => { + mfa.current.setMfaRequired(null); + }); + await waitFor(() => { + expect(mfa.current.mfaRequired).toEqual(null); + }); - setMfaRequired(null); - await waitFor(() => expect(mfa.current.required).toEqual(null)); + // mfa required will be updated to true as usual once the server returns an mfa challenge. + await act(async () => { + mfa.current.getChallengeResponse(); + }); + await waitFor(() => { + expect(mfa.current.challenge).toBeDefined(); + }); + await act(async () => { + return mfa.current.submit(); + }); - // If isMfaRequiredRequest changes, the mfaRequired value should be reset. - setReq({ - ...mockChallengeReq, - isMfaRequiredRequest: { - admin_action: {}, - }, + await waitFor(() => { + expect(mfa.current.mfaRequired).toEqual(true); }); - await waitFor(() => expect(mfa.current.required).toEqual(null)); }); test('mfa challenge error', async () => { @@ -171,14 +177,15 @@ describe('useMfa', () => { const { result: mfa } = renderHook(() => useMfa({})); - await expect(mfa.current.getChallengeResponse).rejects.toThrow(err); - await waitFor(() => { - expect(mfa.current.attempt).toEqual({ - status: 'error', - statusText: err.message, - error: err, - data: null, - }); + await act(async () => { + await expect(mfa.current.getChallengeResponse()).rejects.toThrow(err); + }); + + expect(mfa.current.attempt).toEqual({ + status: 'error', + statusText: err.message, + error: err, + data: null, }); }); @@ -195,19 +202,19 @@ describe('useMfa', () => { }) ); - const respPromise = mfa.current.getChallengeResponse(); - await waitFor(() => { - expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); + let resp; + await act(async () => { + resp = mfa.current.getChallengeResponse(); }); - await mfa.current.submit('webauthn'); - await waitFor(() => { - expect(mfa.current.attempt).toEqual({ - status: 'error', - statusText: err.message, - error: err, - data: null, - }); + expect(auth.getMfaChallenge).toHaveBeenCalledWith(mockChallengeReq); + + await act(async () => mfa.current.submit('webauthn')); + expect(mfa.current.attempt).toEqual({ + status: 'error', + statusText: err.message, + error: err, + data: null, }); // After an error, the mfa response promise remains in an unresolved state, @@ -215,8 +222,9 @@ describe('useMfa', () => { jest .spyOn(auth, 'getMfaChallengeResponse') .mockResolvedValueOnce(mockResponse); - await mfa.current.submit('webauthn'); - expect(await respPromise).toEqual(mockResponse); + + await act(async () => mfa.current.submit('webauthn')); + expect(await resp).toEqual(mockResponse); }); test('reset mfa attempt', async () => { @@ -227,18 +235,22 @@ describe('useMfa', () => { }) ); - const respPromise = mfa.current.getChallengeResponse(); - await waitFor(() => { - expect(auth.getMfaChallenge).toHaveBeenCalled(); + let resp: Promise; + await act(async () => { + resp = mfa.current.getChallengeResponse(); }); - mfa.current.cancelAttempt(); + // Before calling mfa.current.cancelAttempt(), we need to write code that handles rejection of + // resp. Otherwise the test is going to fail because of unhandled promise rejection. + // eslint-disable-next-line jest/valid-expect + const expectedRespRejection = expect(resp).rejects.toEqual( + new MfaCanceledError() + ); - await expect(respPromise).rejects.toThrow(new MfaCanceledError()); + await act(async () => mfa.current.cancelAttempt()); - await waitFor(() => { - expect(mfa.current.attempt.status).toEqual('error'); - }); + await expectedRespRejection; + 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 2bb8054665d69..73fcffb947cca 100644 --- a/web/packages/teleport/src/lib/useMfa.ts +++ b/web/packages/teleport/src/lib/useMfa.ts @@ -37,7 +37,9 @@ import { export type MfaProps = { req?: CreateAuthenticateChallengeRequest; - isMfaRequired?: boolean | null; + // isMfaRequired is an initial state for isMfaRequired. Useful in cases + // where the mfa requirement is discovered and set indirectly by the caller. + isMfaRequired?: boolean; }; type mfaResponsePromiseWithResolvers = { @@ -52,19 +54,11 @@ type mfaResponsePromiseWithResolvers = { * be used to display options to the user and prompt for them to complete * the MFA check. */ -export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { - const [mfaRequired, setMfaRequired] = useState(); +export function useMfa(props?: MfaProps): MfaState { + const [mfaRequired, setMfaRequired] = useState(props?.isMfaRequired); const [options, setMfaOptions] = useState(); const [challenge, setMfaChallenge] = useState(); - useEffect(() => { - setMfaRequired(isMfaRequired); - }, [isMfaRequired]); - - useEffect(() => { - setMfaRequired(null); - }, [req?.isMfaRequiredRequest]); - // getResponse is used to initiate MFA authentication. // 1. Check if MFA is required by getting a new MFA challenge // 2. If MFA is required, set the challenge in the MFA state and wait for it to @@ -77,13 +71,34 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { const [attempt, getResponse, setMfaAttempt] = useAsync( useCallback( async (challenge?: MfaAuthenticateChallenge) => { - // If a previous call determined that MFA is not required, this is a noop. - if (mfaRequired === false) return; + // If a challenge wasn't provided and we previously determined MFA is not + // required, this is a noop. + if (mfaRequired === false && !challenge) return; + + // If a challenge was provided, the client has determined mfa is required. + if (challenge) { + setMfaRequired(true); + } else if (mfaRequired === false) { + // Client previously determined MFA is not required, this is a noop. + return; + } - challenge = challenge ? challenge : await auth.getMfaChallenge(req); + // Caller didn't pass a challenge and the mfa required is true or unknown. if (!challenge) { - setMfaRequired(false); - return; + let req = props.req; + + // We already know MFA is required, skip the extra check. + if (mfaRequired === true) req.isMfaRequiredRequest = null; + + challenge = await auth.getMfaChallenge(props.req); + + // An empty challenge means either mfa is not required, or the user has no mfa devices. + if (!challenge) { + setMfaRequired(false); + return; + } + + setMfaRequired(true); } // Prepare a new promise to collect the mfa response retrieved @@ -101,10 +116,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { 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; @@ -112,7 +124,7 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { setMfaChallenge(null); } }, - [req, mfaRequired] + [props, mfaRequired] ) ); @@ -158,7 +170,8 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { ); return { - required: mfaRequired, + mfaRequired, + setMfaRequired, options, challenge, getChallengeResponse, @@ -168,16 +181,14 @@ export function useMfa({ req, isMfaRequired }: MfaProps): MfaState { }; } -export function useMfaEmitter(emitterSender: EventEmitterMfaSender): MfaState { - const [mfaRequired, setMfaRequired] = useState(false); - - const mfa = useMfa({ isMfaRequired: mfaRequired }); +export function useMfaEmitter( + emitterSender: EventEmitterMfaSender, + mfaProps?: MfaProps +): MfaState { + const mfa = useMfa(mfaProps); useEffect(() => { 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); @@ -193,7 +204,8 @@ export function useMfaEmitter(emitterSender: EventEmitterMfaSender): MfaState { } export type MfaState = { - required: boolean; + mfaRequired: boolean; + setMfaRequired: (r: boolean) => void; options: MfaOption[]; challenge: MfaAuthenticateChallenge; // Generally you wouldn't pass in a challenge, unless you already @@ -209,7 +221,8 @@ export type MfaState = { // used for testing export function makeDefaultMfaState(): MfaState { return { - required: true, + mfaRequired: true, + setMfaRequired: () => null, options: null, challenge: null, getChallengeResponse: async () => null,