Merge pull request 'feat(admin): move a customer's account to an address they can reach (#337)' (#338) from feature/337-admin-change-customer-email into main
Linting / lint (push) Successful in 2m52s
SonarQube Analysis / sonarqube (push) Failing after 29m19s

Reviewed-on: #338
This commit was merged in pull request #338.
This commit is contained in:
2026-09-09 16:38:34 -05:00
15 changed files with 900 additions and 57 deletions
@@ -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;`);
};
+58
View File
@@ -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<void> {
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));
}
+10
View File
@@ -106,6 +106,15 @@ export interface Customers {
unsubscribe_token: string;
}
export interface CustomerEmailChanges {
changed_at: Generated<Timestamp>;
customer_id: number;
id: Generated<number>;
new_email: string;
previous_email: string;
reason: string;
}
export interface CustomerSessions {
created_at: Generated<Timestamp>;
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;
+35
View File
@@ -18,6 +18,7 @@ export type TemplateKey =
| 'favoriteWithdrawn'
| 'cartReminder'
| 'emailChanged'
| 'emailChangedByAdmin'
| 'intakeDraft'
| 'uploadLink';
@@ -119,6 +120,40 @@ export const TEMPLATES: Record<TemplateKey, TemplateDefinition> = {
'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'],
+183 -1
View File
@@ -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 customers email address' });
}
const { rows: taken } = await pool.query<IdRow>(`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<CustomerDetailRow>(
`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<EmailChangeRow>(
`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<CustomerDetailRow>(
`SELECT id, email, nullif(btrim(concat_ws(' ', first_name, last_name)), '') AS name,
+4 -37
View File
@@ -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<void> {
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.
@@ -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<number> {
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([]);
});
});
@@ -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',
@@ -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();
+2 -1
View File
@@ -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<void> {
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
`);
+3 -2
View File
@@ -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) => {
+43 -9
View File
@@ -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.
+121
View File
@@ -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 (
<Modal
title="Move this account to a new email address"
open={open}
onCancel={onClose}
footer={null}
destroyOnHidden
style={{ maxWidth: 'calc(100vw - 32px)' }}
>
<Alert
type="warning"
showIcon
message="Verify the customer before doing this"
description="This is the same operation as an account takeover, and nothing here can tell the difference. Check their answers against the order history on the account first — items bought, dates, the shipping address on file."
style={{ marginBottom: 16 }}
/>
<Paragraph type="secondary">
Moving the account signs the customer out everywhere, removes any saved passkeys, and
cancels reset links already sent to <Text code>{currentEmail}</Text>. A notice goes to that
address, and a confirmation link goes to the new one.
</Paragraph>
<Form form={form} layout="vertical" onFinish={submit}>
<Form.Item
name="email"
label="New email address"
rules={[{ required: true, type: 'email', message: 'A valid email address is required' }]}
>
<Input autoComplete="off" placeholder="what they can actually reach" />
</Form.Item>
<Form.Item
name="reason"
label="How you verified them"
// The server requires this too, and refuses without it. Collected here
// as well so the refusal is not the first the operator hears of it.
rules={[{ required: true, min: 10, message: 'A sentence, not a word — this is the record' }]}
extra="Recorded against the account and never shown to the customer. This is what tells a genuine recovery from a takeover afterwards."
>
<Input.TextArea rows={3} placeholder="Named the last two items bought and the shipping address on file." />
</Form.Item>
<Form.Item style={{ marginBottom: 0 }}>
<Button type="primary" danger htmlType="submit" loading={saving} block>
Move the account
</Button>
</Form.Item>
</Form>
</Modal>
);
}
+106 -6
View File
@@ -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 (
<>
<Title level={5} style={{ marginTop: 24 }}>Address changes</Title>
<Table
rowKey="id"
size="small"
dataSource={changes}
pagination={false}
columns={[
{
title: 'When',
dataIndex: 'changed_at',
render: (v: string) => new Date(v).toLocaleString()
},
{
title: 'Moved',
key: 'moved',
render: (_, row: CustomerEmailChange) => (
<span style={{ fontSize: 12 }}>
{row.previous_email} {row.new_email}
</span>
)
},
// 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 <Spin />;
}
return (
<>
<Descriptions column={1} size="small" bordered>
<Descriptions.Item label="Email">{detail.customer.email}</Descriptions.Item>
<Descriptions.Item label="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. */}
<div style={{ marginTop: 4 }}>
<Button size="small" onClick={onChangeEmail} style={{ paddingInline: 0 }} type="link">
Move to a new address
</Button>
</div>
</Descriptions.Item>
<Descriptions.Item label="Verified">
<Tag color={detail.customer.email_verified ? 'green' : 'default'}>
{detail.customer.email_verified ? 'Verified' : 'Unverified'}
@@ -101,6 +163,8 @@ function CustomerDetailPanel({
]}
/>
)}
<EmailChangeHistory changes={emailChanges} />
</>
);
}
@@ -284,6 +348,8 @@ export default function Customers() {
const [reservedLoading, setReservedLoading] = useState(false);
const [releasing, setReleasing] = useState<number | null>(null);
const [togglingId, setTogglingId] = useState<number | null>(null);
const [emailChanges, setEmailChanges] = useState<CustomerEmailChange[]>([]);
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}
>
<CustomerDetailPanel detail={detail} loading={detailLoading} />
<CustomerDetailPanel
detail={detail}
loading={detailLoading}
emailChanges={emailChanges}
onChangeEmail={() => setMovingEmail(true)}
/>
</Drawer>
{/* Mounted only with a customer in hand, so the modal cannot be opened
against a drawer that has since been closed and emptied. */}
{detail && (
<ChangeCustomerEmail
customerId={detail.customer.id}
currentEmail={detail.customer.email}
open={movingEmail}
onClose={() => 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();
}}
/>
)}
<Modal
title={reservedFor ? `Items reserved by ${reservedFor.email}` : 'Reserved items'}
open={reservedFor !== null}
+48
View File
@@ -43,6 +43,22 @@ export interface CustomerDetail {
orders: CustomerOrder[];
}
/** One recorded admin-initiated address change (#337). */
export interface CustomerEmailChange {
id: number;
previous_email: string;
new_email: string;
reason: string;
changed_at: string;
}
export interface EmailChangeResult {
customer: CustomerDetail['customer'];
previousEmail: string;
/** Removed by the change, and worth telling the customer about. */
passkeysRemoved: number;
}
export async function fetchCustomers(): Promise<CustomerSummary[]> {
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<EmailChangeResult> {
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<CustomerEmailChange[]> {
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<void> {
const res = await fetch(`/api/admin/customers/${customerId}/${disabled ? 'disable' : 'enable'}`, {
method: 'POST'