diff --git a/backend/migrations/1788100000000_add-customer-email-changes.js b/backend/migrations/1788100000000_add-customer-email-changes.js new file mode 100644 index 0000000..2c11d32 --- /dev/null +++ b/backend/migrations/1788100000000_add-customer-email-changes.js @@ -0,0 +1,54 @@ +exports.up = (pgm) => { + pgm.sql(` + -- An email address changed by the shop rather than by the customer (#337). + -- + -- This exists because of what the action is. A customer who has lost access + -- to their mailbox has no self-service route back in, and there should not + -- be one — this shop holds no second proof of identity, and anything + -- invented to fill that gap would be a weaker credential than the one it + -- replaced. So the route is manual: the owner verifies the customer against + -- order history and moves the account to an address they can reach. + -- + -- That is also, exactly, what an account takeover looks like. The two are + -- the same operation and differ only in whether the verification was sound. + -- A hand-written database edit leaves nothing to tell them apart afterwards. + -- This table is what does. + CREATE TABLE IF NOT EXISTS customer_email_changes ( + id SERIAL PRIMARY KEY, + + -- Cascades with the customer, deliberately. Both addresses here are + -- personal data, so a record that outlived an erasure request would keep + -- exactly what the erasure was for. A deleted account also has no + -- takeover left to investigate. + customer_id INTEGER NOT NULL REFERENCES customers(id) ON DELETE CASCADE, + + -- Copied rather than referenced, because the whole point is what the + -- address *was*. The customers row holds the new one and cannot answer + -- this question a moment after the change. + previous_email TEXT NOT NULL, + new_email TEXT NOT NULL, + + -- What the operator typed, and NOT NULL because a change with no stated + -- reason is the one this table exists to make impossible. Never shown to + -- the customer: it is a note about how they were verified, and it can + -- name things the customer should not be handed back. + reason TEXT NOT NULL, + + -- No "who". Admin access is one shared gate secret in front of a single + -- operator (see middleware/adminGate.ts), so a column for it could only + -- ever hold a constant, and a constant dressed up as an identity is worse + -- than an honest absence. If per-admin identity ever arrives, that is + -- when this gains a column and not before. + changed_at TIMESTAMPTZ NOT NULL DEFAULT now() + ); + + -- The read is always "what has happened to this account", so it is scoped + -- by owner and ordered by time. + CREATE INDEX IF NOT EXISTS customer_email_changes_customer_id_idx + ON customer_email_changes (customer_id, changed_at DESC); + `); +}; + +exports.down = (pgm) => { + pgm.sql(`DROP TABLE IF EXISTS customer_email_changes;`); +}; diff --git a/backend/src/customerVerification.ts b/backend/src/customerVerification.ts new file mode 100644 index 0000000..5948563 --- /dev/null +++ b/backend/src/customerVerification.ts @@ -0,0 +1,58 @@ +import crypto from 'node:crypto'; +import { pool } from './db'; +import { sendMail } from './mailer'; +import { renderTemplate, greeting, formatDuration } from './emailTemplates'; +import { getSettings } from './adminSettings'; +import { loadStoredTemplate } from './routes/adminEmailTemplates'; + +/** + * Issuing a "confirm this address" link, for every route that changes an address. + * + * Lifted out of routes/customers.ts when the admin gained the ability to move an + * account to a new address (#337), for the same reason session creation was + * lifted out for passkeys: two implementations that agree today are two + * implementations that can be changed one at a time, and the one that would be + * forgotten is whichever the manual testing does not exercise. The admin path + * runs perhaps once a year, so it is exactly the one that would rot. + * + * Anything that puts a new address on an account belongs here: registration, a + * resend, the customer changing their own, and the shop changing it for them. + */ + +/** + * Supersedes any outstanding link as part of issuing the new one, so a message + * already sitting in an old inbox cannot verify a newer address. Deleting first + * is the part that matters — an un-superseded link means an older message still + * verifies. + * + * Sending is fire-and-forget by the rule the rest of this codebase follows: the + * token row is written first, so a send that fails cannot leave a customer + * believing a link exists that does not, only waiting for one that never came. + */ +export async function issueVerificationEmail( + customerId: number, + email: string, + firstName: string | null, + lastName: string | null = null +): Promise { + await pool.query( + `DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, + [customerId] + ); + const { verifyTokenHours, greetingFormat, greetingFallback } = await getSettings(); + const token = crypto.randomBytes(24).toString('hex'); + await pool.query( + `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, + [token, customerId, new Date(Date.now() + verifyTokenHours * 60 * 60 * 1000)] + ); + const verifyUrl = `${process.env.PUBLIC_URL}/verify-email?token=${token}`; + const template = renderTemplate('verification', await loadStoredTemplate('verification'), { + greeting: greeting(firstName, greetingFormat, greetingFallback, lastName), + firstName: firstName ?? '', + lastName: lastName ?? '', + verifyUrl, + expiresIn: formatDuration(verifyTokenHours) + }); + sendMail(email, template.subject, template.html) + .catch(err => console.error('verify email send failed', err)); +} diff --git a/backend/src/db-kysely/schema.ts b/backend/src/db-kysely/schema.ts index 1294691..77d9eed 100644 --- a/backend/src/db-kysely/schema.ts +++ b/backend/src/db-kysely/schema.ts @@ -106,6 +106,15 @@ export interface Customers { unsubscribe_token: string; } +export interface CustomerEmailChanges { + changed_at: Generated; + customer_id: number; + id: Generated; + new_email: string; + previous_email: string; + reason: string; +} + export interface CustomerSessions { created_at: Generated; customer_id: number; @@ -241,6 +250,7 @@ export interface DB { checkout_items: CheckoutItems; checkouts: Checkouts; customer_credentials: CustomerCredentials; + customer_email_changes: CustomerEmailChanges; customer_sessions: CustomerSessions; customer_tokens: CustomerTokens; customers: Customers; diff --git a/backend/src/emailTemplates.ts b/backend/src/emailTemplates.ts index 1e8cd16..f041e40 100644 --- a/backend/src/emailTemplates.ts +++ b/backend/src/emailTemplates.ts @@ -18,6 +18,7 @@ export type TemplateKey = | 'favoriteWithdrawn' | 'cartReminder' | 'emailChanged' + | 'emailChangedByAdmin' | 'intakeDraft' | 'uploadLink'; @@ -119,6 +120,40 @@ export const TEMPLATES: Record = { 'If you did not, contact us straight away: whoever made the change can now\n' + 'receive password reset links for your account.' }, + emailChangedByAdmin: { + label: 'Email address changed by the shop', + // Its own template rather than reusing emailChanged, because the two are + // addressed to different readers (#337). + // + // The self-service notice says "if you did not make this change, contact + // us". Here somebody already did contact us — that is how the change came + // about — so that sentence would be addressed to a customer who has just + // done the thing it asks for, while the person who actually needs to act on + // it is the one who did nothing. + // + // This is the mail that catches a takeover *by* the recovery route, which + // is the risk the route carries: a stranger who talks their way past the + // verification gets the account, and the only person who can say otherwise + // is whoever still reads the old address. So it goes there, it says plainly + // that the account has moved, and it makes contradicting it the easy reply. + // + // The operator's stated reason is deliberately not a placeholder. It is a + // private note about how somebody was verified, and it can name things the + // customer should not be handed back. + required: ['newEmail'], + available: ['greeting', 'firstName', 'lastName', 'newEmail'], + defaultSubject: 'Your Redefined Designs account has moved to a new email address', + defaultBody: + '{{greeting}}\n\n' + + 'Someone contacted us saying they could no longer get into this account, and\n' + + 'we moved it to **{{newEmail}}** after checking their answers against the\n' + + 'order history on it.\n\n' + + 'If that was you, nothing more is needed — sign in at the new address and\n' + + 'confirm it when you get the message we sent there.\n\n' + + '**If it was not you, reply to this email straight away.** Whoever asked for\n' + + 'the change can now sign in to this account, and we will undo it.' + }, + cartReminder: { label: 'Cart reminder', required: ['itemList', 'cartUrl'], diff --git a/backend/src/routes/adminCustomers.ts b/backend/src/routes/adminCustomers.ts index d86125b..38b44a3 100755 --- a/backend/src/routes/adminCustomers.ts +++ b/backend/src/routes/adminCustomers.ts @@ -1,6 +1,12 @@ import { Router, Request, Response } from 'express'; -import { pool } from '../db'; +import { pool, requireRow } from '../db'; import { asyncRoute } from '../asyncRoute'; +import { isValidEmail, readId } from '../utils'; +import { sendMail } from '../mailer'; +import { renderTemplate, greeting } from '../emailTemplates'; +import { getSettings } from '../adminSettings'; +import { loadStoredTemplate } from './adminEmailTemplates'; +import { issueVerificationEmail } from '../customerVerification'; /** * Row shapes for the reads here, kept in step with their SQL by hand. @@ -67,6 +73,15 @@ interface IdRow { id: number; } +/** One recorded admin-initiated address change (#337). */ +interface EmailChangeRow { + id: number; + previous_email: string; + new_email: string; + reason: string; + changed_at: Date; +} + const router = Router(); router.get('/', asyncRoute(async (_req: Request, res: Response) => { @@ -205,6 +220,173 @@ router.post('/:id/reserved/:itemId/release', asyncRoute(async (req: Request, res } })); +/** The reason the operator typed, or null if it is not usable as one. */ +function readReason(value: unknown): string | null { + if (typeof value !== 'string') return null; + const trimmed = value.trim(); + // A length floor rather than merely non-empty. The record exists to + // distinguish a verified recovery from a takeover afterwards, and "ok" cannot + // do that — but no floor high enough to be gamed is worth having either, so + // this asks for a sentence and trusts the person writing it. + if (trimmed.length < 10) return null; + // Bounded because it is free text going into a TEXT column from a form. + return trimmed.slice(0, 2000); +} + +/** + * Moving an account to an address its owner can actually reach (#337). + * + * This is the third step of the only recovery route a customer who has lost + * their mailbox has, and there is deliberately no self-service equivalent: the + * email address is the root of trust for every other route, this shop holds no + * second proof of identity, and anything invented to fill that gap would be a + * weaker credential than the one it replaced. So the route is manual, and + * `docs/ops/account-recovery.md` describes the verification that has to happen + * before this endpoint is called. + * + * The uncomfortable part, stated plainly: this operation and an account takeover + * are the same operation. They differ only in whether the verification was + * sound, and nothing here can check that. What this can do is make the change + * recorded, announced, and reversible in its effects — which is what everything + * below is for. + * + * No current-password check, unlike the customer's own change. There is no + * password to ask for; the whole premise is that the person asking cannot prove + * anything the system can verify. The admin gate is the only authorisation, and + * the operator's judgement is the only verification. + */ +router.put('/:id/email', asyncRoute(async (req: Request, res: Response) => { + const id = readId(req.params.id); + if (id === null) return res.status(404).json({ error: 'not found' }); + + const { email, reason } = req.body ?? {}; + + const normalized = String(email ?? '').toLowerCase().trim(); + if (!normalized || !isValidEmail(normalized)) { + return res.status(400).json({ error: 'a valid email is required' }); + } + + const stated = readReason(reason); + if (stated === null) { + return res.status(400).json({ + error: 'say why this account is being moved — a sentence naming how the customer was verified' + }); + } + + const { rows } = await pool.query<{ id: number; email: string; first_name: string | null; last_name: string | null }>( + `SELECT id, email, first_name, last_name FROM customers WHERE id = $1`, + [id] + ); + const customer = rows[0]; + if (!customer) return res.status(404).json({ error: 'not found' }); + + if (normalized === customer.email) { + return res.status(400).json({ error: 'that is already this customer’s email address' }); + } + + const { rows: taken } = await pool.query(`SELECT id FROM customers WHERE email = $1`, [normalized]); + if (taken.length) { + return res.status(409).json({ error: 'another account already uses this email address' }); + } + + // Captured before the update, because it is where the notice has to go and + // the row will not be able to answer for it a moment from now. + const previousEmail = customer.email; + + let passkeysRemoved = 0; + const client = await pool.connect(); + try { + await client.query('BEGIN'); + + await client.query( + // Unverified, exactly as the self-service change leaves it. Nobody has + // demonstrated receiving mail at this address yet — a customer describing + // it over the phone is not that, and it is the commonest way this goes + // wrong harmlessly. + `UPDATE customers SET email = $1, email_verified = false WHERE id = $2`, + [normalized, id] + ); + + // Everything the previous holder of this account had, on the reasoning #42 + // settled for password reset. An account being moved to a recovered address + // is in the same position as one being recovered by reset, and the same + // argument applies with more force: here somebody the system cannot + // identify has asked for the change, so a session or a credential surviving + // it would be one the new owner cannot see and cannot revoke. + await client.query(`DELETE FROM customer_sessions WHERE customer_id = $1`, [id]); + const removed = await client.query(`DELETE FROM customer_credentials WHERE customer_id = $1`, [id]); + passkeysRemoved = removed.rowCount ?? 0; + await client.query(`DELETE FROM webauthn_challenges WHERE customer_id = $1`, [id]); + + // Reset links already sent are addressed to the old mailbox, which is the + // one this change is taking away. Leaving them live would let whoever still + // reads it take the account straight back. + await client.query(`DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'password_reset'`, [id]); + + await client.query( + `INSERT INTO customer_email_changes (customer_id, previous_email, new_email, reason) + VALUES ($1, $2, $3, $4)`, + [id, previousEmail, normalized, stated] + ); + + await client.query('COMMIT'); + } catch (err) { + await client.query('ROLLBACK'); + throw err; + } finally { + client.release(); + } + + // Both sends happen after the row is written, never before, so a change that + // failed cannot produce mail saying it succeeded. + await issueVerificationEmail(id, normalized, customer.first_name, customer.last_name); + + // To the address being replaced, which is the whole point. If the recovery + // was sound this reaches nobody, and that costs nothing. If it was not, it + // reaches the real owner — who is the only person who can say so, and the + // only reason this endpoint is safe to have at all. + const { greetingFormat, greetingFallback } = await getSettings(); + const notice = renderTemplate('emailChangedByAdmin', await loadStoredTemplate('emailChangedByAdmin'), { + greeting: greeting(customer.first_name, greetingFormat, greetingFallback, customer.last_name), + firstName: customer.first_name ?? '', + lastName: customer.last_name ?? '', + newEmail: normalized + }); + sendMail(previousEmail, notice.subject, notice.html) + .catch(err => console.error('admin email change notice send failed', err)); + + const { rows: updated } = await pool.query( + `SELECT id, email, nullif(btrim(concat_ws(' ', first_name, last_name)), '') AS name, + email_verified, marketing_consent, marketing_consent_at, created_at + FROM customers WHERE id = $1`, + [id] + ); + + res.json({ + customer: requireRow(updated, 'the customer after the admin email change'), + previousEmail, + // Reported so the operator can tell the customer what they will have to set + // up again, and so a surprising number is visible at the moment it happens + // rather than never. + passkeysRemoved + }); +})); + +/** What has been done to this account's address, and why (#337). */ +router.get('/:id/email-changes', asyncRoute(async (req: Request, res: Response) => { + const id = readId(req.params.id); + if (id === null) return res.status(404).json({ error: 'not found' }); + + const { rows } = await pool.query( + `SELECT id, previous_email, new_email, reason, changed_at + FROM customer_email_changes + WHERE customer_id = $1 + ORDER BY changed_at DESC`, + [id] + ); + res.json(rows); +})); + router.get('/:id', asyncRoute(async (req: Request, res: Response) => { const { rows: customerRows } = await pool.query( `SELECT id, email, nullif(btrim(concat_ws(' ', first_name, last_name)), '') AS name, diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index b90f4a9..fe881c7 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -16,46 +16,13 @@ import { passwordResetRequestLimiter, verificationResendLimiter } from '../rateL // Shared with passkey sign-in, so both paths establish a session identically // rather than in two places that merely agree today (#39). import { setSessionCookie, createSession } from '../customerSession'; +// Registration, a resend, the customer changing their own address and the shop +// changing it for them all need the same three steps, and they now live in one +// place for the same reason session creation does (#337). +import { issueVerificationEmail } from '../customerVerification'; const router = Router(); -// Registration, changing an address, and resending all need the same three -// steps: supersede any outstanding link, mint a new one, send it. Written out -// three times they would drift, and the step most likely to be forgotten is the -// first — which is the one that matters, since an un-superseded link means an -// older message in the inbox still verifies. -// -// Sending is fire-and-forget by the same rule the rest of this file follows: -// the token row is written first, so a send that fails cannot leave a customer -// believing a link exists that does not, only waiting for one that never came. -async function issueVerificationEmail( - customerId: number, - email: string, - firstName: string | null, - lastName: string | null = null -): Promise { - await pool.query( - `DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, - [customerId] - ); - const { verifyTokenHours, greetingFormat, greetingFallback } = await getSettings(); - const token = crypto.randomBytes(24).toString('hex'); - await pool.query( - `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, - [token, customerId, new Date(Date.now() + verifyTokenHours * 60 * 60 * 1000)] - ); - const verifyUrl = `${process.env.PUBLIC_URL}/verify-email?token=${token}`; - const template = renderTemplate('verification', await loadStoredTemplate('verification'), { - greeting: greeting(firstName, greetingFormat, greetingFallback, lastName), - firstName: firstName ?? '', - lastName: lastName ?? '', - verifyUrl, - expiresIn: formatDuration(verifyTokenHours) - }); - sendMail(email, template.subject, template.html) - .catch(err => console.error('verify email send failed', err)); -} - // setSessionCookie and createSession now live in ../customerSession, shared with // passkey sign-in. #39 requires that path to establish a session identically to // this one, and sharing the code is what makes that true rather than intended. diff --git a/backend/tests/integration/adminChangeCustomerEmail.integration.test.ts b/backend/tests/integration/adminChangeCustomerEmail.integration.test.ts new file mode 100644 index 0000000..daf6a6e --- /dev/null +++ b/backend/tests/integration/adminChangeCustomerEmail.integration.test.ts @@ -0,0 +1,231 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool, requireRow } from '../../src/db'; +import { createSession } from '../../src/customerSession'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +const PASSWORD = 'supersecret123'; +const REASON = 'Named the last two items bought and the shipping address on file.'; + +async function register(email: string): Promise { + const res = await request(app) + .post('/api/customers/register') + .send({ firstName: 'Test', lastName: 'Customer', email, password: PASSWORD }); + expect(res.status).toBe(200); + const { rows } = await pool.query<{ id: number }>(`SELECT id FROM customers WHERE email = $1`, [email]); + return requireRow(rows, 'the customer this test just registered').id; +} + +function moveTo(id: number, email: string, reason: string = REASON) { + return request(app).put(`/api/admin/customers/${id}/email`).send({ email, reason }); +} + +/** + * #337. The third step of the only recovery route available to a customer who + * has lost their mailbox — and, structurally, the same operation as an account + * takeover. These tests are mostly about the second half of that sentence. + */ +describe('PUT /api/admin/customers/:id/email', () => { + it('moves the account, and leaves the new address unverified', async () => { + const id = await register('lost@example.com'); + + const res = await moveTo(id, 'Recovered@Example.com '); + + expect(res.status).toBe(200); + expect(res.body.customer.email).toBe('recovered@example.com'); + expect(res.body.previousEmail).toBe('lost@example.com'); + // Nobody has demonstrated receiving mail at the new address. A customer + // reading it out over the phone is not that, and it is the commonest way + // this goes wrong harmlessly. + expect(res.body.customer.email_verified).toBe(false); + }); + + it('records the change with the reason, which is the point of the endpoint', async () => { + const id = await register('recorded@example.com'); + + await moveTo(id, 'new@example.com'); + + const { rows } = await pool.query<{ previous_email: string; new_email: string; reason: string }>( + `SELECT previous_email, new_email, reason FROM customer_email_changes WHERE customer_id = $1`, + [id] + ); + // A hand edit to the database leaves nothing behind. This row is the only + // thing that distinguishes a verified recovery from a takeover afterwards. + expect(rows).toHaveLength(1); + expect(rows[0]).toEqual({ + previous_email: 'recorded@example.com', + new_email: 'new@example.com', + reason: REASON + }); + }); + + it('refuses without a stated reason, rather than recording an empty one', async () => { + const id = await register('noreason@example.com'); + + const res = await moveTo(id, 'new@example.com', ''); + + expect(res.status).toBe(400); + const { rows } = await pool.query<{ email: string }>(`SELECT email FROM customers WHERE id = $1`, [id]); + // Refused entirely, not performed and left unexplained. + expect(requireRow(rows, 'the unchanged customer').email).toBe('noreason@example.com'); + }); + + it('refuses a reason too short to be one', async () => { + const id = await register('terse@example.com'); + + // "ok" cannot tell a recovery from a takeover, which is the only thing the + // field is for. + const res = await moveTo(id, 'new@example.com', 'ok'); + + expect(res.status).toBe(400); + }); + + it('signs the customer out everywhere', async () => { + const id = await register('sessions@example.com'); + const token = await createSession(id); + const asCustomer = () => request(app).get('/api/customers/me').set('Cookie', `rd_session=${token}`); + expect((await asCustomer()).status).toBe(200); + + await moveTo(id, 'new@example.com'); + + // Somebody the system cannot identify asked for this change. A session + // surviving it is one the new owner cannot see and cannot revoke. + expect((await asCustomer()).status).toBe(401); + }); + + it('removes every passkey, and says how many', async () => { + const id = await register('keys@example.com'); + await pool.query( + `INSERT INTO customer_credentials (customer_id, credential_id, public_key, name) + VALUES ($1, 'credential-a', 'not-a-real-key', 'Phone'), + ($1, 'credential-b', 'not-a-real-key', 'Laptop')`, + [id] + ); + + const res = await moveTo(id, 'new@example.com'); + + expect(res.body.passkeysRemoved).toBe(2); + const { rows } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_credentials WHERE customer_id = $1`, + [id] + ); + expect(requireRow(rows, 'a count of credentials').n).toBe(0); + }); + + it('cancels reset links already sent to the old address', async () => { + const id = await register('resetlink@example.com'); + await request(app) + .post('/api/customers/request-password-reset') + .send({ email: 'resetlink@example.com' }); + const before = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_tokens WHERE customer_id = $1 AND kind = 'password_reset'`, + [id] + ); + expect(requireRow(before.rows, 'a count of reset tokens').n).toBe(1); + + await moveTo(id, 'new@example.com'); + + // That link is addressed to the mailbox this change is taking away. Leaving + // it live would let whoever still reads it take the account straight back. + const after = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_tokens WHERE customer_id = $1 AND kind = 'password_reset'`, + [id] + ); + expect(requireRow(after.rows, 'a count of reset tokens').n).toBe(0); + }); + + it('issues a verification link to the new address', async () => { + const id = await register('verify@example.com'); + + await moveTo(id, 'new@example.com'); + + const { rows } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, + [id] + ); + // Exactly one: registration issued a link to the old address, and issuing + // this one has to supersede it, or a message in the mailbox being taken + // away could still verify. + expect(requireRow(rows, 'a count of verification tokens').n).toBe(1); + }); + + it('refuses an address another account already uses', async () => { + const id = await register('mover@example.com'); + await register('occupied@example.com'); + + const res = await moveTo(id, 'occupied@example.com'); + + expect(res.status).toBe(409); + }); + + it('refuses the address the account already has', async () => { + const id = await register('same@example.com'); + + const res = await moveTo(id, 'same@example.com'); + + expect(res.status).toBe(400); + }); + + it('refuses a malformed address', async () => { + const id = await register('malformed@example.com'); + + const res = await moveTo(id, 'not-an-email'); + + expect(res.status).toBe(400); + }); + + it('is a 404 for a customer who does not exist', async () => { + const res = await moveTo(999999, 'new@example.com'); + + expect(res.status).toBe(404); + }); + + it('leaves the customer able to sign in with their existing password', async () => { + const id = await register('stillworks@example.com'); + + await moveTo(id, 'moved@example.com'); + + // The move is not a password reset. The customer knows their password — + // what they lost was the mailbox — so demanding a new one would add a step + // for no gain. + const login = await request(app) + .post('/api/customers/login') + .send({ email: 'moved@example.com', password: PASSWORD }); + expect(login.status).toBe(200); + }); +}); + +describe('GET /api/admin/customers/:id/email-changes', () => { + it('lists what has been done to this account, newest first', async () => { + const id = await register('history@example.com'); + await moveTo(id, 'second@example.com', 'First recovery, verified against order history.'); + await moveTo(id, 'third@example.com', 'Second recovery, verified against the shipping address.'); + + const res = await request(app).get(`/api/admin/customers/${id}/email-changes`); + + expect(res.status).toBe(200); + expect(res.body).toHaveLength(2); + // Newest first, because an account that has been moved twice is the one + // worth looking at and the most recent move is the one in question. + expect(res.body[0].new_email).toBe('third@example.com'); + expect(res.body[1].new_email).toBe('second@example.com'); + }); + + it('is empty for a customer whose address has never been moved', async () => { + const id = await register('untouched@example.com'); + + const res = await request(app).get(`/api/admin/customers/${id}/email-changes`); + + expect(res.status).toBe(200); + expect(res.body).toEqual([]); + }); +}); diff --git a/backend/tests/integration/emailTemplates.integration.test.ts b/backend/tests/integration/emailTemplates.integration.test.ts index a665464..b9c3906 100644 --- a/backend/tests/integration/emailTemplates.integration.test.ts +++ b/backend/tests/integration/emailTemplates.integration.test.ts @@ -26,6 +26,7 @@ describe('GET /api/admin/email-templates', () => { expect(res.body.map((t: { key: string }) => t.key).sort()).toEqual([ 'cartReminder', 'emailChanged', + 'emailChangedByAdmin', 'favoriteSold', 'favoriteWithdrawn', 'intakeDraft', diff --git a/backend/tests/integration/schemaLoss.integration.test.ts b/backend/tests/integration/schemaLoss.integration.test.ts index 6e68234..f91a73d 100644 --- a/backend/tests/integration/schemaLoss.integration.test.ts +++ b/backend/tests/integration/schemaLoss.integration.test.ts @@ -42,7 +42,7 @@ describe('when the database loses its schema', () => { // The count and a few names, not the whole list: the point is that a reader // can tell at a glance this is a missing schema rather than a logic bug. - await expect(assertSchemaPresent('in a test')).rejects.toThrow(/Missing 20 of 20 tables/); + await expect(assertSchemaPresent('in a test')).rejects.toThrow(/Missing 21 of 21 tables/); await expect(assertSchemaPresent('in a test')).rejects.toThrow(/items/); await migrate(); diff --git a/backend/tests/integration/setup/testDb.ts b/backend/tests/integration/setup/testDb.ts index df7c7bb..7004b8e 100755 --- a/backend/tests/integration/setup/testDb.ts +++ b/backend/tests/integration/setup/testDb.ts @@ -38,6 +38,7 @@ const REQUIRED_TABLES = [ 'checkout_items', 'checkouts', 'customer_credentials', + 'customer_email_changes', 'customer_sessions', 'customer_tokens', 'customers', @@ -153,7 +154,7 @@ export async function resetDb(): Promise { await testPool.query(` TRUNCATE TABLE item_drafts, upload_links, orders, checkout_items, checkouts, shipping_addresses, cart_items, carts, customer_tokens, customer_sessions, favorites, - webauthn_challenges, customer_credentials, + webauthn_challenges, customer_credentials, customer_email_changes, customers, item_tags, item_images, items, tags, categories RESTART IDENTITY CASCADE `); diff --git a/backend/tests/unit/emailTemplates.test.ts b/backend/tests/unit/emailTemplates.test.ts index 0d9935a..e06d188 100644 --- a/backend/tests/unit/emailTemplates.test.ts +++ b/backend/tests/unit/emailTemplates.test.ts @@ -265,7 +265,7 @@ describe('greeting, built from the configured format', () => { }); describe('every template can address the customer', () => { - // Not KEYS: this is an invariant of the six customer-facing templates only. + // Not KEYS: this is an invariant of the customer-facing templates only. // intakeDraft and uploadLink notify the shop and a contributor respectively, // not a customer with a name on file, so they are deliberately not held to // it — a hardcoded list is correct here rather than a staleness risk, @@ -277,7 +277,8 @@ describe('every template can address the customer', () => { 'favoriteSold', 'favoriteWithdrawn', 'cartReminder', - 'emailChanged' + 'emailChanged', + 'emailChangedByAdmin' ]; it.each(CUSTOMER_FACING_KEYS)('%s offers greeting, firstName and lastName', (key) => { diff --git a/docs/ops/account-recovery.md b/docs/ops/account-recovery.md index 01da6af..6abf80f 100644 --- a/docs/ops/account-recovery.md +++ b/docs/ops/account-recovery.md @@ -14,7 +14,7 @@ safety net. | Their password | A reset link emailed to them | Yes | | Their passkey or the device holding it | Signing in with their password | Yes | | Every passkey and their password | A reset link emailed to them | Yes | -| Access to their email address | Nothing. See below. | No | +| Access to their email address | Contacting the shop, which moves the account | No | The email address is the root of trust. Every self-service route above ends at it, and none of them can work without it. @@ -91,12 +91,46 @@ The route is manual, and it runs through the shop owner: 1. The customer makes contact by whatever means they have. 2. The owner verifies them against order history — items bought, dates, the shipping address on file. A stranger has none of that. -3. The owner changes the address on the account. +3. The owner opens the customer in Admin, Customers, and uses **Move to a new + address** on the detail drawer. -**Step 3 does not exist yet.** The admin customer screen can disable, enable and -release reservations, but it cannot change an email address. Until it does, the -answer to a locked-out customer is a database edit by hand. That gap is worth -its own issue rather than being smuggled into the passkeys project, because it -is an admin capability with its own audit and notification questions — the -customer whose address is being replaced has to be told, exactly as the -self-service change already tells them. +Step 3 was added by #337. Before it existed the only answer was a database edit +by hand, which left no record of who did it or why. + +### What the move does, and why + +Say plainly what it is: **this operation and an account takeover are the same +operation.** They differ only in whether the verification in step 2 was sound, +and nothing in the software can check that. Everything the move does is aimed at +that fact. + +- **It asks for a written reason, and refuses without one.** The reason is + recorded against the account and never shown to the customer. It is the only + thing that distinguishes a genuine recovery from a takeover afterwards. +- **It emails the address being replaced.** If the recovery was sound this + reaches nobody, which costs nothing. If it was not, it reaches the real owner, + who is the only person who can say so. That mail has its own wording, because + the self-service notice says "if you did not make this change, contact us" and + here somebody already did. +- **It sends a confirmation link to the new address and marks it unverified.** A + customer reading an address out over the phone has not demonstrated they can + receive mail at it. This is the commonest way the move goes wrong harmlessly. +- **It signs the customer out everywhere, removes every passkey, and cancels + outstanding reset links.** Same reasoning as a password reset, with more + force: somebody the system cannot identify asked for this, so a session or + credential surviving it is one the new owner cannot see or revoke, and a reset + link sitting in the old mailbox would let whoever reads it take the account + straight back. +- **It does not change the password.** What the customer lost was the mailbox, + not the password, so demanding a new one adds a step for no gain. + +The history of moves on an account is shown on the same drawer, with the reasons. +It renders nothing at all for the overwhelming majority of customers, who have +never been moved. + +### What it still does not do + +There is no per-admin identity to record. Admin access is one shared gate secret +in front of a single operator, so a "who" column could only ever hold a constant, +and a constant dressed up as an identity is worse than an honest absence. If +per-admin identity ever arrives, the record gains a column then. diff --git a/frontend/src/admin/ChangeCustomerEmail.tsx b/frontend/src/admin/ChangeCustomerEmail.tsx new file mode 100644 index 0000000..35f94a0 --- /dev/null +++ b/frontend/src/admin/ChangeCustomerEmail.tsx @@ -0,0 +1,121 @@ +import { useState } from 'react'; +import Modal from 'antd/es/modal'; +import Form from 'antd/es/form'; +import Input from 'antd/es/input'; +import Alert from 'antd/es/alert'; +import Button from 'antd/es/button'; +import Typography from 'antd/es/typography'; +import message from 'antd/es/message'; +import { changeCustomerEmail } from './adminCustomersApi'; + +const { Paragraph, Text } = Typography; + +type Props = Readonly<{ + customerId: number; + currentEmail: string; + open: boolean; + onClose: () => void; + /** Called after a successful change, so the drawer and the table can refresh. */ + onChanged: () => void; +}>; + +/** + * Moving a customer's account to an address they can reach (#337). + * + * The last step of the only recovery route available to someone who has lost + * their mailbox. There is deliberately no self-service equivalent, because the + * email address is the root of trust for every other route and this shop holds + * no second proof of identity. + * + * The form leads with what this costs rather than burying it, because the + * operator is about to make a decision on someone else's behalf and the + * consequences land on that person, not on them. + */ +export default function ChangeCustomerEmail({ + customerId, + currentEmail, + open, + onClose, + onChanged +}: Props) { + const [form] = Form.useForm(); + const [saving, setSaving] = useState(false); + + async function submit(values: { email: string; reason: string }) { + setSaving(true); + try { + const result = await changeCustomerEmail(customerId, values.email, values.reason); + // Named rather than counted, because "moved to that address" is the fact + // the operator has to repeat back to the customer on the phone. + message.success(`Account moved to ${result.customer.email}`); + if (result.passkeysRemoved > 0) { + // Its own message and a long one. The customer will find their passkeys + // gone and needs to be told why while they are still on the line — + // finding out later looks like a second thing going wrong. + message.warning( + result.passkeysRemoved === 1 + ? 'Their saved passkey was removed. They will need to set it up again.' + : `Their ${result.passkeysRemoved} saved passkeys were removed. They will need to set them up again.`, + 10 + ); + } + form.resetFields(); + onChanged(); + onClose(); + } catch (err) { + message.error((err as Error).message); + } finally { + setSaving(false); + } + } + + return ( + + + + + Moving the account signs the customer out everywhere, removes any saved passkeys, and + cancels reset links already sent to {currentEmail}. A notice goes to that + address, and a confirmation link goes to the new one. + + +
+ + + + + + + + + +
+
+ ); +} diff --git a/frontend/src/admin/Customers.tsx b/frontend/src/admin/Customers.tsx index b798480..d023954 100755 --- a/frontend/src/admin/Customers.tsx +++ b/frontend/src/admin/Customers.tsx @@ -12,12 +12,57 @@ import message from 'antd/es/message'; import type { ColumnsType } from 'antd/es/table'; import { fetchCustomers, fetchCustomerDetail, fetchReservedItems, releaseReservedItem, - setCustomerDisabled, - CustomerSummary, CustomerDetail, ReservedItem + setCustomerDisabled, fetchCustomerEmailChanges, + CustomerSummary, CustomerDetail, ReservedItem, CustomerEmailChange } from './adminCustomersApi'; +import ChangeCustomerEmail from './ChangeCustomerEmail'; const { Title, Text } = Typography; +/** + * Every address this account has been moved between, and why (#337). + * + * Shown on the detail drawer rather than hidden behind a separate screen, + * because the moment it matters is the moment someone is looking at this + * customer wondering whether the account is in the right hands. Empty for + * almost every customer, so it renders nothing at all rather than an empty + * state that would appear on every drawer to say nothing happened. + */ +function EmailChangeHistory({ changes }: Readonly<{ changes: CustomerEmailChange[] }>) { + if (changes.length === 0) return null; + return ( + <> + Address changes + new Date(v).toLocaleString() + }, + { + title: 'Moved', + key: 'moved', + render: (_, row: CustomerEmailChange) => ( + + {row.previous_email} → {row.new_email} + + ) + }, + // The whole reason the record exists, so it is not truncated behind a + // tooltip. A reader deciding whether a change was legitimate needs the + // sentence, not the first few words of it. + { title: 'Reason', dataIndex: 'reason' } + ]} + /> + + ); +} + // The two halves of the disable/re-enable confirm, as components rather than // branches inside the handler: every piece of copy differs between them, so // one decision up front reads better than the same condition asked five times. @@ -50,15 +95,32 @@ function ReEnableWarning() { // branches, and every one of them counted toward Customers(). function CustomerDetailPanel({ detail, - loading -}: Readonly<{ detail: CustomerDetail | null; loading: boolean }>) { + loading, + emailChanges, + onChangeEmail +}: Readonly<{ + detail: CustomerDetail | null; + loading: boolean; + emailChanges: CustomerEmailChange[]; + onChangeEmail: () => void; +}>) { if (loading || !detail) { return ; } return ( <> - {detail.customer.email} + + {detail.customer.email} + {/* Next to the address rather than among the account actions: this is + a thing done *to* this field, and it is reached by someone already + looking at it because a customer told them they cannot. */} +
+ +
+
{detail.customer.email_verified ? 'Verified' : 'Unverified'} @@ -101,6 +163,8 @@ function CustomerDetailPanel({ ]} /> )} + + ); } @@ -284,6 +348,8 @@ export default function Customers() { const [reservedLoading, setReservedLoading] = useState(false); const [releasing, setReleasing] = useState(null); const [togglingId, setTogglingId] = useState(null); + const [emailChanges, setEmailChanges] = useState([]); + const [movingEmail, setMovingEmail] = useState(false); function load() { return fetchCustomers() @@ -298,9 +364,21 @@ export default function Customers() { async function openDetail(id: number) { setDrawerOpen(true); setDetailLoading(true); + // Cleared rather than left standing: the drawer is reused for every row, and + // one customer's address history showing under another's name is the worst + // possible thing for this particular table to get wrong. + setEmailChanges([]); const data = await fetchCustomerDetail(id); setDetail(data); setDetailLoading(false); + // After the detail, and allowed to fail on its own. This is a rare extra + // rather than part of the record, so a drawer that opens without it beats + // one that does not open at all. + try { + setEmailChanges(await fetchCustomerEmailChanges(id)); + } catch { + setEmailChanges([]); + } } async function openReserved(customer: CustomerSummary) { @@ -428,9 +506,31 @@ export default function Customers() { onClose={() => { setDrawerOpen(false); setDetail(null); }} width={480} > - + setMovingEmail(true)} + /> + {/* Mounted only with a customer in hand, so the modal cannot be opened + against a drawer that has since been closed and emptied. */} + {detail && ( + setMovingEmail(false)} + onChanged={() => { + // Both, and in this order. The drawer is what the operator is + // looking at, and the table behind it still shows the old address. + void openDetail(detail.customer.id); + void load(); + }} + /> + )} + { const res = await fetch('/api/admin/customers'); return res.json(); @@ -71,6 +87,38 @@ export async function releaseReservedItem(customerId: number, itemId: number): P } } +/** + * Moves an account to an address its owner can reach (#337). + * + * The reason is required by the server, not merely collected by the form. This + * operation and an account takeover are the same operation, and the recorded + * reason is the only thing that tells them apart afterwards. + */ +export async function changeCustomerEmail( + customerId: number, + email: string, + reason: string +): Promise { + const res = await fetch(`/api/admin/customers/${customerId}/email`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ email, reason }) + }); + // Reporting success for a change that failed would leave the operator telling + // a customer to check an inbox nothing was sent to. + if (!res.ok) { + const detail = await res.json().catch(() => ({})); + throw new Error(detail.error || 'failed to change the email address'); + } + return res.json(); +} + +export async function fetchCustomerEmailChanges(customerId: number): Promise { + const res = await fetch(`/api/admin/customers/${customerId}/email-changes`); + if (!res.ok) throw new Error('failed to load the address history'); + return res.json(); +} + export async function setCustomerDisabled(customerId: number, disabled: boolean): Promise { const res = await fetch(`/api/admin/customers/${customerId}/${disabled ? 'disable' : 'enable'}`, { method: 'POST'