diff --git a/backend/src/google/newCustomer.ts b/backend/src/google/newCustomer.ts new file mode 100644 index 0000000..c18708d --- /dev/null +++ b/backend/src/google/newCustomer.ts @@ -0,0 +1,105 @@ +import crypto from 'node:crypto'; +import { pool } from '../db'; +import type { GoogleIdentity } from './oauth'; + +/** + * Creating a customer from a Google identity (#342). + * + * ## What this deliberately does not do + * + * It refuses when the address already belongs to a customer. Joining those two + * accounts is linking, it is the most security-sensitive decision in this + * project, and it belongs to #343 rather than falling out of an INSERT here. + * Refusing is the safe half of that decision and the only one available until + * the policy is written down. + * + * ## Consent, which is the actual problem in this issue + * + * Registration captures two consents and stores their wording verbatim, and + * marketing consent must start unticked (#56). A customer arriving through + * Google has never seen those checkboxes and **cannot have**: the redirect to + * Google happens before anyone knows whether they are new. + * + * So the account is created with both false and no stored wording, which is + * legally correct — nobody has agreed to anything, and nothing is recorded as + * though they had. What makes it honest rather than merely lawful is that the + * customer is then asked, on a step that shows the same two sentences, through + * the same endpoints registration uses. That is what keeps the stored text + * byte-identical, which is the whole point of storing it. + * + * Skipping that step is allowed and leaves both false. A consent nobody gave is + * the correct default and a perfectly fine resting state. + */ + +export type SignUpOutcome = + | { kind: 'created'; customerId: number } + /** The address is already an account's. #343 decides whether to link. */ + | { kind: 'email-taken' }; + +interface IdRow { + id: number; +} + +export async function createCustomerFromGoogle(identity: GoogleIdentity): Promise { + const client = await pool.connect(); + try { + await client.query('BEGIN'); + + const { rows: existing } = await client.query( + `SELECT id FROM customers WHERE email = $1`, + [identity.email] + ); + if (existing.length) { + await client.query('ROLLBACK'); + return { kind: 'email-taken' }; + } + + const { rows } = await client.query( + `INSERT INTO customers (email, password_hash, first_name, last_name, email_verified, unsubscribe_token) + VALUES ($1, NULL, $2, $3, $4, $5) + RETURNING id`, + [ + identity.email, + // Hints rather than requirements. Registration demands both names + // because every email greets by first name, but Google may return + // neither and refusing the sign-in over it would be absurd — the + // greeting already has a fallback for exactly this. + identity.firstName, + identity.lastName, + // Only on Google's word, never assumed. An unverified assertion is + // worth nothing, and the caller sends the usual confirmation email when + // this is false. + identity.emailVerified, + crypto.randomBytes(16).toString('hex') + ] + ); + // The INSERT above has a RETURNING clause, so no row means the statement + // did not do what it says. + const customer = rows[0]; + if (!customer) throw new Error('the customer INSERT returned no row'); + + // In the same transaction, deliberately. A customer row with no identity is + // an account nobody can sign in to and nobody can recover, because it has + // no password either — the worst possible thing to leave behind. + await client.query( + `INSERT INTO customer_identities (customer_id, provider, provider_sub, last_used_at) + VALUES ($1, 'google', $2, now())`, + [customer.id, identity.sub] + ); + + await client.query('COMMIT'); + return { kind: 'created', customerId: customer.id }; + } catch (err) { + await client.query('ROLLBACK'); + // Two sign-ins racing for the same brand-new address. The SELECT above + // cannot see the other transaction's uncommitted row, so the unique index + // is what actually holds — and losing that race means the account now + // exists, which is 'email-taken' rather than an error. + if ((err as { code?: string }).code === '23505') { + return { kind: 'email-taken' }; + } + throw err; + } finally { + client.release(); + } +} diff --git a/backend/src/routes/googleAuth.ts b/backend/src/routes/googleAuth.ts index 0985f76..8f91e11 100644 --- a/backend/src/routes/googleAuth.ts +++ b/backend/src/routes/googleAuth.ts @@ -5,6 +5,9 @@ import { asyncRoute } from '../asyncRoute'; import { signIn } from '../customerSession'; import { googleConfig } from '../google/config'; import { newAttempt, authorizationUrl, exchangeCode, verifiedIdentity } from '../google/oauth'; +import type { GoogleIdentity } from '../google/oauth'; +import { createCustomerFromGoogle } from '../google/newCustomer'; +import { issueVerificationEmail } from '../customerVerification'; import type { AttemptSecrets } from '../google/oauth'; import { googleSignInLimiter } from '../rateLimit'; import { safeReturnTo } from '../google/returnTo'; @@ -18,12 +21,16 @@ const router = Router(); * and mounted at `/api/auth/google`, away from `/api/customers`, because it is * the first route in this application that a third party redirects into. * - * ## What this phase does and does not do + * ## What this does and does not do * - * It signs in a customer whose Google identity is **already linked**. A - * successful sign-in by somebody with no identity row does nothing yet: account - * creation is #342 and the linking policy is #343, and holding them back keeps - * this change about the protocol alone. + * It signs in a customer whose Google identity is already linked, and creates + * an account for one nobody here has seen (#342). + * + * It refuses when the address already belongs to a customer. Joining those two + * accounts is linking, it is the most security-sensitive decision in this + * project, and it belongs to #343 rather than falling out of an INSERT. + * Refusing is the safe half of that decision and the only half available until + * the policy is written down. * * ## The cookie, and why it is the whole security of the callback * @@ -62,6 +69,15 @@ interface IdentityRow { */ const FAILURE_PATH = '/login?auth=google-failed'; +/** + * Where a customer who has just been created lands. + * + * A route rather than a flag on the storefront, so it is a page with an address + * — reachable again, linkable from the account page later, and rendered by the + * same modal-route machinery every other auth screen uses. + */ +const WELCOME_PATH = '/welcome'; + function setAttemptCookie(res: Response, attempt: Attempt): void { res.cookie(ATTEMPT_COOKIE, Buffer.from(JSON.stringify(attempt)).toString('base64url'), { httpOnly: true, @@ -112,6 +128,49 @@ function secretsMatch(a: string, b: string): boolean { return crypto.timingSafeEqual(digest(a), digest(b)); } +/** + * Creates an account for a Google identity nobody here has seen, and signs in. + * + * A named function rather than an inline block for the reason + * `routesAreWrapped.test.ts` cares about, and because the callback is already + * the longest handler in this file. + * + * The return path is deliberately dropped for a brand-new customer, who lands + * on the consent step instead. That step is worth interrupting for: it is the + * only moment the two consent sentences can honestly be shown, because the + * redirect to Google happened before anyone knew this person was new. + * + * Carrying the path through as a query parameter was the alternative, and it + * was rejected. The consent page would then have to redirect somewhere a URL + * told it to, which is the open-redirect question `safeReturnTo` already + * answers on the server — asked a second time, in a second language, on a page + * an attacker can link to directly. One new customer occasionally landing on + * the storefront rather than back at their cart is the cheaper of the two. + */ +async function signUp(res: Response, identity: GoogleIdentity): Promise { + const outcome = await createCustomerFromGoogle(identity); + + if (outcome.kind === 'email-taken') { + // An account already uses this address, and joining them is #343. Refusing + // is the safe half of that decision: linking on an address is exactly the + // takeover path the policy exists to reason about carefully. + console.warn('[google] refused a sign-up: that address already has an account'); + res.redirect(FAILURE_PATH); + return; + } + + // Only when Google did not vouch for the address. When it did, the customer + // has already demonstrated they receive mail there — which is precisely what + // the confirmation email exists to establish — so sending one would ask them + // to do a thing that is done. + if (!identity.emailVerified) { + await issueVerificationEmail(outcome.customerId, identity.email, identity.firstName, identity.lastName); + } + + await signIn(res, outcome.customerId); + res.redirect(WELCOME_PATH); +} + router.get( '/start', googleSignInLimiter, @@ -178,11 +237,11 @@ router.get( ); const linked = rows[0]; - // No identity row means a customer this shop has never seen through Google. - // Creating one is #342 and linking to an existing account is #343; until - // those land there is nothing to do, and doing nothing must not look like a - // protocol failure. - if (!linked) return res.redirect(FAILURE_PATH); + // Nobody this shop has seen through Google before. Either they are new, or + // they already have an account under this address — and joining those two + // is linking, which is #343 and is refused here until its policy is + // written down rather than falling out of an INSERT. + if (!linked) return signUp(res, identity); // Refused here as well as on the password and passkey paths. Enforcing it // on some routes and not others is how a disabled account keeps a way in, @@ -208,6 +267,6 @@ router.get( ); /** Exported for the tests; nothing else needs the cookie's name. */ -export { ATTEMPT_COOKIE, ATTEMPT_TTL_MS, FAILURE_PATH }; +export { ATTEMPT_COOKIE, ATTEMPT_TTL_MS, FAILURE_PATH, WELCOME_PATH }; export default router; diff --git a/backend/tests/integration/googleSignIn.integration.test.ts b/backend/tests/integration/googleSignIn.integration.test.ts index 023d2aa..21855f1 100644 --- a/backend/tests/integration/googleSignIn.integration.test.ts +++ b/backend/tests/integration/googleSignIn.integration.test.ts @@ -335,7 +335,7 @@ describe('GET /api/auth/google/callback', () => { expect(res.headers['set-cookie'] ?? []).not.toContainEqual(expect.stringContaining('rd_session=')); }); - it('does not sign in an unlinked Google account, and creates nothing', async () => { + it('refuses when the address already belongs to an account, and creates nothing', async () => { await createCustomer('unlinked@example.com'); const { cookie, state, nonce } = await startSignIn(); respondWithToken(idToken(claimsFor(nonce, { email: 'unlinked@example.com' }))); @@ -345,8 +345,9 @@ describe('GET /api/auth/google/callback', () => { .query({ code: 'an-auth-code', state }) .set('Cookie', cookie); - // Account creation is #342 and linking is #343. Matching on the address - // here would be the takeover path both of those exist to decide carefully. + // Joining those two accounts is linking, which is #343. Doing it here on + // the strength of a matching address is the takeover path that decision + // exists to reason about carefully. expect(res.headers.location).toBe('/login?auth=google-failed'); const { rows } = await pool.query<{ n: number }>( `SELECT count(*)::int AS n FROM customer_identities` @@ -354,3 +355,185 @@ describe('GET /api/auth/google/callback', () => { expect(requireRow(rows, 'a count of identities').n).toBe(0); }); }); + +/** + * #342. A Google account nobody here has seen becomes a customer. + * + * The consent behaviour is most of what these assert, because it is the part + * that is easy to get quietly wrong: an account created with a consent nobody + * gave, or with wording that does not match what the customer was shown, is + * still an account that works. + */ +describe('signing up with Google', () => { + async function signUpWith(overrides: Record = {}) { + const { cookie, state, nonce } = await startSignIn(); + respondWithToken(idToken(claimsFor(nonce, overrides))); + return request(app) + .get('/api/auth/google/callback') + .query({ code: 'an-auth-code', state }) + .set('Cookie', cookie); + } + + async function customerBy(email: string) { + const { rows } = await pool.query<{ + id: number; + first_name: string | null; + last_name: string | null; + email_verified: boolean; + password_hash: string | null; + marketing_consent: boolean; + marketing_consent_text: string | null; + analytics_consent: boolean; + analytics_consent_text: string | null; + }>(`SELECT * FROM customers WHERE email = $1`, [email]); + return requireRow(rows, `the customer for ${email}`); + } + + async function countOf(table: 'customers' | 'customer_identities'): Promise { + const { rows } = await pool.query<{ n: number }>(`SELECT count(*)::int AS n FROM ${table}`); + return requireRow(rows, `a count of ${table}`).n; + } + + async function verificationTokens(customerId: number): Promise { + const { rows } = await pool.query<{ n: number }>( + `SELECT count(*)::int AS n FROM customer_tokens WHERE customer_id = $1 AND kind = 'verify_email'`, + [customerId] + ); + return requireRow(rows, 'a count of verification tokens').n; + } + + it('creates a customer and an identity, and signs them in', async () => { + const res = await signUpWith({ email: 'brandnew@example.com' }); + + expect(res.status).toBe(302); + const customer = await customerBy('brandnew@example.com'); + const { rows } = await pool.query<{ customer_id: number }>( + `SELECT customer_id FROM customer_identities WHERE provider_sub = $1`, + [SUB] + ); + expect(requireRow(rows, 'the new identity').customer_id).toBe(customer.id); + expect(res.headers['set-cookie'] as unknown as string[]).toContainEqual( + expect.stringContaining('rd_session=') + ); + }); + + it('lands the new customer on the consent step rather than the storefront', async () => { + // The one moment the two consent sentences can honestly be shown: the + // redirect to Google happened before anyone knew this person was new. + const res = await signUpWith({ email: 'consentstep@example.com' }); + + expect(res.headers.location).toBe('/welcome'); + }); + + it('creates the account with no password at all', async () => { + await signUpWith({ email: 'nopassword@example.com' }); + + // The first accounts in this project's history without one. #344 is where + // the routes that assumed otherwise learn to cope. + expect((await customerBy('nopassword@example.com')).password_hash).toBeNull(); + }); + + it('gives both consents as false, with no stored wording', async () => { + await signUpWith({ email: 'noconsent@example.com' }); + const customer = await customerBy('noconsent@example.com'); + + // Nobody agreed to anything, so nothing is recorded as though they had. A + // stored wording against a false consent would be a record of a + // conversation that never happened. + expect(customer.marketing_consent).toBe(false); + expect(customer.analytics_consent).toBe(false); + expect(customer.marketing_consent_text).toBeNull(); + expect(customer.analytics_consent_text).toBeNull(); + }); + + it('takes the names from the Google profile', async () => { + await signUpWith({ email: 'named@example.com' }); + const customer = await customerBy('named@example.com'); + + expect(customer.first_name).toBe('Test'); + expect(customer.last_name).toBe('Customer'); + }); + + it('creates the account anyway when Google sends no names', async () => { + // Registration demands both because every email greets by first name, but + // Google may return neither and refusing over it would be absurd — the + // greeting already has a fallback for exactly this. + const { cookie, state, nonce } = await startSignIn(); + const claims = claimsFor(nonce, { email: 'nameless@example.com' }) as Record; + delete claims.given_name; + delete claims.family_name; + respondWithToken(idToken(claims)); + + await request(app) + .get('/api/auth/google/callback') + .query({ code: 'an-auth-code', state }) + .set('Cookie', cookie); + + const customer = await customerBy('nameless@example.com'); + expect(customer.first_name).toBeNull(); + expect(customer.last_name).toBeNull(); + }); + + describe('the verified address', () => { + it('is marked verified, and sends no confirmation email, when Google vouches', async () => { + await signUpWith({ email: 'vouched@example.com', email_verified: true }); + const customer = await customerBy('vouched@example.com'); + + expect(customer.email_verified).toBe(true); + // The confirmation email exists to prove the customer receives mail at + // the address. Google has just proved exactly that, so sending one would + // ask them to do a thing that is already done. + expect(await verificationTokens(customer.id)).toBe(0); + }); + + it('is unverified, and does send one, when Google does not', async () => { + await signUpWith({ email: 'unvouched@example.com', email_verified: false }); + const customer = await customerBy('unvouched@example.com'); + + // An unverified assertion is worth nothing, so this account goes through + // the ordinary confirmation exactly as a password sign-up would. + expect(customer.email_verified).toBe(false); + expect(await verificationTokens(customer.id)).toBe(1); + }); + }); + + it('reaches the same customer on a second sign-in, not a second account', async () => { + await signUpWith({ email: 'returning@example.com' }); + const first = await customerBy('returning@example.com'); + + const second = await signUpWith({ email: 'returning@example.com' }); + + // Signed in, and back to the storefront rather than the consent step — + // which is shown once, to somebody who has just been created. + expect(second.headers.location).toBe('/'); + expect(await countOf('customers')).toBe(1); + expect((await customerBy('returning@example.com')).id).toBe(first.id); + }); + + it('reaches the same customer even after they change their Google address', async () => { + await signUpWith({ email: 'was@example.com' }); + const original = await customerBy('was@example.com'); + + // Matched on the subject, which is the whole reason that column exists. An + // email match would have created a second account here — and an address + // that had since been reassigned would have handed this one to a stranger. + const second = await signUpWith({ email: 'now@example.com' }); + + expect(second.headers.location).toBe('/'); + expect(await countOf('customers')).toBe(1); + expect((await customerBy('was@example.com')).id).toBe(original.id); + }); + + it('still refuses a disabled account, which a sign-up must not route around', async () => { + const customerId = await createCustomer('blocked@example.com'); + await linkGoogle(customerId, SUB); + await pool.query(`UPDATE customers SET disabled_at = now() WHERE id = $1`, [customerId]); + + const res = await signUpWith({ email: 'blocked@example.com' }); + + // The identity exists, so this takes the sign-in path and is refused + // there. No second account is created as a way around it. + expect(res.headers.location).toBe('/login?auth=google-failed'); + expect(await countOf('customers')).toBe(1); + }); +}); diff --git a/frontend/src/customer/Welcome.tsx b/frontend/src/customer/Welcome.tsx new file mode 100644 index 0000000..6713fa6 --- /dev/null +++ b/frontend/src/customer/Welcome.tsx @@ -0,0 +1,114 @@ +import { useState } from 'react'; +import Modal from 'antd/es/modal'; +import Checkbox from 'antd/es/checkbox'; +import Button from 'antd/es/button'; +import Space from 'antd/es/space'; +import Alert from 'antd/es/alert'; +import Typography from 'antd/es/typography'; +import { updateConsent, updateAnalyticsConsent } from './customerApi'; +import { MARKETING_CONSENT_TEXT, ANALYTICS_CONSENT_TEXT } from './AuthForm'; + +const { Paragraph, Title } = Typography; + +type Props = Readonly<{ onClose: () => void }>; + +/** + * The consent step a customer sees once, right after signing up with Google (#342). + * + * ## Why this screen has to exist + * + * Registration asks for two consents and stores their wording verbatim, and + * marketing consent must start unticked (#56). Somebody who arrived through + * Google has never seen those checkboxes and could not have: the redirect + * happened before anyone knew whether they were new. + * + * Their account is created with both false, which is legally correct — nobody + * agreed to anything and nothing is recorded as though they had. But leaving it + * there would mean a Google sign-up is never asked at all, and a silent no is + * still a decision made on someone else's behalf. + * + * ## Why the wording is imported rather than written here + * + * These two constants are the same strings the server stores against the + * consent. The record is meant to say what the customer actually saw, so a + * second copy of the sentence that drifted by a word would quietly defeat that. + * Three wordings were already in circulation once before this was shared. + * + * ## Why skipping is a real option, not a soft refusal + * + * Consent has to be as easy to withhold as to give. "Not now" leaves both false + * and closes, and nothing is sent. Both can be changed later from the account + * page, which is where a customer who changes their mind will look. + */ +export default function Welcome({ onClose }: Props) { + const [marketing, setMarketing] = useState(false); + const [analytics, setAnalytics] = useState(false); + const [saving, setSaving] = useState(false); + const [error, setError] = useState(null); + + async function save() { + setSaving(true); + setError(null); + try { + // Two calls to two endpoints, which is the point rather than an + // inefficiency: they are separate consents with separate purposes, and + // the server stores the wording for each independently. + await updateConsent(marketing); + await updateAnalyticsConsent(analytics); + onClose(); + } catch (err) { + // The account exists and they are signed in either way, so this is not a + // failure to recover from — only a preference that did not save. + setError(`Those preferences didn't save — ${(err as Error).message}. You can set them on your account page.`); + } finally { + setSaving(false); + } + } + + return ( + + + Your account is ready and you are signed in. Two optional things, and you can change + either of them later on your account page. + + + {error && } + + + setMarketing(e.target.checked)}> + {MARKETING_CONSENT_TEXT} + + {/* Its own checkbox and independently refusable. Someone has to be able + to take the emails and refuse the tracking, or the consent is not + granular and is not valid. Unticked, and never pre-ticked: Quebec's + Law 25 requires profiling to be off until the person switches it on. */} + setAnalytics(e.target.checked)}> + {ANALYTICS_CONSENT_TEXT} + + + + + + {/* As prominent as it needs to be. Withholding consent has to be as + easy as giving it, and a "Not now" hidden in small print is the + pattern that makes a consent invalid. */} + + + + + Leaving both unticked is fine — we will not email you or share what you browse. + + + ); +} diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index 9a5cba3..87e6142 100755 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -13,6 +13,7 @@ import ErrorFallback from './components/ErrorFallback'; import DevThrow from './components/DevThrow'; import Admin from './admin/Admin'; import AuthRouteModal from './customer/AuthRouteModal'; +import Welcome from './customer/Welcome'; import Account from './customer/Account'; import PrivacyPolicy from './customer/PrivacyPolicy'; import Submit from './intake/Submit'; @@ -45,7 +46,7 @@ const STOREFRONT_BACKDROP: Partial = { pathname: '/', search: '', hash // than as a page of their own. Each stays a real, linkable URL — bookmarkable, // refreshable, and closed by the browser's Back button — while never being // somewhere with no way out. -const MODAL_ROUTES = ['/account', '/login', '/register', '/forgot-password', '/reset-password']; +const MODAL_ROUTES = ['/account', '/login', '/register', '/forgot-password', '/reset-password', '/welcome']; // Respects the OS-level "reduce motion" accessibility setting by turning off // antd's transitions. Beyond the accessibility win, animated popups are a @@ -200,6 +201,10 @@ function AppRoutes() { {modalPath === '/forgot-password' && ( goWithinAuth('/login')} /> )} + {/* One-time, right after a Google sign-up (#342). A route rather than + a flag so it has an address and uses the same modal machinery as + every other auth screen. */} + {modalPath === '/welcome' && } {modalPath === '/reset-password' && (