Merge pull request 'feat(auth): link a Google identity to an account that already exists (#343)' (#350) from feature/343-google-linking into main
Reviewed-on: #350
This commit was merged in pull request #350.
This commit is contained in:
@@ -0,0 +1,76 @@
|
|||||||
|
import { pool } from '../db';
|
||||||
|
import type { GoogleIdentity } from './oauth';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Joining a Google identity to an account that already exists (#343).
|
||||||
|
*
|
||||||
|
* The smallest module in this feature and the one to read most carefully. It is
|
||||||
|
* the point where somebody who has proved nothing to *this* shop is handed an
|
||||||
|
* account that belongs to somebody who did.
|
||||||
|
*
|
||||||
|
* ## The rule, and why it is defensible
|
||||||
|
*
|
||||||
|
* Link only when Google asserts `email_verified` and the address matches an
|
||||||
|
* existing customer exactly. Refuse otherwise.
|
||||||
|
*
|
||||||
|
* Google asserting the address means whoever completed that sign-in
|
||||||
|
* demonstrably controls the mailbox. That mailbox is already the root of trust
|
||||||
|
* for every other route into the account: it is where a password reset goes,
|
||||||
|
* and following a reset link is enough to take the account over completely. So
|
||||||
|
* linking on it grants nothing that was not already reachable, and it spares
|
||||||
|
* the customer who came to Google precisely because they forgot the password.
|
||||||
|
*
|
||||||
|
* **Never link on an unverified address.** That is not a degraded version of the
|
||||||
|
* same thing — it is an account takeover with extra steps, since the assertion
|
||||||
|
* would be one nobody has checked. It is why this is a written rule rather than
|
||||||
|
* a default that arrived with a library.
|
||||||
|
*
|
||||||
|
* ## Why the identity lookup happens before any of this
|
||||||
|
*
|
||||||
|
* The caller matches on `(provider, provider_sub)` first, and only reaches here
|
||||||
|
* when that finds nothing. An identity that has signed in before keeps working
|
||||||
|
* even if the address on either side has since changed, which is the whole
|
||||||
|
* reason the subject claim is what gets stored.
|
||||||
|
*/
|
||||||
|
|
||||||
|
export type LinkOutcome =
|
||||||
|
| { kind: 'linked'; customerId: number }
|
||||||
|
/** Google did not vouch for the address, or nothing matched it. */
|
||||||
|
| { kind: 'refused' };
|
||||||
|
|
||||||
|
interface CustomerRow {
|
||||||
|
id: number;
|
||||||
|
disabled_at: Date | null;
|
||||||
|
}
|
||||||
|
|
||||||
|
export async function linkToExistingCustomer(identity: GoogleIdentity): Promise<LinkOutcome> {
|
||||||
|
// The first thing checked, and it is the whole policy. Everything below is
|
||||||
|
// bookkeeping; this line is the security.
|
||||||
|
if (!identity.emailVerified) return { kind: 'refused' };
|
||||||
|
|
||||||
|
const { rows } = await pool.query<CustomerRow>(
|
||||||
|
// Compared exactly, against an address the caller has already lowercased
|
||||||
|
// and trimmed the way registration does. A stricter comparison here would
|
||||||
|
// silently fail to match and produce a second account for one person
|
||||||
|
// instead of an error anybody sees.
|
||||||
|
`SELECT id, disabled_at FROM customers WHERE email = $1`,
|
||||||
|
[identity.email]
|
||||||
|
);
|
||||||
|
const customer = rows[0];
|
||||||
|
if (!customer) return { kind: 'refused' };
|
||||||
|
|
||||||
|
// Refused here as well as at sign-in. Linking to a disabled account and then
|
||||||
|
// refusing the session would leave the identity attached, so the next attempt
|
||||||
|
// would take the sign-in path instead — turning a disabled account into one
|
||||||
|
// that is merely inconvenient to reach.
|
||||||
|
if (customer.disabled_at !== null) return { kind: 'refused' };
|
||||||
|
|
||||||
|
await pool.query(
|
||||||
|
`INSERT INTO customer_identities (customer_id, provider, provider_sub, last_used_at)
|
||||||
|
VALUES ($1, 'google', $2, now())
|
||||||
|
ON CONFLICT (provider, provider_sub) DO NOTHING`,
|
||||||
|
[customer.id, identity.sub]
|
||||||
|
);
|
||||||
|
|
||||||
|
return { kind: 'linked', customerId: customer.id };
|
||||||
|
}
|
||||||
@@ -5,13 +5,14 @@ import type { GoogleIdentity } from './oauth';
|
|||||||
/**
|
/**
|
||||||
* Creating a customer from a Google identity (#342).
|
* Creating a customer from a Google identity (#342).
|
||||||
*
|
*
|
||||||
* ## What this deliberately does not do
|
* ## What this deliberately does not decide
|
||||||
*
|
*
|
||||||
* It refuses when the address already belongs to a customer. Joining those two
|
* It reports `email-taken` when the address already belongs to a customer, and
|
||||||
* accounts is linking, it is the most security-sensitive decision in this
|
* stops there. Whether to join those two accounts is linking, which is the most
|
||||||
* project, and it belongs to #343 rather than falling out of an INSERT here.
|
* security-sensitive decision in this project and lives in `linkIdentity.ts`
|
||||||
* Refusing is the safe half of that decision and the only one available until
|
* (#343). Deciding it here would mean an account is handed over as a side
|
||||||
* the policy is written down.
|
* effect of an INSERT failing, which is exactly the shape that decision must
|
||||||
|
* never take.
|
||||||
*
|
*
|
||||||
* ## Consent, which is the actual problem in this issue
|
* ## Consent, which is the actual problem in this issue
|
||||||
*
|
*
|
||||||
|
|||||||
@@ -626,6 +626,36 @@ router.post('/me/analytics-consent', requireCustomer, asyncRoute(async (req: Req
|
|||||||
res.status(204).end();
|
res.status(204).end();
|
||||||
}));
|
}));
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Which identity providers this account is signed in with (#343).
|
||||||
|
*
|
||||||
|
* Linking happens automatically when Google vouches for an address that already
|
||||||
|
* has an account, which is defensible but not obvious. A customer who signed up
|
||||||
|
* with a password and later used Google has had two credentials joined without
|
||||||
|
* being asked, and a silent link is indistinguishable from a bug when they
|
||||||
|
* later wonder why the password is no longer needed.
|
||||||
|
*
|
||||||
|
* So it is shown, beside the passkeys, for the reason the passkey list exists
|
||||||
|
* at all: a customer cannot manage credentials they cannot see.
|
||||||
|
*
|
||||||
|
* No unlinking yet. Removing the only way into an account is the question #344
|
||||||
|
* settles, and offering the button before that check runs would be the fastest
|
||||||
|
* possible way to lock somebody out of their own orders.
|
||||||
|
*/
|
||||||
|
router.get('/me/identities', requireCustomer, asyncRoute(async (req: Request, res: Response) => {
|
||||||
|
const { rows } = await pool.query<{ provider: string; created_at: Date; last_used_at: Date | null }>(
|
||||||
|
// No provider_sub. The customer cannot act on it, and it is the one value
|
||||||
|
// that identifies them to the provider — the same reasoning that keeps
|
||||||
|
// credential ids out of the passkey list.
|
||||||
|
`SELECT provider, created_at, last_used_at
|
||||||
|
FROM customer_identities
|
||||||
|
WHERE customer_id = $1
|
||||||
|
ORDER BY created_at`,
|
||||||
|
[req.customerId]
|
||||||
|
);
|
||||||
|
res.json(rows);
|
||||||
|
}));
|
||||||
|
|
||||||
router.get('/me/orders', requireCustomer, asyncRoute(async (req: Request, res: Response) => {
|
router.get('/me/orders', requireCustomer, asyncRoute(async (req: Request, res: Response) => {
|
||||||
const { rows } = await pool.query<CustomerOrderRow>(
|
const { rows } = await pool.query<CustomerOrderRow>(
|
||||||
`SELECT o.id, o.processor, o.amount_cents, o.status, o.created_at, i.name AS item_name
|
`SELECT o.id, o.processor, o.amount_cents, o.status, o.created_at, i.name AS item_name
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import { googleConfig } from '../google/config';
|
|||||||
import { newAttempt, authorizationUrl, exchangeCode, verifiedIdentity } from '../google/oauth';
|
import { newAttempt, authorizationUrl, exchangeCode, verifiedIdentity } from '../google/oauth';
|
||||||
import type { GoogleIdentity } from '../google/oauth';
|
import type { GoogleIdentity } from '../google/oauth';
|
||||||
import { createCustomerFromGoogle } from '../google/newCustomer';
|
import { createCustomerFromGoogle } from '../google/newCustomer';
|
||||||
|
import { linkToExistingCustomer } from '../google/linkIdentity';
|
||||||
import { issueVerificationEmail } from '../customerVerification';
|
import { issueVerificationEmail } from '../customerVerification';
|
||||||
import type { AttemptSecrets } from '../google/oauth';
|
import type { AttemptSecrets } from '../google/oauth';
|
||||||
import { googleSignInLimiter } from '../rateLimit';
|
import { googleSignInLimiter } from '../rateLimit';
|
||||||
@@ -26,11 +27,10 @@ const router = Router();
|
|||||||
* It signs in a customer whose Google identity is already linked, and creates
|
* It signs in a customer whose Google identity is already linked, and creates
|
||||||
* an account for one nobody here has seen (#342).
|
* an account for one nobody here has seen (#342).
|
||||||
*
|
*
|
||||||
* It refuses when the address already belongs to a customer. Joining those two
|
* It also joins a Google identity to an account that already holds the same
|
||||||
* accounts is linking, it is the most security-sensitive decision in this
|
* address — but only when Google vouches for that address (#343). The whole of
|
||||||
* project, and it belongs to #343 rather than falling out of an INSERT.
|
* that policy lives in `google/linkIdentity.ts`, which is the smallest module
|
||||||
* Refusing is the safe half of that decision and the only half available until
|
* in this feature and the one to read most carefully.
|
||||||
* the policy is written down.
|
|
||||||
*
|
*
|
||||||
* ## The cookie, and why it is the whole security of the callback
|
* ## The cookie, and why it is the whole security of the callback
|
||||||
*
|
*
|
||||||
@@ -78,6 +78,15 @@ const FAILURE_PATH = '/login?auth=google-failed';
|
|||||||
*/
|
*/
|
||||||
const WELCOME_PATH = '/welcome';
|
const WELCOME_PATH = '/welcome';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Where a customer goes when they have an account this sign-in cannot reach.
|
||||||
|
*
|
||||||
|
* Its own destination rather than the generic failure, because it is the one
|
||||||
|
* refusal a customer can act on: the login form reads this and says to sign in
|
||||||
|
* with the password they already have.
|
||||||
|
*/
|
||||||
|
const USE_PASSWORD_PATH = '/login?auth=google-use-password';
|
||||||
|
|
||||||
function setAttemptCookie(res: Response, attempt: Attempt): void {
|
function setAttemptCookie(res: Response, attempt: Attempt): void {
|
||||||
res.cookie(ATTEMPT_COOKIE, Buffer.from(JSON.stringify(attempt)).toString('base64url'), {
|
res.cookie(ATTEMPT_COOKIE, Buffer.from(JSON.stringify(attempt)).toString('base64url'), {
|
||||||
httpOnly: true,
|
httpOnly: true,
|
||||||
@@ -129,16 +138,24 @@ function secretsMatch(a: string, b: string): boolean {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Creates an account for a Google identity nobody here has seen, and signs in.
|
* What happens when the identity lookup found nothing: create, link, or refuse.
|
||||||
*
|
*
|
||||||
* A named function rather than an inline block for the reason
|
* A named function rather than an inline block for the reason
|
||||||
* `routesAreWrapped.test.ts` cares about, and because the callback is already
|
* `routesAreWrapped.test.ts` cares about, and because the callback is already
|
||||||
* the longest handler in this file.
|
* the longest handler in this file.
|
||||||
*
|
*
|
||||||
* The return path is deliberately dropped for a brand-new customer, who lands
|
* The order below is the policy from #343, and it is an order rather than a set
|
||||||
* on the consent step instead. That step is worth interrupting for: it is the
|
* of independent checks:
|
||||||
* only moment the two consent sentences can honestly be shown, because the
|
*
|
||||||
* redirect to Google happened before anyone knew this person was new.
|
* 1. Nobody has this address — create the account, and land on the consent step
|
||||||
|
* 2. Somebody does, and Google vouches for it — link, and sign in
|
||||||
|
* 3. Somebody does, and Google does not vouch — refuse, and say to use the
|
||||||
|
* password
|
||||||
|
*
|
||||||
|
* The return path is deliberately dropped in case 1 only. That customer lands
|
||||||
|
* on the consent step, which 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
|
* 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
|
* was rejected. The consent page would then have to redirect somewhere a URL
|
||||||
@@ -147,28 +164,42 @@ function secretsMatch(a: string, b: string): boolean {
|
|||||||
* an attacker can link to directly. One new customer occasionally landing on
|
* 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.
|
* the storefront rather than back at their cart is the cheaper of the two.
|
||||||
*/
|
*/
|
||||||
async function signUp(res: Response, identity: GoogleIdentity): Promise<void> {
|
async function signUpOrLink(res: Response, identity: GoogleIdentity, returnTo: string): Promise<void> {
|
||||||
const outcome = await createCustomerFromGoogle(identity);
|
const outcome = await createCustomerFromGoogle(identity);
|
||||||
|
|
||||||
if (outcome.kind === 'email-taken') {
|
if (outcome.kind === 'created') {
|
||||||
// An account already uses this address, and joining them is #343. Refusing
|
// Only when Google did not vouch for the address. When it did, the customer
|
||||||
// is the safe half of that decision: linking on an address is exactly the
|
// has already demonstrated they receive mail there — which is precisely
|
||||||
// takeover path the policy exists to reason about carefully.
|
// what the confirmation email exists to establish — so sending one would
|
||||||
console.warn('[google] refused a sign-up: that address already has an account');
|
// ask them to do a thing that is done.
|
||||||
res.redirect(FAILURE_PATH);
|
if (!identity.emailVerified) {
|
||||||
|
await issueVerificationEmail(outcome.customerId, identity.email, identity.firstName, identity.lastName);
|
||||||
|
}
|
||||||
|
await signIn(res, outcome.customerId);
|
||||||
|
res.redirect(WELCOME_PATH);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Only when Google did not vouch for the address. When it did, the customer
|
// The address belongs to somebody. Whether that is the same person is the
|
||||||
// has already demonstrated they receive mail there — which is precisely what
|
// question #343 exists to answer, and `linkToExistingCustomer` holds the
|
||||||
// the confirmation email exists to establish — so sending one would ask them
|
// whole of the answer.
|
||||||
// to do a thing that is done.
|
const link = await linkToExistingCustomer(identity);
|
||||||
if (!identity.emailVerified) {
|
if (link.kind === 'refused') {
|
||||||
await issueVerificationEmail(outcome.customerId, identity.email, identity.firstName, identity.lastName);
|
// Deliberately its own destination rather than the generic failure. This is
|
||||||
|
// the one refusal a customer can act on: they have an account, they simply
|
||||||
|
// cannot reach it this way, and telling them to use the password they
|
||||||
|
// already have is more useful than "that did not work".
|
||||||
|
//
|
||||||
|
// It reveals nothing they did not already supply. They arrived holding a
|
||||||
|
// Google account for this address, so being told the address has an account
|
||||||
|
// here tells them about themselves.
|
||||||
|
console.warn('[google] refused a link: the address is taken and Google did not verify it');
|
||||||
|
res.redirect(USE_PASSWORD_PATH);
|
||||||
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
await signIn(res, outcome.customerId);
|
await signIn(res, link.customerId);
|
||||||
res.redirect(WELCOME_PATH);
|
res.redirect(returnTo);
|
||||||
}
|
}
|
||||||
|
|
||||||
router.get(
|
router.get(
|
||||||
@@ -241,7 +272,7 @@ router.get(
|
|||||||
// they already have an account under this address — and joining those two
|
// 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
|
// is linking, which is #343 and is refused here until its policy is
|
||||||
// written down rather than falling out of an INSERT.
|
// written down rather than falling out of an INSERT.
|
||||||
if (!linked) return signUp(res, identity);
|
if (!linked) return signUpOrLink(res, identity, attempt.returnTo);
|
||||||
|
|
||||||
// Refused here as well as on the password and passkey paths. Enforcing it
|
// 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,
|
// on some routes and not others is how a disabled account keeps a way in,
|
||||||
@@ -267,6 +298,6 @@ router.get(
|
|||||||
);
|
);
|
||||||
|
|
||||||
/** Exported for the tests; nothing else needs the cookie's name. */
|
/** Exported for the tests; nothing else needs the cookie's name. */
|
||||||
export { ATTEMPT_COOKIE, ATTEMPT_TTL_MS, FAILURE_PATH, WELCOME_PATH };
|
export { ATTEMPT_COOKIE, ATTEMPT_TTL_MS, FAILURE_PATH, WELCOME_PATH, USE_PASSWORD_PATH };
|
||||||
|
|
||||||
export default router;
|
export default router;
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import request from 'supertest';
|
import request from 'supertest';
|
||||||
import app from '../../src/app';
|
import app from '../../src/app';
|
||||||
import { pool, requireRow } from '../../src/db';
|
import { pool, requireRow } from '../../src/db';
|
||||||
|
import { createSession } from '../../src/customerSession';
|
||||||
import { resetDb, closeDb } from './setup/testDb';
|
import { resetDb, closeDb } from './setup/testDb';
|
||||||
|
|
||||||
const CLIENT_ID = 'test-client.apps.googleusercontent.com';
|
const CLIENT_ID = 'test-client.apps.googleusercontent.com';
|
||||||
@@ -108,6 +109,11 @@ async function linkGoogle(customerId: number, sub = SUB): Promise<void> {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** A session cookie for a customer, without going through any sign-in flow. */
|
||||||
|
async function sessionFor(customerId: number): Promise<string> {
|
||||||
|
return `rd_session=${await createSession(customerId)}`;
|
||||||
|
}
|
||||||
|
|
||||||
describe('GET /api/auth/google/start', () => {
|
describe('GET /api/auth/google/start', () => {
|
||||||
it('sends the customer to Google with the code flow and PKCE', async () => {
|
it('sends the customer to Google with the code flow and PKCE', async () => {
|
||||||
const { res } = await startSignIn();
|
const { res } = await startSignIn();
|
||||||
@@ -537,3 +543,190 @@ describe('signing up with Google', () => {
|
|||||||
expect(await countOf('customers')).toBe(1);
|
expect(await countOf('customers')).toBe(1);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
/**
|
||||||
|
* #343. The most security-sensitive phase of this feature.
|
||||||
|
*
|
||||||
|
* Every test here is about the same question asked from a different angle: when
|
||||||
|
* is it right to hand somebody an account they have not proved they own?
|
||||||
|
*/
|
||||||
|
describe('linking a Google identity to an existing account', () => {
|
||||||
|
async function attempt(overrides: Record<string, unknown> = {}, returnTo?: string) {
|
||||||
|
const { cookie, state, nonce } = await startSignIn(returnTo);
|
||||||
|
respondWithToken(idToken(claimsFor(nonce, overrides)));
|
||||||
|
return request(app)
|
||||||
|
.get('/api/auth/google/callback')
|
||||||
|
.query({ code: 'an-auth-code', state })
|
||||||
|
.set('Cookie', cookie);
|
||||||
|
}
|
||||||
|
|
||||||
|
async function identityCount(customerId: number): Promise<number> {
|
||||||
|
const { rows } = await pool.query<{ n: number }>(
|
||||||
|
`SELECT count(*)::int AS n FROM customer_identities WHERE customer_id = $1`,
|
||||||
|
[customerId]
|
||||||
|
);
|
||||||
|
return requireRow(rows, 'a count of identities').n;
|
||||||
|
}
|
||||||
|
|
||||||
|
it('links when Google vouches for an address an account already holds', async () => {
|
||||||
|
const customerId = await createCustomer('haspassword@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'haspassword@example.com', email_verified: true }, '/cart');
|
||||||
|
|
||||||
|
// Whoever completed that sign-in demonstrably controls the mailbox, which
|
||||||
|
// is already the root of trust for a password reset on this account. So
|
||||||
|
// linking grants nothing that was not already reachable.
|
||||||
|
expect(res.headers.location).toBe('/cart');
|
||||||
|
expect(await identityCount(customerId)).toBe(1);
|
||||||
|
expect(res.headers['set-cookie'] as unknown as string[]).toContainEqual(
|
||||||
|
expect.stringContaining('rd_session=')
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('signs the linked customer into their existing account, not a new one', async () => {
|
||||||
|
const customerId = await createCustomer('same@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'same@example.com', email_verified: true });
|
||||||
|
const session = requireCookie(res.headers['set-cookie'] as unknown as string[], 'rd_session=');
|
||||||
|
const me = await request(app).get('/api/customers/me').set('Cookie', session);
|
||||||
|
|
||||||
|
expect(me.body.id).toBe(customerId);
|
||||||
|
const { rows } = await pool.query<{ n: number }>(`SELECT count(*)::int AS n FROM customers`);
|
||||||
|
expect(requireRow(rows, 'a count of customers').n).toBe(1);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses when Google does not vouch for the address', async () => {
|
||||||
|
const customerId = await createCustomer('unverified@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'unverified@example.com', email_verified: false });
|
||||||
|
|
||||||
|
// The whole policy in one assertion. Linking on an unverified assertion is
|
||||||
|
// not a degraded version of the same thing — it is an account takeover with
|
||||||
|
// extra steps, because nobody has checked the claim.
|
||||||
|
expect(res.headers.location).toBe('/login?auth=google-use-password');
|
||||||
|
expect(await identityCount(customerId)).toBe(0);
|
||||||
|
expect(res.headers['set-cookie'] ?? []).not.toContainEqual(
|
||||||
|
expect.stringContaining('rd_session=')
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses on a merely truthy email_verified, which is the trap', async () => {
|
||||||
|
// The string "false" is truthy. If this check ever becomes a truthiness
|
||||||
|
// test, every unverified Google account links to whatever account holds
|
||||||
|
// its address.
|
||||||
|
const customerId = await createCustomer('trap@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'trap@example.com', email_verified: 'false' });
|
||||||
|
|
||||||
|
expect(res.headers.location).toBe('/login?auth=google-use-password');
|
||||||
|
expect(await identityCount(customerId)).toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('sends the refused customer somewhere they can act on', async () => {
|
||||||
|
// They have an account and simply cannot reach it this way. Telling them to
|
||||||
|
// use the password they already have beats "that did not work", and reveals
|
||||||
|
// nothing: they arrived holding a Google account for this address.
|
||||||
|
await createCustomer('actionable@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'actionable@example.com', email_verified: false });
|
||||||
|
|
||||||
|
expect(res.headers.location).toBe('/login?auth=google-use-password');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses to link to a disabled account', async () => {
|
||||||
|
const customerId = await createCustomer('disabledlink@example.com');
|
||||||
|
await pool.query(`UPDATE customers SET disabled_at = now() WHERE id = $1`, [customerId]);
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'disabledlink@example.com', email_verified: true });
|
||||||
|
|
||||||
|
// Linking and then refusing the session would leave the identity attached,
|
||||||
|
// so the next attempt would take the sign-in path instead — turning a
|
||||||
|
// disabled account into one that is merely inconvenient to reach.
|
||||||
|
expect(await identityCount(customerId)).toBe(0);
|
||||||
|
expect(res.headers['set-cookie'] ?? []).not.toContainEqual(
|
||||||
|
expect.stringContaining('rd_session=')
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('matches the address case-insensitively, as registration stores it', async () => {
|
||||||
|
const customerId = await createCustomer('mixedcase@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'MixedCase@Example.COM', email_verified: true });
|
||||||
|
|
||||||
|
// A stricter comparison than registration's would silently fail to match
|
||||||
|
// and produce a second account for one person, rather than an error anyone
|
||||||
|
// sees.
|
||||||
|
expect(await identityCount(customerId)).toBe(1);
|
||||||
|
expect(res.headers.location).toBe('/');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('prefers the identity over the address once linked', async () => {
|
||||||
|
const withIdentity = await createCustomer('theirs@example.com');
|
||||||
|
await linkGoogle(withIdentity, SUB);
|
||||||
|
// A second customer now holds the address this Google account reports.
|
||||||
|
const withAddress = await createCustomer('moved@example.com');
|
||||||
|
|
||||||
|
const res = await attempt({ email: 'moved@example.com', email_verified: true });
|
||||||
|
const session = requireCookie(res.headers['set-cookie'] as unknown as string[], 'rd_session=');
|
||||||
|
const me = await request(app).get('/api/customers/me').set('Cookie', session);
|
||||||
|
|
||||||
|
// The identity lookup runs first and nothing else is consulted. An identity
|
||||||
|
// that has signed in before keeps working even when the address on either
|
||||||
|
// side has since changed — and the account matching the address is somebody
|
||||||
|
// else's, which is exactly why the order matters.
|
||||||
|
expect(me.body.id).toBe(withIdentity);
|
||||||
|
expect(await identityCount(withAddress)).toBe(0);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('does not link twice when the same customer signs in again', async () => {
|
||||||
|
const customerId = await createCustomer('twice@example.com');
|
||||||
|
|
||||||
|
await attempt({ email: 'twice@example.com', email_verified: true });
|
||||||
|
await attempt({ email: 'twice@example.com', email_verified: true });
|
||||||
|
|
||||||
|
expect(await identityCount(customerId)).toBe(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe('GET /api/customers/me/identities', () => {
|
||||||
|
it('shows the customer what they are linked to', async () => {
|
||||||
|
const customerId = await createCustomer('shown@example.com');
|
||||||
|
await linkGoogle(customerId);
|
||||||
|
const session = await sessionFor(customerId);
|
||||||
|
|
||||||
|
const res = await request(app).get('/api/customers/me/identities').set('Cookie', session);
|
||||||
|
|
||||||
|
expect(res.status).toBe(200);
|
||||||
|
expect(res.body).toHaveLength(1);
|
||||||
|
expect(res.body[0].provider).toBe('google');
|
||||||
|
});
|
||||||
|
|
||||||
|
it('never returns the provider subject', async () => {
|
||||||
|
const customerId = await createCustomer('opaque@example.com');
|
||||||
|
await linkGoogle(customerId);
|
||||||
|
const session = await sessionFor(customerId);
|
||||||
|
|
||||||
|
const res = await request(app).get('/api/customers/me/identities').set('Cookie', session);
|
||||||
|
|
||||||
|
// The customer cannot act on it, and it is the one value that identifies
|
||||||
|
// them to Google — the same reasoning that keeps credential ids out of the
|
||||||
|
// passkey list.
|
||||||
|
expect(JSON.stringify(res.body)).not.toContain(SUB);
|
||||||
|
expect(res.body[0].provider_sub).toBeUndefined();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('is empty for a customer who has never used a provider', async () => {
|
||||||
|
const customerId = await createCustomer('none@example.com');
|
||||||
|
const session = await sessionFor(customerId);
|
||||||
|
|
||||||
|
const res = await request(app).get('/api/customers/me/identities').set('Cookie', session);
|
||||||
|
|
||||||
|
expect(res.body).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it('refuses without a session', async () => {
|
||||||
|
const res = await request(app).get('/api/customers/me/identities');
|
||||||
|
|
||||||
|
expect(res.status).toBe(401);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ import { setFavoriteAlerts } from './favoritesApi';
|
|||||||
import { useCustomerAuth } from './CustomerAuthContext';
|
import { useCustomerAuth } from './CustomerAuthContext';
|
||||||
import AccountDetails from './AccountDetails';
|
import AccountDetails from './AccountDetails';
|
||||||
import Passkeys from './Passkeys';
|
import Passkeys from './Passkeys';
|
||||||
|
import ConnectedAccounts from './ConnectedAccounts';
|
||||||
|
|
||||||
const { Text } = Typography;
|
const { Text } = Typography;
|
||||||
|
|
||||||
@@ -178,6 +179,8 @@ export default function Account({ onClose }: Props) {
|
|||||||
cannot do this is not offered a button that fails (#40). */}
|
cannot do this is not offered a button that fails (#40). */}
|
||||||
<Passkeys />
|
<Passkeys />
|
||||||
|
|
||||||
|
<ConnectedAccounts />
|
||||||
|
|
||||||
<Divider />
|
<Divider />
|
||||||
<Space wrap>
|
<Space wrap>
|
||||||
{/* Order history is a page of its own now. The link stays here because
|
{/* Order history is a page of its own now. The link stays here because
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import { useState } from 'react';
|
import { useState } from 'react';
|
||||||
|
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';
|
||||||
import Button from 'antd/es/button';
|
import Button from 'antd/es/button';
|
||||||
@@ -43,11 +44,38 @@ type Props = Readonly<{
|
|||||||
onSuccess: () => void;
|
onSuccess: () => void;
|
||||||
}>;
|
}>;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* What a Google sign-in that ended badly wants the login form to say (#343).
|
||||||
|
*
|
||||||
|
* Read from the query string because the callback is a redirect: it cannot
|
||||||
|
* return a body, and the customer's browser arrives here having been sent by
|
||||||
|
* Google. A parameter is the only channel there is.
|
||||||
|
*
|
||||||
|
* `google-use-password` is the interesting one. It means the customer has an
|
||||||
|
* account and simply cannot reach it this way, which is the single refusal in
|
||||||
|
* this flow they can act on — so it says what to do rather than what failed.
|
||||||
|
*
|
||||||
|
* It reveals nothing they did not already supply. They arrived holding a Google
|
||||||
|
* account for this address, so being told the address has an account here tells
|
||||||
|
* them only about themselves.
|
||||||
|
*/
|
||||||
|
function googleNotice(reason: string | null): string | null {
|
||||||
|
if (reason === 'google-use-password') {
|
||||||
|
return 'You already have an account with this email address. Log in with your password below.';
|
||||||
|
}
|
||||||
|
if (reason === 'google-failed') {
|
||||||
|
return 'That Google sign-in did not work. You can log in with your password instead.';
|
||||||
|
}
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
|
||||||
// The one implementation of signing in and registering. It was previously
|
// The one implementation of signing in and registering. It was previously
|
||||||
// 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 }: Props) {
|
||||||
|
const [searchParams] = useSearchParams();
|
||||||
|
const notice = googleNotice(searchParams.get('auth'));
|
||||||
const [error, setError] = useState<string | null>(null);
|
const [error, setError] = useState<string | null>(null);
|
||||||
const [loading, setLoading] = useState(false);
|
const [loading, setLoading] = useState(false);
|
||||||
// Separate from `loading`, so the password button does not sit disabled and
|
// Separate from `loading`, so the password button does not sit disabled and
|
||||||
@@ -107,6 +135,12 @@ export default function AuthForm({ mode, onModeChange, onForgotPassword, onSucce
|
|||||||
|
|
||||||
return (
|
return (
|
||||||
<>
|
<>
|
||||||
|
{/* The notice sits above the tabs and below any live error, because it
|
||||||
|
describes how the customer arrived rather than what they just did. An
|
||||||
|
error from this form supersedes it. */}
|
||||||
|
{!error && notice && (
|
||||||
|
<Alert type="info" showIcon message={notice} style={{ marginBottom: 16 }} />
|
||||||
|
)}
|
||||||
{error && <Alert type="error" showIcon message={error} style={{ marginBottom: 16 }} />}
|
{error && <Alert type="error" showIcon message={error} style={{ marginBottom: 16 }} />}
|
||||||
<Tabs
|
<Tabs
|
||||||
activeKey={mode}
|
activeKey={mode}
|
||||||
|
|||||||
@@ -0,0 +1,70 @@
|
|||||||
|
import { useEffect, useState } from 'react';
|
||||||
|
import Typography from 'antd/es/typography';
|
||||||
|
import Tag from 'antd/es/tag';
|
||||||
|
import Spin from 'antd/es/spin';
|
||||||
|
import { fetchIdentities, Identity } from './customerApi';
|
||||||
|
|
||||||
|
const { Title, Text, Paragraph } = Typography;
|
||||||
|
|
||||||
|
const PROVIDER_NAMES: Record<string, string> = { google: 'Google' };
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Which identity providers this account can be signed in with (#343).
|
||||||
|
*
|
||||||
|
* Linking happens automatically when Google vouches for an address that already
|
||||||
|
* has an account here. That is defensible — whoever completed the sign-in
|
||||||
|
* demonstrably controls the mailbox, which is already the root of trust for a
|
||||||
|
* password reset — but it is not obvious, and a customer who signed up with a
|
||||||
|
* password has had two credentials joined without being asked.
|
||||||
|
*
|
||||||
|
* A silent link is indistinguishable from a bug when somebody later wonders why
|
||||||
|
* the password is no longer needed. So it is shown, beside the passkeys, for the
|
||||||
|
* reason the passkey list exists at all: a customer cannot manage credentials
|
||||||
|
* they cannot see.
|
||||||
|
*
|
||||||
|
* Read-only for now. Removing the only way into an account is the question #344
|
||||||
|
* settles, and offering an unlink button before that check runs would be the
|
||||||
|
* fastest way to lock somebody out of their own orders.
|
||||||
|
*/
|
||||||
|
export default function ConnectedAccounts() {
|
||||||
|
const [identities, setIdentities] = useState<Identity[]>([]);
|
||||||
|
const [loading, setLoading] = useState(true);
|
||||||
|
|
||||||
|
useEffect(() => {
|
||||||
|
fetchIdentities()
|
||||||
|
.then(setIdentities)
|
||||||
|
// Silent. This is a supplementary panel on a page whose real content is
|
||||||
|
// elsewhere, and a red error over the account settings because one extra
|
||||||
|
// read failed would be worse than the panel simply not appearing.
|
||||||
|
.catch(() => setIdentities([]))
|
||||||
|
.finally(() => setLoading(false));
|
||||||
|
}, []);
|
||||||
|
|
||||||
|
// Nothing at all for the overwhelming majority, who have never used a
|
||||||
|
// provider. An empty state here would appear on every account page to say
|
||||||
|
// that nothing had happened.
|
||||||
|
if (loading) return <Spin />;
|
||||||
|
if (identities.length === 0) return null;
|
||||||
|
|
||||||
|
return (
|
||||||
|
<>
|
||||||
|
<Title level={5}>Connected accounts</Title>
|
||||||
|
<Paragraph type="secondary" style={{ fontSize: 13 }}>
|
||||||
|
You can sign in with these as well as with your password.
|
||||||
|
</Paragraph>
|
||||||
|
{identities.map((identity) => (
|
||||||
|
<div key={identity.provider} style={{ marginBottom: 8 }}>
|
||||||
|
<Tag color="blue">{PROVIDER_NAMES[identity.provider] ?? identity.provider}</Tag>
|
||||||
|
<Text type="secondary" style={{ fontSize: 12 }}>
|
||||||
|
{/* Last used rather than connected, for the reason the passkey list
|
||||||
|
shows it: it is what tells a customer whether something is still
|
||||||
|
theirs, where a connection date says only that it happened. */}
|
||||||
|
{identity.last_used_at
|
||||||
|
? `last used ${new Date(identity.last_used_at).toLocaleDateString()}`
|
||||||
|
: 'never used to sign in'}
|
||||||
|
</Text>
|
||||||
|
</div>
|
||||||
|
))}
|
||||||
|
</>
|
||||||
|
);
|
||||||
|
}
|
||||||
@@ -183,6 +183,17 @@ export function changeMyEmail(currentPassword: string, email: string): Promise<C
|
|||||||
}).then(res => handle<Customer>(res));
|
}).then(res => handle<Customer>(res));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** One identity provider this account can sign in with (#343). */
|
||||||
|
export interface Identity {
|
||||||
|
provider: string;
|
||||||
|
created_at: string;
|
||||||
|
last_used_at: string | null;
|
||||||
|
}
|
||||||
|
|
||||||
|
export function fetchIdentities(): Promise<Identity[]> {
|
||||||
|
return fetch('/api/customers/me/identities').then(res => handle<Identity[]>(res));
|
||||||
|
}
|
||||||
|
|
||||||
/** A registered passkey, as the account page lists it (#40). */
|
/** A registered passkey, as the account page lists it (#40). */
|
||||||
export interface Passkey {
|
export interface Passkey {
|
||||||
id: number;
|
id: number;
|
||||||
|
|||||||
Reference in New Issue
Block a user