Remove the old AddDevice dialog and substitute it with the new wizard (…

…#38393) * Remove the old AddDevice dialog Also cleans up the accompanying local storage flag. * Rename `canAddMFA` Co-authored-by: Zac Bergquist <[email protected]> * Update a success message Co-authored-by: Zac Bergquist <[email protected]> * review * Review --------- Co-authored-by: Zac Bergquist <[email protected]>
gravitational · Feb 22, 2024 · 9171ba2 · 9171ba2
1 parent 1d4225f
commit 9171ba2
Show file tree

Hide file tree

Showing 13 changed files with 53 additions and 2,718 deletions.
diff --git a/web/packages/teleport/src/Account/Account.story.tsx b/web/packages/teleport/src/Account/Account.story.tsx
@@ -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
@@ -94,8 +94,6 @@ const props: AccountProps = {
   token: '',
   setToken: () => null,
   onAddDevice: () => null,
-  hideAddDevice: () => null,
-  fetchDevices: () => null,
   fetchDevicesAttempt: { status: 'success' },
   createRestrictedTokenAttempt: { status: '' },
   deviceToRemove: null,
@@ -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',
@@ -161,8 +158,8 @@ const props: AccountProps = {
       residentKey: false,
     },
   ],
-  onPasskeyAdded: () => {},
+  onDeviceAdded: () => {},
   isReauthenticationRequired: false,
-  passkeyWizardVisible: false,
-  closePasskeyWizard: () => {},
+  addDeviceWizardVisible: false,
+  closeAddDeviceWizard: () => {},
 };
diff --git a/web/packages/teleport/src/Account/Account.test.tsx b/web/packages/teleport/src/Account/Account.test.tsx
@@ -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}
     ${'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 }) => {

diff --git a/web/packages/teleport/src/Account/Account.tsx b/web/packages/teleport/src/Account/Account.tsx
@@ -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';
@@ -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';
@@ -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}
     />
@@ -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({
@@ -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']>('');
@@ -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 (
@@ -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'
                         : ''
                     }
@@ -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} />
         )}
@@ -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}
         />
       )}
 

diff --git a/web/packages/teleport/src/Account/ManageDevices/AddDevice/AddDevice.story.test.tsx b/web/packages/teleport/src/Account/ManageDevices/AddDevice/AddDevice.story.test.tsx
diff --git a/web/packages/teleport/src/Account/ManageDevices/AddDevice/AddDevice.story.tsx b/web/packages/teleport/src/Account/ManageDevices/AddDevice/AddDevice.story.tsx