Skip to content

Commit

Permalink
WebUI MFA methods refactor (#49679)
Browse files Browse the repository at this point in the history 
* Replace fetchWebAuthnChallenge with getChallenge.

* Replace getWebauthnResponse with getMfaChallengeResponse.

* Update getMfaChallengeResponse to take DeviceType and otp code.

* lint fix.

* Fix tests.

* Fix lint.
  • Loading branch information
@Joerger
Joerger authored Dec 10, 2024
1 parent 986bad7 commit 4677873
Show file tree
Hide file tree
Showing 17 changed files with 263 additions and 172 deletions.
4 changes: 2 additions & 2 deletions web/packages/teleport/src/Account/Account.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ test('adding an MFA device', async () => {
const user = userEvent.setup();
const ctx = createTeleportContext();
jest.spyOn(ctx.mfaService, 'fetchDevices').mockResolvedValue([testPasskey]);
jest.spyOn(auth, 'getChallenge').mockResolvedValue({
jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({
webauthnPublicKey: null,
totpChallenge: true,
ssoChallenge: null,
Expand Down Expand Up @@ -327,7 +327,7 @@ test('removing an MFA method', async () => {
const user = userEvent.setup();
const ctx = createTeleportContext();
jest.spyOn(ctx.mfaService, 'fetchDevices').mockResolvedValue([testMfaMethod]);
jest.spyOn(auth, 'getChallenge').mockResolvedValue({
jest.spyOn(auth, 'getMfaChallenge').mockResolvedValue({
webauthnPublicKey: null,
totpChallenge: false,
ssoChallenge: null,
Expand Down
21 changes: 13 additions & 8 deletions web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.story.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,11 @@ import Dialog from 'design/Dialog';
import { createTeleportContext } from 'teleport/mocks/contexts';
import { ContextProvider } from 'teleport';

import { MfaDevice } from 'teleport/services/mfa';
import { MfaDevice, WebauthnAssertionResponse } from 'teleport/services/mfa';

import {
ChangePasswordStep,
ChangePasswordWizardStepProps,
ReauthenticateStep,
createReauthOptions,
} from './ChangePasswordWizard';
Expand Down Expand Up @@ -107,12 +108,16 @@ const stepProps = {
flowLength: 2,
refCallback: () => {},

// Other props
reauthOptions: defaultReauthOptions,
reauthMethod: defaultReauthOptions[0].value,
credential: { id: '', type: '' },
onReauthMethodChange() {},
onAuthenticated() {},
// Shared props
reauthMethod: 'mfaDevice',
onClose() {},
onSuccess() {},
};

// ReauthenticateStepProps
reauthOptions: defaultReauthOptions,
onReauthMethodChange() {},
onWebauthnResponse() {},

// ChangePasswordStepProps
webauthnResponse: {} as WebauthnAssertionResponse,
} satisfies ChangePasswordWizardStepProps;
40 changes: 28 additions & 12 deletions web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,30 @@ import { userEvent, UserEvent } from '@testing-library/user-event';

import auth, { MfaChallengeScope } from 'teleport/services/auth/auth';

import { MfaChallengeResponse } from 'teleport/services/mfa';

import {
ChangePasswordWizardProps,
createReauthOptions,
} from './ChangePasswordWizard';

import { ChangePasswordWizard } from '.';

const dummyCredential: Credential = {
id: 'cred-id',
type: 'public-key',
const dummyChallengeResponse: MfaChallengeResponse = {
webauthn_response: {
id: 'cred-id',
type: 'public-key',
extensions: {
appid: true,
},
rawId: 'rawId',
response: {
authenticatorData: 'authenticatorData',
clientDataJSON: 'clientDataJSON',
signature: 'signature',
userHandle: 'userHandle',
},
},
};
let user: UserEvent;
let onSuccess: jest.Mock;
Expand Down Expand Up @@ -72,9 +86,10 @@ beforeEach(() => {
user = userEvent.setup();
onSuccess = jest.fn();

jest.spyOn(auth, 'getMfaChallenge').mockResolvedValueOnce(undefined);
jest
.spyOn(auth, 'fetchWebAuthnChallenge')
.mockResolvedValueOnce(dummyCredential);
.spyOn(auth, 'getMfaChallengeResponse')
.mockResolvedValueOnce(dummyChallengeResponse);
jest.spyOn(auth, 'changePassword').mockResolvedValueOnce(undefined);
});

Expand All @@ -89,10 +104,11 @@ describe('with passwordless reauthentication', () => {
);
await user.click(reauthenticateStep.getByText('Passkey'));
await user.click(reauthenticateStep.getByText('Next'));
expect(auth.fetchWebAuthnChallenge).toHaveBeenCalledWith({
expect(auth.getMfaChallenge).toHaveBeenCalledWith({
scope: MfaChallengeScope.CHANGE_PASSWORD,
userVerificationRequirement: 'required',
});
expect(auth.getMfaChallengeResponse).toHaveBeenCalled();
}

it('changes password', async () => {
Expand All @@ -113,7 +129,7 @@ describe('with passwordless reauthentication', () => {
oldPassword: '',
newPassword: 'new-pass1234',
secondFactorToken: '',
credential: dummyCredential,
webauthnResponse: dummyChallengeResponse.webauthn_response,
});
expect(onSuccess).toHaveBeenCalled();
});
Expand Down Expand Up @@ -180,7 +196,7 @@ describe('with WebAuthn MFA reauthentication', () => {
);
await user.click(reauthenticateStep.getByText('MFA Device'));
await user.click(reauthenticateStep.getByText('Next'));
expect(auth.fetchWebAuthnChallenge).toHaveBeenCalledWith({
expect(auth.getMfaChallenge).toHaveBeenCalledWith({
scope: MfaChallengeScope.CHANGE_PASSWORD,
userVerificationRequirement: 'discouraged',
});
Expand Down Expand Up @@ -208,7 +224,7 @@ describe('with WebAuthn MFA reauthentication', () => {
oldPassword: 'current-pass',
newPassword: 'new-pass1234',
secondFactorToken: '',
credential: dummyCredential,
webauthnResponse: dummyChallengeResponse.webauthn_response,
});
expect(onSuccess).toHaveBeenCalled();
});
Expand Down Expand Up @@ -282,7 +298,7 @@ describe('with OTP MFA reauthentication', () => {
);
await user.click(reauthenticateStep.getByText('Authenticator App'));
await user.click(reauthenticateStep.getByText('Next'));
expect(auth.fetchWebAuthnChallenge).not.toHaveBeenCalled();
expect(auth.getMfaChallenge).not.toHaveBeenCalled();
}

it('changes password', async () => {
Expand Down Expand Up @@ -406,11 +422,11 @@ describe('without reauthentication', () => {
'new-pass1234'
);
await user.click(changePasswordStep.getByText('Save Changes'));
expect(auth.fetchWebAuthnChallenge).not.toHaveBeenCalled();
expect(auth.getMfaChallenge).not.toHaveBeenCalled();
expect(auth.changePassword).toHaveBeenCalledWith({
oldPassword: 'current-pass',
newPassword: 'new-pass1234',
credential: undefined,
webauthnResponse: undefined,
secondFactorToken: '',
});
expect(onSuccess).toHaveBeenCalled();
Expand Down
32 changes: 20 additions & 12 deletions web/packages/teleport/src/Account/ChangePasswordWizard/ChangePasswordWizard.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import Box from 'design/Box';

import { ChangePasswordReq } from 'teleport/services/auth';
import auth, { MfaChallengeScope } from 'teleport/services/auth/auth';
import { MfaDevice } from 'teleport/services/mfa';
import { MfaDevice, WebauthnAssertionResponse } from 'teleport/services/mfa';

export interface ChangePasswordWizardProps {
/** MFA type setting, as configured in the cluster's configuration. */
Expand Down Expand Up @@ -66,7 +66,8 @@ export function ChangePasswordWizard({
const [reauthMethod, setReauthMethod] = useState<ReauthenticationMethod>(
reauthOptions[0]?.value
);
const [credential, setCredential] = useState<Credential | undefined>();
const [webauthnResponse, setWebauthnResponse] =
useState<WebauthnAssertionResponse>();
const reauthRequired = reauthOptions.length > 0;

return (
Expand All @@ -84,9 +85,9 @@ export function ChangePasswordWizard({
// Step properties
reauthOptions={reauthOptions}
reauthMethod={reauthMethod}
credential={credential}
onReauthMethodChange={setReauthMethod}
onAuthenticated={setCredential}
webauthnResponse={webauthnResponse}
onWebauthnResponse={setWebauthnResponse}
onClose={onClose}
onSuccess={onSuccess}
/>
Expand Down Expand Up @@ -146,15 +147,15 @@ const wizardFlows = {
withoutReauthentication: [ChangePasswordStep],
};

type ChangePasswordWizardStepProps = StepComponentProps &
export type ChangePasswordWizardStepProps = StepComponentProps &
ReauthenticateStepProps &
ChangePasswordStepProps;

interface ReauthenticateStepProps {
reauthOptions: ReauthenticationOption[];
reauthMethod: ReauthenticationMethod;
onReauthMethodChange(method: ReauthenticationMethod): void;
onAuthenticated(res: Credential): void;
onWebauthnResponse(res: WebauthnAssertionResponse): void;
onClose(): void;
}

Expand All @@ -166,18 +167,25 @@ export function ReauthenticateStep({
reauthOptions,
reauthMethod,
onReauthMethodChange,
onAuthenticated,
onWebauthnResponse,
onClose,
}: ChangePasswordWizardStepProps) {
const [reauthenticateAttempt, reauthenticate] = useAsync(
async (m: ReauthenticationMethod) => {
if (m === 'passwordless' || m === 'mfaDevice') {
const res = await auth.fetchWebAuthnChallenge({
const challenge = await auth.getMfaChallenge({
scope: MfaChallengeScope.CHANGE_PASSWORD,
userVerificationRequirement:
m === 'passwordless' ? 'required' : 'discouraged',
});
onAuthenticated(res);

const response = await auth.getMfaChallengeResponse(
challenge,
'webauthn'
);

// TODO(Joerger): handle non-webauthn response.
onWebauthnResponse(response.webauthn_response);
}
next();
}
Expand Down Expand Up @@ -225,7 +233,7 @@ export function ReauthenticateStep({
}

interface ChangePasswordStepProps {
credential: Credential;
webauthnResponse: WebauthnAssertionResponse;
reauthMethod: ReauthenticationMethod;
onClose(): void;
onSuccess(): void;
Expand All @@ -236,7 +244,7 @@ export function ChangePasswordStep({
prev,
stepIndex,
flowLength,
credential,
webauthnResponse,
reauthMethod,
onClose,
onSuccess,
Expand Down Expand Up @@ -275,7 +283,7 @@ export function ChangePasswordStep({
oldPassword,
newPassword,
secondFactorToken: authCode,
credential,
webauthnResponse,
});
}

Expand Down
5 changes: 3 additions & 2 deletions web/packages/teleport/src/Account/ManageDevices/useManageDevices.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,15 @@ export default function useManageDevices(ctx: Ctx) {

async function onAddDevice(usage: DeviceUsage) {
setNewDeviceUsage(usage);
const response = await auth.getChallenge({
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
if (!response.webauthnPublicKey?.challenge && !response.totpChallenge) {
// TODO(Joerger): privilege token is no longer required to add first device.
if (!challenge) {
createRestrictedTokenAttempt.run(() =>
auth.createRestrictedPrivilegeToken().then(token => {
setToken(token);
Expand Down
12 changes: 9 additions & 3 deletions web/packages/teleport/src/Console/DocumentSsh/useGetScpUrl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,21 @@ export default function useGetScpUrl(addMfaToScpUrls: boolean) {
return cfg.getScpUrl(params);
}
try {
let webauthn = await auth.getWebauthnResponse(
MfaChallengeScope.USER_SESSION
const challenge = await auth.getMfaChallenge({
scope: MfaChallengeScope.USER_SESSION,
});

const response = await auth.getMfaChallengeResponse(
challenge,
'webauthn'
);

setAttempt({
status: 'success',
statusText: '',
});
return cfg.getScpUrl({
webauthn,
webauthn: response.webauthn_response,
...params,
});
} catch (error) {
Expand Down
10 changes: 3 additions & 7 deletions web/packages/teleport/src/Users/useUsers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,16 +87,12 @@ export default function useUsers({
}

async function onCreate(u: User) {
const webauthnResponse = await auth.getWebauthnResponseForAdminAction(true);
const mfaResponse = await auth.getMfaChallengeResponseForAdminAction(true);
return ctx.userService
.createUser(u, ExcludeUserField.Traits, webauthnResponse)
.createUser(u, ExcludeUserField.Traits, mfaResponse)
.then(result => setUsers([result, ...users]))
.then(() =>
ctx.userService.createResetPasswordToken(
u.name,
'invite',
webauthnResponse
)
ctx.userService.createResetPasswordToken(u.name, 'invite', mfaResponse)
);
}

Expand Down
7 changes: 3 additions & 4 deletions web/packages/teleport/src/components/ReAuthenticate/useReAuthenticate.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,10 @@ export default function useReAuthenticate(props: Props) {

if ('onMfaResponse' in props) {
auth
.getWebauthnResponse(props.challengeScope)
.then(webauthnResponse =>
props.onMfaResponse({ webauthn_response: webauthnResponse })
)
.getMfaChallenge({ scope: props.challengeScope })
.then(challenge => auth.getMfaChallengeResponse(challenge, 'webauthn'))
.catch(handleError);

return;
}

Expand Down
15 changes: 7 additions & 8 deletions web/packages/teleport/src/lib/useMfa.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,12 @@ import { useState, useEffect, useCallback } from 'react';

import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender';
import { TermEvent } from 'teleport/lib/term/enums';
import {
parseMfaChallengeJson as parseMfaChallenge,
makeWebauthnAssertionResponse,
} from 'teleport/services/mfa/makeMfa';
import { parseMfaChallengeJson as parseMfaChallenge } from 'teleport/services/mfa/makeMfa';
import {
MfaAuthenticateChallengeJson,
SSOChallenge,
} from 'teleport/services/mfa';
import auth from 'teleport/services/auth/auth';

export function useMfa(emitterSender: EventEmitterMfaSender): MfaState {
const [state, setState] = useState<{
Expand Down Expand Up @@ -86,16 +84,17 @@ export function useMfa(emitterSender: EventEmitterMfaSender): MfaState {
return;
}

navigator.credentials
.get({ publicKey: state.webauthnPublicKey })
auth
.getMfaChallengeResponse({
webauthnPublicKey: state.webauthnPublicKey,
})
.then(res => {
setState(prevState => ({
...prevState,
errorText: '',
webauthnPublicKey: null,
}));
const credential = makeWebauthnAssertionResponse(res);
emitterSender.sendWebAuthn(credential);
emitterSender.sendWebAuthn(res.webauthn_response);
})
.catch((err: Error) => {
setErrorText(err.message);
Expand Down
Loading

0 comments on commit 4677873

Please sign in to comment.