mirror of
https://github.com/supabase/supabase.git
synced 2026-10-05 17:35:10 +03:00
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. <img width="1080" height="850" alt="image" src="https://github.com/user-attachments/assets/67d999e7-06ff-4c0a-a2cc-11b864cb32f4" /> ## 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. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
1 parent
564eab8ad7
commit
1235245e6e
3 files changed
+204
-30
No files matched your search
@@ -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(
|
||||
<DeleteFactorModal
|
||||
visible
|
||||
factorId="some_id"
|
||||
lastFactorToBeDeleted={false}
|
||||
onClose={onClose}
|
||||
hasRecoveryCodes
|
||||
/>
|
||||
)
|
||||
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(
|
||||
<DeleteFactorModal
|
||||
visible
|
||||
factorId="some_id"
|
||||
lastFactorToBeDeleted
|
||||
onClose={onClose}
|
||||
hasRecoveryCodes={false}
|
||||
/>
|
||||
)
|
||||
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(
|
||||
<DeleteFactorModal
|
||||
visible
|
||||
factorId="some_id"
|
||||
lastFactorToBeDeleted
|
||||
onClose={onClose}
|
||||
hasRecoveryCodes
|
||||
/>
|
||||
)
|
||||
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(
|
||||
<DeleteFactorModal
|
||||
visible
|
||||
factorId="some_id"
|
||||
lastFactorToBeDeleted={false}
|
||||
onClose={onClose}
|
||||
hasRecoveryCodes
|
||||
/>
|
||||
)
|
||||
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()
|
||||
})
|
||||
})
|
||||
@@ -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 (
|
||||
<ConfirmationModal
|
||||
size="medium"
|
||||
@@ -42,9 +55,17 @@ const DeleteFactorModal = ({
|
||||
title="Confirm to delete factor"
|
||||
confirmLabel="Delete"
|
||||
confirmLabelLoading="Deleting"
|
||||
loading={isPending}
|
||||
loading={loading}
|
||||
onCancel={onClose}
|
||||
onConfirm={() => 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 = ({
|
||||
<li>
|
||||
You will lose access to any organization that enforces multi-factor authentication
|
||||
</li>
|
||||
{hasRecoveryCodes && <li>Your recovery codes will be deleted too</li>}
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
@@ -74,5 +96,3 @@ const DeleteFactorModal = ({
|
||||
</ConfirmationModal>
|
||||
)
|
||||
}
|
||||
|
||||
export default DeleteFactorModal
|
||||
@@ -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 && (
|
||||
<PageSection>
|
||||
<PageSectionMeta>
|
||||
<PageSectionSummary>
|
||||
@@ -56,26 +60,35 @@ export const TOTPFactors = () => {
|
||||
</PageSectionSummary>
|
||||
</PageSectionMeta>
|
||||
<PageSectionContent aria-live="polite">
|
||||
{recoveryCodesStatus?.status === 'unenrolled' && <GenerateRecoveryCodesModal />}
|
||||
{recoveryCodesStatus?.status === 'available' && recoveryCodesStatus?.data && (
|
||||
<Card>
|
||||
<CardContent className="flex flex-col gap-2">
|
||||
<p
|
||||
className={cn(
|
||||
'text-sm',
|
||||
recoveryCodesStatus.data.remaining < 2 ? 'text-warning' : ''
|
||||
)}
|
||||
>
|
||||
{recoveryCodesStatus.data.remaining}/{recoveryCodesStatus.data.total} recovery
|
||||
codes available
|
||||
</p>
|
||||
<div className="flex gap-2 ml-auto">
|
||||
<RegenerateRecoveryCodesModal />
|
||||
{IS_STAGING_OR_LOCAL && <UnenrollRecoveryCodesModal />}
|
||||
</div>
|
||||
</CardContent>
|
||||
</Card>
|
||||
{recoveryCodesStatusQuery.isError && (
|
||||
<ErrorDisplay
|
||||
title="Failed to load recovery codes"
|
||||
errorMessage="An error occurred while loading recovery codes."
|
||||
/>
|
||||
)}
|
||||
{recoveryCodesStatusQuery.data?.status === 'unenrolled' && (
|
||||
<GenerateRecoveryCodesModal />
|
||||
)}
|
||||
{recoveryCodesStatusQuery.data?.status === 'available' &&
|
||||
recoveryCodesStatusQuery.data?.data && (
|
||||
<Card>
|
||||
<CardContent className="flex flex-col gap-2">
|
||||
<p
|
||||
className={cn(
|
||||
'text-sm',
|
||||
recoveryCodesStatusQuery.data.data.remaining < 2 ? 'text-warning' : ''
|
||||
)}
|
||||
>
|
||||
{recoveryCodesStatusQuery.data.data.remaining}/
|
||||
{recoveryCodesStatusQuery.data.data.total} recovery codes available
|
||||
</p>
|
||||
<div className="flex gap-2 ml-auto">
|
||||
<RegenerateRecoveryCodesModal />
|
||||
<UnenrollRecoveryCodesModal />
|
||||
</div>
|
||||
</CardContent>
|
||||
</Card>
|
||||
)}
|
||||
</PageSectionContent>
|
||||
</PageSection>
|
||||
)}
|
||||
@@ -136,7 +149,11 @@ export const TOTPFactors = () => {
|
||||
Added on {dayjs(factor.created_at).format(DATETIME_FORMAT)}
|
||||
</p>
|
||||
</div>
|
||||
<Button size="tiny" onClick={() => setFactorToBeDeleted(factor.id)}>
|
||||
<Button
|
||||
size="tiny"
|
||||
onClick={() => setFactorToBeDeleted(factor.id)}
|
||||
disabled={disableDeleteFactor}
|
||||
>
|
||||
Delete
|
||||
</Button>
|
||||
</CardContent>
|
||||
@@ -156,6 +173,7 @@ export const TOTPFactors = () => {
|
||||
factorId={factorToBeDeleted}
|
||||
lastFactorToBeDeleted={totpFactors.length === 1}
|
||||
onClose={() => setFactorToBeDeleted(null)}
|
||||
hasRecoveryCodes={recoveryCodesStatusQuery.data?.status === 'available'}
|
||||
/>
|
||||
</>
|
||||
)
|
||||
|
||||
Reference in new issue
Block a user