Merge pull request 'feat(auth): the routes that assumed every customer has a password (#344)' (#351) from feature/344-life-without-a-password into main

Reviewed-on: #351

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<CustomerRecord>(`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<CustomerRecord>(`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' });
   }

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'
     ]);
   });
 

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<number> {
+  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<number> {
+  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<string> {
+  return `rd_session=${await createSession(customerId)}`;
+}
+
+async function storedHash(customerId: number): Promise<string | null> {
+  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<number> {
+      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');
+    });
+  });
+});

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: (
               <>
                 <Paragraph type="secondary">
-                  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.'}
                 </Paragraph>
                 {passwordError && (
                   <Alert type="error" showIcon message={passwordError} style={{ marginBottom: 16 }} />
                 )}
                 <Form layout="vertical" form={passwordForm} onFinish={savePassword}>
-                  <Form.Item
-                    name="currentPassword"
-                    label="Current password"
-                    rules={[{ required: true, message: 'Your current password is required' }]}
-                  >
-                    <Input.Password autoComplete="current-password" />
-                  </Form.Item>
+                  {/* 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 && (
+                    <Form.Item
+                      name="currentPassword"
+                      label="Current password"
+                      rules={[{ required: true, message: 'Your current password is required' }]}
+                    >
+                      <Input.Password autoComplete="current-password" />
+                    </Form.Item>
+                  )}
                   <Form.Item
                     name="newPassword"
                     label="New password"
@@ -193,7 +213,7 @@ export default function AccountDetails({ customer, onChanged }: Props) {
                   </Form.Item>
                   <Form.Item style={{ marginBottom: 0 }}>
                     <Button type="primary" htmlType="submit" loading={busy === 'password'}>
-                      Change password
+                      {hasPassword ? 'Change password' : 'Set password'}
                     </Button>
                   </Form.Item>
                 </Form>

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;
 }