diff --git a/backend/src/emailTemplates.ts b/backend/src/emailTemplates.ts index 136a44f..60e1f12 100644 --- a/backend/src/emailTemplates.ts +++ b/backend/src/emailTemplates.ts @@ -16,7 +16,8 @@ export type TemplateKey = | 'passwordReset' | 'favoriteSold' | 'favoriteWithdrawn' - | 'cartReminder'; + | 'cartReminder' + | 'emailChanged'; export interface TemplateDefinition { /** Shown in the admin so a card is identifiable without reading its body. */ @@ -93,6 +94,23 @@ export const TEMPLATES: Record = { footer: FAVORITE_CONSENT_FOOTER }, + emailChanged: { + label: 'Email address changed', + // Naming the new address is the point: a notice that does not say what + // the address was changed *to* is nearly useless to someone checking + // whether it was them. This is the mail that catches an account + // takeover, so it goes to the address being replaced. + required: ['newEmail'], + available: ['greeting', 'newEmail'], + defaultSubject: 'Your Redefined Designs email address was changed', + defaultBody: + '{{greeting}}\n\n' + + 'The email address on your account was changed to **{{newEmail}}**.\n\n' + + 'If you made this change, nothing more is needed. This message is only a\n' + + 'record of it.\n\n' + + 'If you did not, contact us straight away: whoever made the change can now\n' + + 'receive password reset links for your account.' + }, cartReminder: { label: 'Cart reminder', required: ['itemList', 'cartUrl'], diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index d27f9e4..c92fd39 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -307,9 +307,20 @@ router.get('/me', requireCustomer, asyncRoute(async (req: Request, res: Response // surface without a caller rather than a path in use. router.put('/me', requireCustomer, asyncRoute(async (req: Request, res: Response) => { const { firstName, lastName } = req.body; + // Registration demands both and refuses each by name. Accepting empty values + // here would let a customer clear fields they could not have skipped when + // signing up, which is the same rule disagreeing with itself. + const first = String(firstName ?? '').trim(); + const last = String(lastName ?? '').trim(); + if (!first) { + return res.status(400).json({ error: 'first name is required' }); + } + if (!last) { + return res.status(400).json({ error: 'last name is required' }); + } const { rows } = await pool.query( `UPDATE customers SET first_name = $1, last_name = $2 WHERE id = $3 RETURNING *`, - [String(firstName ?? '').trim() || null, String(lastName ?? '').trim() || null, req.customerId] + [first, last, req.customerId] ); res.json(publicCustomer(rows[0])); })); @@ -326,9 +337,89 @@ router.post('/change-password', requireCustomer, asyncRoute(async (req: Request, } const newHash = await bcrypt.hash(newPassword, 12); await pool.query(`UPDATE customers SET password_hash = $1 WHERE id = $2`, [newHash, req.customerId]); + + // Password reset already ends every session, on the reasoning that a password + // is changed precisely when the old one may be known to someone else. A + // change left the other sessions alive, which is the same reasoning reaching + // the opposite conclusion for no recorded reason. The current session is + // spared so the change does not eject the person making it. + await pool.query( + `DELETE FROM customer_sessions WHERE customer_id = $1 AND token <> $2`, + [req.customerId, req.cookies?.rd_session ?? ''] + ); + res.status(204).end(); })); +// Changing the address a password reset goes to is how an account is taken +// over, so this asks for the current password exactly as change-password does. +// A live session alone is not enough. +router.put('/me/email', requireCustomer, asyncRoute(async (req: Request, res: Response) => { + const { currentPassword, email } = req.body; + + const normalized = String(email ?? '').toLowerCase().trim(); + if (!normalized || !isValidEmail(normalized)) { + return res.status(400).json({ error: 'a valid email is required' }); + } + + const { rows } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); + const customer = rows[0]; + + if (!(await bcrypt.compare(String(currentPassword ?? ''), customer.password_hash))) { + return res.status(401).json({ error: 'current password is incorrect' }); + } + + if (normalized === customer.email) { + return res.status(400).json({ error: 'that is already your 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: 'an account with this email already exists' }); + } + + // Captured before the update, because it is where the notice has to go. + const previousEmail = customer.email; + + await pool.query( + `UPDATE customers SET email = $1, email_verified = false WHERE id = $2`, + [normalized, req.customerId] + ); + + // Supersede any outstanding link, so one already sitting in the old inbox + // cannot be used to verify the new address. + await pool.query( + `DELETE FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, + [req.customerId] + ); + const verifyToken = crypto.randomBytes(24).toString('hex'); + await pool.query( + `INSERT INTO customer_tokens (token, customer_id, kind, expires_at) VALUES ($1, $2, 'verify_email', $3)`, + [verifyToken, req.customerId, new Date(Date.now() + 24 * 60 * 60 * 1000)] + ); + + // Both sends happen after the row is written, never before — the same rule + // favoriteAlerts follows, so a change that failed cannot produce mail saying + // it succeeded. + const verifyUrl = process.env.PUBLIC_URL + '/verify-email?token=' + verifyToken; + const verify = renderTemplate('verification', await loadStoredTemplate('verification'), { + greeting: greeting(customer.first_name), + verifyUrl + }); + sendMail(normalized, verify.subject, verify.html) + .catch(err => console.error('verify email send failed', err)); + + const notice = renderTemplate('emailChanged', await loadStoredTemplate('emailChanged'), { + greeting: greeting(customer.first_name), + newEmail: normalized + }); + sendMail(previousEmail, notice.subject, notice.html) + .catch(err => console.error('email change notice send failed', err)); + + const { rows: updated } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); + res.json(publicCustomer(updated[0])); +})); + router.post('/me/consent', requireCustomer, asyncRoute(async (req: Request, res: Response) => { const consent = !!req.body.marketingConsent; await pool.query( diff --git a/backend/tests/integration/accountDetails.integration.test.ts b/backend/tests/integration/accountDetails.integration.test.ts new file mode 100644 index 0000000..d464972 --- /dev/null +++ b/backend/tests/integration/accountDetails.integration.test.ts @@ -0,0 +1,199 @@ +import request from 'supertest'; +import app from '../../src/app'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +jest.mock('../../src/mailer', () => ({ + sendMail: jest.fn().mockResolvedValue(undefined) +})); +import { sendMail } from '../../src/mailer'; +const sentMail = sendMail as jest.MockedFunction; + +const PASSWORD = 'supersecret123'; + +beforeEach(async () => { + await resetDb(); + sentMail.mockClear(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +async function register(email: string) { + const agent = request.agent(app); + const res = await agent + .post('/api/customers/register') + .send({ email, password: PASSWORD, firstName: 'Thom', lastName: 'Lamb' }); + expect(res.status).toBe(200); + sentMail.mockClear(); + return agent; +} + +const recipients = () => sentMail.mock.calls.map(call => String(call[0])); + +describe('editing your own name', () => { + it('stores both parts', async () => { + const agent = await register('namer@example.com'); + + const res = await agent.put('/api/customers/me').send({ firstName: ' Ada ', lastName: ' Lovelace ' }); + + expect(res.status).toBe(200); + expect(res.body.first_name).toBe('Ada'); + expect(res.body.last_name).toBe('Lovelace'); + }); + + // Registration refuses these individually. Accepting them here would let a + // customer clear fields they could not have skipped when signing up. + it.each([ + [{ firstName: '', lastName: 'Lamb' }, 'first name is required'], + [{ firstName: 'Thom', lastName: ' ' }, 'last name is required'] + ])('refuses %p', async (body, expected) => { + const agent = await register(`blank${Math.random().toString(36).slice(2, 8)}@example.com`); + + const res = await agent.put('/api/customers/me').send(body); + + expect(res.status).toBe(400); + expect(res.body.error).toBe(expected); + }); + + it('refuses an unauthenticated caller', async () => { + const res = await request(app).put('/api/customers/me').send({ firstName: 'A', lastName: 'B' }); + expect(res.status).toBe(401); + }); +}); + +describe('changing your own password', () => { + it('refuses without the current password', async () => { + const agent = await register('pw1@example.com'); + + const res = await agent + .post('/api/customers/change-password') + .send({ currentPassword: 'wrong-password', newPassword: 'brandnewpass1' }); + + expect(res.status).toBe(401); + }); + + // The reason for ending sessions at all: a password is changed precisely when + // the old one may be known to someone else, and a session opened with it + // would otherwise outlive the change. + it('ends other sessions but keeps the one making the change', async () => { + const email = 'pw2@example.com'; + const here = await register(email); + + // A second signed-in device. + const elsewhere = request.agent(app); + expect((await elsewhere.post('/api/customers/login').send({ email, password: PASSWORD })).status).toBe(200); + expect((await elsewhere.get('/api/customers/me')).status).toBe(200); + + const res = await here + .post('/api/customers/change-password') + .send({ currentPassword: PASSWORD, newPassword: 'brandnewpass1' }); + expect(res.status).toBe(204); + + // The other device is signed out; this one is not. Asserted on the status, + // because /me answers an unauthenticated caller with 401 and an error body + // rather than an empty one — the frontend is what turns that into null. + expect((await elsewhere.get('/api/customers/me')).status).toBe(401); + const stillHere = await here.get('/api/customers/me'); + expect(stillHere.status).toBe(200); + expect(stillHere.body.email).toBe(email); + }); + + it('leaves the new password working and the old one not', async () => { + const email = 'pw3@example.com'; + const agent = await register(email); + await agent.post('/api/customers/change-password').send({ + currentPassword: PASSWORD, + newPassword: 'brandnewpass1' + }); + + const stale = request.agent(app); + expect((await stale.post('/api/customers/login').send({ email, password: PASSWORD })).status).toBe(401); + expect((await stale.post('/api/customers/login').send({ email, password: 'brandnewpass1' })).status).toBe(200); + }); +}); + +describe('changing your own email address', () => { + it('requires the current password, because this is how an account is taken over', async () => { + const agent = await register('mail1@example.com'); + + const res = await agent + .put('/api/customers/me/email') + .send({ currentPassword: 'not-it', email: 'attacker@example.com' }); + + expect(res.status).toBe(401); + const { rows } = await pool.query(`SELECT email FROM customers WHERE email = $1`, ['mail1@example.com']); + expect(rows).toHaveLength(1); + expect(sentMail).not.toHaveBeenCalled(); + }); + + it('refuses an address that is not valid', async () => { + const agent = await register('mail2@example.com'); + + const res = await agent + .put('/api/customers/me/email') + .send({ currentPassword: PASSWORD, email: 'not-an-address' }); + + expect(res.status).toBe(400); + }); + + it('refuses an address another account already holds', async () => { + await register('taken@example.com'); + const agent = await register('mail3@example.com'); + + const res = await agent + .put('/api/customers/me/email') + .send({ currentPassword: PASSWORD, email: 'taken@example.com' }); + + expect(res.status).toBe(409); + }); + + it('changes the address, marks it unverified, and mints a fresh token', async () => { + const agent = await register('mail4@example.com'); + + const res = await agent + .put('/api/customers/me/email') + .send({ currentPassword: PASSWORD, email: ' NewAddress@Example.com ' }); + + expect(res.status).toBe(200); + expect(res.body.email).toBe('newaddress@example.com'); + expect(res.body.email_verified).toBe(false); + + const { rows } = await pool.query( + `SELECT COUNT(*)::int AS n FROM customer_tokens t + JOIN customers c ON c.id = t.customer_id + WHERE c.email = $1 AND t.kind = 'verify_email'`, + ['newaddress@example.com'] + ); + expect(rows[0].n).toBe(1); + }); + + // Both messages matter, and they go to different places: verification to the + // new address, and the warning to the one being replaced — which is the only + // thing that tells a real owner their account was taken. + it('mails the new address to verify it and the old one to warn it', async () => { + const agent = await register('old@example.com'); + + await agent.put('/api/customers/me/email').send({ + currentPassword: PASSWORD, + email: 'new@example.com' + }); + + expect(recipients().sort()).toEqual(['new@example.com', 'old@example.com']); + + const notice = sentMail.mock.calls.find(call => String(call[0]) === 'old@example.com'); + expect(String(notice?.[2])).toContain('new@example.com'); + }); + + it('refuses changing to the address already held', async () => { + const agent = await register('same@example.com'); + + const res = await agent + .put('/api/customers/me/email') + .send({ currentPassword: PASSWORD, email: 'same@example.com' }); + + expect(res.status).toBe(400); + }); +}); diff --git a/backend/tests/integration/emailTemplates.integration.test.ts b/backend/tests/integration/emailTemplates.integration.test.ts index 02e6351..d4738c8 100644 --- a/backend/tests/integration/emailTemplates.integration.test.ts +++ b/backend/tests/integration/emailTemplates.integration.test.ts @@ -25,6 +25,7 @@ describe('GET /api/admin/email-templates', () => { expect(res.status).toBe(200); expect(res.body.map((t: { key: string }) => t.key).sort()).toEqual([ 'cartReminder', + 'emailChanged', 'favoriteSold', 'favoriteWithdrawn', 'passwordReset', diff --git a/backend/tests/unit/emailTemplates.test.ts b/backend/tests/unit/emailTemplates.test.ts index 92b231f..92d5735 100644 --- a/backend/tests/unit/emailTemplates.test.ts +++ b/backend/tests/unit/emailTemplates.test.ts @@ -10,7 +10,8 @@ const KEYS: TemplateKey[] = [ 'passwordReset', 'favoriteSold', 'favoriteWithdrawn', - 'cartReminder' + 'cartReminder', + 'emailChanged' ]; describe('the built-in templates', () => {