fix(auth): email-change recovery re-send and confirmation feedback (#2034)
* fix(auth): email-change recovery re-send and confirmation feedback A half-completed secure email change was a dead end: the pending-address short-circuit in /api/account/email swallowed every retry without re-sending mails, so once the confirmation links expired the user could never recover, and confirmation clicks landed on the dashboard with no feedback at all. - /api/account/email: only short-circuit a repeat request while the pending mails are fresh (30 min); a stale pending change falls through to GoTrue, which restarts the change and re-sends both mails - /auth/callback: type=email_change now redirects to a status page (/auth/email-change) that says whether one click remains, the change is complete, or the link was dead, instead of landing silently - auth mail templates: both email-change mails explain that two mails are sent and both links must be clicked - settings: the save button re-enables for the pending address as Skicka igen, so users can trigger the re-send themselves Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sbGMZQE5W7KfSVFjK7E4p * fix(auth): exempt email-change confirmations from the authenticated /auth bounce (skeptic findings) - middleware: let /auth/email-change and /auth/callback?type=email_change through for authenticated users; the bounce to / swallowed confirmation clicks before verifyOtp ran (pre-existing since #2017) - email-change done page resolves the WL-14 landing destination for the CTA - /api/account/email returns resent flag; settings toast says mails were already sent instead of claiming a fresh send Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018sbGMZQE5W7KfSVFjK7E4p --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Mattsson
co-authored by
Claude Fable 5
Claude Fable 5
parent
9f8fa1b692
commit
341d61131a
@@ -0,0 +1,99 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest'
|
||||
import { NextRequest } from 'next/server'
|
||||
|
||||
// The email_change branch returns before any of the invite/team/landing
|
||||
// machinery runs; these mocks only keep the module importable in the test
|
||||
// environment.
|
||||
vi.mock('@/lib/auth/invite-tokens', () => ({ hashInviteToken: vi.fn() }))
|
||||
vi.mock('@/lib/auth/consume-invite-cookie', () => ({
|
||||
INVITE_COOKIE_NAME: 'gnubok-invite-token',
|
||||
}))
|
||||
vi.mock('@/lib/company/landing-server', () => ({
|
||||
resolveLandingDestination: vi.fn().mockResolvedValue('/'),
|
||||
}))
|
||||
vi.mock('@/lib/company/pending-invites', () => ({
|
||||
acceptPendingTeamInviteByToken: vi.fn(),
|
||||
}))
|
||||
|
||||
const verifyOtp = vi.fn()
|
||||
vi.mock('@supabase/ssr', () => ({
|
||||
createServerClient: () => ({
|
||||
auth: {
|
||||
verifyOtp: (...args: unknown[]) => verifyOtp(...args),
|
||||
exchangeCodeForSession: vi.fn(),
|
||||
getUser: vi.fn().mockResolvedValue({ data: { user: null } }),
|
||||
},
|
||||
}),
|
||||
}))
|
||||
|
||||
import { GET } from '../route'
|
||||
|
||||
function makeRequest(params: Record<string, string>) {
|
||||
const url = new URL('https://app.testbrand.example/auth/callback')
|
||||
for (const [key, value] of Object.entries(params)) {
|
||||
url.searchParams.set(key, value)
|
||||
}
|
||||
return new NextRequest(url)
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('GET /auth/callback type=email_change', () => {
|
||||
it('redirects a first confirmation (no session) to the partial status page', async () => {
|
||||
verifyOtp.mockResolvedValue({
|
||||
data: { user: null, session: null },
|
||||
error: null,
|
||||
})
|
||||
|
||||
const res = await GET(makeRequest({ token_hash: 'th', type: 'email_change' }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(
|
||||
'https://app.testbrand.example/auth/email-change?status=partial',
|
||||
)
|
||||
expect(verifyOtp).toHaveBeenCalledWith({
|
||||
token_hash: 'th',
|
||||
type: 'email_change',
|
||||
})
|
||||
})
|
||||
|
||||
it('redirects the completing confirmation (session minted) to the done status page', async () => {
|
||||
verifyOtp.mockResolvedValue({
|
||||
data: { user: { id: 'u1' }, session: { access_token: 'at' } },
|
||||
error: null,
|
||||
})
|
||||
|
||||
const res = await GET(makeRequest({ token_hash: 'th', type: 'email_change' }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(
|
||||
'https://app.testbrand.example/auth/email-change?status=done',
|
||||
)
|
||||
})
|
||||
|
||||
it('redirects an invalid or expired link to the failed status page', async () => {
|
||||
verifyOtp.mockResolvedValue({
|
||||
data: { user: null, session: null },
|
||||
error: { message: 'Email link is invalid or has expired' },
|
||||
})
|
||||
|
||||
const res = await GET(makeRequest({ token_hash: 'th', type: 'email_change' }))
|
||||
|
||||
expect(res.headers.get('location')).toBe(
|
||||
'https://app.testbrand.example/auth/email-change?status=failed',
|
||||
)
|
||||
})
|
||||
|
||||
it('keeps the generic login-error redirect for other failed types', async () => {
|
||||
verifyOtp.mockResolvedValue({
|
||||
data: { user: null, session: null },
|
||||
error: { message: 'Email link is invalid or has expired' },
|
||||
})
|
||||
|
||||
const res = await GET(makeRequest({ token_hash: 'th', type: 'signup' }))
|
||||
|
||||
const location = res.headers.get('location') ?? ''
|
||||
expect(location).toContain('/login?error=auth_error')
|
||||
expect(location).not.toContain('/auth/email-change')
|
||||
})
|
||||
})
|
||||
@@ -63,10 +63,31 @@ export async function GET(request: NextRequest) {
|
||||
}
|
||||
// Handle token hash flow (email verification / magic link)
|
||||
else if (token_hash && type) {
|
||||
const { error } = await supabase.auth.verifyOtp({
|
||||
const { data, error } = await supabase.auth.verifyOtp({
|
||||
token_hash,
|
||||
type: type as 'signup' | 'invite' | 'magiclink' | 'recovery' | 'email_change' | 'email',
|
||||
})
|
||||
|
||||
// Email change never lands silently: with secure email change enabled the
|
||||
// user must confirm from BOTH addresses, and dropping them on the
|
||||
// dashboard (or login) with no message is exactly how a half-completed
|
||||
// change reads as "det funkar inte". The status page tells them whether
|
||||
// one click remains, the change is complete, or the link was dead.
|
||||
// Completing the second confirmation returns a session; the first (or a
|
||||
// click from a logged-out mailbox) does not, which is what separates
|
||||
// 'done' from 'partial'. Cookies are forwarded so a minted session
|
||||
// survives the redirect.
|
||||
if (type === 'email_change') {
|
||||
const status = error ? 'failed' : data?.session ? 'done' : 'partial'
|
||||
const response = NextResponse.redirect(
|
||||
new URL(`/auth/email-change?status=${status}`, origin),
|
||||
)
|
||||
for (const { name, value, options } of pendingCookies) {
|
||||
response.cookies.set({ name, value, ...options })
|
||||
}
|
||||
return response
|
||||
}
|
||||
|
||||
authenticated = !error
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
import Link from 'next/link'
|
||||
import { headers } from 'next/headers'
|
||||
import { MailCheck, MailWarning, Mails } from 'lucide-react'
|
||||
import { Button } from '@/components/ui/button'
|
||||
import { createClient } from '@/lib/supabase/server'
|
||||
import { resolveLandingDestination } from '@/lib/company/landing-server'
|
||||
|
||||
/**
|
||||
* Landing page for email-change confirmation clicks (/auth/callback redirects
|
||||
* here for type=email_change). Swedish-only like the other (auth) surfaces:
|
||||
* the reader may not even have a session, so user-preference locale does not
|
||||
* apply.
|
||||
*
|
||||
* Secure email change requires a click in BOTH mails (new address + current
|
||||
* address), and this page is the only feedback the user gets after each
|
||||
* click, so it must say exactly what remains.
|
||||
*/
|
||||
|
||||
type EmailChangeStatus = 'partial' | 'done' | 'failed'
|
||||
|
||||
const CONTENT: Record<
|
||||
EmailChangeStatus,
|
||||
{ heading: string; body: string; cta: string; href: string }
|
||||
> = {
|
||||
partial: {
|
||||
heading: 'Ett klick kvar',
|
||||
body: 'Din bekräftelse är registrerad. Av säkerhetsskäl skickades två mail, ett till din nya adress och ett till din nuvarande. Öppna det andra mailet och klicka på länken där för att slutföra bytet.',
|
||||
cta: 'Gå till startsidan',
|
||||
href: '/',
|
||||
},
|
||||
done: {
|
||||
heading: 'E-postadressen är ändrad',
|
||||
body: 'Klart! Din nya e-postadress gäller nu när du loggar in med e-post. Loggar du in med Google fortsätter det att fungera som vanligt.',
|
||||
cta: 'Gå till startsidan',
|
||||
href: '/',
|
||||
},
|
||||
failed: {
|
||||
heading: 'Länken är ogiltig eller har gått ut',
|
||||
body: 'Bekräftelselänken kunde inte användas. Begär bytet igen under Inställningar: Konto, så skickas två nya bekräftelsemail direkt.',
|
||||
cta: 'Gå till kontoinställningar',
|
||||
href: '/settings/account',
|
||||
},
|
||||
}
|
||||
|
||||
function isEmailChangeStatus(value: string | undefined): value is EmailChangeStatus {
|
||||
return value === 'partial' || value === 'done' || value === 'failed'
|
||||
}
|
||||
|
||||
export default async function EmailChangeStatusPage({
|
||||
searchParams,
|
||||
}: {
|
||||
searchParams: Promise<{ status?: string }>
|
||||
}) {
|
||||
const { status } = await searchParams
|
||||
const resolved: EmailChangeStatus = isEmailChangeStatus(status) ? status : 'failed'
|
||||
const content = CONTENT[resolved]
|
||||
|
||||
// On completion the CTA goes where the callback would have sent the user
|
||||
// before this page existed (WL-14: byrå staff on their home domain land in
|
||||
// the cockpit, everyone else on the dashboard). Any failure degrades to '/'.
|
||||
let href = content.href
|
||||
if (resolved === 'done') {
|
||||
try {
|
||||
const supabase = await createClient()
|
||||
const {
|
||||
data: { user },
|
||||
} = await supabase.auth.getUser()
|
||||
if (user) {
|
||||
const headerStore = await headers()
|
||||
const host =
|
||||
headerStore.get('x-forwarded-host') ?? headerStore.get('host') ?? ''
|
||||
href = await resolveLandingDestination(supabase, user.id, host)
|
||||
}
|
||||
} catch {
|
||||
href = content.href
|
||||
}
|
||||
}
|
||||
const Icon =
|
||||
resolved === 'done' ? MailCheck : resolved === 'partial' ? Mails : MailWarning
|
||||
|
||||
return (
|
||||
<div className="min-h-dvh flex flex-col items-center justify-center bg-background p-4">
|
||||
<div className="w-full max-w-sm animate-slide-up text-center">
|
||||
<div className="flex justify-center mb-4">
|
||||
<div className="h-14 w-14 rounded-lg bg-secondary flex items-center justify-center">
|
||||
<Icon className="h-7 w-7 text-primary" />
|
||||
</div>
|
||||
</div>
|
||||
<h1 className="font-display text-3xl tracking-tight">{content.heading}</h1>
|
||||
<p className="text-muted-foreground text-sm mt-2">{content.body}</p>
|
||||
<Button asChild className="mt-8 h-11 px-6">
|
||||
<Link href={href}>{content.cta}</Link>
|
||||
</Button>
|
||||
</div>
|
||||
</div>
|
||||
)
|
||||
}
|
||||
@@ -104,12 +104,13 @@ describe('POST /api/account/email', () => {
|
||||
expect(String(options.emailRedirectTo)).toMatch(/\/auth\/callback$/)
|
||||
})
|
||||
|
||||
it('short-circuits a repeat request for the already-pending address', async () => {
|
||||
it('short-circuits a repeat request while the pending mails are fresh', async () => {
|
||||
const { updateUser } = mockUserClient({
|
||||
user: {
|
||||
id: 'user-1',
|
||||
email: 'old@testbrand.example',
|
||||
new_email: 'pending@testbrand.example',
|
||||
email_change_sent_at: new Date(Date.now() - 60_000).toISOString(),
|
||||
} as { id: string; email?: string },
|
||||
})
|
||||
|
||||
@@ -126,6 +127,47 @@ describe('POST /api/account/email', () => {
|
||||
expect(updateUser).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('re-sends when the pending change is stale (expired-link recovery)', async () => {
|
||||
const { updateUser } = mockUserClient({
|
||||
user: {
|
||||
id: 'user-1',
|
||||
email: 'old@testbrand.example',
|
||||
new_email: 'pending@testbrand.example',
|
||||
email_change_sent_at: new Date(
|
||||
Date.now() - 2 * 60 * 60 * 1000,
|
||||
).toISOString(),
|
||||
} as { id: string; email?: string },
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'pending@testbrand.example' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('re-sends when the pending change has no sent timestamp', async () => {
|
||||
const { updateUser } = mockUserClient({
|
||||
user: {
|
||||
id: 'user-1',
|
||||
email: 'old@testbrand.example',
|
||||
new_email: 'pending@testbrand.example',
|
||||
} as { id: string; email?: string },
|
||||
})
|
||||
|
||||
const req = createMockRequest('/api/account/email', {
|
||||
method: 'POST',
|
||||
body: { email: 'pending@testbrand.example' },
|
||||
})
|
||||
const { status } = await parseJsonResponse(await POST(req))
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(updateUser).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('returns 409 when the address already belongs to another account', async () => {
|
||||
mockUserClient({
|
||||
user: { id: 'user-1', email: 'old@testbrand.example' },
|
||||
|
||||
@@ -12,6 +12,11 @@ const ChangeEmailSchema = z.object({
|
||||
email: z.string().trim().toLowerCase().max(320).pipe(z.string().email()),
|
||||
})
|
||||
|
||||
// How long a pending change is considered fresh enough that re-submitting the
|
||||
// same address is a no-op instead of a re-send. Kept well under the token
|
||||
// expiry so a user with a dead link can always get new mails.
|
||||
const FRESH_PENDING_MS = 30 * 60 * 1000
|
||||
|
||||
/**
|
||||
* POST /api/account/email
|
||||
*
|
||||
@@ -50,12 +55,24 @@ export async function POST(request: Request) {
|
||||
}
|
||||
|
||||
// Re-requesting the address that is already awaiting confirmation is a
|
||||
// no-op success rather than another GoTrue round trip (which would re-send
|
||||
// both confirmation mails and eat into the send rate limit). new_email is
|
||||
// absent on the claims-mapped fast path; then GoTrue's own rate limit is
|
||||
// the backstop.
|
||||
// no-op success ONLY while the pending mails are fresh (protects the send
|
||||
// rate limit against double-clicks). Once they are older than that, the
|
||||
// confirmation links may have expired and the user's only recovery path is
|
||||
// re-running the change, so fall through to GoTrue, which restarts the
|
||||
// change and re-sends both mails. new_email/email_change_sent_at are absent
|
||||
// on the claims-mapped fast path; then GoTrue's own rate limit is the
|
||||
// backstop.
|
||||
if (user.new_email && email === user.new_email.toLowerCase()) {
|
||||
return NextResponse.json({ data: { ok: true, pending_email: email } })
|
||||
const sentAt = user.email_change_sent_at
|
||||
? Date.parse(user.email_change_sent_at)
|
||||
: Number.NaN
|
||||
const fresh =
|
||||
Number.isFinite(sentAt) && Date.now() - sentAt < FRESH_PENDING_MS
|
||||
if (fresh) {
|
||||
return NextResponse.json({
|
||||
data: { ok: true, pending_email: email, resent: false },
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// Trusted-origin resolution, not request.url: behind a proxy request.url
|
||||
@@ -98,5 +115,7 @@ export async function POST(request: Request) {
|
||||
|
||||
log.info('email change requested', { userId: user.id })
|
||||
|
||||
return NextResponse.json({ data: { ok: true, pending_email: email } })
|
||||
return NextResponse.json({
|
||||
data: { ok: true, pending_email: email, resent: true },
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user