From 1235245e6ea771fc83ab98714efef8fd603a9d01 Mon Sep 17 00:00:00 2001 From: Gildas Garcia <1122076+djhi@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:28:50 +0200 Subject: [PATCH] Recovery codes: delete recovery codes when deleting the last MFA (#50731) ## Problem The API prevents users from deleting their last MFA when they also have recovery codes. However the UI doesn't and they may see an error instead of being guided. ## Solution Delete the recovery codes first. image ## Review instructions Provide a clear numbered procedure that the PR reviewer can walk through. 1. With an account that have only one MFA and recovery codes generated 2. Delete the MFA => You should see the dialog as in above screenshot. Check the presence of _Your recovery codes will be deleted too_ After deletion, you shouldn't see the Recovery codes section anymore. ## Summary by CodeRabbit - **Bug Fixes** - Improved multi-factor authentication management when recovery codes are available. - Users are warned that recovery codes will be deleted before removing their last authentication factor. - Removing the final authentication factor handles recovery-code deletion first. - Cancelling deletion leaves the factor and recovery codes unchanged. - Recovery-code handling applies only when enabled and relevant to last-factor removal. - Recovery-code management is available in all environments. - Delete actions are disabled while recovery-code status is loading, and an error message appears if recovery codes fail to load. --- .../TOTPFactors/DeleteFactorModal.test.tsx | 136 ++++++++++++++++++ .../Account/TOTPFactors/DeleteFactorModal.tsx | 32 ++++- .../interfaces/Account/TOTPFactors/index.tsx | 66 +++++---- 3 files changed, 204 insertions(+), 30 deletions(-) create mode 100644 apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.test.tsx diff --git a/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.test.tsx b/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.test.tsx new file mode 100644 index 00000000000..6d375ba3223 --- /dev/null +++ b/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.test.tsx @@ -0,0 +1,136 @@ +import { fireEvent, screen, waitFor } from '@testing-library/react' +import { describe, expect, test, vi } from 'vitest' + +import { DeleteFactorModal } from './DeleteFactorModal' +import { auth } from '@/lib/gotrue' +import { customRender } from '@/tests/lib/custom-render' + +describe('DeleteFactorModal', () => { + test("Requests users confirmation before deleting an MFA when it's not the last", async () => { + const unenroll = vi.spyOn(auth.mfa, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const unenrollRecoveryCodes = vi.spyOn(auth.mfa.recoveryCodes, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const onClose = vi.fn() + + customRender( + + ) + await screen.findByText('Confirm to delete factor') + expect(screen.queryByText('Multi-factor authentication will be disabled')).toBeNull() + expect(screen.queryByText('Your recovery codes will be deleted too')).toBeNull() + fireEvent.click(await screen.findByRole('button', { name: 'Delete' })) + await waitFor(() => expect(unenroll).toHaveBeenCalled()) + await waitFor(() => expect(onClose).toHaveBeenCalled()) + expect(unenrollRecoveryCodes).not.toHaveBeenCalled() + }) + test("Requests users confirmation before deleting an MFA when it's the last one but no recovery codes are available", async () => { + const unenroll = vi.spyOn(auth.mfa, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const unenrollRecoveryCodes = vi.spyOn(auth.mfa.recoveryCodes, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const onClose = vi.fn() + + customRender( + + ) + await screen.findByText('Confirm to delete factor') + await screen.findByText('Multi-factor authentication will be disabled') + expect(screen.queryByText('Your recovery codes will be deleted too')).toBeNull() + fireEvent.click(await screen.findByRole('button', { name: 'Delete' })) + await waitFor(() => expect(unenroll).toHaveBeenCalled()) + await waitFor(() => expect(onClose).toHaveBeenCalled()) + expect(unenrollRecoveryCodes).not.toHaveBeenCalled() + }) + test('Deletes the recovery codes when available and this is the last factor', async () => { + const unenroll = vi.spyOn(auth.mfa, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const unenrollRecoveryCodes = vi.spyOn(auth.mfa.recoveryCodes, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const onClose = vi.fn() + + customRender( + + ) + await screen.findByText('Confirm to delete factor') + await screen.findByText('Multi-factor authentication will be disabled') + await screen.findByText('Your recovery codes will be deleted too') + fireEvent.click(await screen.findByRole('button', { name: 'Delete' })) + await waitFor(() => expect(unenrollRecoveryCodes).toHaveBeenCalled()) + await waitFor(() => expect(unenroll).toHaveBeenCalled()) + await waitFor(() => expect(onClose).toHaveBeenCalled()) + }) + test('Allows users to cancel the deletion', async () => { + const unenroll = vi.spyOn(auth.mfa, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const unenrollRecoveryCodes = vi.spyOn(auth.mfa.recoveryCodes, 'unenroll').mockResolvedValue({ + data: { + id: 'some_id', + }, + error: null, + }) + const onClose = vi.fn() + + customRender( + + ) + await screen.findByText('Confirm to delete factor') + expect(screen.queryByText('Your recovery codes will be deleted too')).toBeNull() + fireEvent.click(await screen.findByRole('button', { name: 'Cancel' })) + await waitFor(() => expect(onClose).toHaveBeenCalled()) + expect(unenroll).not.toHaveBeenCalled() + expect(unenrollRecoveryCodes).not.toHaveBeenCalled() + }) +}) diff --git a/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.tsx b/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.tsx index 06f887d3af9..26f01a133b0 100644 --- a/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.tsx +++ b/apps/studio/components/interfaces/Account/TOTPFactors/DeleteFactorModal.tsx @@ -4,25 +4,29 @@ import ConfirmationModal from 'ui-patterns/Dialogs/ConfirmationModal' import { organizationKeys } from '@/data/organizations/keys' import { useMfaUnenrollMutation } from '@/data/profile/mfa-unenroll-mutation' +import { useRecoveryCodesUnenrollMutation } from '@/data/recovery-codes/recovery-codes-unenroll' import { useLastVisitedOrganization } from '@/hooks/misc/useLastVisitedOrganization' interface DeleteFactorModalProps { visible: boolean factorId: string | null lastFactorToBeDeleted: boolean + hasRecoveryCodes: boolean onClose: () => void } -const DeleteFactorModal = ({ +export const DeleteFactorModal = ({ visible, factorId, lastFactorToBeDeleted, + hasRecoveryCodes, onClose, }: DeleteFactorModalProps) => { const queryClient = useQueryClient() + const { lastVisitedOrganization } = useLastVisitedOrganization() - const { mutate: unenroll, isPending } = useMfaUnenrollMutation({ + const unenrollMFAMutation = useMfaUnenrollMutation({ onSuccess: async () => { if (lastVisitedOrganization) { await queryClient.invalidateQueries({ @@ -34,6 +38,15 @@ const DeleteFactorModal = ({ }, }) + const unenrollRecoveryCodesMutation = useRecoveryCodesUnenrollMutation({ + onSuccess: () => { + if (!factorId) return // Should never happen + unenrollMFAMutation.mutate({ factorId }) + }, + }) + + const loading = unenrollMFAMutation.isPending || unenrollRecoveryCodesMutation.isPending + return ( factorId && unenroll({ factorId })} + onConfirm={() => { + // If users have recovery codes and this is the last MFA for their account, + // we must first delete the recovery codes (they don't make sense without any MFA) + const shouldDeleteRecoveryCodes = lastFactorToBeDeleted && hasRecoveryCodes + if (factorId && !shouldDeleteRecoveryCodes) { + return unenrollMFAMutation.mutate({ factorId }) + } + unenrollRecoveryCodesMutation.mutate() + }} alert={{ title: lastFactorToBeDeleted ? 'Multi-factor authentication will be disabled' @@ -63,6 +84,7 @@ const DeleteFactorModal = ({
  • You will lose access to any organization that enforces multi-factor authentication
    • + {hasRecoveryCodes &&
  • Your recovery codes will be deleted too
    • } ) : ( <> @@ -74,5 +96,3 @@ const DeleteFactorModal = ({     ) } - -export default DeleteFactorModal diff --git a/apps/studio/components/interfaces/Account/TOTPFactors/index.tsx b/apps/studio/components/interfaces/Account/TOTPFactors/index.tsx index 801e14635d1..ea277494f14 100644 --- a/apps/studio/components/interfaces/Account/TOTPFactors/index.tsx +++ b/apps/studio/components/interfaces/Account/TOTPFactors/index.tsx @@ -4,6 +4,7 @@ import { Plus } from 'lucide-react' import { useState } from 'react' import { Button, Card, CardContent, cn } from 'ui' import { Admonition } from 'ui-patterns/Admonition' +import { ErrorDisplay } from 'ui-patterns/ErrorDisplay/ErrorDisplay' import { PageSection, PageSectionAside, @@ -16,14 +17,14 @@ import { import { GenericSkeletonLoader } from 'ui-patterns/ShimmeringLoader' import { AddNewFactorModal } from './AddNewFactorModal' -import DeleteFactorModal from './DeleteFactorModal' +import { DeleteFactorModal } from './DeleteFactorModal' import { GenerateRecoveryCodesModal } from './GenerateRecoveryCodesModal' import { RegenerateRecoveryCodesModal } from './RegenerateRecoveryCodesModal' import { UnenrollRecoveryCodesModal } from './UnenrollRecoveryCodesModal' import { AlertError } from '@/components/ui/AlertError' import { useMfaListFactorsQuery } from '@/data/profile/mfa-list-factors-query' import { useRecoveryCodesStatusQuery } from '@/data/recovery-codes/recovery-codes-status-query' -import { DATETIME_FORMAT, IS_STAGING_OR_LOCAL } from '@/lib/constants' +import { DATETIME_FORMAT } from '@/lib/constants' export const TOTPFactors = () => { const [isAddNewFactorOpen, setIsAddNewFactorOpen] = useState(false) @@ -36,15 +37,18 @@ export const TOTPFactors = () => { const shouldShowLockoutWarning = isSuccess && totpFactors.length === 1 const shouldVerifyRecoveryCodes = enableAuthRecoveryCodes && totpFactors.length > 0 - const { data: recoveryCodesStatus } = useRecoveryCodesStatusQuery({ + const recoveryCodesStatusQuery = useRecoveryCodesStatusQuery({ enabled: shouldVerifyRecoveryCodes, }) const handleAddNewApp = () => setIsAddNewFactorOpen(true) + // If recovery codes are enabled, we can't allow to remove an MFA until we know their status + const disableDeleteFactor = shouldVerifyRecoveryCodes && recoveryCodesStatusQuery.isPending + return ( <> - {enableAuthRecoveryCodes && shouldVerifyRecoveryCodes && ( + {shouldVerifyRecoveryCodes && ( @@ -56,26 +60,35 @@ export const TOTPFactors = () => { - {recoveryCodesStatus?.status === 'unenrolled' && } - {recoveryCodesStatus?.status === 'available' && recoveryCodesStatus?.data && ( - - -

    - {recoveryCodesStatus.data.remaining}/{recoveryCodesStatus.data.total} recovery - codes available -

    -
    - - {IS_STAGING_OR_LOCAL && } -
    -     -     + {recoveryCodesStatusQuery.isError && ( + )} + {recoveryCodesStatusQuery.data?.status === 'unenrolled' && ( + + )} + {recoveryCodesStatusQuery.data?.status === 'available' && + recoveryCodesStatusQuery.data?.data && ( + + +

    + {recoveryCodesStatusQuery.data.data.remaining}/ + {recoveryCodesStatusQuery.data.data.total} recovery codes available +

    +
    + + +
    +     +     + )}     )} @@ -136,7 +149,11 @@ export const TOTPFactors = () => { Added on {dayjs(factor.created_at).format(DATETIME_FORMAT)}

    - @@ -156,6 +173,7 @@ export const TOTPFactors = () => { factorId={factorToBeDeleted} lastFactorToBeDeleted={totpFactors.length === 1} onClose={() => setFactorToBeDeleted(null)} + hasRecoveryCodes={recoveryCodesStatusQuery.data?.status === 'available'} /> )