diff --git a/backend/src/routes/customers.ts b/backend/src/routes/customers.ts index 90f424d..ff3609a 100755 --- a/backend/src/routes/customers.ts +++ b/backend/src/routes/customers.ts @@ -181,6 +181,13 @@ function publicCustomer(c: CustomerRecord) { // neither, and the UI has to be able to show that honestly. analytics_consent: analyticsConsent(c), favorite_alerts: c.favorite_alerts, + // Whether, not what (#344). A customer who signed up with Google has none, + // and the account page has to be able to say so — offering "change your + // password" to somebody who has never had one is a dead end, and saying + // nothing leaves them unable to see a credential they are entitled to + // manage. A boolean is the whole of what the UI needs, and the hash itself + // must never leave this function. + has_password: c.password_hash !== null, created_at: c.created_at }; } @@ -519,9 +526,26 @@ router.post('/change-password', requireCustomer, asyncRoute(async (req: Request, } const { rows } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); const customer = requireRow(rows, 'the signed-in customer'); - if (!(await passwordMatches(currentPassword, customer.password_hash))) { - return res.status(401).json({ error: 'current password is incorrect' }); + + // Setting the first password and changing an existing one, in one route + // rather than two (#344). + // + // A customer who signed up with Google has no password, so there is nothing + // to compare against and asking for one would be a dead end — they cannot + // supply a value that was never set. What authorises the change is the + // session they are already holding, which is the same thing that authorises + // every other setting on the account page. + // + // One route because two would be two places to get the guard wrong, and the + // one that would be forgotten is whichever is not on the path exercised by + // hand. The branch is on the stored hash rather than on anything the caller + // sends, so a request cannot talk its way into the first-password case. + if (customer.password_hash !== null) { + if (!(await passwordMatches(currentPassword, customer.password_hash))) { + return res.status(401).json({ error: 'current password is incorrect' }); + } } + const newHash = await bcrypt.hash(newPassword, PASSWORD_HASH_ROUNDS); await pool.query(`UPDATE customers SET password_hash = $1 WHERE id = $2`, [newHash, req.customerId]); @@ -552,6 +576,24 @@ router.put('/me/email', requireCustomer, asyncRoute(async (req: Request, res: Re const { rows } = await pool.query(`SELECT * FROM customers WHERE id = $1`, [req.customerId]); const customer = requireRow(rows, 'the signed-in customer'); + // A customer with no password is refused here rather than waved through, and + // the asymmetry with change-password above is deliberate (#344). + // + // Setting a first password is a change to a credential the customer already + // controls. Changing the email address is a change to *where recovery goes* — + // whoever holds the new address can reset the password and own the account + // outright. That is why this route has always demanded more than a live + // session, and dropping the demand for the accounts that cannot meet it would + // remove the protection from exactly the ones that need it. + // + // So the message says the real thing and gives them the route out, rather + // than claiming a password was wrong when there is no password at all. + if (customer.password_hash === null) { + return res.status(409).json({ + error: 'this account has no password — set one first, then you can change your email address' + }); + } + if (!(await passwordMatches(currentPassword, customer.password_hash))) { return res.status(401).json({ error: 'current password is incorrect' }); } diff --git a/backend/tests/integration/customers.integration.test.ts b/backend/tests/integration/customers.integration.test.ts index e99f3c5..6b98180 100755 --- a/backend/tests/integration/customers.integration.test.ts +++ b/backend/tests/integration/customers.integration.test.ts @@ -82,7 +82,10 @@ describe('POST /api/customers/register', () => { expect(Object.keys(res.body).sort()).toEqual([ 'analytics_consent', 'created_at', 'email', 'email_verified', 'favorite_alerts', - 'first_name', 'id', 'last_name', 'marketing_consent' + // Whether, never what. Added in #344 so the account page can offer to set + // a first password rather than to change one that does not exist; the + // hash itself must never appear in this list. + 'first_name', 'has_password', 'id', 'last_name', 'marketing_consent' ]); }); diff --git a/backend/tests/integration/passwordlessAccounts.integration.test.ts b/backend/tests/integration/passwordlessAccounts.integration.test.ts new file mode 100644 index 0000000..4df385f --- /dev/null +++ b/backend/tests/integration/passwordlessAccounts.integration.test.ts @@ -0,0 +1,363 @@ +import request from 'supertest'; +import bcrypt from 'bcryptjs'; +import app from '../../src/app'; +import { pool, requireRow } from '../../src/db'; +import { createSession } from '../../src/customerSession'; +import { PASSWORD_HASH_ROUNDS } from '../../src/passwordHashing'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +const PASSWORD = 'supersecret123'; + +/** + * A customer who signed up with Google: no password at all (#344). + * + * Inserted rather than driven through the OAuth flow, because what these tests + * are about is the state, not how it was reached. The flow that produces it has + * its own suite. + */ +async function passwordlessCustomer(email: string): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO customers (email, password_hash, first_name, last_name, email_verified, unsubscribe_token) + VALUES ($1, NULL, 'Test', 'Customer', true, $2) RETURNING id`, + [email, `unsub-${email}`] + ); + const id = requireRow(rows, 'the passwordless customer').id; + await pool.query( + `INSERT INTO customer_identities (customer_id, provider, provider_sub) VALUES ($1, 'google', $2)`, + [id, `sub-${email}`] + ); + return id; +} + +async function customerWithPassword(email: string): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO customers (email, password_hash, first_name, last_name, email_verified, unsubscribe_token) + VALUES ($1, $2, 'Test', 'Customer', true, $3) RETURNING id`, + [email, await bcrypt.hash(PASSWORD, PASSWORD_HASH_ROUNDS), `unsub-${email}`] + ); + return requireRow(rows, 'the customer with a password').id; +} + +async function sessionFor(customerId: number): Promise { + return `rd_session=${await createSession(customerId)}`; +} + +async function storedHash(customerId: number): Promise { + const { rows } = await pool.query<{ password_hash: string | null }>( + `SELECT password_hash FROM customers WHERE id = $1`, + [customerId] + ); + return requireRow(rows, 'the customer').password_hash; +} + +describe('an account with no password', () => { + describe('setting a first one', () => { + it('takes no current password, because there is none to give', async () => { + const id = await passwordlessCustomer('first@example.com'); + const session = await sessionFor(id); + + const res = await request(app) + .post('/api/customers/change-password') + .set('Cookie', session) + .send({ newPassword: 'a-brand-new-password' }); + + // Asking for a value that was never set is a dead end. The session they + // are already holding is what authorises this, exactly as it authorises + // every other setting on the account page. + expect(res.status).toBe(204); + expect(await storedHash(id)).not.toBeNull(); + }); + + it('lets them sign in with it afterwards', async () => { + const id = await passwordlessCustomer('cansignin@example.com'); + + await request(app) + .post('/api/customers/change-password') + .set('Cookie', await sessionFor(id)) + .send({ newPassword: 'a-brand-new-password' }); + + const login = await request(app) + .post('/api/customers/login') + .send({ email: 'cansignin@example.com', password: 'a-brand-new-password' }); + expect(login.status).toBe(200); + }); + + it('enforces the same minimum length as registration', async () => { + const id = await passwordlessCustomer('short@example.com'); + + const res = await request(app) + .post('/api/customers/change-password') + .set('Cookie', await sessionFor(id)) + .send({ newPassword: 'short' }); + + expect(res.status).toBe(400); + expect(await storedHash(id)).toBeNull(); + }); + + it('still demands the current one from an account that has a password', async () => { + // The branch is on the stored hash, never on what the caller sends, so a + // request cannot talk its way into the first-password case by omitting a + // field. + const id = await customerWithPassword('haspassword@example.com'); + + const res = await request(app) + .post('/api/customers/change-password') + .set('Cookie', await sessionFor(id)) + .send({ newPassword: 'a-brand-new-password' }); + + expect(res.status).toBe(401); + }); + }); + + describe('signing in with a password', () => { + it('is refused exactly as a wrong password is', async () => { + await passwordlessCustomer('oracle@example.com'); + + const res = await request(app) + .post('/api/customers/login') + .send({ email: 'oracle@example.com', password: 'anything-at-all' }); + + // Answering "this account has no password" would turn the login form into + // an oracle for which customers use Google. One refusal for every cause, + // and the account page is where a signed-in customer learns what they + // have. + expect(res.status).toBe(401); + expect(res.body.error).toBe('invalid email or password'); + }); + + it('is refused for a blank password too, rather than matching an absent hash', async () => { + await passwordlessCustomer('blank@example.com'); + + const res = await request(app) + .post('/api/customers/login') + .send({ email: 'blank@example.com', password: '' }); + + // Both sides missing is the combination most tempting to call a match, + // and calling it one would let anyone sign in as any Google-only customer. + expect(res.status).toBe(401); + }); + }); + + describe('changing the email address', () => { + it('is refused, and says why rather than claiming a password was wrong', async () => { + const id = await passwordlessCustomer('moving@example.com'); + + const res = await request(app) + .put('/api/customers/me/email') + .set('Cookie', await sessionFor(id)) + .send({ email: 'somewhere-else@example.com' }); + + // Changing the address is a change to where recovery goes: whoever holds + // the new one can reset the password and own the account outright. That is + // why this route has always demanded more than a live session, and + // dropping the demand for accounts that cannot meet it would remove the + // protection from exactly the ones that need it. + expect(res.status).toBe(409); + expect(res.body.error).toMatch(/no password/); + }); + + it('works once they have set one', async () => { + const id = await passwordlessCustomer('thenmoving@example.com'); + const session = await sessionFor(id); + await request(app) + .post('/api/customers/change-password') + .set('Cookie', session) + .send({ newPassword: 'a-brand-new-password' }); + + const res = await request(app) + .put('/api/customers/me/email') + .set('Cookie', session) + .send({ email: 'moved@example.com', currentPassword: 'a-brand-new-password' }); + + expect(res.status).toBe(200); + }); + }); + + describe('deleting the account', () => { + it('works, because deletion never asked for a password', async () => { + const id = await passwordlessCustomer('deleting@example.com'); + + const res = await request(app).delete('/api/customers/me').set('Cookie', await sessionFor(id)); + + expect(res.status).toBe(204); + const { rows } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customers WHERE id = $1`, + [id] + ); + expect(requireRow(rows, 'a count of customers').n).toBe(0); + }); + }); + + describe('the passkey lockout guard, which becomes reachable here', () => { + async function givePasskey(customerId: number, credentialId: string): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO customer_credentials (customer_id, credential_id, public_key, name) + VALUES ($1, $2, 'not-a-real-key', 'Phone') RETURNING id`, + [customerId, credentialId] + ); + return requireRow(rows, 'the credential just created').id; + } + + it('refuses to remove the last way into an account with no password', async () => { + // Written in #40 against the condition rather than the schema, and + // unreachable until now because password_hash was NOT NULL. This is the + // first test that actually exercises it. + const id = await passwordlessCustomer('lastway@example.com'); + const credentialId = await givePasskey(id, 'only-credential'); + + const res = await request(app) + .delete(`/api/customers/me/passkeys/${credentialId}`) + .set('Cookie', await sessionFor(id)); + + expect(res.status).toBe(409); + expect(res.body.error).toMatch(/only way you can sign in/); + }); + + it('allows it when a second passkey remains', async () => { + const id = await passwordlessCustomer('twokeys@example.com'); + const first = await givePasskey(id, 'credential-one'); + await givePasskey(id, 'credential-two'); + + const res = await request(app) + .delete(`/api/customers/me/passkeys/${first}`) + .set('Cookie', await sessionFor(id)); + + expect(res.status).toBe(204); + }); + + it('allows it once a password has been set', async () => { + const id = await passwordlessCustomer('nowhaspassword@example.com'); + const credentialId = await givePasskey(id, 'credential-with-password'); + const session = await sessionFor(id); + await request(app) + .post('/api/customers/change-password') + .set('Cookie', session) + .send({ newPassword: 'a-brand-new-password' }); + + const res = await request(app) + .delete(`/api/customers/me/passkeys/${credentialId}`) + .set('Cookie', session); + + expect(res.status).toBe(204); + }); + }); + + describe('resetting a password that was never set', () => { + it('gives them one, which is a reasonable answer rather than an error', async () => { + await passwordlessCustomer('resetting@example.com'); + await request(app) + .post('/api/customers/request-password-reset') + .send({ email: 'resetting@example.com' }); + const { rows } = await pool.query<{ token: string }>( + `SELECT t.token FROM customer_tokens t JOIN customers c ON c.id = t.customer_id + WHERE c.email = $1 AND t.kind = 'password_reset'`, + ['resetting@example.com'] + ); + + const res = await request(app) + .post('/api/customers/reset-password') + .send({ token: requireRow(rows, 'the reset token').token, password: 'a-brand-new-password' }); + + // The reset path sets a hash and does not care whether one was there + // before. A customer who reaches for "forgot password" without ever + // having had one gets a working password, which is what they were asking + // for. + expect(res.status).toBe(200); + }); + + it('removes their passkeys, which is worth knowing rather than assuming', async () => { + // #42 made a reset remove every passkey, on the reasoning that recovery + // has to be complete. That still holds here: nothing about this path + // identifies who asked, and a Google-only customer resetting a password + // they never had is not obviously in a better position than one who did. + const id = await passwordlessCustomer('resetkeys@example.com'); + await pool.query( + `INSERT INTO customer_credentials (customer_id, credential_id, public_key, name) + VALUES ($1, 'reset-credential', 'not-a-real-key', 'Phone')`, + [id] + ); + await request(app) + .post('/api/customers/request-password-reset') + .send({ email: 'resetkeys@example.com' }); + const { rows } = await pool.query<{ token: string }>( + `SELECT t.token FROM customer_tokens t JOIN customers c ON c.id = t.customer_id + WHERE c.email = $1 AND t.kind = 'password_reset'`, + ['resetkeys@example.com'] + ); + + const res = await request(app) + .post('/api/customers/reset-password') + .send({ token: requireRow(rows, 'the reset token').token, password: 'a-brand-new-password' }); + + expect(res.body.passkeysRemoved).toBe(1); + }); + + it('leaves the Google identity attached, so they keep both ways in', async () => { + const id = await passwordlessCustomer('keepsgoogle@example.com'); + await request(app) + .post('/api/customers/request-password-reset') + .send({ email: 'keepsgoogle@example.com' }); + const { rows } = await pool.query<{ token: string }>( + `SELECT t.token FROM customer_tokens t JOIN customers c ON c.id = t.customer_id + WHERE c.email = $1 AND t.kind = 'password_reset'`, + ['keepsgoogle@example.com'] + ); + + await request(app) + .post('/api/customers/reset-password') + .send({ token: requireRow(rows, 'the reset token').token, password: 'a-brand-new-password' }); + + // Deliberately not removed alongside the passkeys. A passkey is a + // credential this shop issued and can revoke; a Google identity is one + // Google holds, and severing it would leave the customer unable to use + // the button they signed up with for no gain — whoever completed the + // reset controls the mailbox either way. + const { rows: identities } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_identities WHERE customer_id = $1`, + [id] + ); + expect(requireRow(identities, 'a count of identities').n).toBe(1); + }); + }); + + describe('what the account page is told', () => { + it('reports has_password false for a Google-only customer', async () => { + const id = await passwordlessCustomer('told@example.com'); + + const res = await request(app).get('/api/customers/me').set('Cookie', await sessionFor(id)); + + expect(res.body.has_password).toBe(false); + }); + + it('reports it true once one is set', async () => { + const id = await passwordlessCustomer('nowtrue@example.com'); + const session = await sessionFor(id); + await request(app) + .post('/api/customers/change-password') + .set('Cookie', session) + .send({ newPassword: 'a-brand-new-password' }); + + const res = await request(app).get('/api/customers/me').set('Cookie', session); + + expect(res.body.has_password).toBe(true); + }); + + it('never returns the hash itself', async () => { + const id = await customerWithPassword('nohash@example.com'); + + const res = await request(app).get('/api/customers/me').set('Cookie', await sessionFor(id)); + + expect(res.body.password_hash).toBeUndefined(); + expect(JSON.stringify(res.body)).not.toContain('$2'); + }); + }); +}); diff --git a/frontend/src/customer/AccountDetails.tsx b/frontend/src/customer/AccountDetails.tsx index e08d233..c64d769 100644 --- a/frontend/src/customer/AccountDetails.tsx +++ b/frontend/src/customer/AccountDetails.tsx @@ -30,6 +30,10 @@ export default function AccountDetails({ customer, onChanged }: Props) { const [passwordForm] = Form.useForm(); const [emailForm] = Form.useForm(); + // A customer who signed up with Google has none, which changes the wording, + // the button, and whether a current-password field exists at all (#344). + const hasPassword = customer.has_password; + async function saveName(values: { firstName: string; lastName: string }) { setBusy('name'); setNameError(null); @@ -59,15 +63,21 @@ export default function AccountDetails({ customer, onChanged }: Props) { } } - async function savePassword(values: { currentPassword: string; newPassword: string }) { + async function savePassword(values: { currentPassword?: string; newPassword: string }) { setBusy('password'); setPasswordError(null); try { - await changeMyPassword(values.currentPassword, values.newPassword); - // Nothing to refresh: this session is deliberately the one kept alive. - // Clearing the fields matters more, since they hold both passwords. + await changeMyPassword(values.currentPassword ?? '', values.newPassword); + // Clearing the fields matters more than anything else here, since they + // hold both passwords. Setting a first one does refresh, because + // has_password has just changed and this panel renders from it. passwordForm.resetFields(); - message.success('Password changed. Other devices have been signed out.'); + if (hasPassword) { + message.success('Password changed. Other devices have been signed out.'); + } else { + onChanged(); + message.success('Password set. You can now sign in with it as well as with Google.'); + } } catch (err) { setPasswordError((err as Error).message); } finally { @@ -151,23 +161,33 @@ export default function AccountDetails({ customer, onChanged }: Props) { }, { key: 'password', - label: 'Change your password', + // Named for what it is for this customer. Offering to change a + // password to somebody who signed up with Google and has never had + // one is a dead end (#344). + label: hasPassword ? 'Change your password' : 'Set a password', children: ( <> - Signing in elsewhere will end. You will stay signed in on this device. + {hasPassword + ? 'Signing in elsewhere will end. You will stay signed in on this device.' + : 'You signed up without a password. Setting one gives you a second way in, alongside the accounts listed below.'} {passwordError && ( )}
- - - + {/* Absent, not disabled, for an account that has none. The + server branches on the stored hash rather than on anything + sent, so there is nothing for this field to carry. */} + {hasPassword && ( + + + + )} diff --git a/frontend/src/customer/customerApi.ts b/frontend/src/customer/customerApi.ts index 83c392a..a035648 100755 --- a/frontend/src/customer/customerApi.ts +++ b/frontend/src/customer/customerApi.ts @@ -18,6 +18,14 @@ export interface Customer { */ analytics_consent: boolean; favorite_alerts: boolean; + /** + * Whether this account has a password at all (#344). + * + * False for anyone who signed up with Google. The account page reads it to + * decide between offering to change a password and offering to set a first + * one, which are different things to somebody who has never had one. + */ + has_password: boolean; created_at: string; }