Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions web/packages/teleport/src/Account/Account.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const LoadedPasskeysOff = () => (
<Account {...props} canAddPasskeys={false} />
);

export const LoadedMfaOff = () => <Account {...props} canAddMFA={false} />;
export const LoadedMfaOff = () => <Account {...props} canAddMfa={false} />;

export const LoadingDevices = () => (
<Account
Expand Down Expand Up @@ -94,8 +94,6 @@ const props: AccountProps = {
token: '',
setToken: () => null,
onAddDevice: () => null,
hideAddDevice: () => null,
fetchDevices: () => null,
fetchDevicesAttempt: { status: 'success' },
createRestrictedTokenAttempt: { status: '' },
deviceToRemove: null,
Expand All @@ -105,12 +103,11 @@ const props: AccountProps = {
hideReAuthenticate: () => null,
hideRemoveDevice: () => null,
isReAuthenticateVisible: false,
isAddDeviceVisible: false,
isRemoveDeviceVisible: false,
isSso: false,
restrictNewDeviceUsage: null,
newDeviceUsage: null,
canAddPasskeys: true,
canAddMFA: true,
canAddMfa: true,
devices: [
{
id: '1',
Expand Down Expand Up @@ -161,8 +158,8 @@ const props: AccountProps = {
residentKey: false,
},
],
onPasskeyAdded: () => {},
onDeviceAdded: () => {},
isReauthenticationRequired: false,
passkeyWizardVisible: false,
closePasskeyWizard: () => {},
addDeviceWizardVisible: false,
closeAddDeviceWizard: () => {},
};
6 changes: 5 additions & 1 deletion web/packages/teleport/src/Account/Account.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,17 +41,21 @@ describe('passkey + mfa button state', () => {
cfg.auth.allowPasswordless = defaultPasswordless;
});

// Note: the "off" and "otp" cases don't make sense with passwordless turned
// on (the auth server wouldn't start in this configuration), but we're still
// testing them for completeness.
test.each`
mfa | pwdless | pkEnabled | mfaEnabled
${'on'} | ${true} | ${true} | ${true}
${'on'} | ${false} | ${false} | ${true}
${'optional'} | ${true} | ${true} | ${true}
${'optional'} | ${false} | ${false} | ${true}
${'otp'} | ${true} | ${false} | ${true}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this test case removed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zmb3 It got broken when I removed a redundant check, but it's also impossible in real life: the OTP option implies passwordless being disabled. The auth server refuses to start if these options are in conflict. Similarly, we don't check what happens if 2fa is set to "off" and passwordless is set to true.

If you still think it's good to check these seemingly impossible conditions, I can bring back this case (and extend the "off" case) to verify that we always enable passwordless when the backend tells us so, regardless of the MFA setting itself.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(On the second thought, I just went on and included these anyway.)

${'otp'} | ${false} | ${false} | ${true}
${'otp'} | ${true} | ${true} | ${true}
${'webauthn'} | ${true} | ${true} | ${true}
${'webauthn'} | ${false} | ${false} | ${true}
${'off'} | ${false} | ${false} | ${false}
${'off'} | ${true} | ${true} | ${false}
`(
'2fa($mfa) with pwdless($pwdless) = passkey($pkEnabled) mfa($mfaEnabled)',
async ({ mfa, pwdless, pkEnabled, mfaEnabled }) => {
Expand Down
74 changes: 23 additions & 51 deletions web/packages/teleport/src/Account/Account.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import { Attempt } from 'shared/hooks/useAttemptNext';
import * as Icon from 'design/Icon';
import { Notification, NotificationItem } from 'shared/components/Notification';

import createMfaOptions from 'shared/utils/createMfaOptions';

import useTeleport from 'teleport/useTeleport';
import { FeatureBox } from 'teleport/components/Layout';
import ReAuthenticate from 'teleport/components/ReAuthenticate';
Expand All @@ -38,7 +36,6 @@ import { AuthDeviceList } from './ManageDevices/AuthDeviceList/AuthDeviceList';
import useManageDevices, {
State as ManageDevicesState,
} from './ManageDevices/useManageDevices';
import AddDevice from './ManageDevices/AddDevice';
import { ActionButton, Header } from './Header';
import { PasswordBox } from './PasswordBox';
import { AddAuthDeviceWizard } from './ManageDevices/AddAuthDeviceWizard';
Expand All @@ -61,32 +58,14 @@ export default function AccountPage({ enterpriseComponent }: AccountPageProps) {
const isSso = ctx.storeUser.isSso();
const manageDevicesState = useManageDevices(ctx);

// Note: we are using the same logic here as the `AddDevice` component uses to
// determine whether to show various options. This creates a duplication of
// logic, but this is a quick bug fix to make sure that we don't show a dialog
// that normally would require an OTP token, but is shown in a passwordless
// context and thus can't progress.
// TODO(bl-nero): When implementing a new device enrollment dialog, refactor
// this so that the options used by both components have the same source of
// truth.
const mfaOptions = createMfaOptions({
auth2faType: cfg.getAuth2faType(),
required: true,
});

const canAddPasskeys =
cfg.isPasswordlessEnabled() &&
mfaOptions.some(option => option.value === 'webauthn');

const canAddMFA = mfaOptions.some(
option => option.value === 'otp' || option.value === 'webauthn'
);
const canAddPasskeys = cfg.isPasswordlessEnabled();
const canAddMfa = cfg.isMfaEnabled();

return (
<Account
isSso={isSso}
canAddPasskeys={canAddPasskeys}
canAddMFA={canAddMFA}
canAddMfa={canAddMfa}
{...manageDevicesState}
enterpriseComponent={enterpriseComponent}
/>
Expand All @@ -96,7 +75,7 @@ export default function AccountPage({ enterpriseComponent }: AccountPageProps) {
export interface AccountProps extends ManageDevicesState, AccountPageProps {
isSso: boolean;
canAddPasskeys: boolean;
canAddMFA: boolean;
canAddMfa: boolean;
}

export function Account({
Expand All @@ -105,33 +84,30 @@ export function Account({
setToken,
onAddDevice,
onRemoveDevice,
onPasskeyAdded,
onDeviceAdded,
deviceToRemove,
fetchDevices,
removeDevice,
fetchDevicesAttempt,
createRestrictedTokenAttempt,
isReAuthenticateVisible,
isAddDeviceVisible,
isRemoveDeviceVisible,
passkeyWizardVisible,
addDeviceWizardVisible,
hideReAuthenticate,
hideAddDevice,
hideRemoveDevice,
closePasskeyWizard,
closeAddDeviceWizard,
isSso,
canAddMFA,
canAddMfa,
canAddPasskeys,
enterpriseComponent: EnterpriseComponent,
restrictNewDeviceUsage,
newDeviceUsage,
}: AccountProps) {
const passkeys = devices.filter(d => d.residentKey);
const mfaDevices = devices.filter(d => !d.residentKey);
const disableAddDevice =
createRestrictedTokenAttempt.status === 'processing' ||
fetchDevicesAttempt.status !== 'success';
const disableAddPasskey = disableAddDevice || !canAddPasskeys;
const disableAddMFA = disableAddDevice || !canAddMFA;
const disableAddMfa = disableAddDevice || !canAddMfa;

const [notifications, setNotifications] = useState<NotificationItem[]>([]);
const [prevFetchStatus, setPrevFetchStatus] = useState<Attempt['status']>('');
Expand Down Expand Up @@ -174,9 +150,13 @@ export function Account({
addNotification('info', 'Your password has been changed.');
}

function onAddPasskeySuccess() {
addNotification('info', 'Passkey successfully saved.');
onPasskeyAdded();
function onAddDeviceSuccess() {
const message =
newDeviceUsage === 'passwordless'
? 'Passkey successfully saved.'
: 'MFA device successfully saved.';
addNotification('info', message);
onDeviceAdded();
}

return (
Expand Down Expand Up @@ -233,9 +213,9 @@ export function Account({
showIndicator={fetchDevicesAttempt.status === 'processing'}
actions={
<ActionButton
disabled={disableAddMFA}
disabled={disableAddMfa}
title={
disableAddMFA
disableAddMfa
? 'Multi-factor authentication is disabled'
: ''
}
Expand All @@ -260,14 +240,6 @@ export function Account({
challengeScope={MfaChallengeScope.MANAGE_DEVICES}
/>
)}
{isAddDeviceVisible && (
<AddDevice
fetchDevices={fetchDevices}
token={token}
onClose={hideAddDevice}
restrictDeviceUsage={restrictNewDeviceUsage}
/>
)}
{EnterpriseComponent && (
<EnterpriseComponent addNotification={addNotification} />
)}
Expand All @@ -281,13 +253,13 @@ export function Account({
/>
)}

{passkeyWizardVisible && (
{addDeviceWizardVisible && (
<AddAuthDeviceWizard
usage={restrictNewDeviceUsage}
usage={newDeviceUsage}
auth2faType={cfg.getAuth2faType()}
privilegeToken={token}
onClose={closePasskeyWizard}
onSuccess={onAddPasskeySuccess}
onClose={closeAddDeviceWizard}
onSuccess={onAddDeviceSuccess}
/>
)}

Expand Down

This file was deleted.

This file was deleted.

Loading