diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx index 738b88890b2dd..ce1bfdfb4b5f9 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx @@ -64,7 +64,7 @@ export function ChangePasswordWizard({ const reauthState = useReAuthenticate({ challengeScope: MfaChallengeScope.CHANGE_PASSWORD, onMfaResponse: async mfaResponse => - setWebauthnResponse(mfaResponse?.webauthn_response), + setWebauthnResponse(mfaResponse.webauthn_response), }); const [reauthMethod, setReauthMethod] = useState(); diff --git a/web/packages/teleport/src/lib/term/tty.ts b/web/packages/teleport/src/lib/term/tty.ts index de5f79a4da624..55aa52823411e 100644 --- a/web/packages/teleport/src/lib/term/tty.ts +++ b/web/packages/teleport/src/lib/term/tty.ts @@ -80,7 +80,7 @@ class Tty extends EventEmitterMfaSender { this.socket.send(bytearray.buffer); } - sendChallengeResponse(resp: MfaChallengeResponse) { + sendChallengeResponse(data: MfaChallengeResponse) { // we want to have the backend listen on a single message type // for any responses. so our data will look like data.webauthn, data.sso, etc // but to be backward compatible, we need to still spread the existing webauthn only fields @@ -88,8 +88,8 @@ class Tty extends EventEmitterMfaSender { // in 19, we can just pass "data" without this extra step // TODO (avatus): DELETE IN 19.0.0 const backwardCompatibleData = { - ...resp?.webauthn_response, - ...resp, + ...data.webauthn_response, + ...data, }; const encoded = this._proto.encodeChallengeResponse( JSON.stringify(backwardCompatibleData) diff --git a/web/packages/teleport/src/services/api/api.test.ts b/web/packages/teleport/src/services/api/api.test.ts index 2cf717cfda8f4..5616df264c662 100644 --- a/web/packages/teleport/src/services/api/api.test.ts +++ b/web/packages/teleport/src/services/api/api.test.ts @@ -16,6 +16,7 @@ * along with this program. If not, see . */ +import { MfaChallengeResponse } from '../mfa'; import api, { defaultRequestOptions, getAuthHeaders, @@ -24,9 +25,18 @@ import api, { } from './api'; describe('api.fetch', () => { - const mockedFetch = jest.spyOn(global, 'fetch').mockResolvedValue({} as any); // we don't care about response + let mockedFetch: jest.SpiedFunction; + beforeEach(() => { + mockedFetch = jest + .spyOn(global, 'fetch') + .mockResolvedValue({ json: async () => ({}), ok: true } as Response); // we don't care about response + }); + + afterEach(() => { + jest.resetAllMocks(); + }); - const mfaResp = { + const mfaResp: MfaChallengeResponse = { webauthn_response: { id: 'some-id', type: 'some-type', @@ -43,7 +53,7 @@ describe('api.fetch', () => { }, }; - const customOpts = { + const customOpts: RequestInit = { method: 'POST', // Override the default header from `defaultRequestOptions`. headers: { @@ -51,10 +61,6 @@ describe('api.fetch', () => { }, }; - afterEach(() => { - jest.resetAllMocks(); - }); - test('default (no optional params provided)', async () => { await api.fetch('/something'); expect(mockedFetch).toHaveBeenCalledTimes(1); @@ -156,7 +162,7 @@ describe('api.fetch', () => { }); }); -// The code below should guard us from changes to api.fetchJson which would cause it to lose type +// The code below should guard us from changes to api.fetchJsonWithMfaAuthnRetry which would cause it to lose type // information, for example by returning `any`. const fooService = { @@ -171,13 +177,13 @@ const makeFoo = (): { foo: string } => { // This is a bogus test to satisfy Jest. We don't even need to execute the code that's in the async // function, we're interested only in the type system checking the code. -test('fetchJson does not return any', () => { +test('fetchJsonWithMfaAuthnRetry does not return any', () => { const bogusFunction = async () => { const result = await fooService.doSomething(); // Reading foo is correct. We add a bogus expect to satisfy Jest. JSON.stringify(result.foo); - // @ts-expect-error If there's no error here, it means that api.fetchJson returns any, which it + // @ts-expect-error If there's no error here, it means that api.fetchJsonWithMfaAuthnRetry returns any, which it // shouldn't. JSON.stringify(result.bar); }; diff --git a/web/packages/teleport/src/services/api/api.ts b/web/packages/teleport/src/services/api/api.ts index f253c43635186..ac386c641f6a4 100644 --- a/web/packages/teleport/src/services/api/api.ts +++ b/web/packages/teleport/src/services/api/api.ts @@ -150,66 +150,31 @@ const api = { customOptions: RequestInit, mfaResponse?: MfaChallengeResponse ): Promise { - const response = await api.fetch(url, customOptions, mfaResponse); - - let json; try { - json = await response.json(); + return await api.fetch(url, customOptions, mfaResponse); } catch (err) { - // error reading JSON - const message = response.ok - ? err.message - : `${response.status} - ${response.url}`; - throw new ApiError({ message, response, opts: { cause: err } }); - } - - if (response.ok) { - return json; - } - - /** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */ - const isRoleNotFoundErr = isRoleNotFoundError(parseError(json)); - if (isRoleNotFoundErr) { - websession.logoutWithoutSlo({ - /* Don't remember location after login, since they may no longer have access to the page they were on. */ - rememberLocation: false, - /* Show "access changed" notice on login page. */ - withAccessChangedMessage: true, - }); - return; + // Retry with MFA if we get an admin action MFA error. + if (!mfaResponse && isAdminActionRequiresMfaError(err)) { + mfaResponse = await api.getAdminActionMfaResponse(); + return api.fetch(url, customOptions, mfaResponse); + } else { + throw err; + } } + }, - // Retry with MFA if we get an admin action missing MFA error. - const isAdminActionMfaError = isAdminActionRequiresMfaError( - parseError(json) - ); - const shouldRetry = isAdminActionMfaError && !mfaResponse; - if (!shouldRetry) { - throw new ApiError({ - message: parseError(json), - response, - proxyVersion: parseProxyVersion(json), - messages: json.messages, - }); - } + async getAdminActionMfaResponse() { + const challenge = await auth.getMfaChallenge({ + scope: MfaChallengeScope.ADMIN_ACTION, + }); - let mfaResponseForRetry; - try { - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.ADMIN_ACTION, - }); - mfaResponseForRetry = await auth.getMfaChallengeResponse(challenge); - } catch { + if (!challenge) { throw new Error( - 'Failed to fetch MFA challenge. Please connect a registered hardware key and try again. If you do not have a hardware key registered, you can add one from your account settings page.' + 'This is an admin-level API request and requires MFA verification. Please try again with a registered MFA device. If you do not have an MFA device registered, you can add one in the account settings page.' ); } - return api.fetchJsonWithMfaAuthnRetry( - url, - customOptions, - mfaResponseForRetry - ); + return auth.getMfaChallengeResponse(challenge); }, /** @@ -254,7 +219,7 @@ const api = { * @param mfaResponse if defined (eg: `fetchJsonWithMfaAuthnRetry`) * will add a custom MFA header field that will hold the mfaResponse. */ - fetch( + async fetch( url: string, customOptions: RequestInit = {}, mfaResponse?: MfaChallengeResponse @@ -280,7 +245,41 @@ const api = { } // native call - return fetch(url, options); + const response = await fetch(url, options); + + let json; + try { + json = await response.json(); + } catch (err) { + // error reading JSON + const message = response.ok + ? err.message + : `${response.status} - ${response.url}`; + throw new ApiError({ message, response, opts: { cause: err } }); + } + + if (response.ok) { + return json; + } + + /** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */ + const isRoleNotFoundErr = isRoleNotFoundError(parseError(json)); + if (isRoleNotFoundErr) { + websession.logoutWithoutSlo({ + /* Don't remember location after login, since they may no longer have access to the page they were on. */ + rememberLocation: false, + /* Show "access changed" notice on login page. */ + withAccessChangedMessage: true, + }); + return; + } + + throw new ApiError({ + message: parseError(json), + response, + proxyVersion: parseProxyVersion(json), + messages: json.messages, + }); }, }; @@ -326,8 +325,8 @@ export function getHostName() { return location.hostname + (location.port ? ':' + location.port : ''); } -function isAdminActionRequiresMfaError(errMessage) { - return errMessage.includes( +function isAdminActionRequiresMfaError(err: Error) { + return err.message.includes( 'admin-level API request requires MFA verification' ); } diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index 100259d6dfc20..3e58e9f5f5f4b 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -238,7 +238,7 @@ const auth = { .then(res => { const request = { action: 'accept', - webauthnAssertionResponse: res?.webauthn_response, + webauthnAssertionResponse: res.webauthn_response, }; return api.put(cfg.getHeadlessSsoPath(transactionId), request); @@ -254,11 +254,11 @@ const auth = { }, // getChallenge gets an MFA challenge for the provided parameters. If is_mfa_required_req - // is provided and it is found that MFA is not required, returns null instead. + // is provided and it is found that MFA is not required, returns undefined instead. async getMfaChallenge( req: CreateAuthenticateChallengeRequest, abortSignal?: AbortSignal - ) { + ): Promise { return api .post( cfg.api.mfaAuthnChallengePath, @@ -274,13 +274,14 @@ const auth = { }, // getChallengeResponse gets an MFA challenge response for the provided parameters. - // If is_mfa_required_req is provided and it is found that MFA is not required, returns null instead. + // If challenge is undefined or has no viable challenge options, returns empty response. async getMfaChallengeResponse( challenge: MfaAuthenticateChallenge, mfaType?: DeviceType, totpCode?: string - ): Promise { - if (!challenge) return; + ): Promise { + // No challenge, return empty response. + if (!challenge) return {}; // TODO(Joerger): If mfaType is not provided by a parent component, use some global context // to display a component, similar to the one used in useMfa. For now we just default to @@ -310,7 +311,7 @@ const auth = { } // No viable challenge, return empty response. - return; + return {}; }, async getWebAuthnChallengeResponse( @@ -439,7 +440,7 @@ const auth = { return auth .getMfaChallenge({ scope, allowReuse, isMfaRequiredRequest }, abortSignal) .then(challenge => auth.getMfaChallengeResponse(challenge, 'webauthn')) - .then(res => res?.webauthn_response); + .then(res => res.webauthn_response); }, getMfaChallengeResponseForAdminAction(allowReuse?: boolean) {