From f3e3c483e5e88ac9ce5ad2fccf7d49028ab27e60 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 22 Nov 2024 12:30:00 -0800 Subject: [PATCH 01/13] Handle unified mfa response to create privileged token. --- lib/web/users.go | 12 ++++++++ .../ReAuthenticate/useReAuthenticate.ts | 7 +++-- .../teleport/src/services/auth/auth.ts | 29 +++++++++++-------- 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/lib/web/users.go b/lib/web/users.go index 88fa8907bafce..f96d0b4405e10 100644 --- a/lib/web/users.go +++ b/lib/web/users.go @@ -30,6 +30,7 @@ import ( "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/types" wantypes "github.com/gravitational/teleport/lib/auth/webauthntypes" + "github.com/gravitational/teleport/lib/client" "github.com/gravitational/teleport/lib/httplib" "github.com/gravitational/teleport/lib/web/ui" ) @@ -251,10 +252,15 @@ func deleteUser(r *http.Request, params httprouter.Params, m userAPIGetter, user } type privilegeTokenRequest struct { + // TODO(Joerger): DELETE IN v19.0.0 in favor of ExistingMFAResponse // SecondFactorToken is the totp code. SecondFactorToken string `json:"secondFactorToken"` + // TODO(Joerger): DELETE IN v19.0.0 in favor of ExistingMFAResponse // WebauthnResponse is the response from authenticators. WebauthnResponse *wantypes.CredentialAssertionResponse `json:"webauthnAssertionResponse"` + // ExistingMFAResponse is an MFA challenge response from an existing device. + // Not required if the user has no existing devices. + ExistingMFAResponse *client.MFAChallengeResponse `json:"existingMfaResponse"` } // createPrivilegeTokenHandle creates and returns a privilege token. @@ -275,6 +281,12 @@ func (h *Handler) createPrivilegeTokenHandle(w http.ResponseWriter, r *http.Requ protoReq.ExistingMFAResponse = &proto.MFAAuthenticateResponse{Response: &proto.MFAAuthenticateResponse_Webauthn{ Webauthn: wantypes.CredentialAssertionResponseToProto(req.WebauthnResponse), }} + case req.ExistingMFAResponse != nil: + var err error + protoReq.ExistingMFAResponse, err = req.ExistingMFAResponse.GetOptionalMFAResponseProtoReq() + if err != nil { + return nil, trace.Wrap(err) + } default: // Can be empty, which means user did not have a second factor registered. } diff --git a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts index 7b2746de0a25c..2a535b5673c6f 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts +++ b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts @@ -48,7 +48,7 @@ export default function useReAuthenticate(props: Props) { setAttempt({ status: 'processing' }); auth - .createPrivilegeTokenWithTotp(secondFactorToken) + .createPrivilegeToken({ totp_code: secondFactorToken }) .then(props.onAuthenticated) .catch(handleError); } @@ -65,8 +65,11 @@ export default function useReAuthenticate(props: Props) { return; } + // Creating privilege tokens always expects the MANAGE_DEVICES webauthn scope. auth - .createPrivilegeTokenWithWebauthn() + .getMfaChallenge({ scope: MfaChallengeScope.MANAGE_DEVICES }) + .then(auth.getMfaChallengeResponse) + .then(auth.createPrivilegeToken) .then(props.onAuthenticated) .catch((err: Error) => { // This catches a webauthn frontend error that occurs on Firefox and replaces it with a more helpful error message. diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index f0f30f7356b6d..f4da45a33b1f7 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -259,10 +259,6 @@ const auth = { return api.put(cfg.getHeadlessSsoPath(transactionId), request); }, - createPrivilegeTokenWithTotp(secondFactorToken: string) { - return api.post(cfg.api.createPrivilegeTokenPath, { secondFactorToken }); - }, - // 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. async getMfaChallenge( @@ -332,18 +328,27 @@ const auth = { ); }, - // TODO(Joerger): Combine with otp endpoint. + createPrivilegeToken(existingMfaResponse: MfaChallengeResponse) { + return api.post(cfg.api.createPrivilegeTokenPath, { + existingMfaResponse, + // TODO(Joerger): DELETE IN v19.0.0 + // Also provide totp/webauthn response in backwards compatible format. + secondFactorToken: existingMfaResponse.totp_code, + webauthnAssertionResponse: existingMfaResponse.webauthn_response, + }); + }, + + // TODO(Joerger): Delete once no longer used by /e createPrivilegeTokenWithWebauthn() { - // Creating privilege tokens always expects the MANAGE_DEVICES webauthn scope. return auth .getMfaChallenge({ scope: MfaChallengeScope.MANAGE_DEVICES }) .then(auth.getMfaChallengeResponse) - .then(res => - api.post(cfg.api.createPrivilegeTokenPath, { - // TODO(Joerger): Handle non-webauthn challenges. - webauthnAssertionResponse: res.webauthn_response, - }) - ); + .then(mfaResp => auth.createPrivilegeToken(mfaResp)); + }, + + // TODO(Joerger): Delete once no longer used by /e + createPrivilegeTokenWithTotp(secondFactorToken: string) { + return api.post(cfg.api.createPrivilegeTokenPath, { secondFactorToken }); }, createRestrictedPrivilegeToken() { From 702eceefd88a71f33f083b711fbd7f19220f0fdb Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 25 Nov 2024 11:30:26 -0800 Subject: [PATCH 02/13] Refactor useReauthenticate and Reauthenticate component. --- .../teleport/src/Account/Account.test.tsx | 37 ++-- web/packages/teleport/src/Account/Account.tsx | 3 - .../wizards/AddAuthDeviceWizard.story.tsx | 98 +++++------ .../wizards/AddAuthDeviceWizard.test.tsx | 122 ++++++------- .../wizards/AddAuthDeviceWizard.tsx | 109 ++++++++---- .../wizards/DeleteAuthDeviceWizard.story.tsx | 24 ++- .../wizards/DeleteAuthDeviceWizard.test.tsx | 38 ++-- .../wizards/DeleteAuthDeviceWizard.tsx | 56 ++++-- .../wizards/ReauthenticateStep.test.tsx | 44 ----- .../wizards/ReauthenticateStep.tsx | 117 +++---------- .../ReAuthenticate/ReAuthenticate.story.tsx | 20 ++- .../ReAuthenticate/ReAuthenticate.tsx | 91 +++++++--- .../ReAuthenticate/useReAuthenticate.ts | 165 ++++++++---------- .../teleport/src/services/auth/auth.ts | 2 + .../teleport/src/services/mfa/mfaOptions.ts | 10 +- 15 files changed, 455 insertions(+), 481 deletions(-) delete mode 100644 web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.test.tsx diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index 3ca45002ffaee..e4d098d1d2002 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -29,7 +29,10 @@ import cfg from 'teleport/config'; import { createTeleportContext } from 'teleport/mocks/contexts'; import { PasswordState } from 'teleport/services/user'; import auth from 'teleport/services/auth/auth'; -import MfaService, { MfaDevice } from 'teleport/services/mfa'; +import MfaService, { + MfaDevice, + WebauthnAssertionResponse, +} from 'teleport/services/mfa'; const defaultAuthType = cfg.auth.second_factor; const defaultPasswordless = cfg.auth.allowPasswordless; @@ -244,10 +247,11 @@ test('adding an MFA device', async () => { const ctx = createTeleportContext(); jest.spyOn(ctx.mfaService, 'fetchDevices').mockResolvedValue([testPasskey]); jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({ - webauthnPublicKey: null, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, totpChallenge: true, ssoChallenge: null, }); + jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); jest .spyOn(auth, 'createNewWebAuthnDevice') .mockResolvedValueOnce(dummyCredential); @@ -255,8 +259,8 @@ test('adding an MFA device', async () => { .spyOn(MfaService.prototype, 'saveNewWebAuthnDevice') .mockResolvedValueOnce(undefined); jest - .spyOn(auth, 'createPrivilegeTokenWithWebauthn') - .mockResolvedValueOnce('webauthn-privilege-token'); + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); cfg.auth.second_factor = 'on'; await renderComponent(ctx); @@ -276,7 +280,7 @@ test('adding an MFA device', async () => { addRequest: { deviceName: 'new-mfa', deviceUsage: 'mfa', - tokenId: 'webauthn-privilege-token', + tokenId: 'privilege-token', }, }); expect( @@ -288,6 +292,12 @@ test('adding a passkey', async () => { const user = userEvent.setup(); const ctx = createTeleportContext(); jest.spyOn(ctx.mfaService, 'fetchDevices').mockResolvedValue([testMfaMethod]); + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({ + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + totpChallenge: true, + ssoChallenge: null, + }); + jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); jest .spyOn(auth, 'createNewWebAuthnDevice') .mockResolvedValueOnce(dummyCredential); @@ -295,8 +305,8 @@ test('adding a passkey', async () => { .spyOn(MfaService.prototype, 'saveNewWebAuthnDevice') .mockResolvedValueOnce(undefined); jest - .spyOn(auth, 'createPrivilegeTokenWithWebauthn') - .mockResolvedValueOnce('webauthn-privilege-token'); + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); cfg.auth.second_factor = 'on'; cfg.auth.allowPasswordless = true; @@ -315,7 +325,7 @@ test('adding a passkey', async () => { addRequest: { deviceName: 'new-passkey', deviceUsage: 'passwordless', - tokenId: 'webauthn-privilege-token', + tokenId: 'privilege-token', }, }); expect( @@ -328,13 +338,14 @@ test('removing an MFA method', async () => { const ctx = createTeleportContext(); jest.spyOn(ctx.mfaService, 'fetchDevices').mockResolvedValue([testMfaMethod]); jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({ - webauthnPublicKey: null, - totpChallenge: false, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + totpChallenge: true, ssoChallenge: null, }); + jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); jest - .spyOn(auth, 'createPrivilegeTokenWithWebauthn') - .mockResolvedValueOnce('webauthn-privilege-token'); + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); jest .spyOn(MfaService.prototype, 'removeDevice') .mockResolvedValueOnce(undefined); @@ -352,7 +363,7 @@ test('removing an MFA method', async () => { await user.click(deleteStep.getByRole('button', { name: 'Delete' })); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( - 'webauthn-privilege-token', + 'privilege-token', 'touch_id' ); expect(screen.queryByTestId('delete-step')).not.toBeInTheDocument(); diff --git a/web/packages/teleport/src/Account/Account.tsx b/web/packages/teleport/src/Account/Account.tsx index 9cd8c47720e2b..c2109fb51dd9d 100644 --- a/web/packages/teleport/src/Account/Account.tsx +++ b/web/packages/teleport/src/Account/Account.tsx @@ -279,7 +279,6 @@ export function Account({ usage={newDeviceUsage} auth2faType={cfg.getAuth2faType()} privilegeToken={token} - devices={devices} onClose={closeAddDeviceWizard} onSuccess={onAddDeviceSuccess} /> @@ -287,8 +286,6 @@ export function Account({ {deviceToRemove && ( ); } -export function ReauthenticateNoOptions() { - return ; -} - export function CreatePasskey() { return ; } @@ -92,7 +83,9 @@ export function CreateMfaHardwareDevice() { } export function CreateMfaAppQrCodeLoading() { - return ; + return ( + + ); } CreateMfaAppQrCodeLoading.parameters = { msw: { @@ -105,10 +98,12 @@ CreateMfaAppQrCodeLoading.parameters = { }, }; -export function CreateMfaAppQrCodeFailed() { - return ; +export function CreateAuthenticatorAppQrCodeFailed() { + return ( + + ); } -CreateMfaAppQrCodeFailed.parameters = { +CreateAuthenticatorAppQrCodeFailed.parameters = { msw: { handlers: [ http.post( @@ -128,10 +123,12 @@ CreateMfaAppQrCodeFailed.parameters = { const dummyQrCode = 'iVBORw0KGgoAAAANSUhEUgAAAB0AAAAdAQMAAABsXfVMAAAABlBMVEUAAAD///+l2Z/dAAAAAnRSTlP//8i138cAAAAJcEhZcwAACxIAAAsSAdLdfvwAAABrSURBVAiZY/gPBAxoxAcxh3qG71fv1zN8iQ8EEReBRACQ+H4ZKPZBFCj7/3v9f4aPU9vqGX4kFtUzfG5mBLK2aNUz/PM3AsmqAk2RNQTquLYLqDdG/z/QlGAgES4CFLu4GygrXF2Pbi+IAADZqFQFAjXZWgAAAABJRU5ErkJggg=='; -export function CreateMfaApp() { - return ; +export function CreateAuthenticatorApp() { + return ( + + ); } -CreateMfaApp.parameters = { +CreateAuthenticatorApp.parameters = { msw: { handlers: [ http.post( @@ -153,7 +150,7 @@ export function SaveMfaHardwareDevice() { } export function SaveMfaAuthenticatorApp() { - return ; + return ; } const stepProps: AddAuthDeviceWizardStepProps = { @@ -165,35 +162,28 @@ const stepProps: AddAuthDeviceWizardStepProps = { flowLength: 1, refCallback: () => {}, - // Other props + // Shared props privilegeToken: 'privilege-token', - usage: 'passwordless' as DeviceUsage, - auth2faType: 'on' as Auth2faType, - credential: { id: 'cred-id', type: 'public-key' }, - newMfaDeviceType: 'webauthn' as Auth2faType, - devices: [ - { - id: '1', - description: 'Authenticator App', - name: 'gizmo', - registeredDate: new Date(1628799417000), - lastUsedDate: new Date(1628799417000), - type: 'totp', - usage: 'mfa', - }, - { - id: '2', - description: 'Hardware Key', - name: 'key', - registeredDate: new Date(1623722252000), - lastUsedDate: new Date(1623981452000), - type: 'webauthn', - usage: 'mfa', - }, - ], - onNewMfaDeviceTypeChange: () => {}, - onDeviceCreated: () => {}, - onAuthenticated: () => {}, + newMfaDeviceType: 'webauthn', onClose: () => {}, onSuccess: () => {}, + usage: 'passwordless' as DeviceUsage, + + // Reauth props + reauthAttempt: {} as Attempt, + clearReauthAttempt: () => {}, + mfaChallengeOptions: [ + MFA_OPTION_WEBAUTHN, + MFA_OPTION_TOTP, + MFA_OPTION_SSO_DEFAULT, + ], + submitWithMfa: async (mfaType?: DeviceType, otpCode?: string) => {}, + + // Create props + mfaRegisterOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP], + onDeviceCreated: (c: Credential) => {}, + onNewMfaDeviceTypeChange: (d: DeviceType) => {}, + + // Save props + credential: { id: 'cred-id', type: 'public-key' }, }; diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx index 3d0c6bbb6bf43..84f60d7eb40d6 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx @@ -19,7 +19,7 @@ import { render, screen } from 'design/utils/testing'; import React from 'react'; -import { within } from '@testing-library/react'; +import { waitFor, within } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; import TeleportContext from 'teleport/teleportContext'; @@ -29,8 +29,6 @@ import auth from 'teleport/services/auth'; import { AddAuthDeviceWizardStepProps } from './AddAuthDeviceWizard'; -import { deviceCases } from './deviceCases'; - import { AddAuthDeviceWizard } from '.'; const dummyCredential: Credential = { id: 'cred-id', type: 'public-key' }; @@ -49,14 +47,6 @@ beforeEach(() => { jest .spyOn(MfaService.prototype, 'saveNewWebAuthnDevice') .mockResolvedValueOnce(undefined); - jest - .spyOn(auth, 'createPrivilegeTokenWithWebauthn') - .mockResolvedValueOnce('webauthn-privilege-token'); - jest - .spyOn(auth, 'createPrivilegeTokenWithTotp') - .mockImplementationOnce(token => - Promise.resolve(`totp-privilege-token-${token}`) - ); jest.spyOn(auth, 'createMfaRegistrationChallenge').mockResolvedValueOnce({ qrCode: 'dummy-qr-code', webauthnPublicKey: {} as PublicKeyCredentialCreationOptions, @@ -73,8 +63,7 @@ function TestWizard(props: Partial = {}) { {}} onSuccess={onSuccess} {...props} @@ -84,12 +73,18 @@ function TestWizard(props: Partial = {}) { } describe('flow without reauthentication', () => { + beforeEach(() => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce({}); + }); + test('adds a passkey', async () => { render( ); - const createStep = within(screen.getByTestId('create-step')); + const createStep = await waitFor(() => { + return within(screen.getByTestId('create-step')); + }); await user.click( createStep.getByRole('button', { name: 'Create a passkey' }) ); @@ -117,7 +112,9 @@ describe('flow without reauthentication', () => { test('adds a WebAuthn MFA', async () => { render(); - const createStep = within(screen.getByTestId('create-step')); + const createStep = await waitFor(() => { + return within(screen.getByTestId('create-step')); + }); await user.click(createStep.getByLabelText('Hardware Device')); await user.click( createStep.getByRole('button', { name: 'Create an MFA method' }) @@ -146,7 +143,10 @@ describe('flow without reauthentication', () => { test('adds an authenticator app', async () => { render(); - const createStep = within(screen.getByTestId('create-step')); + const createStep = await waitFor(() => { + return within(screen.getByTestId('create-step')); + }); + await user.click(createStep.getByLabelText('Authenticator App')); expect(createStep.getByRole('img')).toHaveAttribute( 'src', @@ -172,20 +172,34 @@ describe('flow without reauthentication', () => { }); describe('flow with reauthentication', () => { + beforeEach(() => { + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce({ + totpChallenge: true, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + }); + jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); + jest + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); + }); + test('adds a passkey with WebAuthn reauthentication', async () => { render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); + await user.click(reauthenticateStep.getByText('Verify my identity')); - const createStep = within(screen.getByTestId('create-step')); + const createStep = await waitFor(() => { + return within(screen.getByTestId('create-step')); + }); await user.click( createStep.getByRole('button', { name: 'Create a passkey' }) ); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ - tokenId: 'webauthn-privilege-token', + tokenId: 'privilege-token', deviceUsage: 'passwordless', }); @@ -199,7 +213,7 @@ describe('flow with reauthentication', () => { addRequest: { deviceName: 'new-passkey', deviceUsage: 'passwordless', - tokenId: 'webauthn-privilege-token', + tokenId: 'privilege-token', }, }); expect(onSuccess).toHaveBeenCalled(); @@ -208,9 +222,10 @@ describe('flow with reauthentication', () => { test('adds a passkey with OTP reauthentication', async () => { render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); + await user.click(reauthenticateStep.getByText('Authenticator App')); await user.type( reauthenticateStep.getByLabelText('Authenticator Code'), @@ -218,12 +233,14 @@ describe('flow with reauthentication', () => { ); await user.click(reauthenticateStep.getByText('Verify my identity')); - const createStep = within(screen.getByTestId('create-step')); + const createStep = await waitFor(() => { + return within(screen.getByTestId('create-step')); + }); await user.click( createStep.getByRole('button', { name: 'Create a passkey' }) ); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ - tokenId: 'totp-privilege-token-654987', + tokenId: 'privilege-token', deviceUsage: 'passwordless', }); @@ -237,61 +254,24 @@ describe('flow with reauthentication', () => { addRequest: { deviceName: 'new-passkey', deviceUsage: 'passwordless', - tokenId: 'totp-privilege-token-654987', + tokenId: 'privilege-token', }, }); expect(onSuccess).toHaveBeenCalled(); }); - test('shows all authentication options', async () => { - render( - - ); + test('shows reauthentication options', async () => { + render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); - expect( - reauthenticateStep.queryByLabelText(/passkey or security key/i) - ).toBeVisible(); - expect( - reauthenticateStep.queryByLabelText(/authenticator app/i) - ).toBeVisible(); - }); - - test('limits authentication options to devices owned', async () => { - render( - - ); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); expect( reauthenticateStep.queryByLabelText(/passkey or security key/i) - ).not.toBeInTheDocument(); + ).toBeVisible(); expect( reauthenticateStep.queryByLabelText(/authenticator app/i) ).toBeVisible(); }); - - test.each` - auth2faType | deviceCase | error - ${'otp'} | ${'mfaDevices'} | ${/authenticator app is required/i} - ${'webauthn'} | ${'authApps'} | ${/passkey or security key is required/i} - ${'on'} | ${'none'} | ${/identity verification is required/i} - `( - 'shows an error if no way to authenticate for MFA type "$auth2faType"', - async ({ auth2faType, deviceCase, error }) => { - render( - - ); - - expect(screen.getByText(error)).toBeVisible(); - } - ); }); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx index 79cabebb57a1d..847197e2c12c7 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx @@ -16,7 +16,7 @@ * along with this program. If not, see . */ -import { OutlineDanger } from 'design/Alert/Alert'; +import { Alert, OutlineDanger } from 'design/Alert/Alert'; import { ButtonPrimary, ButtonSecondary } from 'design/Button'; import Dialog from 'design/Dialog'; import Flex from 'design/Flex'; @@ -31,7 +31,6 @@ import { requiredField } from 'shared/components/Validation/rules'; import { useAsync } from 'shared/hooks/useAsync'; import useAttempt from 'shared/hooks/useAttemptNext'; import { Auth2faType } from 'shared/services'; -import createMfaOptions, { MfaOption } from 'shared/utils/createMfaOptions'; import Box from 'design/Box'; @@ -42,7 +41,14 @@ import { P } from 'design/Text/Text'; import auth from 'teleport/services/auth/auth'; import useTeleport from 'teleport/useTeleport'; -import { DeviceUsage, MfaDevice } from 'teleport/services/mfa'; +import { + DeviceType, + DeviceUsage, + getMfaRegisterOptions, + MfaOption, +} from 'teleport/services/mfa'; + +import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; import { PasskeyBlurb } from '../../../components/Passkeys/PasskeyBlurb'; @@ -56,11 +62,6 @@ interface AddAuthDeviceWizardProps { usage: DeviceUsage; /** MFA type setting, as configured in the cluster's configuration. */ auth2faType: Auth2faType; - /** - * A list of user's devices, used for computing the list of available identity - * verification options. - */ - devices: MfaDevice[]; /** * A privilege token that may have been created previously; if present, the * reauthentication step will be skipped. @@ -75,21 +76,51 @@ export function AddAuthDeviceWizard({ privilegeToken: privilegeTokenProp = '', usage, auth2faType, - devices, onClose, onSuccess, }: AddAuthDeviceWizardProps) { - const reauthRequired = !privilegeTokenProp; const [privilegeToken, setPrivilegeToken] = useState(privilegeTokenProp); const [credential, setCredential] = useState(null); - const mfaOptions = createMfaOptions({ - auth2faType, - required: true, + const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = + useReAuthenticate({ + onAuthenticated: setPrivilegeToken, + }); + + // Choose a new device type from the options available for the given 2fa type. + // irrelevant if usage === 'passkey'. + const registerMfaOptions = getMfaRegisterOptions(auth2faType); + const [newMfaDeviceType, setNewMfaDeviceType] = useState( + registerMfaOptions[0].value + ); + + // Attempt to get an MFA challenge for an existing device. If the challenge is + // empty, the user has no existing device (e.g. SSO user) and can register their + // first device without re-authentication. + const [reauthMfaOptions, getMfaOptions] = useAsync(async () => { + return getMfaChallengeOptions(); }); - /** A new MFA device type, irrelevant if usage === 'passkey'. */ - const [newMfaDeviceType, setNewMfaDeviceType] = useState(mfaOptions[0].value); + useEffect(() => { + getMfaOptions(); + }, []); + + // Handle potential error states first. + switch (reauthMfaOptions.status) { + case 'processing': + return ( + +
hi there
+ +
+ ); + case 'error': + return ; + case 'success': + break; + default: + return null; + } return ( 0 + ? 'withReauthentication' + : 'withoutReauthentication' } // Step properties + mfaRegisterOptions={registerMfaOptions} + mfaChallengeOptions={reauthMfaOptions.data} + reauthAttempt={attempt} + clearReauthAttempt={clearAttempt} + submitWithMfa={submitWithMfa} usage={usage} - auth2faType={auth2faType} privilegeToken={privilegeToken} credential={credential} newMfaDeviceType={newMfaDeviceType} - devices={devices} onClose={onClose} - onAuthenticated={setPrivilegeToken} onNewMfaDeviceTypeChange={setNewMfaDeviceType} onDeviceCreated={setCredential} onSuccess={onSuccess} @@ -131,10 +166,10 @@ export type AddAuthDeviceWizardStepProps = StepComponentProps & SaveKeyStepProps; interface CreateDeviceStepProps { usage: DeviceUsage; - auth2faType: Auth2faType; + mfaRegisterOptions: MfaOption[]; privilegeToken: string; - newMfaDeviceType: Auth2faType; - onNewMfaDeviceTypeChange(o: Auth2faType): void; + newMfaDeviceType: DeviceType; + onNewMfaDeviceTypeChange(o: DeviceType): void; onClose(): void; onDeviceCreated(c: Credential): void; } @@ -146,9 +181,9 @@ export function CreateDeviceStep({ stepIndex, flowLength, usage, - auth2faType, privilegeToken, newMfaDeviceType, + mfaRegisterOptions, onNewMfaDeviceTypeChange, onClose, onDeviceCreated, @@ -158,6 +193,7 @@ export function CreateDeviceStep({ if (usage === 'passwordless' || newMfaDeviceType === 'webauthn') { createPasskeyAttempt.run(async () => { const credential = await auth.createNewWebAuthnDevice({ + // TODO(Joerger): Skip privilege token step, just pass in mfa response. tokenId: privilegeToken, deviceUsage: usage, }); @@ -193,7 +229,7 @@ export function CreateDeviceStep({ )} {usage === 'mfa' && ( - // Be more specific about the WebAuthn device type (it's not a passkey). + // Be more specific about the WebAuthn device type (it's not a passkey). + mfaRegisterOptions = mfaRegisterOptions.map((o: MfaOption) => o.value === 'webauthn' ? { ...o, label: 'Hardware Device' } : o ); @@ -243,17 +276,17 @@ function CreateMfaBox({ Multi-factor type { - onNewMfaDeviceTypeChange(o as Auth2faType); + onNewMfaDeviceTypeChange(o as DeviceType); }} /> - {newMfaDeviceType === 'otp' && ( + {newMfaDeviceType === 'totp' && ( )} @@ -308,7 +341,7 @@ interface SaveKeyStepProps { privilegeToken: string; credential: Credential; usage: DeviceUsage; - newMfaDeviceType: Auth2faType; + newMfaDeviceType: DeviceType; onSuccess(): void; } @@ -396,7 +429,7 @@ export function SaveDeviceStep({ readonly={saveAttempt.attempt.status === 'processing'} /> - {usage === 'mfa' && newMfaDeviceType === 'otp' && ( + {usage === 'mfa' && newMfaDeviceType === 'totp' && ( {}, - // Other props - devices: [dummyHardwareDevice, dummyPasskey], + // Delete props deviceToDelete: dummyPasskey, privilegeToken: 'privilege-token', - auth2faType: 'optional', - onAuthenticated: () => {}, onClose: () => {}, onSuccess: () => {}, + + // Other props + reauthAttempt: {} as Attempt, + clearReauthAttempt: () => {}, + mfaChallengeOptions: [ + MFA_OPTION_WEBAUTHN, + MFA_OPTION_TOTP, + MFA_OPTION_SSO_DEFAULT, + ], + submitWithMfa: async (mfaType?: DeviceType, otpCode?: string) => {}, }; diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx index 60b11e46cf805..e51c5cf15df30 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx @@ -19,7 +19,7 @@ import { render, screen } from 'design/utils/testing'; import React from 'react'; -import { within } from '@testing-library/react'; +import { waitFor, within } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; import TeleportContext from 'teleport/teleportContext'; @@ -42,14 +42,14 @@ beforeEach(() => { user = userEvent.setup(); onSuccess = jest.fn(); + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce({ + totpChallenge: true, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + }); + jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); jest - .spyOn(auth, 'createPrivilegeTokenWithWebauthn') - .mockResolvedValueOnce('webauthn-privilege-token'); - jest - .spyOn(auth, 'createPrivilegeTokenWithTotp') - .mockImplementationOnce(token => - Promise.resolve(`totp-privilege-token-${token}`) - ); + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); jest .spyOn(MfaService.prototype, 'removeDevice') .mockResolvedValueOnce(undefined); @@ -61,9 +61,7 @@ function TestWizard(props: Partial) { return ( {}} onSuccess={onSuccess} {...props} @@ -75,14 +73,17 @@ function TestWizard(props: Partial) { test('deletes a device with WebAuthn reauthentication', async () => { render(); - const reauthenticateStep = within(screen.getByTestId('reauthenticate-step')); + let reauthenticateStep; + await waitFor(() => { + reauthenticateStep = within(screen.getByTestId('reauthenticate-step')); + }); await user.click(reauthenticateStep.getByText('Verify my identity')); const deleteStep = within(screen.getByTestId('delete-step')); await user.click(deleteStep.getByRole('button', { name: 'Delete' })); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( - 'webauthn-privilege-token', + 'privilege-token', 'TouchID' ); expect(onSuccess).toHaveBeenCalled(); @@ -91,7 +92,10 @@ test('deletes a device with WebAuthn reauthentication', async () => { test('deletes a device with OTP reauthentication', async () => { render(); - const reauthenticateStep = within(screen.getByTestId('reauthenticate-step')); + let reauthenticateStep; + await waitFor(() => { + reauthenticateStep = within(screen.getByTestId('reauthenticate-step')); + }); await user.click(reauthenticateStep.getByText('Authenticator App')); await user.type( reauthenticateStep.getByLabelText('Authenticator Code'), @@ -103,7 +107,7 @@ test('deletes a device with OTP reauthentication', async () => { await user.click(deleteStep.getByRole('button', { name: 'Delete' })); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( - 'totp-privilege-token-654987', + 'privilege-token', 'TouchID' ); }); @@ -126,9 +130,9 @@ test.each([ async ({ device, title, message }) => { render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); await user.click(reauthenticateStep.getByText('Verify my identity')); const deleteStep = within(screen.getByTestId('delete-step')); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx index 44460e9cb6c6a..b1bec7b139dad 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx @@ -16,36 +16,34 @@ * along with this program. If not, see . */ -import { OutlineDanger } from 'design/Alert/Alert'; +import { Alert, OutlineDanger } from 'design/Alert/Alert'; import { ButtonSecondary, ButtonWarning } from 'design/Button'; import Dialog from 'design/Dialog'; import Flex from 'design/Flex'; import { StepComponentProps, StepSlider } from 'design/StepSlider'; -import React, { useState } from 'react'; +import React, { useEffect, useState } from 'react'; import useAttempt from 'shared/hooks/useAttemptNext'; -import { Auth2faType } from 'shared/services'; import Box from 'design/Box'; import { StepHeader } from 'design/StepSlider'; +import { useAsync } from 'shared/hooks/useAsync'; + +import Indicator from 'design/Indicator'; + import useTeleport from 'teleport/useTeleport'; import { MfaDevice } from 'teleport/services/mfa'; +import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; + import { ReauthenticateStep, ReauthenticateStepProps, } from './ReauthenticateStep'; interface DeleteAuthDeviceWizardProps { - /** MFA type setting, as configured in the cluster's configuration. */ - auth2faType: Auth2faType; - /** - * A list of user's devices, used for computing the list of available identity - * verification options. - */ - devices: MfaDevice[]; /** Device to be removed. */ deviceToDelete: MfaDevice; onClose(): void; @@ -54,14 +52,41 @@ interface DeleteAuthDeviceWizardProps { /** A wizard for deleting MFA and passkey devices. */ export function DeleteAuthDeviceWizard({ - auth2faType, - devices, deviceToDelete, onClose, onSuccess, }: DeleteAuthDeviceWizardProps) { const [privilegeToken, setPrivilegeToken] = useState(''); + const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = + useReAuthenticate({ + onAuthenticated: setPrivilegeToken, + }); + + const [challengeOptions, getChallengeOptions] = useAsync(async () => { + return getMfaChallengeOptions(); + }); + + useEffect(() => { + getChallengeOptions(); + }, []); + + // Handle potential error states first. + switch (challengeOptions.status) { + case 'processing': + return ( + + + + ); + case 'error': + return ; + case 'success': + break; + default: + return null; + } + return ( diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.test.tsx deleted file mode 100644 index 88f18c1d724ef..0000000000000 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.test.tsx +++ /dev/null @@ -1,44 +0,0 @@ -/** - * Teleport - * Copyright (C) 2024 Gravitational, Inc. - * - * This program is free software: you can redistribute it and/or modify - * it under the terms of the GNU Affero General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU Affero General Public License for more details. - * - * You should have received a copy of the GNU Affero General Public License - * along with this program. If not, see . - */ - -import { createReauthOptions } from './ReauthenticateStep'; -import { deviceCases } from './deviceCases'; - -test.each` - auth2faType | deviceCase | methods - ${'otp'} | ${'all'} | ${['otp']} - ${'off'} | ${'all'} | ${[]} - ${'optional'} | ${'all'} | ${['webauthn', 'otp']} - ${'on'} | ${'all'} | ${['webauthn', 'otp']} - ${'webauthn'} | ${'all'} | ${['webauthn']} - ${'optional'} | ${'authApps'} | ${['otp']} - ${'optional'} | ${'mfaDevices'} | ${['webauthn']} - ${'optional'} | ${'passkeys'} | ${['webauthn']} - ${'on'} | ${'none'} | ${[]} - ${'webauthn'} | ${'authApps'} | ${[]} - ${'otp'} | ${'mfaDevices'} | ${[]} -`( - 'createReauthOptions: auth2faType=$auth2faType, devices=$deviceCase', - ({ auth2faType, methods, deviceCase }) => { - const devices = deviceCases[deviceCase]; - const reauthMethods = createReauthOptions(auth2faType, devices).map( - o => o.value - ); - expect(reauthMethods).toEqual(methods); - } -); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx index 22f53a16cc49f..b7e703daef72c 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx @@ -20,54 +20,43 @@ import { OutlineDanger } from 'design/Alert/Alert'; import { ButtonPrimary, ButtonSecondary } from 'design/Button'; import Flex from 'design/Flex'; import { RadioGroup } from 'design/RadioGroup'; -import React, { useState, FormEvent } from 'react'; +import React, { useState, FormEvent, useEffect } from 'react'; import FieldInput from 'shared/components/FieldInput'; import Validation, { Validator } from 'shared/components/Validation'; import { requiredField } from 'shared/components/Validation/rules'; -import { Auth2faType } from 'shared/services'; -import createMfaOptions, { MfaOption } from 'shared/utils/createMfaOptions'; import { StepComponentProps, StepHeader } from 'design/StepSlider'; import Box from 'design/Box'; import { Attempt } from 'shared/hooks/useAttemptNext'; -import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; -import { MfaDevice } from 'teleport/services/mfa'; +import { DeviceType } from 'teleport/services/mfa'; +import { MfaOption } from 'teleport/services/mfa'; export type ReauthenticateStepProps = StepComponentProps & { - auth2faType: Auth2faType; - devices: MfaDevice[]; - onAuthenticated(privilegeToken: string): void; + reauthAttempt: Attempt; + clearReauthAttempt(): void; + mfaChallengeOptions: MfaOption[]; + submitWithMfa(mfaType?: DeviceType, otpCode?: string): Promise; onClose(): void; }; + export function ReauthenticateStep({ next, refCallback, stepIndex, flowLength, - auth2faType, - devices, + reauthAttempt: attempt, + mfaChallengeOptions, + clearReauthAttempt: clearAttempt, + submitWithMfa, onClose, - onAuthenticated: onAuthenticatedProp, }: ReauthenticateStepProps) { - const onAuthenticated = (privilegeToken: string) => { - onAuthenticatedProp(privilegeToken); - next(); - }; - const { attempt, clearAttempt, submitWithTotp, submitWithWebauthn } = - useReAuthenticate({ - onAuthenticated, - }); - const mfaOptions = createReauthOptions(auth2faType, devices); - - const [mfaOption, setMfaOption] = useState( - mfaOptions[0]?.value - ); - const [authCode, setAuthCode] = useState(''); + const [otpCode, setOtpCode] = useState(''); + const [mfaOption, setMfaOption] = useState(mfaChallengeOptions[0].value); - const onAuthCodeChanged = (e: React.ChangeEvent) => { - setAuthCode(e.target.value); + const onOtpCodeChanged = (e: React.ChangeEvent) => { + setOtpCode(e.target.value); }; const onReauthenticate = ( @@ -76,19 +65,10 @@ export function ReauthenticateStep({ ) => { e.preventDefault(); if (!validator.validate()) return; - if (mfaOption === 'webauthn') { - submitWithWebauthn(); - } - if (mfaOption === 'otp') { - submitWithTotp(authCode); - } + submitWithMfa(mfaOption, otpCode).then(next); }; - const errorMessage = getReauthenticationErrorMessage( - auth2faType, - mfaOptions.length, - attempt - ); + const errorMessage = getReauthenticationErrorMessage(attempt); return (
@@ -106,26 +86,26 @@ export function ReauthenticateStep({
onReauthenticate(e, validator)}> { - setMfaOption(o as Auth2faType); + setMfaOption(o as DeviceType); clearAttempt(); }} /> - {mfaOption === 'otp' && ( + {mfaOption === 'totp' && ( )} @@ -150,45 +130,8 @@ export function ReauthenticateStep({
); } -function getReauthenticationErrorMessage( - auth2faType: Auth2faType, - numMfaOptions: number, - attempt: Attempt -): string { - if (numMfaOptions === 0) { - switch (auth2faType) { - case 'on': - return ( - "Identity verification is required, but you don't have any" + - 'passkeys or MFA methods registered. This may mean that the' + - 'server configuration has changed. Please contact your ' + - 'administrator.' - ); - case 'otp': - return ( - 'Identity verification using authenticator app is required, but ' + - "you don't have any authenticator apps registered. This may mean " + - 'that the server configuration has changed. Please contact your ' + - 'administrator.' - ); - case 'webauthn': - return ( - 'Identity verification using a passkey or security key is required, but ' + - "you don't have any such devices registered. This may mean " + - 'that the server configuration has changed. Please contact your ' + - 'administrator.' - ); - case 'optional': - case 'off': - // This error message is not useful, but this condition should never - // happen, and if it does, it means something is broken, and we don't - // have a clue anyway. - return 'Unable to verify identity'; - default: - auth2faType satisfies never; - } - } +function getReauthenticationErrorMessage(attempt: Attempt): string { if (attempt.status === 'failed') { // This message relies on the status message produced by the auth server in // lib/auth/Server.checkOTP function. Please keep these in sync. @@ -199,15 +142,3 @@ function getReauthenticationErrorMessage( } } } - -export function createReauthOptions( - auth2faType: Auth2faType, - devices: MfaDevice[] -): MfaOption[] { - return createMfaOptions({ auth2faType, required: true }).filter( - ({ value }) => { - const deviceType = value === 'otp' ? 'totp' : value; - return devices.some(({ type }) => type === deviceType); - } - ); -} diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx index 088bedae810a2..e3e784e691c44 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx @@ -18,8 +18,12 @@ import React from 'react'; -import { State } from './useReAuthenticate'; -import { ReAuthenticate } from './ReAuthenticate'; +import { ReAuthenticate, State } from './ReAuthenticate'; +import { + MFA_OPTION_SSO_DEFAULT, + MFA_OPTION_TOTP, + MFA_OPTION_WEBAUTHN, +} from 'teleport/services/mfa'; export default { title: 'Teleport/ReAuthenticate', @@ -41,10 +45,12 @@ export const Failed = () => ( const props: State = { attempt: { status: '' }, clearAttempt: () => null, - submitWithTotp: () => null, - submitWithWebauthn: () => null, - preferredMfaType: 'webauthn', + getMfaChallenge: () => null, + getMfaChallengeOptions: async () => [ + MFA_OPTION_WEBAUTHN, + MFA_OPTION_TOTP, + MFA_OPTION_SSO_DEFAULT, + ], + submitWithMfa: () => null, onClose: () => null, - auth2faType: 'on', - actionText: 'performing this action', }; diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx index c344262c7fd62..261f7efe9152f 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx @@ -16,55 +16,88 @@ * along with this program. If not, see . */ -import React, { useState } from 'react'; -import { Flex, Box, Text, ButtonPrimary, ButtonSecondary } from 'design'; +import React, { useEffect, useState } from 'react'; +import { + Flex, + Box, + Text, + ButtonPrimary, + ButtonSecondary, + Indicator, +} from 'design'; import Dialog, { DialogHeader, DialogTitle, DialogContent, DialogFooter, } from 'design/Dialog'; -import { Danger } from 'design/Alert'; +import { Alert, Danger } from 'design/Alert'; import Validation from 'shared/components/Validation'; import { requiredToken } from 'shared/components/Validation/rules'; import FieldInput from 'shared/components/FieldInput'; import FieldSelect from 'shared/components/FieldSelect'; -import createMfaOptions, { MfaOption } from 'shared/utils/createMfaOptions'; -import useReAuthenticate, { State, Props } from './useReAuthenticate'; +import { useAsync } from 'shared/hooks/useAsync'; + +import { MfaOption } from 'teleport/services/mfa'; + +import useReAuthenticate, { + ReauthState, + ReauthProps, +} from './useReAuthenticate'; + +export type Props = ReauthProps & { + onClose: () => void; +}; export default function Container(props: Props) { const state = useReAuthenticate(props); - return ; + return ; } +export type State = ReauthState & { + onClose: () => void; +}; + export function ReAuthenticate({ + onClose, attempt, clearAttempt, - submitWithTotp, - submitWithWebauthn, - onClose, - auth2faType, - preferredMfaType, - actionText, + getMfaChallengeOptions, + submitWithMfa, }: State) { - const [otpToken, setOtpToken] = useState(''); - const mfaOptions = createMfaOptions({ - auth2faType: auth2faType, - preferredType: preferredMfaType, - required: true, + const [otpCode, setOtpToken] = useState(''); + const [mfaOption, setMfaOption] = useState(); + + const [challengeOptions, getChallengeOptions] = useAsync(async () => { + const mfaOptions = getMfaChallengeOptions(); + setMfaOption(mfaOptions[0]); + return mfaOptions; }); - const [mfaOption, setMfaOption] = useState(mfaOptions[0]); + + useEffect(() => { + getChallengeOptions(); + }, []); + + // Handle potential error states first. + switch (challengeOptions.status) { + case 'processing': + return ( + + + + ); + case 'error': + return ; + case 'success': + break; + default: + return null; + } function onSubmit(e: React.MouseEvent) { e.preventDefault(); - - if (mfaOption?.value === 'webauthn') { - submitWithWebauthn(); - } - if (mfaOption?.value === 'otp') { - submitWithTotp(otpToken); - } + submitWithMfa(mfaOption.value, otpCode); } return ( @@ -83,7 +116,7 @@ export function ReAuthenticate({ Verify your identity You must verify your identity with one of your existing - two-factor devices before {actionText}. + two-factor devices before performing this action. {attempt.status === 'failed' && ( @@ -97,7 +130,7 @@ export function ReAuthenticate({ width="60%" label="Two-factor Type" value={mfaOption} - options={mfaOptions} + options={challengeOptions.data} onChange={(o: MfaOption) => { setMfaOption(o); clearAttempt(); @@ -109,13 +142,13 @@ export function ReAuthenticate({ elevated={true} /> - {mfaOption.value === 'otp' && ( + {mfaOption?.value === 'totp' && ( setOtpToken(e.target.value)} placeholder="123 456" readonly={attempt.status === 'processing'} diff --git a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts index 2a535b5673c6f..4d9e991277827 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts +++ b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts @@ -16,13 +16,19 @@ * along with this program. If not, see . */ -import useAttempt from 'shared/hooks/useAttemptNext'; +import { useState } from 'react'; +import useAttempt, { Attempt } from 'shared/hooks/useAttemptNext'; -import cfg from 'teleport/config'; import auth from 'teleport/services/auth'; import { MfaChallengeScope } from 'teleport/services/auth/auth'; -import type { MfaChallengeResponse } from 'teleport/services/mfa'; +import { + getMfaChallengeOptions as getChallengeOptions, + DeviceType, + MfaAuthenticateChallenge, + MfaChallengeResponse, + MfaOption, +} from 'teleport/services/mfa'; // useReAuthenticate will have different "submit" behaviors depending on: // - If prop field `onMfaResponse` is defined, after a user submits, the @@ -31,60 +37,69 @@ import type { MfaChallengeResponse } from 'teleport/services/mfa'; // user's MFA response are submitted with the request to get a privilege // token, and after successfully obtaining the token, the function // `onAuthenticated` will be called with this token. -export default function useReAuthenticate(props: Props) { - const { onClose, actionText = defaultActionText } = props; - +export default function useReAuthenticate(props: ReauthProps): ReauthState { // Note that attempt state "success" is not used or required. // After the user submits, the control is passed back - // to the caller who is reponsible for rendering the `ReAuthenticate` + // to the caller who is responsible for rendering the `ReAuthenticate` // component. - const { attempt, setAttempt, handleError } = useAttempt(''); - - function submitWithTotp(secondFactorToken: string) { - if ('onMfaResponse' in props) { - props.onMfaResponse({ totp_code: secondFactorToken }); + const { attempt, setAttempt } = useAttempt(''); + + const [challenge, setMfaChallenge] = useState(null); + + // Provide a custom error handler to catch a webauthn frontend error that occurs + // on Firefox and replace it with a more helpful error message. + const handleError = (err: Error) => { + if (err.message.includes('attempt was made to use an object that is not')) { + setAttempt({ + status: 'failed', + statusText: + 'The two-factor device you used is not registered on this account. You must verify using a device that has already been registered.', + }); + return; + } else { + setAttempt({ status: 'failed', statusText: err.message }); return; } + }; - setAttempt({ status: 'processing' }); - auth - .createPrivilegeToken({ totp_code: secondFactorToken }) - .then(props.onAuthenticated) - .catch(handleError); - } - - function submitWithWebauthn() { - setAttempt({ status: 'processing' }); - - if ('onMfaResponse' in props) { + // TODO(Joerger): Replace onAuthenticated with onMfaResponse at call sites (/e). + if (props.onAuthenticated) { + // Creating privilege tokens always expects the MANAGE_DEVICES webauthn scope. + props.challengeScope = MfaChallengeScope.MANAGE_DEVICES; + props.onMfaResponse = mfaResponse => { auth - .getMfaChallenge({ scope: props.challengeScope }) - .then(challenge => auth.getMfaChallengeResponse(challenge, 'webauthn')) + .createPrivilegeToken(mfaResponse) + .then(props.onAuthenticated) .catch(handleError); + }; + } - return; + async function getMfaChallenge() { + if (challenge) { + return challenge; } - // Creating privilege tokens always expects the MANAGE_DEVICES webauthn scope. - auth - .getMfaChallenge({ scope: MfaChallengeScope.MANAGE_DEVICES }) - .then(auth.getMfaChallengeResponse) - .then(auth.createPrivilegeToken) - .then(props.onAuthenticated) - .catch((err: Error) => { - // This catches a webauthn frontend error that occurs on Firefox and replaces it with a more helpful error message. - if ( - err.message.includes('attempt was made to use an object that is not') - ) { - setAttempt({ - status: 'failed', - statusText: - 'The two-factor device you used is not registered on this account. You must verify using a device that has already been registered.', - }); - } else { - setAttempt({ status: 'failed', statusText: err.message }); - } - }); + return auth.getMfaChallenge({ scope: props.challengeScope }).then(chal => { + setMfaChallenge(chal); + return chal; + }); + } + + function clearMfaChallenge() { + setMfaChallenge(null); + } + + function getMfaChallengeOptions() { + return getMfaChallenge().then(getChallengeOptions); + } + + function submitWithMfa(mfaType?: DeviceType, totp_code?: string) { + setAttempt({ status: 'processing' }); + return getMfaChallenge() + .then(chal => auth.getMfaChallengeResponse(chal, mfaType, totp_code)) + .then(props.onMfaResponse) + .finally(clearMfaChallenge) + .catch(handleError); } function clearAttempt() { @@ -94,53 +109,23 @@ export default function useReAuthenticate(props: Props) { return { attempt, clearAttempt, - submitWithTotp, - submitWithWebauthn, - auth2faType: cfg.getAuth2faType(), - preferredMfaType: cfg.getPreferredMfaType(), - actionText, - onClose, + getMfaChallenge, + getMfaChallengeOptions, + submitWithMfa, }; } -const defaultActionText = 'performing this action'; - -type BaseProps = { - onClose?: () => void; - /** - * The text that will be appended to the text in the re-authentication dialog. - * - * Default value: "performing this action" - * - * Example: If `actionText` is set to "registering a new device" then the dialog will say - * "You must verify your identity with one of your existing two-factor devices before registering a new device." - * - * */ - actionText?: string; +export type ReauthProps = { + challengeScope?: MfaChallengeScope; + onMfaResponse?(res: MfaChallengeResponse): void; + // TODO(Joerger): Remove in favor of onMfaResponse, make onMfaResponse required. + onAuthenticated?(privilegeTokenId: string): void; }; -// MfaResponseProps defines a function -// that accepts a MFA response. No -// authentication has been done at this point. -type MfaResponseProps = BaseProps & { - onMfaResponse(res: MfaChallengeResponse): void; - /** - * The MFA challenge scope of the action to perform, as defined in webauthn.proto. - */ - challengeScope: MfaChallengeScope; - onAuthenticated?: never; +export type ReauthState = { + attempt: Attempt; + clearAttempt: () => void; + getMfaChallenge: () => Promise; + getMfaChallengeOptions: () => Promise; + submitWithMfa: (mfaType?: DeviceType, totp_code?: string) => Promise; }; - -// DefaultProps defines a function that -// accepts a privilegeTokenId that is only -// obtained after MFA response has been -// validated. -type DefaultProps = BaseProps & { - onAuthenticated(privilegeTokenId: string): void; - onMfaResponse?: never; - challengeScope?: never; -}; - -export type Props = MfaResponseProps | DefaultProps; - -export type State = ReturnType; diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index f4da45a33b1f7..c26a5fe0641f4 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -16,6 +16,8 @@ * along with this program. If not, see . */ +import { Auth2faType } from 'shared/services'; + import api from 'teleport/services/api'; import cfg from 'teleport/config'; import { diff --git a/web/packages/teleport/src/services/mfa/mfaOptions.ts b/web/packages/teleport/src/services/mfa/mfaOptions.ts index 4bbe1dceb65f1..5f63b7ecdb12c 100644 --- a/web/packages/teleport/src/services/mfa/mfaOptions.ts +++ b/web/packages/teleport/src/services/mfa/mfaOptions.ts @@ -57,16 +57,22 @@ export type MfaOption = { label: string; }; -const MFA_OPTION_WEBAUTHN: MfaOption = { +export const MFA_OPTION_WEBAUTHN: MfaOption = { value: 'webauthn', label: 'Passkey or Security Key', }; -const MFA_OPTION_TOTP: MfaOption = { +export const MFA_OPTION_TOTP: MfaOption = { value: 'totp', label: 'Authenticator App', }; +// used in tests, returned by getSsoOptions(null). +export const MFA_OPTION_SSO_DEFAULT: MfaOption = { + value: 'sso', + label: 'SSO', +}; + const getSsoOption = (ssoChallenge: SSOChallenge): MfaOption => { return { value: 'sso', From 1a1e648efa63da10eaed927c49492d458c4a1fb3 Mon Sep 17 00:00:00 2001 From: joerger Date: Wed, 27 Nov 2024 16:56:45 -0800 Subject: [PATCH 03/13] Refactor ChangePasswordWizard to use useReauthenticate. --- .../teleport/src/Account/Account.test.tsx | 9 + .../ChangePasswordWizard.story.tsx | 61 ++--- .../ChangePasswordWizard.test.tsx | 209 +++++------------- .../ChangePasswordWizard.tsx | 195 ++++++++-------- .../wizards/AddAuthDeviceWizard.test.tsx | 2 +- .../wizards/AddAuthDeviceWizard.tsx | 2 +- .../wizards/ReauthenticateStep.tsx | 10 +- .../teleport/src/Account/PasswordBox.tsx | 7 +- .../ReAuthenticate/ReAuthenticate.story.tsx | 1 + .../ReAuthenticate/useReAuthenticate.ts | 17 ++ .../teleport/src/services/auth/auth.ts | 11 +- .../teleport/src/services/auth/types.ts | 5 +- .../teleport/src/services/mfa/mfaOptions.ts | 10 +- 13 files changed, 227 insertions(+), 312 deletions(-) diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index e4d098d1d2002..2ef6cdc931e25 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -192,6 +192,14 @@ test('password change', async () => { const ctx = createTeleportContext(); ctx.storeUser.setState({ passwordState: PasswordState.PASSWORD_STATE_UNSET }); jest.spyOn(ctx.mfaService, 'fetchDevices').mockResolvedValue([]); + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({ + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + totpChallenge: true, + }); + jest.spyOn(auth, 'getMfaChallengeResponse').mockResolvedValueOnce({}); + jest + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); jest.spyOn(auth, 'changePassword').mockResolvedValueOnce(undefined); await renderComponent(ctx); @@ -201,6 +209,7 @@ test('password change', async () => { // Change the password await user.click(screen.getByRole('button', { name: 'Change Password' })); + await user.click(screen.getByRole('button', { name: 'Next' })); await user.type(screen.getByLabelText('Current Password'), 'old-password'); await user.type(screen.getByLabelText('New Password'), 'asdfasdfasdfasdf'); await user.type( diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx index 3ec850a0aeba7..3876fdd06b8fb 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx @@ -23,13 +23,18 @@ import Dialog from 'design/Dialog'; import { createTeleportContext } from 'teleport/mocks/contexts'; import { ContextProvider } from 'teleport'; -import { MfaDevice, WebauthnAssertionResponse } from 'teleport/services/mfa'; +import { + MFA_OPTION_SSO_DEFAULT, + MFA_OPTION_TOTP, + WebauthnAssertionResponse, +} from 'teleport/services/mfa'; import { ChangePasswordStep, ChangePasswordWizardStepProps, + REAUTH_OPTION_PASSKEY, + REAUTH_OPTION_WEBAUTHN, ReauthenticateStep, - createReauthOptions, } from './ChangePasswordWizard'; export default { @@ -60,44 +65,16 @@ export function ChangePasswordWithPasswordlessVerification() { } export function ChangePasswordWithMfaDeviceVerification() { - return ; + return ; } export function ChangePasswordWithOtpVerification() { - return ; + return ; } -const devices: MfaDevice[] = [ - { - id: '1', - description: 'Hardware Key', - name: 'touch_id', - registeredDate: new Date(1628799417000), - lastUsedDate: new Date(1628799417000), - type: 'webauthn', - usage: 'passwordless', - }, - { - id: '2', - description: 'Hardware Key', - name: 'solokey', - registeredDate: new Date(1623722252000), - lastUsedDate: new Date(1623981452000), - type: 'webauthn', - usage: 'mfa', - }, - { - id: '3', - description: 'Authenticator App', - name: 'iPhone', - registeredDate: new Date(1618711052000), - lastUsedDate: new Date(1626472652000), - type: 'totp', - usage: 'mfa', - }, -]; - -const defaultReauthOptions = createReauthOptions('optional', true, devices); +export function ChangePasswordWithSsoVerification() { + return ; +} const stepProps = { // StepComponentProps @@ -109,14 +86,20 @@ const stepProps = { refCallback: () => {}, // Shared props - reauthMethod: 'mfaDevice', + reauthMethod: 'passwordless', onClose() {}, onSuccess() {}, // ReauthenticateStepProps - reauthOptions: defaultReauthOptions, - onReauthMethodChange() {}, - onWebauthnResponse() {}, + reauthOptions: [ + REAUTH_OPTION_PASSKEY, + REAUTH_OPTION_WEBAUTHN, + MFA_OPTION_TOTP, + MFA_OPTION_SSO_DEFAULT, + ], + onReauthMethodChange: () => {}, + submitWithPasswordless: async () => {}, + submitWithMfa: async () => {}, // ChangePasswordStepProps webauthnResponse: {} as WebauthnAssertionResponse, diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx index d23469ead98fd..6ff139610cc8d 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx @@ -19,16 +19,23 @@ import { render, screen } from 'design/utils/testing'; import React from 'react'; -import { within } from '@testing-library/react'; +import { waitFor, within } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; -import { MfaChallengeResponse } from 'teleport/services/mfa'; +import { + MFA_OPTION_SSO_DEFAULT, + MFA_OPTION_TOTP, + MFA_OPTION_WEBAUTHN, + MfaChallengeResponse, +} from 'teleport/services/mfa'; import { ChangePasswordWizardProps, - createReauthOptions, + getReauthOptions, + REAUTH_OPTION_PASSKEY, + REAUTH_OPTION_WEBAUTHN, } from './ChangePasswordWizard'; import { ChangePasswordWizard } from '.'; @@ -52,29 +59,10 @@ const dummyChallengeResponse: MfaChallengeResponse = { let user: UserEvent; let onSuccess: jest.Mock; -function twice(arr) { - return [...arr, ...arr]; -} - -// Repeat devices twice to make sure we support multiple devices of the same -// type and purpose. -const deviceCases = { - all: twice([ - { type: 'totp', usage: 'mfa' }, - { type: 'webauthn', usage: 'mfa' }, - { type: 'webauthn', usage: 'passwordless' }, - ]), - authApps: twice([{ type: 'totp', usage: 'mfa' }]), - mfaDevices: twice([{ type: 'webauthn', usage: 'mfa' }]), - passkeys: twice([{ type: 'webauthn', usage: 'passwordless' }]), -}; - function TestWizard(props: Partial = {}) { return ( {}} onSuccess={onSuccess} {...props} @@ -86,7 +74,10 @@ beforeEach(() => { user = userEvent.setup(); onSuccess = jest.fn(); - jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(undefined); + jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({ + totpChallenge: true, + webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, + }); jest .spyOn(auth, 'getMfaChallengeResponse') .mockResolvedValueOnce(dummyChallengeResponse); @@ -99,9 +90,13 @@ describe('with passwordless reauthentication', () => { async function reauthenticate() { render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); + expect(auth.getMfaChallenge).toHaveBeenCalledWith({ + scope: MfaChallengeScope.CHANGE_PASSWORD, + }); + await user.click(reauthenticateStep.getByText('Passkey')); await user.click(reauthenticateStep.getByText('Next')); expect(auth.getMfaChallenge).toHaveBeenCalledWith({ @@ -128,8 +123,10 @@ describe('with passwordless reauthentication', () => { expect(auth.changePassword).toHaveBeenCalledWith({ oldPassword: '', newPassword: 'new-pass1234', - secondFactorToken: '', - webauthnResponse: dummyChallengeResponse.webauthn_response, + mfaResponse: { + totp_code: '', + webauthn_response: dummyChallengeResponse.webauthn_response, + }, }); expect(onSuccess).toHaveBeenCalled(); }); @@ -191,15 +188,16 @@ describe('with WebAuthn MFA reauthentication', () => { async function reauthenticate() { render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); - await user.click(reauthenticateStep.getByText('MFA Device')); - await user.click(reauthenticateStep.getByText('Next')); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); expect(auth.getMfaChallenge).toHaveBeenCalledWith({ scope: MfaChallengeScope.CHANGE_PASSWORD, - userVerificationRequirement: 'discouraged', }); + + await user.click(reauthenticateStep.getByText('Security Key')); + await user.click(reauthenticateStep.getByText('Next')); + expect(auth.getMfaChallengeResponse).toHaveBeenCalled(); } it('changes password', async () => { @@ -223,8 +221,10 @@ describe('with WebAuthn MFA reauthentication', () => { expect(auth.changePassword).toHaveBeenCalledWith({ oldPassword: 'current-pass', newPassword: 'new-pass1234', - secondFactorToken: '', - webauthnResponse: dummyChallengeResponse.webauthn_response, + mfaResponse: { + totp_code: '', + webauthn_response: dummyChallengeResponse.webauthn_response, + }, }); expect(onSuccess).toHaveBeenCalled(); }); @@ -293,12 +293,11 @@ describe('with OTP MFA reauthentication', () => { async function reauthenticate() { render(); - const reauthenticateStep = within( - screen.getByTestId('reauthenticate-step') - ); + const reauthenticateStep = await waitFor(() => { + return within(screen.getByTestId('reauthenticate-step')); + }); await user.click(reauthenticateStep.getByText('Authenticator App')); await user.click(reauthenticateStep.getByText('Next')); - expect(auth.getMfaChallenge).not.toHaveBeenCalled(); } it('changes password', async () => { @@ -326,7 +325,9 @@ describe('with OTP MFA reauthentication', () => { expect(auth.changePassword).toHaveBeenCalledWith({ oldPassword: 'current-pass', newPassword: 'new-pass1234', - secondFactorToken: '654321', + mfaResponse: { + totp_code: '654321', + }, }); expect(onSuccess).toHaveBeenCalled(); }); @@ -402,121 +403,17 @@ describe('with OTP MFA reauthentication', () => { }); }); -describe('without reauthentication', () => { - it('changes password', async () => { - render(); - - const changePasswordStep = within( - screen.getByTestId('change-password-step') - ); - await user.type( - changePasswordStep.getByLabelText('Current Password'), - 'current-pass' - ); - await user.type( - changePasswordStep.getByLabelText('New Password'), - 'new-pass1234' - ); - await user.type( - changePasswordStep.getByLabelText('Confirm Password'), - 'new-pass1234' - ); - await user.click(changePasswordStep.getByText('Save Changes')); - expect(auth.getMfaChallenge).not.toHaveBeenCalled(); - expect(auth.changePassword).toHaveBeenCalledWith({ - oldPassword: 'current-pass', - newPassword: 'new-pass1234', - webauthnResponse: undefined, - secondFactorToken: '', - }); - expect(onSuccess).toHaveBeenCalled(); - }); - - it('cancels changing password', async () => { - render(); - - const changePasswordStep = within( - screen.getByTestId('change-password-step') - ); - await user.type( - changePasswordStep.getByLabelText('New Password'), - 'new-pass1234' - ); - await user.type( - changePasswordStep.getByLabelText('Confirm Password'), - 'new-pass1234' - ); - await user.click(changePasswordStep.getByText('Cancel')); - expect(auth.changePassword).not.toHaveBeenCalled(); - expect(onSuccess).not.toHaveBeenCalled(); - }); - - it('validates the password form', async () => { - render(); - - const changePasswordStep = within( - screen.getByTestId('change-password-step') - ); - await user.type( - changePasswordStep.getByLabelText('New Password'), - 'new-pass123' - ); - await user.type( - changePasswordStep.getByLabelText('Confirm Password'), - 'new-pass123' - ); - await user.click(changePasswordStep.getByText('Save Changes')); - expect(auth.changePassword).not.toHaveBeenCalled(); - expect(onSuccess).not.toHaveBeenCalled(); - expect(changePasswordStep.getByLabelText('New Password')).toBeInvalid(); - expect( - changePasswordStep.getByLabelText('New Password') - ).toHaveAccessibleDescription('Enter at least 12 characters'); - expect(changePasswordStep.getByLabelText('Current Password')).toBeInvalid(); - expect( - changePasswordStep.getByLabelText('Current Password') - ).toHaveAccessibleDescription('Current Password is required'); - - await user.type( - changePasswordStep.getByLabelText('Current Password'), - 'current-pass' - ); - await user.type( - changePasswordStep.getByLabelText('New Password'), - 'new-pass1234' - ); - await user.click(changePasswordStep.getByText('Save Changes')); - expect(auth.changePassword).not.toHaveBeenCalled(); - expect(onSuccess).not.toHaveBeenCalled(); - expect(changePasswordStep.getByLabelText('Confirm Password')).toBeInvalid(); - expect( - changePasswordStep.getByLabelText('Confirm Password') - ).toHaveAccessibleDescription('Password does not match'); - }); -}); - test.each` - auth2faType | passwordless | deviceCase | methods - ${'otp'} | ${false} | ${'all'} | ${['otp']} - ${'off'} | ${false} | ${'all'} | ${[]} - ${'optional'} | ${false} | ${'all'} | ${['mfaDevice', 'otp']} - ${'on'} | ${false} | ${'all'} | ${['mfaDevice', 'otp']} - ${'webauthn'} | ${false} | ${'all'} | ${['mfaDevice']} - ${'optional'} | ${true} | ${'all'} | ${['passwordless', 'mfaDevice', 'otp']} - ${'on'} | ${true} | ${'all'} | ${['passwordless', 'mfaDevice', 'otp']} - ${'webauthn'} | ${true} | ${'all'} | ${['passwordless', 'mfaDevice']} - ${'optional'} | ${true} | ${'authApps'} | ${['otp']} - ${'optional'} | ${true} | ${'mfaDevices'} | ${['mfaDevice']} - ${'optional'} | ${true} | ${'passkeys'} | ${['passwordless']} + mfaOptions | hasPasswordless | reauthOptions + ${[MFA_OPTION_TOTP]} | ${false} | ${[MFA_OPTION_TOTP]} + ${[MFA_OPTION_WEBAUTHN]} | ${false} | ${[REAUTH_OPTION_WEBAUTHN]} + ${[MFA_OPTION_TOTP, MFA_OPTION_WEBAUTHN]} | ${false} | ${[MFA_OPTION_TOTP, REAUTH_OPTION_WEBAUTHN]} + ${[MFA_OPTION_WEBAUTHN]} | ${true} | ${[REAUTH_OPTION_PASSKEY, REAUTH_OPTION_WEBAUTHN]} + ${[MFA_OPTION_TOTP, MFA_OPTION_WEBAUTHN]} | ${true} | ${[REAUTH_OPTION_PASSKEY, MFA_OPTION_TOTP, REAUTH_OPTION_WEBAUTHN]} `( - 'createReauthOptions: auth2faType=$auth2faType, passwordless=$passwordless, devices=$deviceCase', - ({ auth2faType, passwordless, methods, deviceCase }) => { - const devices = deviceCases[deviceCase]; - const reauthMethods = createReauthOptions( - auth2faType, - passwordless, - devices - ).map(o => o.value); - expect(reauthMethods).toEqual(methods); + 'getReauthOptions: mfaOptions=$mfaOptions, hasPasswordless=$hasPasswordless, devices=$deviceCase', + ({ mfaOptions, hasPasswordless, reauthOptions }) => { + const gotReauthOptions = getReauthOptions(mfaOptions, hasPasswordless); + expect(gotReauthOptions).toEqual(reauthOptions); } ); diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx index 3a66e65a070ce..d2eb2cc4b45dc 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx @@ -17,13 +17,13 @@ */ import styled from 'styled-components'; -import { OutlineDanger } from 'design/Alert/Alert'; +import { Alert, OutlineDanger } from 'design/Alert/Alert'; import { ButtonPrimary, ButtonSecondary } from 'design/Button'; import Dialog from 'design/Dialog'; import Flex from 'design/Flex'; import { RadioGroup } from 'design/RadioGroup'; import { StepComponentProps, StepSlider, StepHeader } from 'design/StepSlider'; -import React, { useState } from 'react'; +import React, { useEffect, useState } from 'react'; import FieldInput from 'shared/components/FieldInput'; import Validation, { Validator } from 'shared/components/Validation'; import { @@ -32,43 +32,71 @@ import { requiredPassword, } from 'shared/components/Validation/rules'; import { useAsync } from 'shared/hooks/useAsync'; -import { Auth2faType } from 'shared/services'; import Box from 'design/Box'; import { ChangePasswordReq } from 'teleport/services/auth'; import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; -import { MfaDevice, WebauthnAssertionResponse } from 'teleport/services/mfa'; +import { + DeviceType, + MfaOption, + WebauthnAssertionResponse, +} from 'teleport/services/mfa'; +import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; +import Indicator from 'design/Indicator'; export interface ChangePasswordWizardProps { - /** MFA type setting, as configured in the cluster's configuration. */ - auth2faType: Auth2faType; - /** Determines whether the cluster allows passwordless login. */ - passwordlessEnabled: boolean; - /** A list of available authentication devices. */ - devices: MfaDevice[]; + hasPasswordless: boolean; onClose(): void; onSuccess(): void; } export function ChangePasswordWizard({ - auth2faType, - passwordlessEnabled, - devices, + hasPasswordless, onClose, onSuccess, }: ChangePasswordWizardProps) { - const reauthOptions = createReauthOptions( - auth2faType, - passwordlessEnabled, - devices - ); - const [reauthMethod, setReauthMethod] = useState( - reauthOptions[0]?.value - ); const [webauthnResponse, setWebauthnResponse] = useState(); - const reauthRequired = reauthOptions.length > 0; + const { getMfaChallengeOptions, submitWithMfa, submitWithPasswordless } = + useReAuthenticate({ + challengeScope: MfaChallengeScope.CHANGE_PASSWORD, + onMfaResponse: mfaResponse => { + setWebauthnResponse(mfaResponse.webauthn_response); + }, + }); + + // Attempt to get an MFA challenge for an existing device. If the challenge is + // empty, the user has no existing device (e.g. SSO user) and can register their + // first device without re-authentication. + const [reauthOptions, initReauthOptions] = useAsync(async () => { + let mfaOptions = await getMfaChallengeOptions(); + const reauthOptions = getReauthOptions(mfaOptions, hasPasswordless); + setReauthMethod(reauthOptions[0].value); + return reauthOptions; + }); + + useEffect(() => { + initReauthOptions(); + }, []); + + const [reauthMethod, setReauthMethod] = useState(); + + // Handle potential error states first. + switch (reauthOptions.status) { + case 'processing': + return ( + + + + ); + case 'error': + return ; + case 'success': + break; + default: + return null; + } return ( @@ -95,56 +122,41 @@ export function ChangePasswordWizard({ ); } -type ReauthenticationMethod = 'passwordless' | 'mfaDevice' | 'otp'; +type ReauthenticationMethod = 'passwordless' | DeviceType; type ReauthenticationOption = { value: ReauthenticationMethod; label: string; }; -export function createReauthOptions( - auth2faType: Auth2faType, - passwordlessEnabled: boolean, - devices: MfaDevice[] -) { - const options: ReauthenticationOption[] = []; - - const methodsAllowedByDevices = {}; - for (const d of devices) { - methodsAllowedByDevices[reauthMethodForDevice(d)] = true; - } - - if (passwordlessEnabled && 'passwordless' in methodsAllowedByDevices) { - options.push({ value: 'passwordless', label: 'Passkey' }); - } +export const REAUTH_OPTION_WEBAUTHN: ReauthenticationOption = { + value: 'webauthn', + label: 'Security Key', +}; - const mfaEnabled = auth2faType === 'on' || auth2faType === 'optional'; +export const REAUTH_OPTION_PASSKEY: ReauthenticationOption = { + value: 'passwordless', + label: 'Passkey', +}; - if ( - (auth2faType === 'webauthn' || mfaEnabled) && - 'mfaDevice' in methodsAllowedByDevices - ) { - options.push({ value: 'mfaDevice', label: 'MFA Device' }); - } +export function getReauthOptions( + mfaOptions: MfaOption[], + hasPasswordless: boolean +) { + // Be more specific about the WebAuthn device type (it's not a passkey). + const reauthOptions = mfaOptions.map((o: ReauthenticationOption) => + o.value === 'webauthn' ? REAUTH_OPTION_WEBAUTHN : o + ); - if ( - (auth2faType === 'otp' || mfaEnabled) && - 'otp' in methodsAllowedByDevices - ) { - options.push({ value: 'otp', label: 'Authenticator App' }); + // Add passwordless as the default if available. + if (hasPasswordless) { + reauthOptions.unshift(REAUTH_OPTION_PASSKEY); } - return options; -} - -/** Returns the reauthentication method supported by a given device. */ -function reauthMethodForDevice(d: MfaDevice): ReauthenticationMethod { - if (d.usage === 'passwordless') return 'passwordless'; - return d.type === 'totp' ? 'otp' : 'mfaDevice'; + return reauthOptions; } const wizardFlows = { withReauthentication: [ReauthenticateStep, ChangePasswordStep], - withoutReauthentication: [ChangePasswordStep], }; export type ChangePasswordWizardStepProps = StepComponentProps & @@ -155,7 +167,8 @@ interface ReauthenticateStepProps { reauthOptions: ReauthenticationOption[]; reauthMethod: ReauthenticationMethod; onReauthMethodChange(method: ReauthenticationMethod): void; - onWebauthnResponse(res: WebauthnAssertionResponse): void; + submitWithPasswordless(): Promise; + submitWithMfa(mfaType?: DeviceType): Promise; onClose(): void; } @@ -167,29 +180,27 @@ export function ReauthenticateStep({ reauthOptions, reauthMethod, onReauthMethodChange, - onWebauthnResponse, + submitWithPasswordless, + submitWithMfa, onClose, }: ChangePasswordWizardStepProps) { - const [reauthenticateAttempt, reauthenticate] = useAsync( - async (m: ReauthenticationMethod) => { - if (m === 'passwordless' || m === 'mfaDevice') { - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.CHANGE_PASSWORD, - userVerificationRequirement: - m === 'passwordless' ? 'required' : 'discouraged', - }); - - const response = await auth.getMfaChallengeResponse( - challenge, - 'webauthn' - ); - - // TODO(Joerger): handle non-webauthn response. - onWebauthnResponse(response.webauthn_response); + const [reauthAttempt, reauthenticate] = useAsync( + async (reauthMethod: ReauthenticationMethod) => { + switch (reauthMethod) { + case 'passwordless': + await submitWithPasswordless(); + break; + case 'totp': + // totp is handled in the ChangePasswordStep + break; + default: + await submitWithMfa(reauthMethod); + break; } next(); } ); + const onReauthenticate = (e: React.FormEvent) => { e.preventDefault(); reauthenticate(reauthMethod); @@ -204,8 +215,8 @@ export function ReauthenticateStep({ title="Verify Identity" /> - {reauthenticateAttempt.status === 'error' && ( - {reauthenticateAttempt.statusText} + {reauthAttempt.status === 'error' && ( + {reauthAttempt.statusText} )} Verification Method onReauthenticate(e)}> @@ -252,9 +263,9 @@ export function ChangePasswordStep({ const [oldPassword, setOldPassword] = useState(''); const [newPassword, setNewPassword] = useState(''); const [newPassConfirmed, setNewPassConfirmed] = useState(''); - const [authCode, setAuthCode] = useState(''); + const [otpCode, setOtpCode] = useState(''); const onAuthCodeChanged = (e: React.ChangeEvent) => { - setAuthCode(e.target.value); + setOtpCode(e.target.value); }; const [changePasswordAttempt, changePassword] = useAsync( async (req: ChangePasswordReq) => { @@ -269,7 +280,7 @@ export function ChangePasswordStep({ setOldPassword(''); setNewPassword(''); setNewPassConfirmed(''); - setAuthCode(''); + setOtpCode(''); } async function onSubmit( @@ -282,8 +293,10 @@ export function ChangePasswordStep({ await changePassword({ oldPassword, newPassword, - secondFactorToken: authCode, - webauthnResponse, + mfaResponse: { + totp_code: otpCode, + webauthn_response: webauthnResponse, + }, }); } @@ -328,14 +341,14 @@ export function ChangePasswordStep({ type="password" placeholder="Confirm Password" /> - {reauthMethod === 'otp' && ( + {reauthMethod === 'totp' && ( { const createStep = await waitFor(() => { return within(screen.getByTestId('create-step')); }); - await user.click(createStep.getByLabelText('Hardware Device')); + await user.click(createStep.getByLabelText('Security Key')); await user.click( createStep.getByRole('button', { name: 'Create an MFA method' }) ); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx index 847197e2c12c7..396c594e86f6e 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx @@ -268,7 +268,7 @@ function CreateMfaBox({ }) { // Be more specific about the WebAuthn device type (it's not a passkey). mfaRegisterOptions = mfaRegisterOptions.map((o: MfaOption) => - o.value === 'webauthn' ? { ...o, label: 'Hardware Device' } : o + o.value === 'webauthn' ? { ...o, label: 'Security Key' } : o ); return ( diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx index b7e703daef72c..c36e133e73c7b 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx @@ -46,9 +46,9 @@ export function ReauthenticateStep({ refCallback, stepIndex, flowLength, - reauthAttempt: attempt, mfaChallengeOptions, - clearReauthAttempt: clearAttempt, + reauthAttempt, + clearReauthAttempt, submitWithMfa, onClose, }: ReauthenticateStepProps) { @@ -68,7 +68,7 @@ export function ReauthenticateStep({ submitWithMfa(mfaOption, otpCode).then(next); }; - const errorMessage = getReauthenticationErrorMessage(attempt); + const errorMessage = getReauthenticationErrorMessage(reauthAttempt); return (
@@ -94,7 +94,7 @@ export function ReauthenticateStep({ mb={4} onChange={o => { setMfaOption(o as DeviceType); - clearAttempt(); + clearReauthAttempt(); }} /> {mfaOption === 'totp' && ( @@ -106,7 +106,7 @@ export function ReauthenticateStep({ value={otpCode} placeholder="123 456" onChange={onOtpCodeChanged} - readonly={attempt.status === 'processing'} + readonly={reauthAttempt.status === 'processing'} /> )} diff --git a/web/packages/teleport/src/Account/PasswordBox.tsx b/web/packages/teleport/src/Account/PasswordBox.tsx index f4ce622668517..bef6f8220745a 100644 --- a/web/packages/teleport/src/Account/PasswordBox.tsx +++ b/web/packages/teleport/src/Account/PasswordBox.tsx @@ -78,9 +78,10 @@ export function PasswordBox({ {dialogOpen && ( dev.usage === 'passwordless') + } onClose={() => setDialogOpen(false)} onSuccess={onSuccess} /> diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx index e3e784e691c44..bf107c81164ea 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx @@ -52,5 +52,6 @@ const props: State = { MFA_OPTION_SSO_DEFAULT, ], submitWithMfa: () => null, + submitWithPasswordless: () => null, onClose: () => null, }; diff --git a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts index 4d9e991277827..1e7d1337db3f2 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts +++ b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts @@ -102,6 +102,21 @@ export default function useReAuthenticate(props: ReauthProps): ReauthState { .catch(handleError); } + function submitWithPasswordless() { + setAttempt({ status: 'processing' }); + // Always get a new passwordless challenge, the challenge stored in state is for mfa + // and will also be overwritten in the backend by the passwordless challenge. + return auth + .getMfaChallenge({ + scope: props.challengeScope, + userVerificationRequirement: 'required', + }) + .then(chal => auth.getMfaChallengeResponse(chal, 'webauthn')) + .then(props.onMfaResponse) + .finally(clearMfaChallenge) + .catch(handleError); + } + function clearAttempt() { setAttempt({ status: '' }); } @@ -112,6 +127,7 @@ export default function useReAuthenticate(props: ReauthProps): ReauthState { getMfaChallenge, getMfaChallengeOptions, submitWithMfa, + submitWithPasswordless, }; } @@ -128,4 +144,5 @@ export type ReauthState = { getMfaChallenge: () => Promise; getMfaChallengeOptions: () => Promise; submitWithMfa: (mfaType?: DeviceType, totp_code?: string) => Promise; + submitWithPasswordless: () => Promise; }; diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index c26a5fe0641f4..a818eda3e1f5b 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -210,17 +210,12 @@ const auth = { }); }, - changePassword({ - oldPassword, - newPassword, - secondFactorToken, - webauthnResponse, - }: ChangePasswordReq) { + changePassword({ oldPassword, newPassword, mfaResponse }: ChangePasswordReq) { const data = { old_password: base64EncodeUnicode(oldPassword), new_password: base64EncodeUnicode(newPassword), - second_factor_token: secondFactorToken, - webauthnAssertionResponse: webauthnResponse, + second_factor_token: mfaResponse.totp_code, + webauthnAssertionResponse: mfaResponse.webauthn_response, }; return api.put(cfg.api.changeUserPasswordPath, data); diff --git a/web/packages/teleport/src/services/auth/types.ts b/web/packages/teleport/src/services/auth/types.ts index 7c74f666d9db1..57fb003ab7975 100644 --- a/web/packages/teleport/src/services/auth/types.ts +++ b/web/packages/teleport/src/services/auth/types.ts @@ -18,7 +18,7 @@ import { EventMeta } from 'teleport/services/userEvent'; -import { DeviceUsage, WebauthnAssertionResponse } from '../mfa'; +import { DeviceUsage, MfaChallengeResponse } from '../mfa'; import { IsMfaRequiredRequest, MfaChallengeScope } from './auth'; @@ -73,8 +73,7 @@ export type CreateAuthenticateChallengeRequest = { export type ChangePasswordReq = { oldPassword: string; newPassword: string; - secondFactorToken: string; - webauthnResponse?: WebauthnAssertionResponse; + mfaResponse?: MfaChallengeResponse; }; export type CreateNewHardwareDeviceRequest = { diff --git a/web/packages/teleport/src/services/mfa/mfaOptions.ts b/web/packages/teleport/src/services/mfa/mfaOptions.ts index 5f63b7ecdb12c..4137c3562d5f4 100644 --- a/web/packages/teleport/src/services/mfa/mfaOptions.ts +++ b/web/packages/teleport/src/services/mfa/mfaOptions.ts @@ -32,7 +32,7 @@ export function getMfaChallengeOptions(mfaChallenge: MfaAuthenticateChallenge) { } if (mfaChallenge?.ssoChallenge) { - mfaOptions.push(getSsoOption(mfaChallenge.ssoChallenge)); + mfaOptions.push(getSsoMfaOption(mfaChallenge.ssoChallenge)); } return mfaOptions; @@ -67,18 +67,18 @@ export const MFA_OPTION_TOTP: MfaOption = { label: 'Authenticator App', }; -// used in tests, returned by getSsoOptions(null). +// SSO MFA option used in tests. export const MFA_OPTION_SSO_DEFAULT: MfaOption = { value: 'sso', label: 'SSO', }; -const getSsoOption = (ssoChallenge: SSOChallenge): MfaOption => { +const getSsoMfaOption = (ssoChallenge: SSOChallenge): MfaOption => { return { value: 'sso', label: - ssoChallenge.device?.displayName || - ssoChallenge.device?.connectorId || + ssoChallenge?.device?.displayName || + ssoChallenge?.device?.connectorId || 'SSO', }; }; From f8789724d5d382b11aad27fc5ed640cc99f1c13f Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 9 Dec 2024 13:49:32 -0800 Subject: [PATCH 04/13] Fix mfa challenge option preference order; Fix reauthenticate component initial mfa option state. --- .../src/components/ReAuthenticate/ReAuthenticate.tsx | 2 +- .../teleport/src/services/mfa/mfaOptions.test.ts | 2 +- web/packages/teleport/src/services/mfa/mfaOptions.ts | 9 +++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx index 261f7efe9152f..ac293d4c7da7d 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx @@ -70,7 +70,7 @@ export function ReAuthenticate({ const [mfaOption, setMfaOption] = useState(); const [challengeOptions, getChallengeOptions] = useAsync(async () => { - const mfaOptions = getMfaChallengeOptions(); + const mfaOptions = await getMfaChallengeOptions(); setMfaOption(mfaOptions[0]); return mfaOptions; }); diff --git a/web/packages/teleport/src/services/mfa/mfaOptions.test.ts b/web/packages/teleport/src/services/mfa/mfaOptions.test.ts index 5c430a05be8a8..81eec4be87a80 100644 --- a/web/packages/teleport/src/services/mfa/mfaOptions.test.ts +++ b/web/packages/teleport/src/services/mfa/mfaOptions.test.ts @@ -94,7 +94,7 @@ describe('test retrieving mfa options from MFA Challenge', () => { webauthnPublicKey: {} as PublicKeyCredentialRequestOptions, ssoChallenge: Object.create(SSOChallenge), }, - expect: ['webauthn', 'totp', 'sso'], + expect: ['webauthn', 'sso', 'totp'], }, ]; diff --git a/web/packages/teleport/src/services/mfa/mfaOptions.ts b/web/packages/teleport/src/services/mfa/mfaOptions.ts index 4137c3562d5f4..96510d31e668f 100644 --- a/web/packages/teleport/src/services/mfa/mfaOptions.ts +++ b/web/packages/teleport/src/services/mfa/mfaOptions.ts @@ -20,6 +20,7 @@ import { Auth2faType } from 'shared/services'; import { DeviceType, MfaAuthenticateChallenge, SSOChallenge } from './types'; +// returns mfa challenge options in order of preferences: WebAuthn > SSO > TOTP. export function getMfaChallengeOptions(mfaChallenge: MfaAuthenticateChallenge) { const mfaOptions: MfaOption[] = []; @@ -27,14 +28,14 @@ export function getMfaChallengeOptions(mfaChallenge: MfaAuthenticateChallenge) { mfaOptions.push(MFA_OPTION_WEBAUTHN); } - if (mfaChallenge?.totpChallenge) { - mfaOptions.push(MFA_OPTION_TOTP); - } - if (mfaChallenge?.ssoChallenge) { mfaOptions.push(getSsoMfaOption(mfaChallenge.ssoChallenge)); } + if (mfaChallenge?.totpChallenge) { + mfaOptions.push(MFA_OPTION_TOTP); + } + return mfaOptions; } From 37d61491dbae5329de003c61d2279713d1f672c9 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 9 Dec 2024 13:57:39 -0800 Subject: [PATCH 05/13] Remove useReAuthenticate's onAuthenticated parameter. --- .../wizards/AddAuthDeviceWizard.tsx | 7 ++++-- .../wizards/DeleteAuthDeviceWizard.tsx | 6 ++++- .../ReAuthenticate/useReAuthenticate.ts | 25 ++----------------- 3 files changed, 12 insertions(+), 26 deletions(-) diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx index 396c594e86f6e..bd0291736e037 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx @@ -38,7 +38,7 @@ import { StepHeader } from 'design/StepSlider'; import { P } from 'design/Text/Text'; -import auth from 'teleport/services/auth/auth'; +import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; import useTeleport from 'teleport/useTeleport'; import { @@ -84,7 +84,10 @@ export function AddAuthDeviceWizard({ const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = useReAuthenticate({ - onAuthenticated: setPrivilegeToken, + challengeScope: MfaChallengeScope.MANAGE_DEVICES, + onMfaResponse: mfaResponse => { + auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken); + }, }); // Choose a new device type from the options available for the given 2fa type. diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx index b1bec7b139dad..df51e825c4051 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx @@ -42,6 +42,7 @@ import { ReauthenticateStep, ReauthenticateStepProps, } from './ReauthenticateStep'; +import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; interface DeleteAuthDeviceWizardProps { /** Device to be removed. */ @@ -60,7 +61,10 @@ export function DeleteAuthDeviceWizard({ const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = useReAuthenticate({ - onAuthenticated: setPrivilegeToken, + challengeScope: MfaChallengeScope.MANAGE_DEVICES, + onMfaResponse: mfaResponse => { + auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken); + }, }); const [challengeOptions, getChallengeOptions] = useAsync(async () => { diff --git a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts index 1e7d1337db3f2..7c4b11d132164 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts +++ b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts @@ -30,13 +30,6 @@ import { MfaOption, } from 'teleport/services/mfa'; -// useReAuthenticate will have different "submit" behaviors depending on: -// - If prop field `onMfaResponse` is defined, after a user submits, the -// function `onMfaResponse` is called with the user's MFA response. -// - If prop field `onAuthenticated` is defined, after a user submits, the -// user's MFA response are submitted with the request to get a privilege -// token, and after successfully obtaining the token, the function -// `onAuthenticated` will be called with this token. export default function useReAuthenticate(props: ReauthProps): ReauthState { // Note that attempt state "success" is not used or required. // After the user submits, the control is passed back @@ -62,18 +55,6 @@ export default function useReAuthenticate(props: ReauthProps): ReauthState { } }; - // TODO(Joerger): Replace onAuthenticated with onMfaResponse at call sites (/e). - if (props.onAuthenticated) { - // Creating privilege tokens always expects the MANAGE_DEVICES webauthn scope. - props.challengeScope = MfaChallengeScope.MANAGE_DEVICES; - props.onMfaResponse = mfaResponse => { - auth - .createPrivilegeToken(mfaResponse) - .then(props.onAuthenticated) - .catch(handleError); - }; - } - async function getMfaChallenge() { if (challenge) { return challenge; @@ -132,10 +113,8 @@ export default function useReAuthenticate(props: ReauthProps): ReauthState { } export type ReauthProps = { - challengeScope?: MfaChallengeScope; - onMfaResponse?(res: MfaChallengeResponse): void; - // TODO(Joerger): Remove in favor of onMfaResponse, make onMfaResponse required. - onAuthenticated?(privilegeTokenId: string): void; + challengeScope: MfaChallengeScope; + onMfaResponse(res: MfaChallengeResponse): void; }; export type ReauthState = { From 977f307353ad8ca4b2fa629edca729c57e0bb22a Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 9 Dec 2024 14:25:44 -0800 Subject: [PATCH 06/13] Fix lint. --- .../teleport/src/Account/Account.test.tsx | 5 +- .../ChangePasswordWizard.test.tsx | 25 ++-- .../ChangePasswordWizard.tsx | 3 +- .../Account/ManageDevices/useManageDevices.ts | 4 +- .../wizards/AddAuthDeviceWizard.story.tsx | 10 +- .../wizards/AddAuthDeviceWizard.test.tsx | 113 ++++++++---------- .../wizards/DeleteAuthDeviceWizard.story.tsx | 6 +- .../wizards/DeleteAuthDeviceWizard.test.tsx | 41 +++---- .../wizards/DeleteAuthDeviceWizard.tsx | 3 +- .../wizards/ReauthenticateStep.tsx | 2 +- .../ReAuthenticate/ReAuthenticate.story.tsx | 3 +- .../teleport/src/services/auth/auth.ts | 2 - 12 files changed, 94 insertions(+), 123 deletions(-) diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index 2ef6cdc931e25..af4340e48a90b 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -29,10 +29,7 @@ import cfg from 'teleport/config'; import { createTeleportContext } from 'teleport/mocks/contexts'; import { PasswordState } from 'teleport/services/user'; import auth from 'teleport/services/auth/auth'; -import MfaService, { - MfaDevice, - WebauthnAssertionResponse, -} from 'teleport/services/mfa'; +import MfaService, { MfaDevice } from 'teleport/services/mfa'; const defaultAuthType = cfg.auth.second_factor; const defaultPasswordless = cfg.auth.allowPasswordless; diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx index 6ff139610cc8d..eeeb4da32cf88 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx @@ -25,7 +25,6 @@ import { userEvent, UserEvent } from '@testing-library/user-event'; import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; import { - MFA_OPTION_SSO_DEFAULT, MFA_OPTION_TOTP, MFA_OPTION_WEBAUTHN, MfaChallengeResponse, @@ -90,15 +89,15 @@ describe('with passwordless reauthentication', () => { async function reauthenticate() { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); expect(auth.getMfaChallenge).toHaveBeenCalledWith({ scope: MfaChallengeScope.CHANGE_PASSWORD, }); - await user.click(reauthenticateStep.getByText('Passkey')); - await user.click(reauthenticateStep.getByText('Next')); + await user.click(screen.getByText('Passkey')); + await user.click(screen.getByText('Next')); expect(auth.getMfaChallenge).toHaveBeenCalledWith({ scope: MfaChallengeScope.CHANGE_PASSWORD, userVerificationRequirement: 'required', @@ -188,15 +187,15 @@ describe('with WebAuthn MFA reauthentication', () => { async function reauthenticate() { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); expect(auth.getMfaChallenge).toHaveBeenCalledWith({ scope: MfaChallengeScope.CHANGE_PASSWORD, }); - await user.click(reauthenticateStep.getByText('Security Key')); - await user.click(reauthenticateStep.getByText('Next')); + await user.click(screen.getByText('Security Key')); + await user.click(screen.getByText('Next')); expect(auth.getMfaChallengeResponse).toHaveBeenCalled(); } @@ -293,11 +292,11 @@ describe('with OTP MFA reauthentication', () => { async function reauthenticate() { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - await user.click(reauthenticateStep.getByText('Authenticator App')); - await user.click(reauthenticateStep.getByText('Next')); + await user.click(screen.getByText('Authenticator App')); + await user.click(screen.getByText('Next')); } it('changes password', async () => { diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx index d2eb2cc4b45dc..56fda8f9cd90c 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx @@ -35,6 +35,8 @@ import { useAsync } from 'shared/hooks/useAsync'; import Box from 'design/Box'; +import Indicator from 'design/Indicator'; + import { ChangePasswordReq } from 'teleport/services/auth'; import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; import { @@ -43,7 +45,6 @@ import { WebauthnAssertionResponse, } from 'teleport/services/mfa'; import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; -import Indicator from 'design/Indicator'; export interface ChangePasswordWizardProps { hasPasswordless: boolean; diff --git a/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts b/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts index 7c6ec1917673f..6d1776166ae93 100644 --- a/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts +++ b/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts @@ -19,11 +19,9 @@ import { useEffect, useState } from 'react'; import useAttempt from 'shared/hooks/useAttemptNext'; -import Ctx from 'teleport/teleportContext'; import cfg from 'teleport/config'; -import auth from 'teleport/services/auth'; import { DeviceUsage, MfaDevice } from 'teleport/services/mfa'; -import { MfaChallengeScope } from 'teleport/services/auth/auth'; +import Ctx from 'teleport/teleportContext'; export default function useManageDevices(ctx: Ctx) { const [devices, setDevices] = useState([]); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx index edd6e72596009..c1b1acd279a12 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx @@ -22,13 +22,14 @@ import Dialog from 'design/Dialog'; import { http, HttpResponse, delay } from 'msw'; +import { Attempt } from 'shared/hooks/useAttemptNext'; + import { createTeleportContext } from 'teleport/mocks/contexts'; import { ContextProvider } from 'teleport/index'; import cfg from 'teleport/config'; import { - DeviceType, DeviceUsage, MFA_OPTION_SSO_DEFAULT, MFA_OPTION_TOTP, @@ -41,7 +42,6 @@ import { SaveDeviceStep, } from './AddAuthDeviceWizard'; import { ReauthenticateStep } from './ReauthenticateStep'; -import { Attempt } from 'shared/hooks/useAttemptNext'; export default { title: 'teleport/Account/Manage Devices/Add Device Wizard', @@ -177,12 +177,12 @@ const stepProps: AddAuthDeviceWizardStepProps = { MFA_OPTION_TOTP, MFA_OPTION_SSO_DEFAULT, ], - submitWithMfa: async (mfaType?: DeviceType, otpCode?: string) => {}, + submitWithMfa: async () => {}, // Create props mfaRegisterOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP], - onDeviceCreated: (c: Credential) => {}, - onNewMfaDeviceTypeChange: (d: DeviceType) => {}, + onDeviceCreated: () => {}, + onNewMfaDeviceTypeChange: () => {}, // Save props credential: { id: 'cred-id', type: 'public-key' }, diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx index 0a200980cee4a..f54bd8ee22e90 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx @@ -19,7 +19,7 @@ import { render, screen } from 'design/utils/testing'; import React from 'react'; -import { waitFor, within } from '@testing-library/react'; +import { waitFor } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; import TeleportContext from 'teleport/teleportContext'; @@ -82,22 +82,18 @@ describe('flow without reauthentication', () => { ); - const createStep = await waitFor(() => { - return within(screen.getByTestId('create-step')); + await waitFor(() => { + expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); - await user.click( - createStep.getByRole('button', { name: 'Create a passkey' }) - ); + await user.click(screen.getByRole('button', { name: 'Create a passkey' })); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', deviceUsage: 'passwordless', }); - const saveStep = within(screen.getByTestId('save-step')); - await user.type(saveStep.getByLabelText('Passkey Nickname'), 'new-passkey'); - await user.click( - saveStep.getByRole('button', { name: 'Save the Passkey' }) - ); + expect(screen.getByTestId('save-step')).toBeInTheDocument(); + await user.type(screen.getByLabelText('Passkey Nickname'), 'new-passkey'); + await user.click(screen.getByRole('button', { name: 'Save the Passkey' })); expect(ctx.mfaService.saveNewWebAuthnDevice).toHaveBeenCalledWith({ credential: dummyCredential, addRequest: { @@ -112,22 +108,22 @@ describe('flow without reauthentication', () => { test('adds a WebAuthn MFA', async () => { render(); - const createStep = await waitFor(() => { - return within(screen.getByTestId('create-step')); + await waitFor(() => { + expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); - await user.click(createStep.getByLabelText('Security Key')); + await user.click(screen.getByLabelText('Security Key')); await user.click( - createStep.getByRole('button', { name: 'Create an MFA method' }) + screen.getByRole('button', { name: 'Create an MFA method' }) ); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', deviceUsage: 'mfa', }); - const saveStep = within(screen.getByTestId('save-step')); - await user.type(saveStep.getByLabelText('MFA Method Name'), 'new-mfa'); + expect(screen.getByTestId('save-step')).toBeInTheDocument(); + await user.type(screen.getByLabelText('MFA Method Name'), 'new-mfa'); await user.click( - saveStep.getByRole('button', { name: 'Save the MFA method' }) + screen.getByRole('button', { name: 'Save the MFA method' }) ); expect(ctx.mfaService.saveNewWebAuthnDevice).toHaveBeenCalledWith({ credential: dummyCredential, @@ -143,24 +139,24 @@ describe('flow without reauthentication', () => { test('adds an authenticator app', async () => { render(); - const createStep = await waitFor(() => { - return within(screen.getByTestId('create-step')); + await waitFor(() => { + expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); - await user.click(createStep.getByLabelText('Authenticator App')); - expect(createStep.getByRole('img')).toHaveAttribute( + await user.click(screen.getByLabelText('Authenticator App')); + expect(screen.getByRole('img')).toHaveAttribute( 'src', 'data:image/png;base64,dummy-qr-code' ); await user.click( - createStep.getByRole('button', { name: 'Create an MFA method' }) + screen.getByRole('button', { name: 'Create an MFA method' }) ); - const saveStep = within(screen.getByTestId('save-step')); - await user.type(saveStep.getByLabelText('MFA Method Name'), 'new-mfa'); - await user.type(saveStep.getByLabelText(/Authenticator Code/), '345678'); + expect(screen.getByTestId('save-step')).toBeInTheDocument(); + await user.type(screen.getByLabelText('MFA Method Name'), 'new-mfa'); + await user.type(screen.getByLabelText(/Authenticator Code/), '345678'); await user.click( - saveStep.getByRole('button', { name: 'Save the MFA method' }) + screen.getByRole('button', { name: 'Save the MFA method' }) ); expect(ctx.mfaService.addNewTotpDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', @@ -186,28 +182,24 @@ describe('flow with reauthentication', () => { test('adds a passkey with WebAuthn reauthentication', async () => { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - await user.click(reauthenticateStep.getByText('Verify my identity')); + await user.click(screen.getByText('Verify my identity')); - const createStep = await waitFor(() => { - return within(screen.getByTestId('create-step')); + await waitFor(() => { + expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); - await user.click( - createStep.getByRole('button', { name: 'Create a passkey' }) - ); + await user.click(screen.getByRole('button', { name: 'Create a passkey' })); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', deviceUsage: 'passwordless', }); - const saveStep = within(screen.getByTestId('save-step')); - await user.type(saveStep.getByLabelText('Passkey Nickname'), 'new-passkey'); - await user.click( - saveStep.getByRole('button', { name: 'Save the Passkey' }) - ); + expect(screen.getByTestId('save-step')).toBeInTheDocument(); + await user.type(screen.getByLabelText('Passkey Nickname'), 'new-passkey'); + await user.click(screen.getByRole('button', { name: 'Save the Passkey' })); expect(ctx.mfaService.saveNewWebAuthnDevice).toHaveBeenCalledWith({ credential: dummyCredential, addRequest: { @@ -222,33 +214,26 @@ describe('flow with reauthentication', () => { test('adds a passkey with OTP reauthentication', async () => { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - await user.click(reauthenticateStep.getByText('Authenticator App')); - await user.type( - reauthenticateStep.getByLabelText('Authenticator Code'), - '654987' - ); - await user.click(reauthenticateStep.getByText('Verify my identity')); + await user.click(screen.getByText('Authenticator App')); + await user.type(screen.getByLabelText('Authenticator Code'), '654987'); + await user.click(screen.getByText('Verify my identity')); - const createStep = await waitFor(() => { - return within(screen.getByTestId('create-step')); + await waitFor(() => { + expect(screen.getByTestId('create-step')).toBeInTheDocument(); }); - await user.click( - createStep.getByRole('button', { name: 'Create a passkey' }) - ); + await user.click(screen.getByRole('button', { name: 'Create a passkey' })); expect(auth.createNewWebAuthnDevice).toHaveBeenCalledWith({ tokenId: 'privilege-token', deviceUsage: 'passwordless', }); - const saveStep = within(screen.getByTestId('save-step')); - await user.type(saveStep.getByLabelText('Passkey Nickname'), 'new-passkey'); - await user.click( - saveStep.getByRole('button', { name: 'Save the Passkey' }) - ); + expect(screen.getByTestId('save-step')).toBeInTheDocument(); + await user.type(screen.getByLabelText('Passkey Nickname'), 'new-passkey'); + await user.click(screen.getByRole('button', { name: 'Save the Passkey' })); expect(ctx.mfaService.saveNewWebAuthnDevice).toHaveBeenCalledWith({ credential: dummyCredential, addRequest: { @@ -263,15 +248,11 @@ describe('flow with reauthentication', () => { test('shows reauthentication options', async () => { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - expect( - reauthenticateStep.queryByLabelText(/passkey or security key/i) - ).toBeVisible(); - expect( - reauthenticateStep.queryByLabelText(/authenticator app/i) - ).toBeVisible(); + expect(screen.queryByLabelText(/passkey or security key/i)).toBeVisible(); + expect(screen.queryByLabelText(/authenticator app/i)).toBeVisible(); }); }); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx index da707ef1445f6..3cbde51f69c79 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx @@ -20,11 +20,12 @@ import React from 'react'; import Dialog from 'design/Dialog'; +import { Attempt } from 'shared/hooks/useAttemptNext'; + import { createTeleportContext } from 'teleport/mocks/contexts'; import { ContextProvider } from 'teleport/index'; import { - DeviceType, MFA_OPTION_SSO_DEFAULT, MFA_OPTION_TOTP, MFA_OPTION_WEBAUTHN, @@ -36,7 +37,6 @@ import { DeleteDeviceStep, } from './DeleteAuthDeviceWizard'; import { ReauthenticateStep } from './ReauthenticateStep'; -import { Attempt } from 'shared/hooks/useAttemptNext'; export default { title: 'teleport/Account/Manage Devices/Delete Device Wizard', @@ -121,5 +121,5 @@ const stepProps: DeleteAuthDeviceWizardStepProps = { MFA_OPTION_TOTP, MFA_OPTION_SSO_DEFAULT, ], - submitWithMfa: async (mfaType?: DeviceType, otpCode?: string) => {}, + submitWithMfa: async () => {}, }; diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx index e51c5cf15df30..76bfd5d72e5d9 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.test.tsx @@ -19,7 +19,7 @@ import { render, screen } from 'design/utils/testing'; import React from 'react'; -import { waitFor, within } from '@testing-library/react'; +import { waitFor } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; import TeleportContext from 'teleport/teleportContext'; @@ -29,7 +29,7 @@ import auth from 'teleport/services/auth'; import { DeleteAuthDeviceWizardStepProps } from './DeleteAuthDeviceWizard'; -import { dummyPasskey, dummyHardwareDevice, deviceCases } from './deviceCases'; +import { dummyPasskey, dummyHardwareDevice } from './deviceCases'; import { DeleteAuthDeviceWizard } from '.'; @@ -73,14 +73,13 @@ function TestWizard(props: Partial) { test('deletes a device with WebAuthn reauthentication', async () => { render(); - let reauthenticateStep; await waitFor(() => { - reauthenticateStep = within(screen.getByTestId('reauthenticate-step')); + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - await user.click(reauthenticateStep.getByText('Verify my identity')); + await user.click(screen.getByText('Verify my identity')); - const deleteStep = within(screen.getByTestId('delete-step')); - await user.click(deleteStep.getByRole('button', { name: 'Delete' })); + expect(screen.getByTestId('delete-step')).toBeInTheDocument(); + await user.click(screen.getByRole('button', { name: 'Delete' })); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( 'privilege-token', @@ -92,19 +91,15 @@ test('deletes a device with WebAuthn reauthentication', async () => { test('deletes a device with OTP reauthentication', async () => { render(); - let reauthenticateStep; await waitFor(() => { - reauthenticateStep = within(screen.getByTestId('reauthenticate-step')); + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - await user.click(reauthenticateStep.getByText('Authenticator App')); - await user.type( - reauthenticateStep.getByLabelText('Authenticator Code'), - '654987' - ); - await user.click(reauthenticateStep.getByText('Verify my identity')); + await user.click(screen.getByText('Authenticator App')); + await user.type(screen.getByLabelText('Authenticator Code'), '654987'); + await user.click(screen.getByText('Verify my identity')); - const deleteStep = within(screen.getByTestId('delete-step')); - await user.click(deleteStep.getByRole('button', { name: 'Delete' })); + expect(screen.getByTestId('delete-step')).toBeInTheDocument(); + await user.click(screen.getByRole('button', { name: 'Delete' })); expect(ctx.mfaService.removeDevice).toHaveBeenCalledWith( 'privilege-token', @@ -130,13 +125,13 @@ test.each([ async ({ device, title, message }) => { render(); - const reauthenticateStep = await waitFor(() => { - return within(screen.getByTestId('reauthenticate-step')); + await waitFor(() => { + expect(screen.getByTestId('reauthenticate-step')).toBeInTheDocument(); }); - await user.click(reauthenticateStep.getByText('Verify my identity')); + await user.click(screen.getByText('Verify my identity')); - const deleteStep = within(screen.getByTestId('delete-step')); - expect(deleteStep.getByText(title)).toBeVisible(); - expect(deleteStep.getByText(message)).toBeVisible(); + expect(screen.getByTestId('delete-step')).toBeInTheDocument(); + expect(screen.getByText(title)).toBeVisible(); + expect(screen.getByText(message)).toBeVisible(); } ); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx index df51e825c4051..dcd6f4d910e15 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx @@ -38,11 +38,12 @@ import { MfaDevice } from 'teleport/services/mfa'; import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; +import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; + import { ReauthenticateStep, ReauthenticateStepProps, } from './ReauthenticateStep'; -import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; interface DeleteAuthDeviceWizardProps { /** Device to be removed. */ diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx index c36e133e73c7b..806d54c910919 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx @@ -20,7 +20,7 @@ import { OutlineDanger } from 'design/Alert/Alert'; import { ButtonPrimary, ButtonSecondary } from 'design/Button'; import Flex from 'design/Flex'; import { RadioGroup } from 'design/RadioGroup'; -import React, { useState, FormEvent, useEffect } from 'react'; +import React, { useState, FormEvent } from 'react'; import FieldInput from 'shared/components/FieldInput'; import Validation, { Validator } from 'shared/components/Validation'; import { requiredField } from 'shared/components/Validation/rules'; diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx index bf107c81164ea..a202663b5daa7 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx @@ -18,13 +18,14 @@ import React from 'react'; -import { ReAuthenticate, State } from './ReAuthenticate'; import { MFA_OPTION_SSO_DEFAULT, MFA_OPTION_TOTP, MFA_OPTION_WEBAUTHN, } from 'teleport/services/mfa'; +import { ReAuthenticate, State } from './ReAuthenticate'; + export default { title: 'Teleport/ReAuthenticate', }; diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index a818eda3e1f5b..c6c9ea277846d 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -16,8 +16,6 @@ * along with this program. If not, see . */ -import { Auth2faType } from 'shared/services'; - import api from 'teleport/services/api'; import cfg from 'teleport/config'; import { From 420b647a80e334f864121cefc70ede454d4cdb32 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 9 Dec 2024 17:22:51 -0800 Subject: [PATCH 07/13] Fix flaky test. --- web/packages/teleport/src/Account/Account.test.tsx | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index af4340e48a90b..bd7d565de022b 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -271,7 +271,9 @@ test('adding an MFA device', async () => { await renderComponent(ctx); await user.click(screen.getByRole('button', { name: 'Add MFA' })); - await user.click(screen.getByRole('button', { name: 'Verify my identity' })); + await waitFor(async () => { + user.click(screen.getByRole('button', { name: 'Verify my identity' })); + }); await user.click( screen.getByRole('button', { name: 'Create an MFA method' }) ); @@ -318,7 +320,9 @@ test('adding a passkey', async () => { await renderComponent(ctx); await user.click(screen.getByRole('button', { name: 'Add a Passkey' })); - await user.click(screen.getByRole('button', { name: 'Verify my identity' })); + await waitFor(async () => { + user.click(screen.getByRole('button', { name: 'Verify my identity' })); + }); await user.click(screen.getByRole('button', { name: 'Create a passkey' })); await user.type(screen.getByLabelText('Passkey Nickname'), 'new-passkey'); From 118253dfd40cfe503b7d1f6e3c08eba8a4af80c1 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 9 Dec 2024 19:05:03 -0800 Subject: [PATCH 08/13] Remove extra createPrivilegeToken call from the account page; Add new tokenless mfa endpoints to register/delete mfa devices; add TODOs to use new endpoints in v19+. --- lib/web/apiserver.go | 3 + lib/web/apiserver_test.go | 8 +- lib/web/mfa.go | 105 +++++++++++++++++- web/packages/teleport/src/Account/Account.tsx | 21 +--- .../Account/ManageDevices/useManageDevices.ts | 25 +---- .../wizards/AddAuthDeviceWizard.tsx | 24 ++-- .../wizards/DeleteAuthDeviceWizard.tsx | 5 + .../teleport/src/Account/PasswordBox.tsx | 7 +- .../teleport/src/services/auth/auth.ts | 2 +- 9 files changed, 135 insertions(+), 65 deletions(-) diff --git a/lib/web/apiserver.go b/lib/web/apiserver.go index 6c5f952290536..62ecb813a5b6e 100644 --- a/lib/web/apiserver.go +++ b/lib/web/apiserver.go @@ -917,7 +917,10 @@ func (h *Handler) bindDefaultEndpoints() { // MFA private endpoints. h.GET("/webapi/mfa/devices", h.WithAuth(h.getMFADevicesHandle)) + h.DELETE("/webapi/mfa/devices", h.WithAuth(h.deleteMFADeviceHandle)) h.POST("/webapi/mfa/authenticatechallenge", h.WithAuth(h.createAuthenticateChallengeHandle)) + h.POST("/webapi/mfa/registerchallenge", h.WithAuth(h.createRegisterChallengeHandle)) + h.POST("/webapi/mfa/devices", h.WithAuth(h.addMFADeviceHandle)) // DEPRECATED in favor of mfa/authenticatechallenge. // TODO(bl-nero): DELETE IN 17.0.0 diff --git a/lib/web/apiserver_test.go b/lib/web/apiserver_test.go index 543369ce4b64b..2b9c430a7190d 100644 --- a/lib/web/apiserver_test.go +++ b/lib/web/apiserver_test.go @@ -5250,24 +5250,24 @@ func TestCreateRegisterChallenge(t *testing.T) { tests := []struct { name string - req *createRegisterChallengeRequest + req *createRegisterChallengeWithTokenRequest assertChallenge func(t *testing.T, c *client.MFARegisterChallenge) }{ { name: "totp", - req: &createRegisterChallengeRequest{ + req: &createRegisterChallengeWithTokenRequest{ DeviceType: "totp", }, }, { name: "webauthn", - req: &createRegisterChallengeRequest{ + req: &createRegisterChallengeWithTokenRequest{ DeviceType: "webauthn", }, }, { name: "passwordless", - req: &createRegisterChallengeRequest{ + req: &createRegisterChallengeWithTokenRequest{ DeviceType: "webauthn", DeviceUsage: "passwordless", }, diff --git a/lib/web/mfa.go b/lib/web/mfa.go index 9116966a9ed40..2ab9bfa281636 100644 --- a/lib/web/mfa.go +++ b/lib/web/mfa.go @@ -75,6 +75,41 @@ func (h *Handler) deleteMFADeviceWithTokenHandle(w http.ResponseWriter, r *http. return OK(), nil } +type deleteMfaDeviceRequest struct { + // DeviceName is the name of the device to delete. + DeviceName string `json:"deviceName"` + // ExistingMFAResponse is an MFA challenge response from an existing device. + // Not required if the user has no existing devices. + ExistingMFAResponse *client.MFAChallengeResponse `json:"existingMfaResponse"` +} + +// deleteMFADeviceHandle deletes an mfa device for the user defined in the `token`, given as a query parameter. +func (h *Handler) deleteMFADeviceHandle(w http.ResponseWriter, r *http.Request, p httprouter.Params, c *SessionContext) (interface{}, error) { + var req deleteMfaDeviceRequest + if err := httplib.ReadJSON(r, &req); err != nil { + return nil, trace.Wrap(err) + } + + mfaResponse, err := req.ExistingMFAResponse.GetOptionalMFAResponseProtoReq() + if err != nil { + return nil, trace.Wrap(err) + } + + clt, err := c.GetClient() + if err != nil { + return nil, trace.Wrap(err) + } + + if err := clt.DeleteMFADeviceSync(r.Context(), &proto.DeleteMFADeviceSyncRequest{ + DeviceName: req.DeviceName, + ExistingMFAResponse: mfaResponse, + }); err != nil { + return nil, trace.Wrap(err) + } + + return OK(), nil +} + type addMFADeviceRequest struct { // PrivilegeTokenID is privilege token id. PrivilegeTokenID string `json:"tokenId"` @@ -203,7 +238,7 @@ func (h *Handler) createAuthenticateChallengeWithTokenHandle(w http.ResponseWrit return makeAuthenticateChallenge(chal), nil } -type createRegisterChallengeRequest struct { +type createRegisterChallengeWithTokenRequest struct { // DeviceType is the type of MFA device to get a register challenge for. DeviceType string `json:"deviceType"` // DeviceUsage is the intended usage of the device (MFA, Passwordless, etc). @@ -214,7 +249,7 @@ type createRegisterChallengeRequest struct { // createRegisterChallengeWithTokenHandle creates and returns MFA register challenges for a new device for the specified device type. func (h *Handler) createRegisterChallengeWithTokenHandle(w http.ResponseWriter, r *http.Request, p httprouter.Params) (interface{}, error) { - var req createRegisterChallengeRequest + var req createRegisterChallengeWithTokenRequest if err := httplib.ReadJSON(r, &req); err != nil { return nil, trace.Wrap(err) } @@ -256,6 +291,72 @@ func (h *Handler) createRegisterChallengeWithTokenHandle(w http.ResponseWriter, return resp, nil } +type createRegisterChallengeRequest struct { + // DeviceType is the type of MFA device to get a register challenge for. + DeviceType string `json:"deviceType"` + // DeviceUsage is the intended usage of the device (MFA, Passwordless, etc). + // It mimics the proto.DeviceUsage enum. + // Defaults to MFA. + DeviceUsage string `json:"deviceUsage"` + // ExistingMFAResponse is an MFA challenge response from an existing device. + // Not required if the user has no existing devices. + ExistingMFAResponse *client.MFAChallengeResponse `json:"existingMfaResponse"` +} + +// createRegisterChallengeHandle creates and returns MFA register challenges for a new device for the specified device type. +func (h *Handler) createRegisterChallengeHandle(w http.ResponseWriter, r *http.Request, p httprouter.Params, c *SessionContext) (interface{}, error) { + var req createRegisterChallengeRequest + if err := httplib.ReadJSON(r, &req); err != nil { + return nil, trace.Wrap(err) + } + + var deviceType proto.DeviceType + switch req.DeviceType { + case "totp": + deviceType = proto.DeviceType_DEVICE_TYPE_TOTP + case "webauthn": + deviceType = proto.DeviceType_DEVICE_TYPE_WEBAUTHN + default: + return nil, trace.BadParameter("MFA device type %q unsupported", req.DeviceType) + } + + deviceUsage, err := getDeviceUsage(req.DeviceUsage) + if err != nil { + return nil, trace.Wrap(err) + } + + mfaResponse, err := req.ExistingMFAResponse.GetOptionalMFAResponseProtoReq() + if err != nil { + return nil, trace.Wrap(err) + } + + clt, err := c.GetClient() + if err != nil { + return nil, trace.Wrap(err) + } + + chal, err := clt.CreateRegisterChallenge(r.Context(), &proto.CreateRegisterChallengeRequest{ + DeviceType: deviceType, + DeviceUsage: deviceUsage, + ExistingMFAResponse: mfaResponse, + }) + if err != nil { + return nil, trace.Wrap(err) + } + + resp := &client.MFARegisterChallenge{} + switch chal.GetRequest().(type) { + case *proto.MFARegisterChallenge_TOTP: + resp.TOTP = &client.TOTPRegisterChallenge{ + QRCode: chal.GetTOTP().GetQRCode(), + } + case *proto.MFARegisterChallenge_Webauthn: + resp.Webauthn = wantypes.CredentialCreationFromProto(chal.GetWebauthn()) + } + + return resp, nil +} + func getDeviceUsage(reqUsage string) (proto.DeviceUsage, error) { var deviceUsage proto.DeviceUsage switch strings.ToLower(reqUsage) { diff --git a/web/packages/teleport/src/Account/Account.tsx b/web/packages/teleport/src/Account/Account.tsx index c2109fb51dd9d..db0fbff3d46a6 100644 --- a/web/packages/teleport/src/Account/Account.tsx +++ b/web/packages/teleport/src/Account/Account.tsx @@ -105,14 +105,12 @@ export interface AccountProps extends ManageDevicesState, AccountPageProps { export function Account({ devices, - token, onAddDevice, onRemoveDevice, onDeviceAdded, onDeviceRemoved, deviceToRemove, fetchDevicesAttempt, - createRestrictedTokenAttempt, addDeviceWizardVisible, hideRemoveDevice, closeAddDeviceWizard, @@ -127,11 +125,8 @@ export function Account({ }: AccountProps) { const passkeys = devices.filter(d => d.usage === 'passwordless'); const mfaDevices = devices.filter(d => d.usage === 'mfa'); - const disableAddDevice = - createRestrictedTokenAttempt.status === 'processing' || - fetchDevicesAttempt.status !== 'success'; - const disableAddPasskey = disableAddDevice || !canAddPasskeys; - const disableAddMfa = disableAddDevice || !canAddMfa; + const disableAddPasskey = !canAddPasskeys; + const disableAddMfa = !canAddMfa; let mfaPillState = undefined; if (fetchDevicesAttempt.status !== 'processing') { @@ -140,7 +135,6 @@ export function Account({ const [notifications, setNotifications] = useState([]); const [prevFetchStatus, setPrevFetchStatus] = useState(''); - const [prevTokenStatus, setPrevTokenStatus] = useState(''); function addNotification(severity: NotificationSeverity, content: string) { setNotifications(n => [ @@ -165,13 +159,6 @@ export function Account({ } } - if (prevTokenStatus !== createRestrictedTokenAttempt.status) { - setPrevTokenStatus(createRestrictedTokenAttempt.status); - if (createRestrictedTokenAttempt.status === 'failed') { - addNotification('error', createRestrictedTokenAttempt.statusText); - } - } - function onPasswordChange() { addNotification('info', 'Your password has been changed.'); onPasswordChangeCb(); @@ -220,9 +207,6 @@ export function Account({ {!isSso && ( diff --git a/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts b/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts index 6d1776166ae93..1ff3f7ba594e6 100644 --- a/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts +++ b/web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts @@ -26,7 +26,6 @@ import Ctx from 'teleport/teleportContext'; export default function useManageDevices(ctx: Ctx) { const [devices, setDevices] = useState([]); const [deviceToRemove, setDeviceToRemove] = useState(); - const [token, setToken] = useState(''); const fetchDevicesAttempt = useAttempt(''); const [newDeviceUsage, setNewDeviceUsage] = useState('passwordless'); @@ -36,8 +35,6 @@ export default function useManageDevices(ctx: Ctx) { // the user has no devices yet and thus can't authenticate using the ReAuthenticate dialog const createRestrictedTokenAttempt = useAttempt(''); - const isReauthenticationRequired = !token; - function fetchDevices() { fetchDevicesAttempt.run(() => ctx.mfaService.fetchDevices().then(setDevices) @@ -46,30 +43,12 @@ export default function useManageDevices(ctx: Ctx) { async function onAddDevice(usage: DeviceUsage) { setNewDeviceUsage(usage); - const challenge = await auth.getMfaChallenge({ - scope: MfaChallengeScope.MANAGE_DEVICES, - }); - // If the user doesn't receieve any challenges from the backend, that means - // they have no valid devices to be challenged and should instead use a privilege token - // to add a new device. - // TODO (avatus): add SSO challenge here as well when we add SSO for MFA - // TODO(Joerger): privilege token is no longer required to add first device. - if (!challenge) { - createRestrictedTokenAttempt.run(() => - auth.createRestrictedPrivilegeToken().then(token => { - setToken(token); - setAddDeviceWizardVisible(true); - }) - ); - } else { - setAddDeviceWizardVisible(true); - } + setAddDeviceWizardVisible(true); } function onDeviceAdded() { fetchDevices(); setAddDeviceWizardVisible(false); - setToken(null); } function onRemoveDevice(device: MfaDevice) { @@ -93,7 +72,6 @@ export default function useManageDevices(ctx: Ctx) { return { devices, - token, onAddDevice, onRemoveDevice, onDeviceAdded, @@ -101,7 +79,6 @@ export default function useManageDevices(ctx: Ctx) { deviceToRemove, fetchDevicesAttempt: fetchDevicesAttempt.attempt, createRestrictedTokenAttempt: createRestrictedTokenAttempt.attempt, - isReauthenticationRequired, addDeviceWizardVisible, hideRemoveDevice, closeAddDeviceWizard, diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx index bd0291736e037..6a029fd78c7d5 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx @@ -62,24 +62,18 @@ interface AddAuthDeviceWizardProps { usage: DeviceUsage; /** MFA type setting, as configured in the cluster's configuration. */ auth2faType: Auth2faType; - /** - * A privilege token that may have been created previously; if present, the - * reauthentication step will be skipped. - */ - privilegeToken?: string; onClose(): void; onSuccess(): void; } /** A wizard for adding MFA and passkey devices. */ export function AddAuthDeviceWizard({ - privilegeToken: privilegeTokenProp = '', usage, auth2faType, onClose, onSuccess, }: AddAuthDeviceWizardProps) { - const [privilegeToken, setPrivilegeToken] = useState(privilegeTokenProp); + const [privilegeToken, setPrivilegeToken] = useState(); const [credential, setCredential] = useState(null); const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = @@ -101,7 +95,20 @@ export function AddAuthDeviceWizard({ // empty, the user has no existing device (e.g. SSO user) and can register their // first device without re-authentication. const [reauthMfaOptions, getMfaOptions] = useAsync(async () => { - return getMfaChallengeOptions(); + const reauthMfaOptions = await getMfaChallengeOptions(); + + // registering first device does not require reauth, just get a privilege token. + // + // TODO(Joerger): v19.0.0 + // Registering first device does not require a privilege token anymore, + // but the existing web register endpoint requires privilege token. + // We have a new endpoint "/v1/webapi/users/privilege" which does not + // require token, but can't be used until v19 for backwards compatibility. + if (reauthMfaOptions.length === 0) { + await auth.createPrivilegeToken().then(setPrivilegeToken); + } + + return reauthMfaOptions; }); useEffect(() => { @@ -113,7 +120,6 @@ export function AddAuthDeviceWizard({ case 'processing': return ( -
hi there
); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx index dcd6f4d910e15..0ee99393df36b 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx @@ -64,6 +64,11 @@ export function DeleteAuthDeviceWizard({ useReAuthenticate({ challengeScope: MfaChallengeScope.MANAGE_DEVICES, onMfaResponse: mfaResponse => { + // TODO(Joerger): v19.0.0 + // Devices can be deleted with an MFA response, so exchanging it for a + // privilege token adds an unnecessary API call. The device deletion + // endpoint requires a token, but the new endpoint "DELETE: /webapi/mfa/devices" + // can be used after v19 backwards compatibly. auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken); }, }); diff --git a/web/packages/teleport/src/Account/PasswordBox.tsx b/web/packages/teleport/src/Account/PasswordBox.tsx index bef6f8220745a..dfb0c7dae981e 100644 --- a/web/packages/teleport/src/Account/PasswordBox.tsx +++ b/web/packages/teleport/src/Account/PasswordBox.tsx @@ -33,14 +33,12 @@ import { ChangePasswordWizard } from './ChangePasswordWizard'; import { StatePill, AuthMethodState } from './StatePill'; export interface PasswordBoxProps { - changeDisabled: boolean; devices: MfaDevice[]; passwordState: PasswordState; onPasswordChange: () => void; } export function PasswordBox({ - changeDisabled, devices, passwordState, onPasswordChange, @@ -67,10 +65,7 @@ export function PasswordBox({ } icon={} actions={ - setDialogOpen(true)} - > + setDialogOpen(true)}> Change Password } diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index c6c9ea277846d..45cadd4a8fdf0 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -323,7 +323,7 @@ const auth = { ); }, - createPrivilegeToken(existingMfaResponse: MfaChallengeResponse) { + createPrivilegeToken(existingMfaResponse?: MfaChallengeResponse) { return api.post(cfg.api.createPrivilegeTokenPath, { existingMfaResponse, // TODO(Joerger): DELETE IN v19.0.0 From 6ffc55750c3d4b343319b2ecd53423034880c11e Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 10 Dec 2024 18:40:36 -0800 Subject: [PATCH 09/13] Fix tests. --- .../teleport/src/Account/Account.test.tsx | 20 ++++++++++++------- .../wizards/AddAuthDeviceWizard.test.tsx | 3 +++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index bd7d565de022b..4154fd98bfbee 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -240,8 +240,8 @@ test('loading state', async () => { expect( within(screen.getByTestId('mfa-list')).getByTestId('indicator-wrapper') ).toBeVisible(); - expect(screen.getByText(/add a passkey/i)).toBeDisabled(); - expect(screen.getByText(/add mfa/i)).toBeDisabled(); + expect(screen.getByText(/add a passkey/i)).toBeVisible(); + expect(screen.getByText(/add mfa/i)).toBeVisible(); expect( screen.queryByTestId('passwordless-state-pill') ).not.toBeInTheDocument(); @@ -272,11 +272,15 @@ test('adding an MFA device', async () => { await renderComponent(ctx); await user.click(screen.getByRole('button', { name: 'Add MFA' })); await waitFor(async () => { - user.click(screen.getByRole('button', { name: 'Verify my identity' })); + await user.click( + screen.getByRole('button', { name: 'Verify my identity' }) + ); + }); + await waitFor(async () => { + await user.click( + screen.getByRole('button', { name: 'Create an MFA method' }) + ); }); - await user.click( - screen.getByRole('button', { name: 'Create an MFA method' }) - ); await user.type(screen.getByLabelText('MFA Method Name'), 'new-mfa'); // The final assertion can be accidentally made irrelevant if the button name @@ -321,7 +325,9 @@ test('adding a passkey', async () => { await renderComponent(ctx); await user.click(screen.getByRole('button', { name: 'Add a Passkey' })); await waitFor(async () => { - user.click(screen.getByRole('button', { name: 'Verify my identity' })); + await user.click( + screen.getByRole('button', { name: 'Verify my identity' }) + ); }); await user.click(screen.getByRole('button', { name: 'Create a passkey' })); await user.type(screen.getByLabelText('Passkey Nickname'), 'new-passkey'); diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx index f54bd8ee22e90..25750a77ef4b1 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx @@ -75,6 +75,9 @@ function TestWizard(props: Partial = {}) { describe('flow without reauthentication', () => { beforeEach(() => { jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce({}); + jest + .spyOn(auth, 'createPrivilegeToken') + .mockResolvedValueOnce('privilege-token'); }); test('adds a passkey', async () => { From ba138ebec6577790f90ef66ea89674bebb7438fb Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 13 Dec 2024 16:58:23 -0800 Subject: [PATCH 10/13] Fix error handling in Web MFA flow. --- .../teleport/src/Account/Account.test.tsx | 4 +- .../ChangePasswordWizard.tsx | 159 ++++++++------- .../wizards/AddAuthDeviceWizard.tsx | 66 +++--- .../wizards/DeleteAuthDeviceWizard.tsx | 45 ++-- .../wizards/ReauthenticateStep.tsx | 48 ++--- .../TestConnection/TestConnection.tsx | 8 +- .../TestConnection/TestConnection.tsx | 4 +- .../Server/TestConnection/TestConnection.tsx | 2 +- .../ReAuthenticate/ReAuthenticate.tsx | 65 +++--- .../ReAuthenticate/useReAuthenticate.ts | 193 ++++++++++-------- .../teleport/src/services/auth/auth.ts | 4 +- 11 files changed, 303 insertions(+), 295 deletions(-) diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index 4154fd98bfbee..671750591a71b 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -206,7 +206,9 @@ test('password change', async () => { // Change the password await user.click(screen.getByRole('button', { name: 'Change Password' })); - await user.click(screen.getByRole('button', { name: 'Next' })); + await waitFor(async () => { + await user.click(screen.getByRole('button', { name: 'Next' })); + }); await user.type(screen.getByLabelText('Current Password'), 'old-password'); await user.type(screen.getByLabelText('New Password'), 'asdfasdfasdfasdf'); await user.type( diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx index 56fda8f9cd90c..a5eb1e8fc7204 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx @@ -16,14 +16,13 @@ * along with this program. If not, see . */ -import styled from 'styled-components'; import { Alert, OutlineDanger } from 'design/Alert/Alert'; import { ButtonPrimary, ButtonSecondary } from 'design/Button'; import Dialog from 'design/Dialog'; import Flex from 'design/Flex'; import { RadioGroup } from 'design/RadioGroup'; -import { StepComponentProps, StepSlider, StepHeader } from 'design/StepSlider'; -import React, { useEffect, useState } from 'react'; +import { StepComponentProps, StepHeader, StepSlider } from 'design/StepSlider'; +import React, { useCallback, useEffect, useState } from 'react'; import FieldInput from 'shared/components/FieldInput'; import Validation, { Validator } from 'shared/components/Validation'; import { @@ -32,11 +31,15 @@ import { requiredPassword, } from 'shared/components/Validation/rules'; import { useAsync } from 'shared/hooks/useAsync'; +import styled from 'styled-components'; import Box from 'design/Box'; import Indicator from 'design/Indicator'; +import useReAuthenticate, { + ReauthState, +} from 'teleport/components/ReAuthenticate/useReAuthenticate'; import { ChangePasswordReq } from 'teleport/services/auth'; import auth, { MfaChallengeScope } from 'teleport/services/auth/auth'; import { @@ -44,7 +47,6 @@ import { MfaOption, WebauthnAssertionResponse, } from 'teleport/services/mfa'; -import useReAuthenticate from 'teleport/components/ReAuthenticate/useReAuthenticate'; export interface ChangePasswordWizardProps { hasPasswordless: boolean; @@ -59,46 +61,15 @@ export function ChangePasswordWizard({ }: ChangePasswordWizardProps) { const [webauthnResponse, setWebauthnResponse] = useState(); - const { getMfaChallengeOptions, submitWithMfa, submitWithPasswordless } = - useReAuthenticate({ - challengeScope: MfaChallengeScope.CHANGE_PASSWORD, - onMfaResponse: mfaResponse => { - setWebauthnResponse(mfaResponse.webauthn_response); - }, - }); - // Attempt to get an MFA challenge for an existing device. If the challenge is - // empty, the user has no existing device (e.g. SSO user) and can register their - // first device without re-authentication. - const [reauthOptions, initReauthOptions] = useAsync(async () => { - let mfaOptions = await getMfaChallengeOptions(); - const reauthOptions = getReauthOptions(mfaOptions, hasPasswordless); - setReauthMethod(reauthOptions[0].value); - return reauthOptions; + const reauthState = useReAuthenticate({ + challengeScope: MfaChallengeScope.CHANGE_PASSWORD, + onMfaResponse: async mfaResponse => + setWebauthnResponse(mfaResponse.webauthn_response), }); - useEffect(() => { - initReauthOptions(); - }, []); - const [reauthMethod, setReauthMethod] = useState(); - // Handle potential error states first. - switch (reauthOptions.status) { - case 'processing': - return ( - - - - ); - case 'error': - return ; - case 'success': - break; - default: - return null; - } - return ( @@ -165,11 +135,10 @@ export type ChangePasswordWizardStepProps = StepComponentProps & ChangePasswordStepProps; interface ReauthenticateStepProps { - reauthOptions: ReauthenticationOption[]; + hasPasswordless: boolean; reauthMethod: ReauthenticationMethod; - onReauthMethodChange(method: ReauthenticationMethod): void; - submitWithPasswordless(): Promise; - submitWithMfa(mfaType?: DeviceType): Promise; + setReauthMethod(method: ReauthenticationMethod): void; + reauthState: ReauthState; onClose(): void; } @@ -178,28 +147,45 @@ export function ReauthenticateStep({ refCallback, stepIndex, flowLength, - reauthOptions, + hasPasswordless, reauthMethod, - onReauthMethodChange, - submitWithPasswordless, - submitWithMfa, + setReauthMethod, + reauthState: { + initAttempt, + mfaOptions, + submitWithMfa, + clearSubmitAttempt, + submitAttempt, + }, onClose, }: ChangePasswordWizardStepProps) { - const [reauthAttempt, reauthenticate] = useAsync( + const [reauthOptions, initReauthOptions] = useAsync( + useCallback(async () => { + const reauthOptions = getReauthOptions(mfaOptions, hasPasswordless); + setReauthMethod(reauthOptions[0].value); + return reauthOptions; + }, [hasPasswordless, mfaOptions, setReauthMethod]) + ); + + useEffect(() => { + initReauthOptions(); + }, [initReauthOptions]); + + const reauthenticate = useCallback( async (reauthMethod: ReauthenticationMethod) => { - switch (reauthMethod) { - case 'passwordless': - await submitWithPasswordless(); - break; - case 'totp': - // totp is handled in the ChangePasswordStep - break; - default: - await submitWithMfa(reauthMethod); - break; - } - next(); - } + // totp is handled in the ChangePasswordStep + if (reauthMethod === 'totp') return next(); + + const mfaType = + reauthMethod === 'passwordless' ? 'webauthn' : reauthMethod; + const deviceUsage = + reauthMethod === 'passwordless' ? 'passwordless' : 'mfa'; + + submitWithMfa(mfaType, deviceUsage).then(([, err]) => { + if (!err) next(); + }); + }, + [submitWithMfa, next] ); const onReauthenticate = (e: React.FormEvent) => { @@ -207,6 +193,38 @@ export function ReauthenticateStep({ reauthenticate(reauthMethod); }; + // Handle potential error states first. + switch (initAttempt.status) { + case 'processing': + return ( + + + + ); + case 'error': + return ; + case 'success': + break; + default: + return null; + } + + // Handle potential error states first. + switch (reauthOptions.status) { + case 'processing': + return ( + + + + ); + case 'error': + return ; + case 'success': + break; + default: + return null; + } + return ( @@ -216,20 +234,23 @@ export function ReauthenticateStep({ title="Verify Identity" /> - {reauthAttempt.status === 'error' && ( - {reauthAttempt.statusText} + {submitAttempt.status === 'error' && ( + {submitAttempt.statusText} )} Verification Method onReauthenticate(e)}> { + setReauthMethod(o as DeviceType); + clearSubmitAttempt(); + }} /> diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx index 6a029fd78c7d5..bbce32df63f98 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.tsx @@ -24,7 +24,7 @@ import Image from 'design/Image'; import Indicator from 'design/Indicator'; import { RadioGroup } from 'design/RadioGroup'; import { StepComponentProps, StepSlider } from 'design/StepSlider'; -import React, { useState, useEffect, FormEvent } from 'react'; +import React, { FormEvent, useEffect, useState } from 'react'; import FieldInput from 'shared/components/FieldInput'; import Validation, { Validator } from 'shared/components/Validation'; import { requiredField } from 'shared/components/Validation/rules'; @@ -76,13 +76,17 @@ export function AddAuthDeviceWizard({ const [privilegeToken, setPrivilegeToken] = useState(); const [credential, setCredential] = useState(null); - const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = - useReAuthenticate({ - challengeScope: MfaChallengeScope.MANAGE_DEVICES, - onMfaResponse: mfaResponse => { - auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken); - }, - }); + const reauthState = useReAuthenticate({ + challengeScope: MfaChallengeScope.MANAGE_DEVICES, + onMfaResponse: mfaResponse => + // TODO(Joerger): Instead of getting a privilege token, we should get + // // a register challenge with the mfa response directly. For good UX, this would + // // require some refactoring to the flow so the user can choose a device type before + // // completing an mfa check and getting an otp/webauthn register challenge, or + // // allowing the backend to return a flexible register challenge + // await auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken); + auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken), + }); // Choose a new device type from the options available for the given 2fa type. // irrelevant if usage === 'passkey'. @@ -91,32 +95,23 @@ export function AddAuthDeviceWizard({ registerMfaOptions[0].value ); - // Attempt to get an MFA challenge for an existing device. If the challenge is - // empty, the user has no existing device (e.g. SSO user) and can register their - // first device without re-authentication. - const [reauthMfaOptions, getMfaOptions] = useAsync(async () => { - const reauthMfaOptions = await getMfaChallengeOptions(); - - // registering first device does not require reauth, just get a privilege token. - // - // TODO(Joerger): v19.0.0 - // Registering first device does not require a privilege token anymore, - // but the existing web register endpoint requires privilege token. - // We have a new endpoint "/v1/webapi/users/privilege" which does not - // require token, but can't be used until v19 for backwards compatibility. - if (reauthMfaOptions.length === 0) { - await auth.createPrivilegeToken().then(setPrivilegeToken); - } - - return reauthMfaOptions; - }); - + // If the user has no mfa devices registered, they can create a privilege token + // without an mfa response. + // + // TODO(Joerger): v19.0.0 + // A user without devices can register their first device without a privilege token + // too, but the existing web register endpoint requires privilege token. + // We have a new endpoint "/v1/webapi/users/devices" which does not + // require token, but can't be used until v19 for backwards compatibility. + // Once in use, we can leave privilege token empty here. useEffect(() => { - getMfaOptions(); - }, []); + if (reauthState.mfaOptions?.length === 0) { + auth.createPrivilegeToken().then(setPrivilegeToken); + } + }, [reauthState.mfaOptions]); // Handle potential error states first. - switch (reauthMfaOptions.status) { + switch (reauthState.initAttempt.status) { case 'processing': return ( @@ -124,7 +119,7 @@ export function AddAuthDeviceWizard({ ); case 'error': - return ; + return ; case 'success': break; default: @@ -141,16 +136,13 @@ export function AddAuthDeviceWizard({ 0 + reauthState.mfaOptions.length > 0 ? 'withReauthentication' : 'withoutReauthentication' } // Step properties mfaRegisterOptions={registerMfaOptions} - mfaChallengeOptions={reauthMfaOptions.data} - reauthAttempt={attempt} - clearReauthAttempt={clearAttempt} - submitWithMfa={submitWithMfa} + reauthState={reauthState} usage={usage} privilegeToken={privilegeToken} credential={credential} diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx index 0ee99393df36b..13d6e8a67ae73 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.tsx @@ -20,16 +20,12 @@ import { Alert, OutlineDanger } from 'design/Alert/Alert'; import { ButtonSecondary, ButtonWarning } from 'design/Button'; import Dialog from 'design/Dialog'; import Flex from 'design/Flex'; -import { StepComponentProps, StepSlider } from 'design/StepSlider'; -import React, { useEffect, useState } from 'react'; +import { StepComponentProps, StepHeader, StepSlider } from 'design/StepSlider'; +import { useState } from 'react'; import useAttempt from 'shared/hooks/useAttemptNext'; import Box from 'design/Box'; -import { StepHeader } from 'design/StepSlider'; - -import { useAsync } from 'shared/hooks/useAsync'; - import Indicator from 'design/Indicator'; import useTeleport from 'teleport/useTeleport'; @@ -60,29 +56,19 @@ export function DeleteAuthDeviceWizard({ }: DeleteAuthDeviceWizardProps) { const [privilegeToken, setPrivilegeToken] = useState(''); - const { attempt, clearAttempt, getMfaChallengeOptions, submitWithMfa } = - useReAuthenticate({ - challengeScope: MfaChallengeScope.MANAGE_DEVICES, - onMfaResponse: mfaResponse => { - // TODO(Joerger): v19.0.0 - // Devices can be deleted with an MFA response, so exchanging it for a - // privilege token adds an unnecessary API call. The device deletion - // endpoint requires a token, but the new endpoint "DELETE: /webapi/mfa/devices" - // can be used after v19 backwards compatibly. - auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken); - }, - }); - - const [challengeOptions, getChallengeOptions] = useAsync(async () => { - return getMfaChallengeOptions(); + const reauthState = useReAuthenticate({ + challengeScope: MfaChallengeScope.MANAGE_DEVICES, + onMfaResponse: mfaResponse => + // TODO(Joerger): v19.0.0 + // Devices can be deleted with an MFA response, so exchanging it for a + // privilege token adds an unnecessary API call. The device deletion + // endpoint requires a token, but the new endpoint "DELETE: /webapi/mfa/devices" + // does not and can be used in v19 backwards compatibly. + auth.createPrivilegeToken(mfaResponse).then(setPrivilegeToken), }); - useEffect(() => { - getChallengeOptions(); - }, []); - // Handle potential error states first. - switch (challengeOptions.status) { + switch (reauthState.initAttempt.status) { case 'processing': return ( @@ -90,7 +76,7 @@ export function DeleteAuthDeviceWizard({ ); case 'error': - return ; + return ; case 'success': break; default: @@ -108,10 +94,7 @@ export function DeleteAuthDeviceWizard({ flows={wizardFlows} currFlow="default" // Step properties - reauthAttempt={attempt} - clearReauthAttempt={clearAttempt} - mfaChallengeOptions={challengeOptions.data} - submitWithMfa={submitWithMfa} + reauthState={reauthState} deviceToDelete={deviceToDelete} privilegeToken={privilegeToken} onClose={onClose} diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx index 806d54c910919..5dd0fd28eafb5 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/ReauthenticateStep.tsx @@ -20,24 +20,19 @@ import { OutlineDanger } from 'design/Alert/Alert'; import { ButtonPrimary, ButtonSecondary } from 'design/Button'; import Flex from 'design/Flex'; import { RadioGroup } from 'design/RadioGroup'; -import React, { useState, FormEvent } from 'react'; +import { StepComponentProps, StepHeader } from 'design/StepSlider'; +import React, { FormEvent, useState } from 'react'; import FieldInput from 'shared/components/FieldInput'; import Validation, { Validator } from 'shared/components/Validation'; import { requiredField } from 'shared/components/Validation/rules'; -import { StepComponentProps, StepHeader } from 'design/StepSlider'; import Box from 'design/Box'; -import { Attempt } from 'shared/hooks/useAttemptNext'; - +import { ReauthState } from 'teleport/components/ReAuthenticate/useReAuthenticate'; import { DeviceType } from 'teleport/services/mfa'; -import { MfaOption } from 'teleport/services/mfa'; export type ReauthenticateStepProps = StepComponentProps & { - reauthAttempt: Attempt; - clearReauthAttempt(): void; - mfaChallengeOptions: MfaOption[]; - submitWithMfa(mfaType?: DeviceType, otpCode?: string): Promise; + reauthState: ReauthState; onClose(): void; }; @@ -46,14 +41,11 @@ export function ReauthenticateStep({ refCallback, stepIndex, flowLength, - mfaChallengeOptions, - reauthAttempt, - clearReauthAttempt, - submitWithMfa, + reauthState: { mfaOptions, submitWithMfa, submitAttempt, clearSubmitAttempt }, onClose, }: ReauthenticateStepProps) { const [otpCode, setOtpCode] = useState(''); - const [mfaOption, setMfaOption] = useState(mfaChallengeOptions[0].value); + const [mfaOption, setMfaOption] = useState(mfaOptions[0].value); const onOtpCodeChanged = (e: React.ChangeEvent) => { setOtpCode(e.target.value); @@ -65,11 +57,11 @@ export function ReauthenticateStep({ ) => { e.preventDefault(); if (!validator.validate()) return; - submitWithMfa(mfaOption, otpCode).then(next); + submitWithMfa(mfaOption, 'mfa', otpCode).then(([, err]) => { + if (!err) next(); + }); }; - const errorMessage = getReauthenticationErrorMessage(reauthAttempt); - return (
@@ -79,14 +71,16 @@ export function ReauthenticateStep({ title="Verify Identity" /> - {errorMessage && {errorMessage}} + {submitAttempt.status === 'error' && ( + {submitAttempt.statusText} + )} {mfaOption && Multi-factor type} {({ validator }) => ( onReauthenticate(e, validator)}> { setMfaOption(o as DeviceType); - clearReauthAttempt(); + clearSubmitAttempt(); }} /> {mfaOption === 'totp' && ( @@ -106,7 +100,7 @@ export function ReauthenticateStep({ value={otpCode} placeholder="123 456" onChange={onOtpCodeChanged} - readonly={reauthAttempt.status === 'processing'} + readonly={submitAttempt.status === 'processing'} /> )} @@ -130,15 +124,3 @@ export function ReauthenticateStep({
); } - -function getReauthenticationErrorMessage(attempt: Attempt): string { - if (attempt.status === 'failed') { - // This message relies on the status message produced by the auth server in - // lib/auth/Server.checkOTP function. Please keep these in sync. - if (attempt.statusText === 'invalid totp token') { - return 'Invalid authenticator code'; - } else { - return attempt.statusText; - } - } -} diff --git a/web/packages/teleport/src/Discover/ConnectMyComputer/TestConnection/TestConnection.tsx b/web/packages/teleport/src/Discover/ConnectMyComputer/TestConnection/TestConnection.tsx index 1b5ae9e6a780a..eddce1b82a260 100644 --- a/web/packages/teleport/src/Discover/ConnectMyComputer/TestConnection/TestConnection.tsx +++ b/web/packages/teleport/src/Discover/ConnectMyComputer/TestConnection/TestConnection.tsx @@ -170,13 +170,13 @@ export function TestConnection(props: AgentStepProps) { {showMfaDialog && ( - testConnection({ + onMfaResponse={async res => { + await testConnection({ login: selectedLoginOpt.value, sshPrincipalSelectionMode, mfaResponse: res, - }) - } + }); + }} onClose={cancelMfaDialog} challengeScope={MfaChallengeScope.USER_SESSION} /> diff --git a/web/packages/teleport/src/Discover/Kubernetes/TestConnection/TestConnection.tsx b/web/packages/teleport/src/Discover/Kubernetes/TestConnection/TestConnection.tsx index 6933383168cd6..d2c3a0812f48e 100644 --- a/web/packages/teleport/src/Discover/Kubernetes/TestConnection/TestConnection.tsx +++ b/web/packages/teleport/src/Discover/Kubernetes/TestConnection/TestConnection.tsx @@ -101,7 +101,9 @@ export function TestConnection({ {showMfaDialog && ( testConnection(makeTestConnRequest(), res)} + onMfaResponse={async res => + testConnection(makeTestConnRequest(), res) + } onClose={cancelMfaDialog} challengeScope={MfaChallengeScope.USER_SESSION} /> diff --git a/web/packages/teleport/src/Discover/Server/TestConnection/TestConnection.tsx b/web/packages/teleport/src/Discover/Server/TestConnection/TestConnection.tsx index 54195cb3e6cc1..403b38feab3af 100644 --- a/web/packages/teleport/src/Discover/Server/TestConnection/TestConnection.tsx +++ b/web/packages/teleport/src/Discover/Server/TestConnection/TestConnection.tsx @@ -87,7 +87,7 @@ export function TestConnection(props: AgentStepProps) { {showMfaDialog && ( testConnection(selectedOpt.value, res)} + onMfaResponse={async res => testConnection(selectedOpt.value, res)} onClose={cancelMfaDialog} challengeScope={MfaChallengeScope.USER_SESSION} /> diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx index ac293d4c7da7d..7add4a659fb55 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx @@ -16,34 +16,32 @@ * along with this program. If not, see . */ -import React, { useEffect, useState } from 'react'; import { - Flex, Box, - Text, ButtonPrimary, ButtonSecondary, + Flex, Indicator, + Text, } from 'design'; +import { Alert, Danger } from 'design/Alert'; import Dialog, { - DialogHeader, - DialogTitle, DialogContent, DialogFooter, + DialogHeader, + DialogTitle, } from 'design/Dialog'; -import { Alert, Danger } from 'design/Alert'; -import Validation from 'shared/components/Validation'; -import { requiredToken } from 'shared/components/Validation/rules'; +import React, { useEffect, useState } from 'react'; import FieldInput from 'shared/components/FieldInput'; import FieldSelect from 'shared/components/FieldSelect'; - -import { useAsync } from 'shared/hooks/useAsync'; +import Validation, { Validator } from 'shared/components/Validation'; +import { requiredToken } from 'shared/components/Validation/rules'; import { MfaOption } from 'teleport/services/mfa'; import useReAuthenticate, { - ReauthState, ReauthProps, + ReauthState, } from './useReAuthenticate'; export type Props = ReauthProps & { @@ -61,26 +59,21 @@ export type State = ReauthState & { export function ReAuthenticate({ onClose, - attempt, - clearAttempt, - getMfaChallengeOptions, + initAttempt, + mfaOptions, submitWithMfa, + submitAttempt, + clearSubmitAttempt, }: State) { const [otpCode, setOtpToken] = useState(''); const [mfaOption, setMfaOption] = useState(); - const [challengeOptions, getChallengeOptions] = useAsync(async () => { - const mfaOptions = await getMfaChallengeOptions(); - setMfaOption(mfaOptions[0]); - return mfaOptions; - }); - useEffect(() => { - getChallengeOptions(); - }, []); + if (mfaOptions?.length) setMfaOption(mfaOptions[0]); + }, [mfaOptions]); // Handle potential error states first. - switch (challengeOptions.status) { + switch (initAttempt.status) { case 'processing': return ( @@ -88,16 +81,20 @@ export function ReAuthenticate({ ); case 'error': - return ; + return ; case 'success': break; default: return null; } - function onSubmit(e: React.MouseEvent) { + function onReauthenticate( + e: React.MouseEvent, + validator: Validator + ) { e.preventDefault(); - submitWithMfa(mfaOption.value, otpCode); + if (!validator.validate()) return; + submitWithMfa(mfaOption.value, 'mfa', otpCode); } return ( @@ -119,9 +116,9 @@ export function ReAuthenticate({ two-factor devices before performing this action. - {attempt.status === 'failed' && ( + {submitAttempt.status === 'error' && ( - {attempt.statusText} + {submitAttempt.statusText} )} @@ -130,15 +127,15 @@ export function ReAuthenticate({ width="60%" label="Two-factor Type" value={mfaOption} - options={challengeOptions.data} + options={mfaOptions} onChange={(o: MfaOption) => { setMfaOption(o); - clearAttempt(); + clearSubmitAttempt(); }} data-testid="mfa-select" mr={3} mb={0} - isDisabled={attempt.status === 'processing'} + isDisabled={submitAttempt.status === 'processing'} elevated={true} /> @@ -151,7 +148,7 @@ export function ReAuthenticate({ value={otpCode} onChange={e => setOtpToken(e.target.value)} placeholder="123 456" - readonly={attempt.status === 'processing'} + readonly={submitAttempt.status === 'processing'} mb={0} /> )} @@ -160,8 +157,8 @@ export function ReAuthenticate({ validator.validate() && onSubmit(e)} - disabled={attempt.status === 'processing'} + onClick={e => onReauthenticate(e, validator)} + disabled={submitAttempt.status === 'processing'} mr={3} mt={3} type="submit" diff --git a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts index 7c4b11d132164..ed8c73f3fe6da 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts +++ b/web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts @@ -16,112 +16,141 @@ * along with this program. If not, see . */ -import { useState } from 'react'; -import useAttempt, { Attempt } from 'shared/hooks/useAttemptNext'; +import { useCallback, useEffect, useState } from 'react'; +import { Attempt, makeEmptyAttempt, useAsync } from 'shared/hooks/useAsync'; import auth from 'teleport/services/auth'; import { MfaChallengeScope } from 'teleport/services/auth/auth'; import { - getMfaChallengeOptions as getChallengeOptions, DeviceType, + DeviceUsage, + getMfaChallengeOptions, MfaAuthenticateChallenge, MfaChallengeResponse, MfaOption, } from 'teleport/services/mfa'; -export default function useReAuthenticate(props: ReauthProps): ReauthState { - // Note that attempt state "success" is not used or required. - // After the user submits, the control is passed back - // to the caller who is responsible for rendering the `ReAuthenticate` - // component. - const { attempt, setAttempt } = useAttempt(''); - - const [challenge, setMfaChallenge] = useState(null); - - // Provide a custom error handler to catch a webauthn frontend error that occurs - // on Firefox and replace it with a more helpful error message. - const handleError = (err: Error) => { - if (err.message.includes('attempt was made to use an object that is not')) { - setAttempt({ - status: 'failed', - statusText: - 'The two-factor device you used is not registered on this account. You must verify using a device that has already been registered.', - }); - return; - } else { - setAttempt({ status: 'failed', statusText: err.message }); - return; - } - }; - - async function getMfaChallenge() { - if (challenge) { - return challenge; - } +export default function useReAuthenticate({ + challengeScope, + onMfaResponse, +}: ReauthProps): ReauthState { + const [mfaOptions, setMfaOptions] = useState(); + const [challengeState, setChallengeState] = useState(); - return auth.getMfaChallenge({ scope: props.challengeScope }).then(chal => { - setMfaChallenge(chal); - return chal; + const [initAttempt, init] = useAsync(async () => { + const challenge = await auth.getMfaChallenge({ + scope: challengeScope, }); - } - - function clearMfaChallenge() { - setMfaChallenge(null); - } - - function getMfaChallengeOptions() { - return getMfaChallenge().then(getChallengeOptions); - } - - function submitWithMfa(mfaType?: DeviceType, totp_code?: string) { - setAttempt({ status: 'processing' }); - return getMfaChallenge() - .then(chal => auth.getMfaChallengeResponse(chal, mfaType, totp_code)) - .then(props.onMfaResponse) - .finally(clearMfaChallenge) - .catch(handleError); - } - - function submitWithPasswordless() { - setAttempt({ status: 'processing' }); - // Always get a new passwordless challenge, the challenge stored in state is for mfa - // and will also be overwritten in the backend by the passwordless challenge. - return auth - .getMfaChallenge({ - scope: props.challengeScope, - userVerificationRequirement: 'required', - }) - .then(chal => auth.getMfaChallengeResponse(chal, 'webauthn')) - .then(props.onMfaResponse) - .finally(clearMfaChallenge) - .catch(handleError); - } - function clearAttempt() { - setAttempt({ status: '' }); + setChallengeState({ challenge, deviceUsage: 'mfa' }); + setMfaOptions(getMfaChallengeOptions(challenge)); + }); + + useEffect(() => { + init(); + }, []); + + const getChallenge = useCallback( + async (deviceUsage: DeviceUsage = 'mfa') => { + if (challengeState?.deviceUsage === deviceUsage) { + return challengeState.challenge; + } + + // If the challenge state is empty, used, or has different args, + // retrieve a new mfa challenge and set it in the state. + const challenge = await auth.getMfaChallenge({ + scope: challengeScope, + userVerificationRequirement: + deviceUsage === 'passwordless' ? 'required' : 'discouraged', + }); + setChallengeState({ + challenge, + deviceUsage, + }); + return challenge; + }, + [challengeState, challengeScope] + ); + + const [submitAttempt, submitWithMfa, setSubmitAttempt] = useAsync( + useCallback( + async ( + mfaType?: DeviceType, + deviceUsage?: DeviceUsage, + totpCode?: string + ) => { + const challenge = await getChallenge(deviceUsage); + + let response: MfaChallengeResponse; + try { + response = await auth.getMfaChallengeResponse( + challenge, + mfaType, + totpCode + ); + } catch (err) { + throw new Error(getReAuthenticationErrorMessage(err)); + } + + try { + await onMfaResponse(response); + } finally { + // once onMfaResponse is called, assume the challenge + // has been consumed and clear the state. + setChallengeState(null); + } + }, + [getChallenge, onMfaResponse] + ) + ); + + function clearSubmitAttempt() { + setSubmitAttempt(makeEmptyAttempt()); } return { - attempt, - clearAttempt, - getMfaChallenge, - getMfaChallengeOptions, + initAttempt, + mfaOptions, submitWithMfa, - submitWithPasswordless, + submitAttempt, + clearSubmitAttempt, }; } export type ReauthProps = { challengeScope: MfaChallengeScope; - onMfaResponse(res: MfaChallengeResponse): void; + onMfaResponse(res: MfaChallengeResponse): Promise; }; export type ReauthState = { - attempt: Attempt; - clearAttempt: () => void; - getMfaChallenge: () => Promise; - getMfaChallengeOptions: () => Promise; - submitWithMfa: (mfaType?: DeviceType, totp_code?: string) => Promise; - submitWithPasswordless: () => Promise; + initAttempt: Attempt; + mfaOptions: MfaOption[]; + submitWithMfa: ( + mfaType?: DeviceType, + deviceUsage?: DeviceUsage, + totpCode?: string + ) => Promise<[void, Error]>; + submitAttempt: Attempt; + clearSubmitAttempt: () => void; +}; + +type challengeState = { + challenge: MfaAuthenticateChallenge; + deviceUsage: DeviceUsage; }; + +function getReAuthenticationErrorMessage(err: Error): string { + if (err.message.includes('attempt was made to use an object that is not')) { + // Catch a webauthn frontend error that occurs on Firefox and replace it with a more helpful error message. + return 'The two-factor device you used is not registered on this account. You must verify using a device that has already been registered.'; + } + + if (err.message === 'invalid totp token') { + // This message relies on the status message produced by the auth server in + // lib/auth/Server.checkOTP function. Please keep these in sync. + return 'Invalid authenticator code'; + } + + return err.message; +} diff --git a/web/packages/teleport/src/services/auth/auth.ts b/web/packages/teleport/src/services/auth/auth.ts index 45cadd4a8fdf0..3724f1dc8b056 100644 --- a/web/packages/teleport/src/services/auth/auth.ts +++ b/web/packages/teleport/src/services/auth/auth.ts @@ -328,8 +328,8 @@ const auth = { existingMfaResponse, // TODO(Joerger): DELETE IN v19.0.0 // Also provide totp/webauthn response in backwards compatible format. - secondFactorToken: existingMfaResponse.totp_code, - webauthnAssertionResponse: existingMfaResponse.webauthn_response, + secondFactorToken: existingMfaResponse?.totp_code, + webauthnAssertionResponse: existingMfaResponse?.webauthn_response, }); }, From 4483300454211dd5e8e2ed5dce1ddb7cc83718f8 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 13 Dec 2024 16:59:10 -0800 Subject: [PATCH 11/13] Update e ref. --- e | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e b/e index a76c72e9f7418..cd63b234dd514 160000 --- a/e +++ b/e @@ -1 +1 @@ -Subproject commit a76c72e9f7418d1704b02361a04837c5202c4530 +Subproject commit cd63b234dd514a409c5a68ce7bb0609cc6ff4800 From 89e4639a9ed0d33795cd19706bcc023d1ed95c23 Mon Sep 17 00:00:00 2001 From: joerger Date: Fri, 13 Dec 2024 17:48:38 -0800 Subject: [PATCH 12/13] Fix stories. --- .../teleport/src/Account/Account.story.tsx | 4 +- .../teleport/src/Account/Account.test.tsx | 4 +- .../ChangePasswordWizard.story.tsx | 28 ++++++------- .../ChangePasswordWizard.test.tsx | 1 - .../wizards/AddAuthDeviceWizard.story.tsx | 31 +++++++------- .../wizards/AddAuthDeviceWizard.test.tsx | 5 +-- .../wizards/DeleteAuthDeviceWizard.story.tsx | 28 ++++++------- .../ReAuthenticate/ReAuthenticate.story.tsx | 41 ++++++++++++------- .../ReAuthenticate/ReAuthenticate.tsx | 17 ++++---- 9 files changed, 85 insertions(+), 74 deletions(-) diff --git a/web/packages/teleport/src/Account/Account.story.tsx b/web/packages/teleport/src/Account/Account.story.tsx index c1803ab8aeb6d..7cae89ccc7c35 100644 --- a/web/packages/teleport/src/Account/Account.story.tsx +++ b/web/packages/teleport/src/Account/Account.story.tsx @@ -87,7 +87,7 @@ export const LoadingDevicesFailed = () => ( ); export const RemoveDialog = () => ( - + ); export const RestrictedTokenCreateProcessing = () => ( @@ -110,7 +110,6 @@ export const RestrictedTokenCreateFailed = () => ( ); const props: AccountProps = { - token: '', onAddDevice: () => null, fetchDevicesAttempt: { status: 'success' }, createRestrictedTokenAttempt: { status: '' }, @@ -179,7 +178,6 @@ const props: AccountProps = { }, ], onDeviceAdded: () => {}, - isReauthenticationRequired: false, addDeviceWizardVisible: false, closeAddDeviceWizard: () => {}, passwordState: PasswordState.PASSWORD_STATE_SET, diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx index 671750591a71b..e8e3ae37b9ea0 100644 --- a/web/packages/teleport/src/Account/Account.test.tsx +++ b/web/packages/teleport/src/Account/Account.test.tsx @@ -16,8 +16,8 @@ * along with this program. If not, see . */ -import { render, screen, waitFor } from 'design/utils/testing'; import { within } from '@testing-library/react'; +import { render, screen, waitFor } from 'design/utils/testing'; import userEvent from '@testing-library/user-event'; @@ -27,9 +27,9 @@ import TeleportContext from 'teleport/teleportContext'; import { AccountPage as Account } from 'teleport/Account/Account'; import cfg from 'teleport/config'; import { createTeleportContext } from 'teleport/mocks/contexts'; -import { PasswordState } from 'teleport/services/user'; import auth from 'teleport/services/auth/auth'; import MfaService, { MfaDevice } from 'teleport/services/mfa'; +import { PasswordState } from 'teleport/services/user'; const defaultAuthType = cfg.auth.second_factor; const defaultPasswordless = cfg.auth.allowPasswordless; diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx index 3876fdd06b8fb..a280c0fe194e1 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx @@ -16,24 +16,24 @@ * along with this program. If not, see . */ -import React from 'react'; - import Dialog from 'design/Dialog'; -import { createTeleportContext } from 'teleport/mocks/contexts'; import { ContextProvider } from 'teleport'; +import { createTeleportContext } from 'teleport/mocks/contexts'; import { MFA_OPTION_SSO_DEFAULT, MFA_OPTION_TOTP, + MFA_OPTION_WEBAUTHN, WebauthnAssertionResponse, } from 'teleport/services/mfa'; +import { makeEmptyAttempt } from 'shared/hooks/useAsync'; +import { ReauthState } from 'teleport/components/ReAuthenticate/useReAuthenticate'; + import { ChangePasswordStep, ChangePasswordWizardStepProps, - REAUTH_OPTION_PASSKEY, - REAUTH_OPTION_WEBAUTHN, ReauthenticateStep, } from './ChangePasswordWizard'; @@ -91,15 +91,15 @@ const stepProps = { onSuccess() {}, // ReauthenticateStepProps - reauthOptions: [ - REAUTH_OPTION_PASSKEY, - REAUTH_OPTION_WEBAUTHN, - MFA_OPTION_TOTP, - MFA_OPTION_SSO_DEFAULT, - ], - onReauthMethodChange: () => {}, - submitWithPasswordless: async () => {}, - submitWithMfa: async () => {}, + hasPasswordless: true, + setReauthMethod: () => {}, + reauthState: { + initAttempt: { status: 'success' }, + mfaOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP, MFA_OPTION_SSO_DEFAULT], + submitWithMfa: async () => null, + submitAttempt: makeEmptyAttempt(), + clearSubmitAttempt: () => {}, + } as ReauthState, // ChangePasswordStepProps webauthnResponse: {} as WebauthnAssertionResponse, diff --git a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx index eeeb4da32cf88..e7212af322784 100644 --- a/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx +++ b/web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx @@ -17,7 +17,6 @@ */ import { render, screen } from 'design/utils/testing'; -import React from 'react'; import { waitFor, within } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx index c1b1acd279a12..21699064f423a 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.story.tsx @@ -16,16 +16,12 @@ * along with this program. If not, see . */ -import React from 'react'; - import Dialog from 'design/Dialog'; -import { http, HttpResponse, delay } from 'msw'; - -import { Attempt } from 'shared/hooks/useAttemptNext'; +import { delay, http, HttpResponse } from 'msw'; -import { createTeleportContext } from 'teleport/mocks/contexts'; import { ContextProvider } from 'teleport/index'; +import { createTeleportContext } from 'teleport/mocks/contexts'; import cfg from 'teleport/config'; @@ -36,6 +32,10 @@ import { MFA_OPTION_WEBAUTHN, } from 'teleport/services/mfa'; +import { ReauthState } from 'teleport/components/ReAuthenticate/useReAuthenticate'; + +import { makeEmptyAttempt } from 'shared/hooks/useAsync'; + import { AddAuthDeviceWizardStepProps, CreateDeviceStep, @@ -67,7 +67,10 @@ export function ReauthenticateLimitedOptions() { return ( ); } @@ -170,14 +173,12 @@ const stepProps: AddAuthDeviceWizardStepProps = { usage: 'passwordless' as DeviceUsage, // Reauth props - reauthAttempt: {} as Attempt, - clearReauthAttempt: () => {}, - mfaChallengeOptions: [ - MFA_OPTION_WEBAUTHN, - MFA_OPTION_TOTP, - MFA_OPTION_SSO_DEFAULT, - ], - submitWithMfa: async () => {}, + reauthState: { + mfaOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP, MFA_OPTION_SSO_DEFAULT], + submitWithMfa: async () => null, + submitAttempt: makeEmptyAttempt(), + clearSubmitAttempt: () => {}, + } as ReauthState, // Create props mfaRegisterOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP], diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx index 25750a77ef4b1..96a2fc05a404f 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/AddAuthDeviceWizard.test.tsx @@ -17,15 +17,14 @@ */ import { render, screen } from 'design/utils/testing'; -import React from 'react'; import { waitFor } from '@testing-library/react'; import { userEvent, UserEvent } from '@testing-library/user-event'; -import TeleportContext from 'teleport/teleportContext'; import { ContextProvider } from 'teleport'; -import MfaService from 'teleport/services/mfa'; import auth from 'teleport/services/auth'; +import MfaService from 'teleport/services/mfa'; +import TeleportContext from 'teleport/teleportContext'; import { AddAuthDeviceWizardStepProps } from './AddAuthDeviceWizard'; diff --git a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx index 3cbde51f69c79..aa27cbf76432b 100644 --- a/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx +++ b/web/packages/teleport/src/Account/ManageDevices/wizards/DeleteAuthDeviceWizard.story.tsx @@ -16,14 +16,10 @@ * along with this program. If not, see . */ -import React from 'react'; - import Dialog from 'design/Dialog'; -import { Attempt } from 'shared/hooks/useAttemptNext'; - -import { createTeleportContext } from 'teleport/mocks/contexts'; import { ContextProvider } from 'teleport/index'; +import { createTeleportContext } from 'teleport/mocks/contexts'; import { MFA_OPTION_SSO_DEFAULT, @@ -32,6 +28,10 @@ import { MfaDevice, } from 'teleport/services/mfa'; +import { ReauthState } from 'teleport/components/ReAuthenticate/useReAuthenticate'; + +import { makeEmptyAttempt } from 'shared/hooks/useAsync'; + import { DeleteAuthDeviceWizardStepProps, DeleteDeviceStep, @@ -107,19 +107,17 @@ const stepProps: DeleteAuthDeviceWizardStepProps = { flowLength: 2, refCallback: () => {}, + // Reauth props + reauthState: { + mfaOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP, MFA_OPTION_SSO_DEFAULT], + submitWithMfa: async () => null, + submitAttempt: makeEmptyAttempt(), + clearSubmitAttempt: () => {}, + } as ReauthState, + // Delete props deviceToDelete: dummyPasskey, privilegeToken: 'privilege-token', onClose: () => {}, onSuccess: () => {}, - - // Other props - reauthAttempt: {} as Attempt, - clearReauthAttempt: () => {}, - mfaChallengeOptions: [ - MFA_OPTION_WEBAUTHN, - MFA_OPTION_TOTP, - MFA_OPTION_SSO_DEFAULT, - ], - submitWithMfa: async () => {}, }; diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx index a202663b5daa7..e97dacd7c4b29 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.story.tsx @@ -16,15 +16,16 @@ * along with this program. If not, see . */ -import React from 'react'; - import { MFA_OPTION_SSO_DEFAULT, MFA_OPTION_TOTP, MFA_OPTION_WEBAUTHN, } from 'teleport/services/mfa'; +import { makeEmptyAttempt } from 'shared/hooks/useAsync'; + import { ReAuthenticate, State } from './ReAuthenticate'; +import { ReauthState } from './useReAuthenticate'; export default { title: 'Teleport/ReAuthenticate', @@ -33,26 +34,38 @@ export default { export const Loaded = () => ; export const Processing = () => ( - + ); export const Failed = () => ( ); const props: State = { - attempt: { status: '' }, - clearAttempt: () => null, - getMfaChallenge: () => null, - getMfaChallengeOptions: async () => [ - MFA_OPTION_WEBAUTHN, - MFA_OPTION_TOTP, - MFA_OPTION_SSO_DEFAULT, - ], - submitWithMfa: () => null, - submitWithPasswordless: () => null, + reauthState: { + initAttempt: { status: 'success' }, + mfaOptions: [MFA_OPTION_WEBAUTHN, MFA_OPTION_TOTP, MFA_OPTION_SSO_DEFAULT], + submitWithMfa: async () => null, + submitAttempt: makeEmptyAttempt(), + clearSubmitAttempt: () => {}, + } as ReauthState, + onClose: () => null, }; diff --git a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx index 7add4a659fb55..6dfc8a0f96f3c 100644 --- a/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx +++ b/web/packages/teleport/src/components/ReAuthenticate/ReAuthenticate.tsx @@ -50,20 +50,23 @@ export type Props = ReauthProps & { export default function Container(props: Props) { const state = useReAuthenticate(props); - return ; + return ; } -export type State = ReauthState & { +export type State = { + reauthState: ReauthState; onClose: () => void; }; export function ReAuthenticate({ onClose, - initAttempt, - mfaOptions, - submitWithMfa, - submitAttempt, - clearSubmitAttempt, + reauthState: { + initAttempt, + mfaOptions, + submitWithMfa, + submitAttempt, + clearSubmitAttempt, + }, }: State) { const [otpCode, setOtpToken] = useState(''); const [mfaOption, setMfaOption] = useState(); From 1348d70ceff6f9476e6eefc555e4bdf3ec5cef28 Mon Sep 17 00:00:00 2001 From: joerger Date: Mon, 16 Dec 2024 17:17:30 -0800 Subject: [PATCH 13/13] Fix lint. --- .../src/Discover/Database/TestConnection/TestConnection.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/packages/teleport/src/Discover/Database/TestConnection/TestConnection.tsx b/web/packages/teleport/src/Discover/Database/TestConnection/TestConnection.tsx index 5faaf84b6080a..928f3c783fdc6 100644 --- a/web/packages/teleport/src/Discover/Database/TestConnection/TestConnection.tsx +++ b/web/packages/teleport/src/Discover/Database/TestConnection/TestConnection.tsx @@ -117,7 +117,7 @@ export function TestConnection() { {showMfaDialog && ( testConnection(validator, res)} + onMfaResponse={async res => testConnection(validator, res)} onClose={cancelMfaDialog} challengeScope={MfaChallengeScope.USER_SESSION} />