Merge pull request 'feat(auth): groundwork for signing in with Google (#340)' (#346) from feature/340-google-groundwork into main

Reviewed-on: #346

backend/migrations/1788200000000_add-social-identities.js

@@ -0,0 +1,77 @@
+exports.up = (pgm) => {
+  pgm.sql(`
+    -- A sign-in that belongs to somebody else's identity provider (#340).
+    --
+    -- A table rather than columns on customers, because one customer may hold
+    -- more than one: Google today, and Apple if #332 ever decides in its
+    -- favour. Columns would mean a second provider is a migration and a third
+    -- is an embarrassment.
+    --
+    -- Same shape as customer_credentials, and for the same reason: a row that
+    -- links this account to something a third party can vouch for, deleted with
+    -- the customer because an identity that outlived its owner could
+    -- authenticate as a customer who no longer exists.
+    CREATE TABLE IF NOT EXISTS customer_identities (
+      id SERIAL PRIMARY KEY,
+      customer_id INTEGER NOT NULL REFERENCES customers(id) ON DELETE CASCADE,
+
+      -- 'google' today. Not a CHECK constraint: the values come from this
+      -- codebase rather than from a request, and the project has no enum types
+      -- elsewhere.
+      provider TEXT NOT NULL,
+
+      -- The provider's subject claim, and **never the email**.
+      --
+      -- This is the whole security posture of the table in one column. An email
+      -- is a display value that its owner can change and that a provider may
+      -- reassign; a subject is opaque, stable for the life of the account, and
+      -- means nothing outside the provider that issued it. Matching on the
+      -- email would strand a customer who changed theirs and, far worse, hand
+      -- their account to whoever inherited the old address.
+      provider_sub TEXT NOT NULL,
+
+      created_at TIMESTAMPTZ NOT NULL DEFAULT now(),
+
+      -- Null until first used, exactly as on a passkey. It is what tells two
+      -- entries apart on an account page where the names are similar.
+      last_used_at TIMESTAMPTZ,
+
+      -- Unique across the pair, not on the subject alone. Two providers could
+      -- in principle issue the same opaque string and it would mean nothing —
+      -- but the same provider issuing one subject to two accounts here means
+      -- something has gone wrong rather than that two people share an identity.
+      UNIQUE (provider, provider_sub)
+    );
+
+    -- Listing what an account is linked to is the common read, and it is always
+    -- scoped by owner.
+    CREATE INDEX IF NOT EXISTS customer_identities_customer_id_idx
+      ON customer_identities (customer_id);
+
+    -- The change with the widest blast radius in the whole project (#332).
+    --
+    -- A customer who signed up through Google has no password and never will
+    -- unless they ask for one, so the column has to admit that. Making it
+    -- nullable is one line; what it costs is that every read of it is now a
+    -- question rather than a fact, and the three that compare against it with
+    -- bcrypt have been made to ask first.
+    --
+    -- Nothing writes a NULL yet. The first accounts without a password arrive
+    -- with the sign-up path, and this is deliberately landed before them so the
+    -- schema change can be reviewed on its own.
+    ALTER TABLE customers ALTER COLUMN password_hash DROP NOT NULL;
+  `);
+};
+
+exports.down = (pgm) => {
+  pgm.sql(`
+    DROP TABLE IF EXISTS customer_identities;
+
+    -- Deliberately not restored. Re-adding NOT NULL fails outright if any
+    -- passwordless customer exists by then, and a down migration that destroys
+    -- accounts to satisfy a constraint would be far worse than a column that is
+    -- merely more permissive than it needs to be. Reversing this properly means
+    -- deciding what happens to those customers, which is not a schema decision.
+    SELECT 1;
+  `);
+};

backend/src/db-kysely/schema.ts

@@ -102,7 +102,7 @@ export interface Customers {
   marketing_consent: Generated<boolean>;
   marketing_consent_at: Timestamp | null;
   marketing_consent_text: string | null;
-  password_hash: string;
+  password_hash: string | null;
   unsubscribe_token: string;
 }
 
@@ -115,6 +115,15 @@ export interface CustomerEmailChanges {
   reason: string;
 }
 
+export interface CustomerIdentities {
+  created_at: Generated<Timestamp>;
+  customer_id: number;
+  id: Generated<number>;
+  last_used_at: Timestamp | null;
+  provider: string;
+  provider_sub: string;
+}
+
 export interface CustomerSessions {
   created_at: Generated<Timestamp>;
   customer_id: number;
@@ -251,6 +260,7 @@ export interface DB {
   checkouts: Checkouts;
   customer_credentials: CustomerCredentials;
   customer_email_changes: CustomerEmailChanges;
+  customer_identities: CustomerIdentities;
   customer_sessions: CustomerSessions;
   customer_tokens: CustomerTokens;
   customers: Customers;

backend/src/envValidation.ts

@@ -152,6 +152,57 @@ function checkMail(env: NodeJS.ProcessEnv): EnvValidation {
   return { errors, warnings };
 }
 
+/**
+ * Google sign-in is all or nothing (#340).
+ *
+ * Half-configured is the case worth naming. With neither value set the feature
+ * reports itself disabled and the button never appears, which is a legitimate
+ * environment. With one set, the token exchange fails at the moment a customer
+ * presses the button — the worst possible time to discover a typo in a stack
+ * variable.
+ *
+ * An error rather than a warning, matching the SMTP pair above: both refuse to
+ * start rather than serving something that is visibly offered and cannot work.
+ */
+function checkGoogleSignIn(env: NodeJS.ProcessEnv): EnvValidation {
+  const hasId = isPresent(env, 'GOOGLE_CLIENT_ID');
+  const hasSecret = isPresent(env, 'GOOGLE_CLIENT_SECRET');
+
+  if (hasId && !hasSecret) {
+    return {
+      errors: ['GOOGLE_CLIENT_SECRET is required when GOOGLE_CLIENT_ID is set — set both or neither.'],
+      warnings: []
+    };
+  }
+  if (hasSecret && !hasId) {
+    return {
+      errors: ['GOOGLE_CLIENT_ID is required when GOOGLE_CLIENT_SECRET is set — set both or neither.'],
+      warnings: []
+    };
+  }
+  if (!hasId) {
+    return {
+      errors: [],
+      warnings: ['Google sign-in is not configured — the button will not be offered.']
+    };
+  }
+
+  // Only meaningful once the credentials exist, and only a warning: a
+  // deployment with no PUBLIC_URL falls back to localhost, which is right for
+  // local development and wrong everywhere else in a way worth saying out loud.
+  if (!isPresent(env, 'PUBLIC_URL')) {
+    return {
+      errors: [],
+      warnings: [
+        'Google sign-in is configured but PUBLIC_URL is not, so the redirect URI falls back to ' +
+          'localhost. Correct locally; anywhere else, Google will refuse the callback.'
+      ]
+    };
+  }
+
+  return { errors: [], warnings: [] };
+}
+
 function checkAdminGate(env: NodeJS.ProcessEnv): string[] {
   if (isPresent(env, 'ADMIN_GATE_SECRET')) {
     return [];
@@ -227,6 +278,7 @@ function checkUploadsOrigin(env: NodeJS.ProcessEnv): EnvValidation {
 export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation {
   const mail = checkMail(env);
   const uploads = checkUploadsOrigin(env);
+  const google = checkGoogleSignIn(env);
 
   return {
     errors: [
@@ -234,8 +286,16 @@ export function validateEnv(env: NodeJS.ProcessEnv): EnvValidation {
       ...checkDemoMode(env),
       ...checkPayPal(env),
       ...mail.errors,
-      ...uploads.errors
+      ...uploads.errors,
+      ...google.errors
     ],
-    warnings: [...mail.warnings, ...checkAdminGate(env), ...uploads.warnings, ...checkDraftingKey(env), ...checkIntakeActionSecret(env)]
+    warnings: [
+      ...mail.warnings,
+      ...checkAdminGate(env),
+      ...uploads.warnings,
+      ...checkDraftingKey(env),
+      ...checkIntakeActionSecret(env),
+      ...google.warnings
+    ]
   };
 }

backend/src/google/config.ts

@@ -0,0 +1,113 @@
+/**
+ * Who this application is, as far as Google is concerned (#340).
+ *
+ * ## Why the redirect URI is derived rather than written down
+ *
+ * It is registered in two places that must agree exactly: Google's console, and
+ * every authorization request this server sends. Google compares them as
+ * strings — scheme, host, port, path, trailing slash and case all count — and
+ * answers a mismatch with `redirect_uri_mismatch`, which is accurate and tells
+ * you nothing about which half is wrong.
+ *
+ * So it comes from `PUBLIC_URL`, the same value every customer-facing link is
+ * already built from, exactly as the WebAuthn Relying Party ID does (#37). One
+ * source, and it is the one that is already correct in any environment where
+ * mail works.
+ *
+ * ## The consequence worth stating plainly
+ *
+ * Google refuses a redirect URI whose host is not under an **authorized
+ * domain**, and a domain can only be authorized after ownership has been proved
+ * by DNS in Search Console. `localhost` is the sole exemption.
+ *
+ * `qa-redefined-designs.bermudalamb.synology.me` therefore **cannot ever be
+ * used**: Synology owns the registrable domain above it, so there is no record
+ * to add and nothing to prove. This is the same wall #285 hit with Cloudflare.
+ *
+ * | Environment | Redirect URI | Works |
+ * | --- | --- | --- |
+ * | Local | `http://localhost:3000/...` | Yes, by exemption |
+ * | QA on the Synology host | — | **No, and cannot** |
+ * | QA on `qa.redefined-designs.com` | `https://qa.redefined-designs.com/...` | After #313 |
+ * | Production | `https://redefined-designs.com/...` | After #313 |
+ *
+ * So this feature is built and exercised locally, and QA cannot see it until QA
+ * moves onto a subdomain of the real domain. That is a `PUBLIC_URL` change and
+ * one console entry, not a code change — this module follows `PUBLIC_URL`
+ * wherever it points. See #345.
+ */
+
+/** The callback path. One constant, because it appears in two sentences. */
+export const GOOGLE_CALLBACK_PATH = '/api/auth/google/callback';
+
+/**
+ * Local development, where `PUBLIC_URL` is legitimately unset.
+ *
+ * `envValidation` requires `PUBLIC_URL` only when SMTP is configured, so a local
+ * setup that cannot send mail does not have it. Falling back to the backend's
+ * own port rather than refusing keeps that setup working, and `localhost` is
+ * the one host Google will accept without an authorized domain — so the fallback
+ * is also the only value that could possibly work here.
+ */
+const LOCAL_ORIGIN = 'http://localhost:3000';
+
+export interface GoogleConfig {
+  clientId: string;
+  clientSecret: string;
+  /** Absolute, and byte-identical to what is registered in Google's console. */
+  redirectUri: string;
+  /**
+   * Whether to offer Google sign-in at all.
+   *
+   * False when either credential is missing, and the button is then **absent
+   * rather than disabled** — the same choice #41 made for a browser without
+   * WebAuthn. A developer without credentials gets a storefront that works and
+   * simply does not offer the option, rather than one that offers it and fails.
+   */
+  enabled: boolean;
+}
+
+/**
+ * The Google configuration for this environment.
+ *
+ * Takes the environment as an argument so it can be tested without touching
+ * `process.env`, and reads it on each call rather than at import time: the
+ * module would otherwise capture whatever was set when it was first required,
+ * which in tests is whatever the previous suite happened to leave behind.
+ *
+ * Throws on a `PUBLIC_URL` that is set but unparseable, for the reason
+ * `relyingParty` does: that is a deployment already producing broken links in
+ * every email, so failing here is not the first thing to go wrong — it is the
+ * first thing to say so.
+ */
+export function googleConfig(env: NodeJS.ProcessEnv = process.env): GoogleConfig {
+  const clientId = (env.GOOGLE_CLIENT_ID ?? '').trim();
+  const clientSecret = (env.GOOGLE_CLIENT_SECRET ?? '').trim();
+  const publicUrl = (env.PUBLIC_URL ?? '').trim();
+
+  let base = LOCAL_ORIGIN;
+  if (publicUrl !== '') {
+    try {
+      // `origin` normalises away any path, trailing slash or default port,
+      // which is what makes the result stable regardless of how PUBLIC_URL was
+      // written. A trailing slash there would otherwise produce a double slash
+      // here and a mismatch at Google.
+      base = new URL(publicUrl).origin;
+    } catch {
+      throw new Error(
+        `PUBLIC_URL is not a URL (${publicUrl}), so the Google redirect URI cannot be derived ` +
+          'from it. Google compares that value as an exact string, so this is refused rather ' +
+          'than guessed at.'
+      );
+    }
+  }
+
+  return {
+    clientId,
+    clientSecret,
+    redirectUri: `${base}${GOOGLE_CALLBACK_PATH}`,
+    // Both, or neither. One without the other cannot complete a token exchange,
+    // and offering a button that always fails is worse than offering none.
+    enabled: clientId !== '' && clientSecret !== ''
+  };
+}

backend/src/passwordHashing.ts

@@ -1,3 +1,5 @@
+import bcrypt from 'bcryptjs';
+
 /**
  * How expensive a password hash is, and why that differs under test.
  *
@@ -47,3 +49,30 @@ export function hashRoundsFor(nodeEnv: string | undefined): number {
 
 /** Resolved once at import: NODE_ENV does not change while the process runs. */
 export const PASSWORD_HASH_ROUNDS = hashRoundsFor(process.env.NODE_ENV);
+
+/**
+ * Whether a supplied password matches a stored hash that may not exist (#340).
+ *
+ * `customers.password_hash` became nullable when social sign-in arrived, and a
+ * null one is not an edge case to tidy away — it is a customer who signed up
+ * through Google and has never set a password. There is nothing to compare
+ * against, so the answer is no.
+ *
+ * This exists because the alternative is worse than a wrong answer.
+ * `bcrypt.compare` throws `Illegal arguments` on a null hash rather than
+ * returning false, so every call site that forgot the check would answer a
+ * sign-in attempt with a 500 instead of a refusal — and a 500 on the login route
+ * is also an oracle, since it happens for exactly the accounts that have no
+ * password.
+ *
+ * One function rather than a null check repeated at each call site, so the
+ * question is asked the same way in all three places and a fourth cannot forget
+ * to ask it.
+ */
+export async function passwordMatches(
+  supplied: unknown,
+  storedHash: string | null | undefined
+): Promise<boolean> {
+  if (!storedHash) return false;
+  return bcrypt.compare(String(supplied ?? ''), storedHash);
+}

backend/src/routes/customers.ts

@@ -1,6 +1,6 @@
 import { Router, Request, Response } from 'express';
 import bcrypt from 'bcryptjs';
-import { PASSWORD_HASH_ROUNDS } from '../passwordHashing';
+import { PASSWORD_HASH_ROUNDS, passwordMatches } from '../passwordHashing';
 import crypto from 'node:crypto';
 import { pool, requireRow } from '../db';
 import { requireCustomer } from '../middleware/customerAuth';
@@ -57,7 +57,7 @@ interface CustomerRow {
  * Kept in step with the schema by hand; nothing checks this against Postgres.
  */
 interface CustomerRecord extends CustomerRow {
-  password_hash: string;
+  password_hash: string | null;
   disabled_at: Date | null;
   unsubscribe_token: string;
   marketing_consent_at: Date | null;
@@ -418,7 +418,7 @@ router.post('/login', asyncRoute(async (req: Request, res: Response) => {
   const { email, password } = req.body;
   const { rows } = await pool.query<CustomerRecord>(`SELECT * FROM customers WHERE email = $1`, [String(email || '').toLowerCase().trim()]);
   const customer = rows[0];
-  if (!customer || !(await bcrypt.compare(password || '', customer.password_hash))) {
+  if (!customer || !(await passwordMatches(password, customer.password_hash))) {
     return res.status(401).json({ error: 'invalid email or password' });
   }
   // Only after the password checks out, so a wrong password still looks like a
@@ -519,7 +519,7 @@ 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 bcrypt.compare(currentPassword || '', customer.password_hash))) {
+  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);
@@ -552,7 +552,7 @@ 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');
 
-  if (!(await bcrypt.compare(String(currentPassword ?? ''), customer.password_hash))) {
+  if (!(await passwordMatches(currentPassword, customer.password_hash))) {
     return res.status(401).json({ error: 'current password is incorrect' });
   }
 

backend/tests/integration/schemaLoss.integration.test.ts

@@ -42,7 +42,7 @@ describe('when the database loses its schema', () => {
 
     // The count and a few names, not the whole list: the point is that a reader
     // can tell at a glance this is a missing schema rather than a logic bug.
-    await expect(assertSchemaPresent('in a test')).rejects.toThrow(/Missing 21 of 21 tables/);
+    await expect(assertSchemaPresent('in a test')).rejects.toThrow(/Missing 22 of 22 tables/);
     await expect(assertSchemaPresent('in a test')).rejects.toThrow(/items/);
 
     await migrate();

backend/tests/integration/setup/testDb.ts

@@ -39,6 +39,7 @@ const REQUIRED_TABLES = [
   'checkouts',
   'customer_credentials',
   'customer_email_changes',
+  'customer_identities',
   'customer_sessions',
   'customer_tokens',
   'customers',
@@ -154,7 +155,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, customer_email_changes,
+        webauthn_challenges, customer_credentials, customer_email_changes, customer_identities,
         customers, item_tags, item_images, items, tags, categories
       RESTART IDENTITY CASCADE
     `);

backend/tests/unit/envValidation.test.ts

@@ -251,4 +251,44 @@ describe('UPLOADS_BASE_URL', () => {
       expect(warnings.join(' ')).not.toMatch(/INTAKE_ACTION_SECRET/);
     });
   });
+
+  // #340. All or nothing, and half-configured is the case worth catching: the
+  // failure would otherwise arrive when a customer presses the button.
+  describe('Google sign-in credentials', () => {
+    const BOTH = { GOOGLE_CLIENT_ID: 'id', GOOGLE_CLIENT_SECRET: 'shh', PUBLIC_URL: 'https://x.test' };
+
+    it('are not required', () => {
+      expect(validateEnv(MINIMAL).errors).toEqual([]);
+    });
+
+    it('warn when absent, so a missing button has a stated reason', () => {
+      expect(validateEnv(MINIMAL).warnings.join(' ')).toMatch(/Google sign-in is not configured/);
+    });
+
+    it('refuse to start with only the client id', () => {
+      const { errors } = validateEnv(withEnv({ GOOGLE_CLIENT_ID: 'id' }));
+      expect(errors.join(' ')).toMatch(/GOOGLE_CLIENT_SECRET is required/);
+    });
+
+    it('refuse to start with only the client secret', () => {
+      const { errors } = validateEnv(withEnv({ GOOGLE_CLIENT_SECRET: 'shh' }));
+      expect(errors.join(' ')).toMatch(/GOOGLE_CLIENT_ID is required/);
+    });
+
+    it('are accepted when both are set alongside PUBLIC_URL', () => {
+      const { errors, warnings } = validateEnv(withEnv(BOTH));
+      expect(errors).toEqual([]);
+      expect(warnings.join(' ')).not.toMatch(/Google sign-in/);
+    });
+
+    it('warn when configured without PUBLIC_URL, because the callback falls back to localhost', () => {
+      // Correct locally and wrong everywhere else, which is precisely the shape
+      // that needs saying out loud rather than failing at Google later.
+      const { errors, warnings } = validateEnv(
+        withEnv({ GOOGLE_CLIENT_ID: 'id', GOOGLE_CLIENT_SECRET: 'shh' })
+      );
+      expect(errors).toEqual([]);
+      expect(warnings.join(' ')).toMatch(/redirect URI falls back to localhost/);
+    });
+  });
 });

backend/tests/unit/googleConfig.test.ts

@@ -0,0 +1,80 @@
+import { googleConfig, GOOGLE_CALLBACK_PATH } from '../../src/google/config';
+
+const CREDENTIALS = { GOOGLE_CLIENT_ID: 'id.apps.googleusercontent.com', GOOGLE_CLIENT_SECRET: 'shh' };
+
+/**
+ * The redirect URI is compared by Google as an exact string, and a mismatch
+ * answers `redirect_uri_mismatch` — accurate, and silent about which half is
+ * wrong. So these assert the derivation itself rather than that the function
+ * returns something, and most of them are about what must *not* end up in it.
+ */
+describe('googleConfig', () => {
+  it('builds the redirect URI from PUBLIC_URL', () => {
+    const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'https://redefined-designs.com' });
+
+    expect(config.redirectUri).toBe(`https://redefined-designs.com${GOOGLE_CALLBACK_PATH}`);
+  });
+
+  it('normalises a trailing slash away, rather than producing a double slash', () => {
+    // The failure this prevents is worth naming: "https://x.com//api/..." is a
+    // different string from the one registered in the console, and it is an
+    // easy way to write PUBLIC_URL.
+    const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'https://redefined-designs.com/' });
+
+    expect(config.redirectUri).toBe(`https://redefined-designs.com${GOOGLE_CALLBACK_PATH}`);
+    expect(config.redirectUri).not.toContain('//api');
+  });
+
+  it('discards a path on PUBLIC_URL', () => {
+    const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'https://redefined-designs.com/shop' });
+
+    expect(config.redirectUri).toBe(`https://redefined-designs.com${GOOGLE_CALLBACK_PATH}`);
+  });
+
+  it('keeps a non-default port, because the console entry carries one too', () => {
+    const config = googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'http://localhost:5173' });
+
+    expect(config.redirectUri).toBe(`http://localhost:5173${GOOGLE_CALLBACK_PATH}`);
+  });
+
+  it('falls back to localhost when PUBLIC_URL is unset', () => {
+    // Local development legitimately has no PUBLIC_URL: envValidation only
+    // demands it alongside SMTP. localhost is also the one host Google accepts
+    // without an authorized domain, so the fallback is the only value that
+    // could work here anyway.
+    const config = googleConfig({ ...CREDENTIALS });
+
+    expect(config.redirectUri).toBe(`http://localhost:3000${GOOGLE_CALLBACK_PATH}`);
+  });
+
+  it('refuses a PUBLIC_URL that is not a URL', () => {
+    expect(() => googleConfig({ ...CREDENTIALS, PUBLIC_URL: 'redefined-designs.com' })).toThrow(
+      /not a URL/
+    );
+  });
+
+  describe('enabled', () => {
+    it('is true only when both credentials are present', () => {
+      expect(googleConfig(CREDENTIALS).enabled).toBe(true);
+    });
+
+    it('is false when neither is set, which is a legitimate environment', () => {
+      // A developer without credentials gets a storefront that works and does
+      // not offer the button, rather than one that offers it and fails.
+      expect(googleConfig({}).enabled).toBe(false);
+    });
+
+    it.each([
+      ['only the id', { GOOGLE_CLIENT_ID: 'id' }],
+      ['only the secret', { GOOGLE_CLIENT_SECRET: 'shh' }]
+    ])('is false with %s', (_label, env) => {
+      expect(googleConfig(env).enabled).toBe(false);
+    });
+
+    it('treats whitespace as absent', () => {
+      // A stack variable set to an empty string is the same as one never set,
+      // and it is the shape a copied-and-blanked entry takes.
+      expect(googleConfig({ GOOGLE_CLIENT_ID: '  ', GOOGLE_CLIENT_SECRET: 'shh' }).enabled).toBe(false);
+    });
+  });
+});

backend/tests/unit/passwordMatches.test.ts

@@ -0,0 +1,50 @@
+import bcrypt from 'bcryptjs';
+import { passwordMatches, TEST_ROUNDS } from '../../src/passwordHashing';
+
+/**
+ * `customers.password_hash` became nullable in #340, and a null one is not an
+ * edge case to tidy away — it is a customer who signed up through Google and
+ * has never set a password.
+ *
+ * The reason this is a function rather than a null check at each call site is
+ * what the second test asserts: `bcrypt.compare` throws on a null hash instead
+ * of returning false, so a forgotten check answers a sign-in attempt with a 500.
+ * On the login route that is also an oracle, because it happens for exactly the
+ * accounts that have no password.
+ */
+describe('passwordMatches', () => {
+  it('matches a correct password against a real hash', async () => {
+    const hash = await bcrypt.hash('supersecret123', TEST_ROUNDS);
+
+    await expect(passwordMatches('supersecret123', hash)).resolves.toBe(true);
+  });
+
+  it('refuses a wrong password against a real hash', async () => {
+    const hash = await bcrypt.hash('supersecret123', TEST_ROUNDS);
+
+    await expect(passwordMatches('nope', hash)).resolves.toBe(false);
+  });
+
+  it.each([
+    ['null', null],
+    ['undefined', undefined],
+    ['an empty string', '']
+  ])('answers false rather than throwing when the stored hash is %s', async (_label, stored) => {
+    // The whole point. bcrypt.compare throws "Illegal arguments" here.
+    await expect(passwordMatches('anything', stored)).resolves.toBe(false);
+  });
+
+  it('answers false for a missing password rather than throwing', async () => {
+    const hash = await bcrypt.hash('supersecret123', TEST_ROUNDS);
+
+    await expect(passwordMatches(undefined, hash)).resolves.toBe(false);
+    await expect(passwordMatches(null, hash)).resolves.toBe(false);
+  });
+
+  it('does not treat an empty password as matching an absent hash', async () => {
+    // Both sides missing is the combination that would be most tempting to call
+    // a match, and it would let anyone sign in as any social-only customer by
+    // submitting a blank password.
+    await expect(passwordMatches('', null)).resolves.toBe(false);
+  });
+});

docker-compose.prod.yml

@@ -104,6 +104,13 @@
 #   INTAKE_ACTION_SECRET  Optional. Signs the regenerate and discard links in the
 #                         intake notification email (#224). Absent, the email
 #                         still sends and carries no shortcuts.
+#   GOOGLE_CLIENT_ID      Optional, and all-or-nothing with the secret below —
+#   GOOGLE_CLIENT_SECRET  setting one without the other refuses to boot (#340).
+#                         Both unset means Google sign-in is simply not offered.
+#                         The redirect URI is derived from PUBLIC_URL and must
+#                         match what is registered in the Google Auth Platform
+#                         exactly, so changing the domain means updating that
+#                         console too (#313).
 #   REMBG_URL             Optional. The background-removal sidecar, e.g.
 #                         http://rembg-syn:7000. Unset turns the feature off.
 #
@@ -260,6 +267,11 @@ services:
       # review queue without shortcuts. Rotating it revokes every outstanding
       # link, which is how a leaked one is dealt with.
       - INTAKE_ACTION_SECRET=${INTAKE_ACTION_SECRET:-}
+      # Both or neither. envValidation refuses to start on one without the
+      # other, because the failure would otherwise arrive at the moment a
+      # customer presses the button (#340).
+      - GOOGLE_CLIENT_ID=${GOOGLE_CLIENT_ID:-}
+      - GOOGLE_CLIENT_SECRET=${GOOGLE_CLIENT_SECRET:-}
     volumes:
       # Production's own uploads directory. QA writes to
       # /volume1/configs/redefined-designs-qa/uploads; sharing this one would

docker-compose.qa.yml

@@ -195,6 +195,20 @@ services:
       # rotating it revokes every outstanding link, which is the intended way to
       # deal with a leak.
       - INTAKE_ACTION_SECRET=${QA_INTAKE_ACTION_SECRET:-}
+      # Deliberately left empty, and it is not an oversight (#340, #345).
+      #
+      # Google refuses a redirect URI whose host is not under a domain whose
+      # ownership has been proved by DNS, and nobody can prove ownership of
+      # *.bermudalamb.synology.me because Synology owns the registrable domain
+      # above it. Same wall as #285. So QA cannot run Google sign-in at all
+      # while it lives on this hostname, and setting these would only produce a
+      # button that fails at Google.
+      #
+      # It becomes possible when #313 moves QA to qa.redefined-designs.com:
+      # set both here, set PUBLIC_URL to the new host, and add the matching
+      # callback in the Google Auth Platform. No code change either way.
+      - GOOGLE_CLIENT_ID=
+      - GOOGLE_CLIENT_SECRET=
     volumes:
       # Separate uploads directory. Sharing production's would let a QA run
       # write into, and a QA teardown delete, real product images.