10 Commits
Author SHA1 Message Date
bermudalamb 0361ef35d0 Merge pull request 'docs(auth): correct the claim that QA could never run Google sign-in' (#356) from docs/correct-qa-google-claim into main
Linting / lint (push) Successful in 3m13s
SonarQube Analysis / sonarqube (push) Failing after 31m6s
Reviewed-on: #356
2026-09-11 15:16:28 -05:00
synAdminandClaude Opus 5 f5a29127fb docs(auth): correct the claim that QA could never run Google sign-in
Linting / lint (pull_request) Successful in 3m44s
SonarQube Analysis / sonarqube (pull_request) Failing after 28m29s
It can, and it does. Registering the QA callback under Authorized redirect URIs was all it took.

The claim was that qa-redefined-designs.bermudalamb.synology.me could never be registered, because Google requires a redirect URI's host to sit under a domain whose ownership has been proved by DNS, and Synology owns the domain above that one. It was inferred from #285, where Cloudflare's free tier genuinely cannot be applied to that hostname, and asserted with far more confidence than the inference supported. What was actually established is narrower: localhost is exempt from the authorized-domain rules, and a domain listed as an authorized domain has to be verified in Search Console. Whether either applied here was never checked.

It was not a harmless error. On the strength of it, QA testing of this feature was documented as blocked behind #313, the QA compose file hardcoded its credentials to empty rather than reading the stack, #345 recorded it as a constraint, and #332 closed with it written into the summary. A QA deploy was spent on it.

So the correction is left in place rather than the wrong sentences quietly deleted. A document that silently stops saying something teaches nobody why it said it, and this is the second time in this feature that a confident inference about somebody else's platform has cost a day — the first being the assumption that a passing local build said anything about another machine.

What replaces it is the thing that was always true and never written down plainly: every environment sends a redirect URI derived from its own PUBLIC_URL, and each one has to exist verbatim in the console. There is now one table listing all four, including the localhost:5173 entry that local development needs and that Phase 0 originally omitted — the omission that cost an hour of redirect_uri_mismatch before any of this.

The QA compose comment now says which URL to register rather than why it cannot be. The ops document gains the steps QA actually took, in order, with a note on why registering before setting the variables is the order that matters: a button that appears before its callback exists fails at Google, where nothing in the storefront logs explains it.

Verified: backend tsc clean, the QA compose file still parses, and no file in the tree still claims the hostname is unusable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-11 14:48:46 -05:00
bermudalamb c9dccfe4a2 Merge pull request 'chore(qa): read the Google credentials from the stack, like every other secret' (#355) from chore/qa-google-credentials-from-stack into main
Linting / lint (push) Successful in 3m36s
SonarQube Analysis / sonarqube (push) Failing after 30m5s
Reviewed-on: #355
2026-09-11 14:19:20 -05:00
synAdminandClaude Opus 5 903a1d8b76 chore(qa): read the Google credentials from the stack, like every other secret
Linting / lint (pull_request) Successful in 3m28s
SonarQube Analysis / sonarqube (pull_request) Failing after 28m14s
QA_GOOGLE_CLIENT_ID and QA_GOOGLE_CLIENT_SECRET were set on the QA stack and went nowhere, because #340 hardcoded the container's values empty rather than reading anything. The button stayed missing, correctly, but for a reason the file gave no way to discover: every other secret in it is read from a QA_-prefixed stack variable, and these two were the odd ones out.

So they are wired the way the rest of the file works. The deploy that prompted this cost nothing except time, and the next one would have cost the same again.

Wiring them is not the same as enabling them, and the comment now leads with that. **Leave both stack variables unset until QA moves off *.bermudalamb.synology.me.** 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 that one, because Synology owns the registrable domain above it — the same wall #285 hit with Cloudflare. Setting them today produces a button that fails at Google with redirect_uri_mismatch, and there is no console entry that could satisfy it.

Once #313 moves QA to qa.redefined-designs.com it is three steps and no code: set the two variables, point PUBLIC_URL at the new host, and add the matching callback under Clients in the Google Auth Platform.

Production already read its pair from the stack and is unchanged. The QA variable documentation at the top of the file gains an entry, matching the style of the others.

Verified: both files still parse as YAML, both substitutions resolve to the intended stack variables, and the compose environment guard passes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-11 14:17:51 -05:00
bermudalamb 838749df64 Merge pull request 'fix(auth): offer Google on the sign-up tab, not only on Log In (#345)' (#354) from fix/google-button-missing-on-signup into main
Linting / lint (push) Successful in 3m19s
SonarQube Analysis / sonarqube (push) Failing after 32m51s
Reviewed-on: #354
2026-09-11 10:06:54 -05:00
synAdminandClaude Opus 5 3958bda489 fix(auth): offer Google on the sign-up tab, not only on Log In (#345)
The button was rendered inside the Log In tab's form only, so a visitor on Create Account saw no social option at all. Reported from a local run.

The mistake came from following the passkey button too closely. A passkey belongs only on Log In, and correctly so: you cannot register an account with one, since registration requires an account to register it against. Google is the opposite case. Creating an account is precisely what a new customer reaches for it to do, so leaving it off the sign-up tab hid the feature from the people it helps most — and hid it on the tab the modal opens on by default.

The label differs by tab and nothing else does. One endpoint serves both: it signs in a known identity, links a verified address, or creates an account, and the customer neither knows nor cares which will happen. So the wording matches what they came to that tab to do rather than what the server ends up doing. Both spellings are given in Google's identity guidelines alongside the mark.

The two consent checkboxes above it are deliberately not carried across. Google takes the customer off this site entirely, and a tick that survived that round trip would be a consent recorded from a form nobody submitted. They are asked again, with the same wording and through the same endpoints, on the step they land on afterwards — which is what #342 built that step for.

The end-to-end test asserts absence, like the one beside it, because local and QA have no credentials and absence is the behaviour that actually runs there.

Verified: frontend tsc, lint and build clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-11 10:06:54 -05:00
bermudalamb b82b989cc8 Merge pull request 'fix(e2e): match an email template tab on its whole name, not a prefix' (#353) from fix/email-template-tab-name-collision into main
Linting / lint (push) Successful in 3m2s
SonarQube Analysis / sonarqube (push) Failing after 31m8s
Reviewed-on: #353
2026-09-11 10:04:42 -05:00
synAdminandClaude Opus 5 c2ee0b0c5d fix(e2e): match an email template tab on its whole name, not a prefix
SonarQube Analysis / sonarqube (pull_request) Canceled after 0s
Linting / lint (pull_request) Successful in 3m47s
CI failed on a test nobody had touched:

    strict mode violation: getByRole('tab', { name: /Email address changed/ })
    resolved to 2 elements

The locators in AdminEmails built an unanchored regex from the label, so a template whose name merely began with another's matched both. #337 added "Email address changed by the shop" alongside "Email address changed" and broke the assertion above it.

The failure named the assertion rather than the new template, which is what made it worth more than a rename. It is the same shape as the switch locator that silently retargeted in #317: a loose locator that keeps passing until something new is added nearby, and then fails somewhere that says nothing about the cause.

So the fix is the locator rather than the label. One helper now builds a matcher anchored at both ends, allowing only the optional "Customised" suffix a tab carries once its template has been edited, and escaping the label because these are copy and copy acquires brackets and full stops eventually. Both railTab and customisedTab go through it, and callers pass plain strings instead of assembling regexes at each call site.

The new template is added to the list the test walks, which is what it should have had in #337.

Verified the matcher against every real label plus the pairs that collide, including that a customised-only match still rejects an unedited tab. Frontend tsc and lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-11 09:59:05 -05:00
bermudalamb 843c51dd91 Merge pull request 'feat(auth): offer Google sign-in on the login form (#345)' (#352) from feature/345-google-button into main
Linting / lint (push) Successful in 2m49s
SonarQube Analysis / sonarqube (push) Failing after 30m25s
Reviewed-on: #352
2026-09-10 16:47:29 -05:00
synAdminandClaude Opus 5 2c6ac4d2be feat(auth): offer Google sign-in on the login form (#345)
Linting / lint (pull_request) Successful in 3m49s
SonarQube Analysis / sonarqube (pull_request) Failing after 30m35s
The last of the six, and the first a customer can see. The auth form is the single sign-in implementation rendered by both the route modal and the cart prompt, so the button goes in one place and appears in both.

Below the passkey button, which is below the password form. The order is deliberate and it is not about preference: a passkey is already on the device in front of the customer, while Google is a round trip to somebody else's site, and passwords are how every existing customer signs in. Each step down that list asks more of the person using it.

Absent rather than disabled where it is not configured, which is the same call #41 made for a browser without WebAuthn. It matters more here, because being unconfigured is the normal state rather than the exception: local development has no credentials, and QA cannot have any until #313. The storefront advertises a boolean through the existing public config, never the client id — the browser has no use for one, since the whole flow is a redirect the server builds.

Google's mark is inlined as SVG with their published colours and geometry. A hand-drawn approximation of somebody else's trademark is a compliance problem rather than a style choice, and a second origin on the sign-in path is a second thing that can be down.

The button is a navigation rather than a fetch, which makes it unlike every other control on that form. The flow leaves the application entirely, so there is no promise to await and no error to catch — the callback decides and redirects.

Where to return to is supplied by the caller, because only the caller knows. The route modal renders over a backdrop location and its own path is /login, so reading the current URL there would send the customer back to the form they just left; the router builds it from the backdrop instead. The cart prompt uses the page it interrupted. It cannot resume the interrupted action the way onSuccess does — the redirect leaves the app — so the customer lands back on the page and presses the button again.

That value is validated on the server and not in the browser. It has to be, since anyone can type the URL, and doing it in one place beats doing it twice in two languages.

The end-to-end test asserts the button is ABSENT, which is the behaviour local and QA actually have, and then signs in with the password form to show that its absence changes nothing. That is the point of putting the alternatives below rather than above.

docs/ops/google-sign-in.md records what has to be true outside the repository: the seven sections of the Google Auth Platform, the three scopes that keep publishing out of a verification review, the cutover checklist for #313, and the production smoke test. It states plainly that QA on the Synology hostname is impossible rather than merely unconfigured, because Google will not accept a redirect URI whose domain nobody can prove they own — the same wall #285 hit with Cloudflare.

The failure that document warns about hardest is leaving the consent screen in Testing. Only listed test users can then sign in, the refusal happens on Google's own page, and nothing reaches the storefront at all — so a customer reports a broken button and the logs are silent.

Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration and end-to-end suites need a database this machine has no Docker for.

Closes #345

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-10 15:13:18 -05:00
14 changed files with 508 additions and 46 deletions
+11 -1
View File
@@ -20,6 +20,7 @@ import customersRouter from './routes/customers';
import passkeysRouter from './routes/passkeys'; import passkeysRouter from './routes/passkeys';
import passkeyLoginRouter from './routes/passkeyLogin'; import passkeyLoginRouter from './routes/passkeyLogin';
import googleAuthRouter from './routes/googleAuth'; import googleAuthRouter from './routes/googleAuth';
import { googleConfig } from './google/config';
import publicRouter from './routes/public'; import publicRouter from './routes/public';
import cartRouter from './routes/cart'; import cartRouter from './routes/cart';
import shippingAddressesRouter from './routes/shippingAddresses'; import shippingAddressesRouter from './routes/shippingAddresses';
@@ -72,7 +73,16 @@ app.get('/api/config', (_req, res) => {
// keeps QA out of production's Brevo account: QA sets no key, so no QA // keeps QA out of production's Brevo account: QA sets no key, so no QA
// browsing is ever reported, and there is no flag anyone can forget to // browsing is ever reported, and there is no flag anyone can forget to
// turn off. Same shape as paypalClientId above. // turn off. Same shape as paypalClientId above.
brevoTrackerKey: process.env.BREVO_TRACKER_KEY?.trim() || null brevoTrackerKey: process.env.BREVO_TRACKER_KEY?.trim() || null,
// Whether to offer the Google button at all (#345). A boolean, never the
// client id: the browser does not need it, because the whole flow is a
// redirect this server builds.
//
// Absent rather than disabled is the point. A developer with no credentials
// gets a storefront that works and simply does not offer the option, the
// same choice #41 made for a browser without WebAuthn — and QA, which
// cannot have credentials until #313, gets the same.
googleSignIn: googleConfig().enabled
}); });
}); });
+22 -17
View File
@@ -14,27 +14,32 @@
* source, and it is the one that is already correct in any environment where * source, and it is the one that is already correct in any environment where
* mail works. * mail works.
* *
* ## The consequence worth stating plainly * ## Every environment needs its own console entry
* *
* Google refuses a redirect URI whose host is not under an **authorized * Whatever this resolves to has to exist, verbatim, under Authorized redirect
* domain**, and a domain can only be authorized after ownership has been proved * URIs for the client this app uses. Google compares the two as strings, and a
* by DNS in Search Console. `localhost` is the sole exemption. * mismatch is answered with `redirect_uri_mismatch` — accurate, and silent
* about which half is wrong.
* *
* `qa-redefined-designs.bermudalamb.synology.me` therefore **cannot ever be * | Environment | Redirect URI |
* 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. * | Local, Vite | `http://localhost:5173/api/auth/google/callback` |
* | Local, built | `http://localhost:3000/api/auth/google/callback` |
* | QA | `https://qa-redefined-designs.bermudalamb.synology.me/api/auth/google/callback` |
* | Production | `https://redefined-designs.com/api/auth/google/callback` |
* *
* | Environment | Redirect URI | Works | * Local development needs the 5173 one, because that is where the dev server
* | --- | --- | --- | * serves the app; the 3000 one only applies when the backend serves a built
* | Local | `http://localhost:3000/...` | Yes, by exemption | * frontend.
* | 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 * An earlier version of this comment claimed the QA hostname could never be
* moves onto a subdomain of the real domain. That is a `PUBLIC_URL` change and * registered, because it sits under a domain Synology owns. **That was wrong**,
* one console entry, not a code change — this module follows `PUBLIC_URL` * and it is recorded here rather than quietly deleted: it was asserted from the
* wherever it points. See #345. * shape of #285, which is a related but different problem, and it sent QA
* testing of this feature behind #313 for no reason. Adding the URI works.
*
* This module needs no change in any environment. It follows `PUBLIC_URL`
* wherever it points.
*/ */
/** The callback path. One constant, because it appears in two sentences. */ /** The callback path. One constant, because it appears in two sentences. */
@@ -730,3 +730,54 @@ describe('GET /api/customers/me/identities', () => {
expect(res.status).toBe(401); expect(res.status).toBe(401);
}); });
}); });
/**
* Whether the storefront offers a Google button at all (#345).
*
* A boolean and never the client id: the browser does not need one, because
* the whole flow is a redirect the server builds.
*/
describe('GET /api/config, google sign-in', () => {
// The suite-wide beforeEach configures Google so the flow above can run.
// These tests are about the unconfigured case too, so they start from clean.
beforeEach(() => {
delete process.env.GOOGLE_CLIENT_ID;
delete process.env.GOOGLE_CLIENT_SECRET;
});
it('is false when the environment has no credentials', async () => {
const res = await request(app).get('/api/config');
// Which is the state of local development, and of QA until #313 moves it
// off a hostname whose domain nobody can prove they own.
expect(res.body.googleSignIn).toBe(false);
});
it('is true when both credentials are set', async () => {
process.env.GOOGLE_CLIENT_ID = 'id.apps.googleusercontent.com';
process.env.GOOGLE_CLIENT_SECRET = 'shh';
const res = await request(app).get('/api/config');
expect(res.body.googleSignIn).toBe(true);
});
it('is false with only one of the pair, matching what the backend refuses to boot on', async () => {
process.env.GOOGLE_CLIENT_ID = 'id.apps.googleusercontent.com';
const res = await request(app).get('/api/config');
expect(res.body.googleSignIn).toBe(false);
});
it('never sends the client id or secret to the browser', async () => {
process.env.GOOGLE_CLIENT_ID = 'id.apps.googleusercontent.com';
process.env.GOOGLE_CLIENT_SECRET = 'a-real-looking-secret';
const res = await request(app).get('/api/config');
const body = JSON.stringify(res.body);
expect(body).not.toContain('a-real-looking-secret');
expect(body).not.toContain('googleusercontent');
});
});
+26 -12
View File
@@ -63,6 +63,12 @@
# in the notification email (#224). Absent, the email still # in the notification email (#224). Absent, the email still
# sends and simply carries no shortcuts. Its own value, not # sends and simply carries no shortcuts. Its own value, not
# production's: a link signed with it acts without a login. # production's: a link signed with it acts without a login.
# QA_GOOGLE_CLIENT_ID — optional, and all-or-nothing with the secret below:
# QA_GOOGLE_CLIENT_SECRET setting one without the other refuses to boot
# (#340). Both unset means the Google button is not offered
# at all, which is the right answer until QA's callback URL
# is registered in the Google Auth Platform. See the note
# beside the values themselves for the exact URL (#345).
# QA_REMBG_URL — optional. The background-removal sidecar, e.g. # QA_REMBG_URL — optional. The background-removal sidecar, e.g.
# http://rembg-syn:7000. Unset turns the feature off rather # http://rembg-syn:7000. Unset turns the feature off rather
# than breaking anything. The sidecar must be on the same # than breaking anything. The sidecar must be on the same
@@ -195,20 +201,28 @@ services:
# rotating it revokes every outstanding link, which is the intended way to # rotating it revokes every outstanding link, which is the intended way to
# deal with a leak. # deal with a leak.
- INTAKE_ACTION_SECRET=${QA_INTAKE_ACTION_SECRET:-} - INTAKE_ACTION_SECRET=${QA_INTAKE_ACTION_SECRET:-}
# Deliberately left empty, and it is not an oversight (#340, #345). # Read from the stack like every other QA secret, rather than hardcoded
# empty as they were in #340. Leaving them unreadable made this file the
# odd one out and cost a QA deploy: the variables were set on the stack,
# nothing read them, and the button stayed missing with no explanation.
# #
# Google refuses a redirect URI whose host is not under a domain whose # Setting these needs one thing done first: the QA callback registered
# ownership has been proved by DNS, and nobody can prove ownership of # under Authorized redirect URIs for this client in the Google Auth
# *.bermudalamb.synology.me because Synology owns the registrable domain # Platform, exactly as it appears below. Google compares the two as
# above it. Same wall as #285. So QA cannot run Google sign-in at all # strings and answers a mismatch with redirect_uri_mismatch.
# 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: # https://qa-redefined-designs.bermudalamb.synology.me/api/auth/google/callback
# 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. # An earlier version of this comment said that URI could never be
- GOOGLE_CLIENT_ID= # registered, because Synology owns the domain above it. That was wrong,
- GOOGLE_CLIENT_SECRET= # and the correction is left here rather than removed: it was inferred
# from #285, which is a related but different problem, and it put QA
# testing of this feature behind #313 for no reason.
#
# When #313 moves QA to qa.redefined-designs.com, point PUBLIC_URL at the
# new host and register that callback too. No code change either way.
- GOOGLE_CLIENT_ID=${QA_GOOGLE_CLIENT_ID:-}
- GOOGLE_CLIENT_SECRET=${QA_GOOGLE_CLIENT_SECRET:-}
volumes: volumes:
# Separate uploads directory. Sharing production's would let a QA run # Separate uploads directory. Sharing production's would let a QA run
# write into, and a QA teardown delete, real product images. # write into, and a QA teardown delete, real product images.
+130
View File
@@ -0,0 +1,130 @@
# Google sign-in
What has to be true outside the repository for the Google button to work, and
what to do at the domain cutover. The code side is #332 and the six issues under
it; this is only the parts that live in a browser tab at Google.
## Where it is configured
The **Google Auth Platform** in the Google Cloud Console, in one project. There
is one consent screen per project and every OAuth client in it shares that
screen, so what appears there is the production identity even while testing.
| Section | What it holds |
| --- | --- |
| Branding | App name, support email, authorized domains, the three app links |
| Audience | External, publishing status, test users |
| Clients | The OAuth client, its redirect URIs, the id and secret |
| Data Access | Exactly `openid`, `email`, `profile` |
| Verification Center | Nothing to submit, and it should stay that way |
## Redirect URIs, one per environment
Every environment sends a redirect URI derived from its own `PUBLIC_URL`, and
each one has to exist verbatim under **Authorized redirect URIs** on the client
this app uses. Google compares them as strings and answers a mismatch with
`redirect_uri_mismatch`, which is accurate and says nothing about which half is
wrong.
| Environment | Redirect URI |
| --- | --- |
| Local, Vite dev server | `http://localhost:5173/api/auth/google/callback` |
| Local, backend serving a build | `http://localhost:3000/api/auth/google/callback` |
| QA | `https://qa-redefined-designs.bermudalamb.synology.me/api/auth/google/callback` |
| Production | `https://redefined-designs.com/api/auth/google/callback` |
Local development needs the 5173 entry, because that is where the dev server
serves the app. The 3000 one applies only when the backend serves a built
frontend, which local development does not produce.
### A correction
An earlier version of this document said the QA hostname **could never be
registered**, because it sits under a domain Synology owns rather than one we
do. That was wrong. Adding the URI works.
The claim is recorded here rather than quietly removed, because of what it
cost. It was inferred from #285, where Cloudflare genuinely cannot be applied to
that hostname, and asserted with far more confidence than the inference
supported. On the strength of it, QA testing of Google sign-in was documented as
blocked behind #313, the QA compose file hardcoded its credentials to empty, and
two issues recorded it as fact.
What is true, and is all that was ever established: `localhost` is exempt from
the authorized-domain rules, and a domain listed as an authorized domain has to
be verified in Search Console. Whether either of those actually applied to this
hostname, and how, was never checked.
## Scopes, and why publishing needs no review
`openid` produces the id token carrying the subject claim, which is the identity
stored. `email` carries the address and the `email_verified` flag the linking
policy turns on. `profile` carries the names used when an account is created.
All three are non-sensitive. Requesting only them is what lets the app publish
without verification and without customers seeing an unverified-app warning.
**Add one sensitive scope and publishing becomes a review with a video
walkthrough and a wait measured in weeks.** Nothing in this feature needs one.
Uploading an app logo also triggers a brand review, which is why Branding has
none.
## Turning it on in QA
Already done, and recorded here because the order matters.
1. Register the QA callback under **Clients**, Authorized redirect URIs:
`https://qa-redefined-designs.bermudalamb.synology.me/api/auth/google/callback`
2. Set `QA_GOOGLE_CLIENT_ID` and `QA_GOOGLE_CLIENT_SECRET` on the QA stack. Both
or neither — the backend refuses to start on one without the other, because
the failure would otherwise arrive the moment a customer presses the button.
3. Redeploy.
Registering first is the point. Setting the variables makes the button appear,
and a button that appears before its callback exists fails at Google rather than
in the storefront, where nothing in the logs explains it.
## The cutover checklist, for #313
1. Point QA at `qa.redefined-designs.com` and set its `PUBLIC_URL` to match.
2. In **Clients**, add the new QA callback:
`https://qa.redefined-designs.com/api/auth/google/callback`
3. Confirm the production callback is registered:
`https://redefined-designs.com/api/auth/google/callback`
4. In **Audience**, move the publishing status from Testing to **In production**.
Do it once the domain resolves, so the home page and privacy links Google
shows actually answer.
The old QA callback can be left registered until the hostname is retired. An
extra entry costs nothing and removing it early breaks QA for no gain.
No code changes at any step. The redirect URI is derived from `PUBLIC_URL`, so
the environment variable and the console entry are the whole of it.
**Leaving it in Testing is the failure to watch for.** Only listed test users can
sign in, and the refusal happens on Google's own page, so nothing reaches the
storefront and nothing appears in its logs. A customer reports a broken button
and the logs are silent.
## The production smoke test
The consent screen, the redirect and the domain are all environment-specific, so
QA proves the flow and not the configuration. After the cutover:
1. Sign in with a Google account that has never been used on the site. A new
customer is created and lands on the consent step.
2. Sign in again with the same account. It reaches the same customer rather than
a second one.
3. Check the account page lists Google under connected accounts.
## What is not offered, and why
**Unlinking.** A customer cannot detach their Google account. Removing the only
way into an account is guarded for passkeys and the same guard would be needed
here first. Worth its own issue when somebody actually asks.
**Apple.** A separate decision with a materially different cost, set out on
#332: a paid developer programme, a client secret that expires every six months,
no `localhost` redirect URIs at all, and a name and email returned exactly once.
Apple is required for iOS apps offering third-party sign-in, and this is a
website, so that rule does not apply here.
+8
View File
@@ -61,6 +61,14 @@ export interface SiteConfig {
* A key alone does not start tracking — see brevo.ts. * A key alone does not start tracking — see brevo.ts.
*/ */
brevoTrackerKey: string | null; brevoTrackerKey: string | null;
/**
* Whether Google sign-in is configured in this environment (#345).
*
* False locally without credentials, and false in QA until #313 moves it off
* the Synology hostname — Google refuses a redirect URI whose domain nobody
* can prove they own. The button is then absent rather than disabled.
*/
googleSignIn: boolean;
} }
export async function fetchConfig(): Promise<SiteConfig> { export async function fetchConfig(): Promise<SiteConfig> {
+64 -5
View File
@@ -1,4 +1,4 @@
import { useState } from 'react'; import { useEffect, useState } from 'react';
import { useSearchParams } from 'react-router-dom'; import { useSearchParams } from 'react-router-dom';
import Form from 'antd/es/form'; import Form from 'antd/es/form';
import Input from 'antd/es/input'; import Input from 'antd/es/input';
@@ -10,6 +10,8 @@ import Typography from 'antd/es/typography';
import Divider from 'antd/es/divider'; import Divider from 'antd/es/divider';
import { registerCustomer, loginCustomer, signInWithPasskey, passkeysSupported } from './customerApi'; import { registerCustomer, loginCustomer, signInWithPasskey, passkeysSupported } from './customerApi';
import { useCustomerAuth } from './CustomerAuthContext'; import { useCustomerAuth } from './CustomerAuthContext';
import GoogleSignInButton from './GoogleSignInButton';
import { fetchConfig } from '../api';
const { Text } = Typography; const { Text } = Typography;
@@ -42,6 +44,15 @@ type Props = Readonly<{
// route closes back to the page behind it, while the cart and favorite // route closes back to the page behind it, while the cart and favorite
// prompts resume the action the customer was interrupted doing. // prompts resume the action the customer was interrupted doing.
onSuccess: () => void; onSuccess: () => void;
/**
* Where a Google sign-in should return the customer (#345).
*
* Supplied by the caller because only the caller knows: the route modal has a
* page behind it, and the cart prompt has the page it interrupted. An OAuth
* redirect leaves the application entirely, so this cannot be recovered
* afterwards the way onSuccess recovers it for every other path.
*/
returnTo?: string;
}>; }>;
/** /**
@@ -73,7 +84,7 @@ function googleNotice(reason: string | null): string | null {
// written twice — once as the /login and /register pages, once inside the // written twice — once as the /login and /register pages, once inside the
// prompt shown when a signed-out visitor adds to the cart — which had already // prompt shown when a signed-out visitor adds to the cart — which had already
// drifted in consent wording and in which links each offered. // drifted in consent wording and in which links each offered.
export default function AuthForm({ mode, onModeChange, onForgotPassword, onSuccess }: Props) { export default function AuthForm({ mode, onModeChange, onForgotPassword, onSuccess, returnTo = '/' }: Props) {
const [searchParams] = useSearchParams(); const [searchParams] = useSearchParams();
const notice = googleNotice(searchParams.get('auth')); const notice = googleNotice(searchParams.get('auth'));
const [error, setError] = useState<string | null>(null); const [error, setError] = useState<string | null>(null);
@@ -89,6 +100,20 @@ export default function AuthForm({ mode, onModeChange, onForgotPassword, onSucce
// rather than whether pressing it works. // rather than whether pressing it works.
const canUsePasskeys = passkeysSupported(); const canUsePasskeys = passkeysSupported();
// Whether this environment has Google credentials at all. Fetched rather than
// built in, because one image serves every environment — and false is the
// right starting value: a button that appears a moment late is better than
// one that appears and then vanishes.
const [googleEnabled, setGoogleEnabled] = useState(false);
useEffect(() => {
fetchConfig()
.then((config) => setGoogleEnabled(config.googleSignIn))
// Silent, and the button simply never appears. The password form behind
// it works regardless, which is the whole reason it is below rather than
// above.
.catch(() => setGoogleEnabled(false));
}, []);
async function submit(action: () => Promise<unknown>) { async function submit(action: () => Promise<unknown>) {
setLoading(true); setLoading(true);
setError(null); setError(null);
@@ -208,6 +233,27 @@ export default function AuthForm({ mode, onModeChange, onForgotPassword, onSucce
By creating an account you agree to our{' '} By creating an account you agree to our{' '}
<a href="/privacy" target="_blank" rel="noopener noreferrer">Privacy Policy</a>. <a href="/privacy" target="_blank" rel="noopener noreferrer">Privacy Policy</a>.
</Text> </Text>
{/* On this tab too, and its absence here was a bug (#345).
A passkey belongs only on Log In, because you cannot
register an account with one — but creating an account is
exactly what a new customer reaches for Google to do, so
leaving it off the sign-up tab hid the feature from the
people it helps most.
The two consent boxes above are not carried across. Google
takes the customer off this site entirely, and a tick that
survived that round trip would be a consent recorded from a
form nobody submitted. They are asked again, with the same
wording, on the step they land on (#342). */}
{googleEnabled && (
<>
<Divider plain style={{ marginBlock: 16 }}>
<Text type="secondary" style={{ fontSize: 12 }}>or</Text>
</Divider>
<GoogleSignInButton returnTo={returnTo} intent="sign-up" />
</>
)}
</Form> </Form>
) )
}, },
@@ -238,11 +284,13 @@ export default function AuthForm({ mode, onModeChange, onForgotPassword, onSucce
works for everyone. Absent entirely where WebAuthn is not works for everyone. Absent entirely where WebAuthn is not
available, rather than shown disabled: a greyed button available, rather than shown disabled: a greyed button
invites a customer to wonder what they are missing (#41). */} invites a customer to wonder what they are missing (#41). */}
{(canUsePasskeys || googleEnabled) && (
<Divider plain style={{ marginBlock: 16 }}>
<Text type="secondary" style={{ fontSize: 12 }}>or</Text>
</Divider>
)}
{canUsePasskeys && ( {canUsePasskeys && (
<> <>
<Divider plain style={{ marginBlock: 16 }}>
<Text type="secondary" style={{ fontSize: 12 }}>or</Text>
</Divider>
<Button <Button
block block
loading={passkeyLoading} loading={passkeyLoading}
@@ -258,6 +306,17 @@ export default function AuthForm({ mode, onModeChange, onForgotPassword, onSucce
</Text> </Text>
</> </>
)} )}
{/* Below the passkey button, which is below the password form.
The order is deliberate and it is not about preference: a
passkey is already on the device in front of the customer,
while Google is a round trip to somebody else's site. Absent
rather than disabled where it is not configured, for the
same reason as the one above (#345). */}
{googleEnabled && (
<div style={{ marginTop: canUsePasskeys ? 16 : 0 }}>
<GoogleSignInButton returnTo={returnTo} />
</div>
)}
</Form> </Form>
) )
} }
@@ -37,6 +37,12 @@ export default function AuthPromptModal({ open, onClose, onSuccess }: Props) {
onClose(); onClose();
navigate('/forgot-password', { state: { background: location } }); navigate('/forgot-password', { state: { background: location } });
}} }}
// The page the customer was on when this interrupted them, which is
// where a Google round trip should put them back (#345). Unlike
// onSuccess it cannot resume the interrupted action — the redirect
// leaves the application — so it returns them to the page and they
// press the button again.
returnTo={`${location.pathname}${location.search}`}
onSuccess={onSuccess} onSuccess={onSuccess}
/> />
</Modal> </Modal>
+11 -1
View File
@@ -7,6 +7,15 @@ type Props = Readonly<{
// Moving between the auth routes, supplied by the router so the rule about // Moving between the auth routes, supplied by the router so the rule about
// keeping the whole detour to one history entry lives in one place. // keeping the whole detour to one history entry lives in one place.
onNavigate: (path: string) => void; onNavigate: (path: string) => void;
/**
* The page behind this modal, for a Google sign-in to return to (#345).
*
* Supplied by the router, which is the only thing that knows it: this modal
* renders over a backdrop location, and its own path is /login, so reading
* the current URL here would send the customer back to the form they just
* left.
*/
returnTo: string;
}>; }>;
const TITLES: Record<AuthMode, string> = { const TITLES: Record<AuthMode, string> = {
@@ -18,7 +27,7 @@ const TITLES: Record<AuthMode, string> = {
// clicks Log in while browsing and changes their mind is not stranded. Both // clicks Log in while browsing and changes their mind is not stranded. Both
// stay real routes: /reset-password links to /login, and customers may have // stay real routes: /reset-password links to /login, and customers may have
// bookmarks. // bookmarks.
export default function AuthRouteModal({ mode, onClose, onNavigate }: Props) { export default function AuthRouteModal({ mode, onClose, onNavigate, returnTo }: Props) {
return ( return (
<Modal <Modal
title={TITLES[mode]} title={TITLES[mode]}
@@ -36,6 +45,7 @@ export default function AuthRouteModal({ mode, onClose, onNavigate }: Props) {
// in while browsing wants to carry on browsing rather than be moved to // in while browsing wants to carry on browsing rather than be moved to
// their account page. // their account page.
onSuccess={onClose} onSuccess={onClose}
returnTo={returnTo}
/> />
</Modal> </Modal>
); );
@@ -0,0 +1,79 @@
import Button from 'antd/es/button';
/**
* Google's own mark, inlined as SVG (#345).
*
* Their identity guidelines specify the four colours and the geometry, and a
* hand-drawn approximation of somebody else's trademark is a compliance problem
* rather than a style choice. These are the published values.
*
* Inlined rather than fetched, for the reason every other asset in this app is:
* a second origin is a second thing that can be down, blocked, or slow, and
* this one sits on the sign-in path.
*/
function GoogleMark() {
return (
<svg width="18" height="18" viewBox="0 0 18 18" aria-hidden="true" focusable="false">
<path
fill="#4285F4"
d="M17.64 9.2c0-.64-.06-1.25-.16-1.84H9v3.48h4.84a4.14 4.14 0 0 1-1.8 2.72v2.26h2.92c1.7-1.57 2.68-3.88 2.68-6.62z"
/>
<path
fill="#34A853"
d="M9 18c2.43 0 4.47-.8 5.96-2.18l-2.92-2.26c-.8.54-1.84.86-3.04.86-2.34 0-4.32-1.58-5.03-3.7H.96v2.34A9 9 0 0 0 9 18z"
/>
<path
fill="#FBBC05"
d="M3.97 10.72a5.4 5.4 0 0 1 0-3.44V4.94H.96a9 9 0 0 0 0 8.12l3.01-2.34z"
/>
<path
fill="#EA4335"
d="M9 3.58c1.32 0 2.5.45 3.44 1.35l2.58-2.59C13.46.9 11.43 0 9 0A9 9 0 0 0 .96 4.94l3.01 2.34C4.68 5.16 6.66 3.58 9 3.58z"
/>
</svg>
);
}
type Props = Readonly<{
/** Where to send the customer back to. Validated again on the server. */
returnTo: string;
/**
* Which tab this sits on, which changes only the wording.
*
* One endpoint serves both: it signs in a known identity, links a verified
* address, or creates an account. The customer does not know or care which
* of those will happen, so the label matches what they came to the tab to
* do rather than what the server ends up doing.
*
* Both spellings are in Google identity guidelines alongside the mark.
*/
intent?: 'sign-in' | 'sign-up';
}>;
/**
* Signing in with Google (#345).
*
* A navigation rather than a fetch, which is what makes this different from
* every other control on the auth form. The flow leaves this application
* entirely, so there is no promise to await and no error to catch here — the
* server's callback decides what happens and redirects accordingly.
*
* `returnTo` is sent as a query parameter and **validated on the server**, not
* here. It has to be, since anyone can type the URL, and doing it in one place
* beats doing it in two languages. See `google/returnTo.ts`.
*/
export default function GoogleSignInButton({ returnTo, intent = 'sign-in' }: Props) {
return (
<Button
block
icon={<GoogleMark />}
onClick={() => {
// assign rather than the router: this is a full page departure to
// another origin, and react-router would try to match it as a route.
window.location.assign(`/api/auth/google/start?returnTo=${encodeURIComponent(returnTo)}`);
}}
>
{intent === 'sign-up' ? 'Sign up with Google' : 'Sign in with Google'}
</Button>
);
}
+6 -2
View File
@@ -148,6 +148,10 @@ function AppRoutes() {
// the storefront, so closing always lands somewhere real. // the storefront, so closing always lands somewhere real.
const background = state?.background; const background = state?.background;
const backdrop = modalPath ? background ?? { ...location, ...STOREFRONT_BACKDROP } : location; const backdrop = modalPath ? background ?? { ...location, ...STOREFRONT_BACKDROP } : location;
// Where a Google sign-in should land the customer: the page behind the modal,
// not the modal's own path. Built here because the backdrop is only known
// here, and validated again on the server (#345).
const returnTo = `${backdrop.pathname}${backdrop.search ?? ''}`;
function closeModal() { function closeModal() {
// Back, when there is somewhere to go back to, so closing the modal and // Back, when there is somewhere to go back to, so closing the modal and
@@ -193,10 +197,10 @@ function AppRoutes() {
{import.meta.env.DEV && <DevThrow scope="modal" />} {import.meta.env.DEV && <DevThrow scope="modal" />}
{modalPath === '/account' && <Account onClose={closeModal} />} {modalPath === '/account' && <Account onClose={closeModal} />}
{modalPath === '/login' && ( {modalPath === '/login' && (
<AuthRouteModal mode="login" onClose={closeModal} onNavigate={goWithinAuth} /> <AuthRouteModal mode="login" onClose={closeModal} onNavigate={goWithinAuth} returnTo={returnTo} />
)} )}
{modalPath === '/register' && ( {modalPath === '/register' && (
<AuthRouteModal mode="register" onClose={closeModal} onNavigate={goWithinAuth} /> <AuthRouteModal mode="register" onClose={closeModal} onNavigate={goWithinAuth} returnTo={returnTo} />
)} )}
{modalPath === '/forgot-password' && ( {modalPath === '/forgot-password' && (
<ForgotPassword onClose={closeModal} onBackToSignIn={() => goWithinAuth('/login')} /> <ForgotPassword onClose={closeModal} onBackToSignIn={() => goWithinAuth('/login')} />
+41
View File
@@ -119,6 +119,47 @@ test.describe('Customer accounts', () => {
await header.waitForSignedIn(); await header.waitForSignedIn();
}); });
// #345. Local development has no Google credentials, and neither does QA
// until #313 moves it off a hostname whose domain nobody can prove they own.
// So the button being ABSENT is the behaviour under test here, and it is the
// one that matters: a control that appears and then fails at Google is worse
// than one that was never offered.
// The sign-up tab, which #345 left it off entirely. A passkey belongs only on
// Log In, because you cannot register an account with one — but creating an
// account is exactly what a new customer reaches for Google to do, so its
// absence there hid the feature from the people it helps most.
//
// Asserted as absent for the same reason as the login one: local and QA have
// no credentials, so absence is the behaviour that actually runs here.
test('offers no Google button on the sign-up tab either, when unconfigured', async ({
authModal
}) => {
await authModal.gotoRegister();
await expect(
authModal.registerDialog.getByRole('button', { name: /with Google/i })
).toHaveCount(0);
});
test('offers no Google button when the environment is not configured for it', async ({
authModal,
accountModal,
customer,
header
}) => {
await accountModal.openAndLogOut();
await expect(header.logInButton).toBeVisible();
await authModal.gotoLogIn();
const google = authModal.logInDialog.getByRole('button', { name: /Sign in with Google/i });
await expect(google).toHaveCount(0);
// And the password form is untouched by its absence, which is the whole
// reason the alternatives sit below it rather than above.
await authModal.logIn(customer.email, customer.password);
await header.waitForSignedIn();
});
test('rejects login with the wrong password', async ({ page, customer, accountModal, authModal, header }) => { test('rejects login with the wrong password', async ({ page, customer, accountModal, authModal, header }) => {
await accountModal.openAndLogOut(); await accountModal.openAndLogOut();
await expect(header.logInButton).toBeVisible(); await expect(header.logInButton).toBeVisible();
+7 -3
View File
@@ -25,14 +25,18 @@ test.describe('Editing the customer emails', () => {
'Favorited item sold', 'Favorited item sold',
'Favorited item withdrawn', 'Favorited item withdrawn',
'Cart reminder', 'Cart reminder',
'Email address changed' 'Email address changed',
// Added in #337, and the reason these are matched on the whole
// accessible name rather than as substrings: it extends the label above
// it, so an unanchored match resolved to both tabs.
'Email address changed by the shop'
]) { ]) {
await expect(adminEmails.railTab(new RegExp(label))).toBeVisible(); await expect(adminEmails.railTab(label)).toBeVisible();
} }
// Only a customised template is marked, so which ones have been changed is // Only a customised template is marked, so which ones have been changed is
// visible without opening each one. An untouched template carries nothing. // visible without opening each one. An untouched template carries nothing.
await expect(adminEmails.railTab(/Password reset/)).toBeVisible(); await expect(adminEmails.railTab('Password reset')).toBeVisible();
await expect(adminEmails.customisedTab('Password reset')).toHaveCount(0); await expect(adminEmails.customisedTab('Password reset')).toHaveCount(0);
}); });
+46 -5
View File
@@ -1,5 +1,29 @@
import { FrameLocator, Locator, Page, expect } from '@playwright/test'; import { FrameLocator, Locator, Page, expect } from '@playwright/test';
/**
* A matcher for one template label, against a tab's *whole* accessible name.
*
* A tab is named for its template, plus the word "Customised" once it has been
* edited — the dot beside it carries that as an aria-label, so the state is not
* colour-only.
*
* Anchored at both ends, which is the entire point of this function. The
* locators here used to build an unanchored regex from the label, so a template
* whose name merely *began* with another's matched both. Adding "Email address
* changed by the shop" alongside "Email address changed" broke a passing test
* with a strict-mode violation naming the assertion rather than the new
* template — the same shape as the switch locator that silently retargeted in
* #317, and the same cost to diagnose.
*
* The escape matters for the same reason: these labels are copy, and copy
* acquires brackets and full stops eventually.
*/
function nameMatching(label: string, options: { customised?: boolean } = {}): RegExp {
const escaped = label.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
const suffix = options.customised ? '\\s+Customised' : '(?:\\s+Customised)?';
return new RegExp(`^${escaped}${suffix}$`);
}
/** /**
* The Emails tab: a vertical rail of template types and one editor at a time. * The Emails tab: a vertical rail of template types and one editor at a time.
* *
@@ -30,13 +54,30 @@ export class AdminEmails {
return this.page.getByRole('button', { name: `Insert {{${name}}}` }); return this.page.getByRole('button', { name: `Insert {{${name}}}` });
} }
/** One template's entry in the rail. */ /**
railTab(label: string | RegExp): Locator { * One template's entry in the rail, matched on its whole accessible name.
return this.page.getByRole('tab', { name: label }); *
* A tab's accessible name is the template's label, plus the word "Customised"
* when it has been edited — the dot beside it carries that as an aria-label so
* the state is not colour-only.
*
* Anchored at both ends, which is the point of this helper rather than a bare
* substring match. These locators used to build an unanchored regex from the
* label, so a template whose name merely *began* with another's matched both.
* Adding "Email address changed by the shop" beside "Email address changed"
* broke a passing test with a strict-mode violation, and the failure named the
* assertion rather than the new template — the same shape as the switch
* locator that silently retargeted in #317.
*
* The escape matters for the same reason: a label is copy, and copy acquires
* brackets and full stops eventually.
*/
railTab(label: string): Locator {
return this.page.getByRole('tab', { name: nameMatching(label) });
} }
customisedTab(label: string): Locator { customisedTab(label: string): Locator {
return this.page.getByRole('tab', { name: new RegExp(`${label}.*Customised`) }); return this.page.getByRole('tab', { name: nameMatching(label, { customised: true }) });
} }
subject(label: string): Locator { subject(label: string): Locator {
@@ -65,7 +106,7 @@ export class AdminEmails {
* resolved mid-swap finds the outgoing one. * resolved mid-swap finds the outgoing one.
*/ */
async openTemplate(label: string): Promise<void> { async openTemplate(label: string): Promise<void> {
await this.railTab(new RegExp(label)).click(); await this.railTab(label).click();
await expect(this.subject(label)).toBeVisible(); await expect(this.subject(label)).toBeVisible();
} }
} }