From b1838192acb30a5712103e4c1224147bb94e7097 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Sun, 31 May 2026 16:30:27 -0400 Subject: [PATCH 1/3] feat(auth): link-github route + /account banner for legacy users Implements POST /api/auth/link-github + the GitHub callback's link-mode branch + the /account banner that nags legacy-password users toward binding a GitHub identity, completing the login-migration track per specs/api/auth.md and specs/behaviors/account-migration.md. The OAuth round-trip is overloaded: `cfp_oauth_session` cookie now carries a `mode` claim ('login' | 'link') and an optional linkPersonId. The verifier defaults pre-link cookies to 'login' for back-compat so existing flows are unchanged. The callback handler branches on mode; link-mode skips identity matching and session minting entirely, calls the new completeLinkCallback() helper, and redirects to /account?... POST /api/auth/link-github is auth-required, fast-fails when the caller already has a GitHub link, signs a 10-min link-mode cookie, and 302s to GitHub. The callback validates the GitHub identity isn't bound to a different Person (409 github_id_in_use_elsewhere) and mutates the calling Person's githubUserId/githubLogin/githubLinkedAt. PrivateProfile.email is NOT auto-refreshed in v1; that needs a consent screen which is deferred. SPA: useAuth() now exposes hasGitHubLink + lastLoginMethod from /api/auth/me. /account renders a dismissable banner above the cards when hasGitHubLink is false and the session was minted via legacy password or password reset. The Identity card swaps its "Manage on GitHub" button for a "Connect GitHub" form when unlinked. Success and error states surface via toast on /account?linked=github or /account?error=. 5 new API tests + 5 new web tests; full sweep (api 387 / web 68 / shared 75) clean; lint + type-check green. This closes the login-migration impl track (phases A-D). Future work on sunset, self-service unlink, and email-consent gets its own plans. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/api/src/auth/github-oauth.ts | 104 +++++++ apps/api/src/auth/oauth-session-cookie.ts | 48 +++- apps/api/src/routes/auth.ts | 106 +++++++- apps/api/src/services/github-account.ts | 29 ++ apps/api/tests/auth-link-github.test.ts | 315 ++++++++++++++++++++++ apps/web/src/hooks/useAuth.tsx | 55 +++- apps/web/src/screens/Account.tsx | 101 ++++++- apps/web/tests/Account.test.tsx | 157 +++++++++++ 8 files changed, 880 insertions(+), 35 deletions(-) create mode 100644 apps/api/tests/auth-link-github.test.ts create mode 100644 apps/web/tests/Account.test.tsx diff --git a/apps/api/src/auth/github-oauth.ts b/apps/api/src/auth/github-oauth.ts index f8c2ef3..52be5a8 100644 --- a/apps/api/src/auth/github-oauth.ts +++ b/apps/api/src/auth/github-oauth.ts @@ -57,6 +57,110 @@ export type CallbackErrorCode = | 'github_unreachable' | 'email_unverified'; +/** + * Outcomes for the link-github callback. Mirrors `CallbackOutcome` but + * with the link-specific error codes from specs/api/auth.md. + */ +export type LinkCallbackOutcome = + | { kind: 'linked'; personId: string } + | { kind: 'error'; code: LinkCallbackErrorCode }; + +export type LinkCallbackErrorCode = + | 'github_unreachable' + | 'github_already_linked' + | 'github_id_in_use_elsewhere'; + +export interface CompleteLinkCallbackParams { + readonly fastify: FastifyInstance; + readonly request: FastifyRequest; + readonly code: string; + readonly codeVerifier: string; + readonly redirectUri: string; + readonly linkPersonId: string; +} + +/** + * Run the link-mode OAuth callback pipeline. Differs from + * `completeCallback`: + * - No matching; the target Person is named in the cookie. + * - Two conflict cases: the calling Person already has a link, or + * the GitHub identity is bound to a different Person. + * - No session minted; the user is already signed-in. + * + * The callback route is responsible for redirecting to + * `/account?linked=github` on success or `/account?error=` on + * any of the error outcomes. + */ +export async function completeLinkCallback( + params: CompleteLinkCallbackParams, +): Promise { + const { fastify, request, code, codeVerifier, redirectUri, linkPersonId } = params; + const cfg = fastify.config; + + if (!cfg.GITHUB_OAUTH_CLIENT_ID || !cfg.GITHUB_OAUTH_CLIENT_SECRET) { + return { kind: 'error', code: 'github_unreachable' }; + } + + const linkingPerson = fastify.inMemoryState.people.get(linkPersonId); + if (!linkingPerson || linkingPerson.deletedAt) { + // The cookie pointed at a person who no longer exists or is deleted. + // Treat as github_unreachable for the user — this should be very rare + // (cookie is 10m and Persons rarely vanish in that window). + return { kind: 'error', code: 'github_unreachable' }; + } + if (typeof linkingPerson.githubUserId === 'number') { + return { kind: 'error', code: 'github_already_linked' }; + } + + let accessToken: string; + try { + accessToken = await exchangeCodeForToken({ + clientId: cfg.GITHUB_OAUTH_CLIENT_ID, + clientSecret: cfg.GITHUB_OAUTH_CLIENT_SECRET, + code, + codeVerifier, + redirectUri, + }); + } catch (err) { + fastify.log.warn({ err }, 'link-github: token exchange failed'); + return { kind: 'error', code: 'github_unreachable' }; + } + + let identity: ResolvedGitHubIdentity; + try { + const [ghUser, rawEmails] = await Promise.all([ + fetchGitHubUser(accessToken), + fetchGitHubEmails(accessToken), + ]); + identity = resolveIdentitySnapshot(ghUser, rawEmails); + } catch (err) { + fastify.log.warn({ err }, 'link-github: user/emails fetch failed'); + return { kind: 'error', code: 'github_unreachable' }; + } + + // Conflict: this GitHub identity is bound to a different Person. + for (const person of fastify.inMemoryState.people.values()) { + if (person.githubUserId === identity.id && person.id !== linkPersonId) { + return { kind: 'error', code: 'github_id_in_use_elsewhere' }; + } + } + + const result = await fastify.store.transact( + buildTransactionOptions({ + request, + action: 'person.github-link', + subjectType: 'person', + subjectId: linkPersonId, + subjectSlug: linkingPerson.slug, + responseCode: 302, + }), + async (tx) => fastify.services.githubAccount.linkToExisting(tx, linkingPerson, identity), + ); + result.value.stateApply.apply(fastify.inMemoryState, fastify.fts); + + return { kind: 'linked', personId: linkPersonId }; +} + export interface CompleteCallbackParams { readonly fastify: FastifyInstance; readonly request: FastifyRequest; diff --git a/apps/api/src/auth/oauth-session-cookie.ts b/apps/api/src/auth/oauth-session-cookie.ts index 6346459..1441bbb 100644 --- a/apps/api/src/auth/oauth-session-cookie.ts +++ b/apps/api/src/auth/oauth-session-cookie.ts @@ -15,10 +15,21 @@ import { uuidv7 } from 'uuidv7'; const OAUTH_SESSION_TTL_SECONDS = 10 * 60; const CLOCK_SKEW_SECONDS = 60; +/** + * The OAuth round-trip can be either a fresh sign-in (`login`, the default) + * or a link-existing-account-to-GitHub flow (`link`). `linkPersonId` is set + * iff `mode === 'link'` and identifies the signed-in Person who initiated + * the linking; the callback uses it to mutate the right Person record. + * + * Pre-link-flow cookies don't carry these fields; verify defaults `mode` + * to `'login'` so the existing flow stays back-compat. + */ export interface OAuthSessionClaims { readonly state: string; readonly codeVerifier: string; readonly return: string; + readonly mode?: 'login' | 'link'; + readonly linkPersonId?: string; } function keyBytes(signingKey: string): Uint8Array { @@ -30,18 +41,24 @@ export async function signOAuthSession( signingKey: string, ): Promise { const now = Math.floor(Date.now() / 1000); - return new SignJWT({ + const payload: Partial & { + state: string; + codeVerifier: string; + return: string; + scope: string; + mode?: 'login' | 'link'; + linkPersonId?: string; + } = { state: claims.state, codeVerifier: claims.codeVerifier, return: claims.return, scope: 'oauth_session', jti: uuidv7(), - } satisfies Partial & { - state: string; - codeVerifier: string; - return: string; - scope: string; - }) + }; + if (claims.mode) payload.mode = claims.mode; + if (claims.linkPersonId) payload.linkPersonId = claims.linkPersonId; + + return new SignJWT(payload) .setProtectedHeader({ alg: 'HS256' }) .setIssuedAt(now) .setExpirationTime(now + OAUTH_SESSION_TTL_SECONDS) @@ -69,5 +86,20 @@ export async function verifyOAuthSession( throw new Error('Invalid oauth session claims'); } - return { state, codeVerifier, return: returnUrl }; + // Default mode = 'login' for back-compat with cookies issued before + // the link-flow shipped. linkPersonId is only present in link mode. + const rawMode = payload['mode']; + const mode: 'login' | 'link' = rawMode === 'link' ? 'link' : 'login'; + const rawLinkPersonId = payload['linkPersonId']; + const linkPersonId = typeof rawLinkPersonId === 'string' ? rawLinkPersonId : undefined; + + if (mode === 'link' && !linkPersonId) { + throw new Error('Invalid oauth session claims: link mode requires linkPersonId'); + } + + const out: OAuthSessionClaims = { state, codeVerifier, return: returnUrl, mode }; + if (linkPersonId) { + return { ...out, linkPersonId }; + } + return out; } diff --git a/apps/api/src/routes/auth.ts b/apps/api/src/routes/auth.ts index f6851d2..c249c0c 100644 --- a/apps/api/src/routes/auth.ts +++ b/apps/api/src/routes/auth.ts @@ -41,7 +41,7 @@ import { signOAuthSession, verifyOAuthSession, } from '../auth/oauth-session-cookie.js'; -import { buildAuthorizeUrl, completeCallback } from '../auth/github-oauth.js'; +import { buildAuthorizeUrl, completeCallback, completeLinkCallback } from '../auth/github-oauth.js'; function clientIp(request: FastifyRequest): string { const forwarded = request.headers['x-forwarded-for']; @@ -75,6 +75,10 @@ function loginErrorRedirect(reply: FastifyReply, code: string): FastifyReply { return reply.redirect(`/login?error=${encodeURIComponent(code)}`); } +function accountErrorRedirect(reply: FastifyReply, code: string): FastifyReply { + return reply.redirect(`/account?error=${encodeURIComponent(code)}`); +} + async function persistSessionMetadata( fastify: FastifyInstance, request: FastifyRequest, @@ -207,9 +211,32 @@ export async function authRoutes(fastify: FastifyInstance): Promise { return loginErrorRedirect(reply, 'oauth_state_mismatch'); } + const isLinkMode = sessionClaims.mode === 'link'; + if (!query.code) { clearOAuthCookies(reply); - return loginErrorRedirect(reply, 'github_unreachable'); + return isLinkMode + ? accountErrorRedirect(reply, 'github_unreachable') + : loginErrorRedirect(reply, 'github_unreachable'); + } + + // Link mode: completely separate pipeline. No matching, no session + // mint — just bind the GitHub identity to the named Person and + // redirect back to /account. + if (isLinkMode && sessionClaims.linkPersonId) { + const linkOutcome = await completeLinkCallback({ + fastify, + request, + code: query.code, + codeVerifier: sessionClaims.codeVerifier, + redirectUri: callbackRedirectUri(request), + linkPersonId: sessionClaims.linkPersonId, + }); + clearOAuthCookies(reply); + if (linkOutcome.kind === 'error') { + return accountErrorRedirect(reply, linkOutcome.code); + } + return reply.redirect('/account?linked=github'); } // Pipeline: code → token → user/emails → match → outcome. @@ -274,6 +301,81 @@ export async function authRoutes(fastify: FastifyInstance): Promise { }, ); + // --------------------------------------------------------------------------- + // POST /api/auth/link-github — initiate GitHub-link flow for current session + // --------------------------------------------------------------------------- + // + // Per specs/api/auth.md `POST /api/auth/link-github`. Auth-required. Signs + // a link-mode `cfp_oauth_session` cookie carrying the current personId, + // then 302s to GitHub OAuth. The callback at `/api/auth/github/callback` + // recognizes the mode and binds the GitHub identity to the signed-in + // Person instead of minting a new session. + // --------------------------------------------------------------------------- + + fastify.post( + '/api/auth/link-github', + { + schema: { + tags: ['auth'], + summary: 'Link the current session to a GitHub identity', + querystring: { + type: 'object', + properties: { return: { type: 'string' } }, + }, + }, + }, + async (request, reply) => { + requireAuth(request, ['user']); + const cfg = fastify.config; + if (!cfg.GITHUB_OAUTH_CLIENT_ID || !cfg.GITHUB_OAUTH_CLIENT_SECRET) { + return accountErrorRedirect(reply, 'github_unreachable'); + } + + const personId = request.session.person?.id; + if (!personId) { + // requireAuth above already throws on no session; this is purely + // a type-narrowing guard for the linePersonId argument below. + throw new UnauthenticatedError('No session', 'no_session'); + } + + // Fast-fail before round-tripping to GitHub if already linked. + const person = fastify.inMemoryState.people.get(personId); + if (person && typeof person.githubUserId === 'number') { + return accountErrorRedirect(reply, 'github_already_linked'); + } + + const { return: returnParam } = request.query as { return?: string }; + const returnPath = safeReturnPath(returnParam) === '/' ? '/account' : safeReturnPath(returnParam); + + const state = generateCsrfState(); + const codeVerifier = generatePkceVerifier(); + const codeChallenge = pkceChallengeFromVerifier(codeVerifier); + + const sessionToken = await signOAuthSession( + { + state, + codeVerifier, + return: returnPath, + mode: 'link', + linkPersonId: personId, + }, + cfg.CFP_JWT_SIGNING_KEY, + ); + + setOAuthStateCookie(reply, state, cfg.NODE_ENV); + setOAuthSessionCookie(reply, sessionToken, cfg.NODE_ENV); + + const url = buildAuthorizeUrl({ + clientId: cfg.GITHUB_OAUTH_CLIENT_ID, + redirectUri: callbackRedirectUri(request), + state, + codeChallenge, + }); + + return reply.redirect(url); + }, + ); + // --------------------------------------------------------------------------- // POST /api/auth/login — legacy password sign-in // diff --git a/apps/api/src/services/github-account.ts b/apps/api/src/services/github-account.ts index ada94f7..29c35d7 100644 --- a/apps/api/src/services/github-account.ts +++ b/apps/api/src/services/github-account.ts @@ -179,4 +179,33 @@ export class GitHubAccountService { return { person: updated, stateApply, publicChanged }; } + + /** + * Bind a GitHub identity to a Person that currently has none. Used by + * `POST /api/auth/link-github`'s callback branch. Per + * specs/behaviors/account-migration.md the link records the GitHub + * fields but does NOT refresh `PrivateProfile.email` in v1 — that + * requires a consent toggle on a link-confirmation screen that + * doesn't yet exist. The user's existing email-on-file stays. + * + * Caller is responsible for the conflict checks (the Person isn't + * already linked, and the GitHub identity isn't bound to a *different* + * Person); this method assumes both invariants hold. + */ + async linkToExisting( + tx: DualStoreTx, + existing: Person, + identity: ResolvedGitHubIdentity, + ): Promise<{ person: Person; stateApply: StateApply }> { + const now = nowIso(); + const updated: Person = PersonSchema.parse({ + ...existing, + githubUserId: identity.id, + githubLogin: identity.login, + githubLinkedAt: now, + updatedAt: now, + }); + await tx.public.people.upsert(updated); + return { person: updated, stateApply: new StateApply().upsertPerson(updated) }; + } } diff --git a/apps/api/tests/auth-link-github.test.ts b/apps/api/tests/auth-link-github.test.ts new file mode 100644 index 0000000..20a72fd --- /dev/null +++ b/apps/api/tests/auth-link-github.test.ts @@ -0,0 +1,315 @@ +/** + * Tests for `POST /api/auth/link-github` + the GitHub callback's + * link-mode branch per specs/api/auth.md and + * specs/behaviors/account-migration.md. + * + * Two scenarios: + * 1. Initiate link — auth-required, fast-fail when already linked, + * happy path issues a link-mode oauth-session cookie and 302s to + * GitHub. + * 2. Callback link-mode — happy path mutates Person.githubUserId, + * conflict cases redirect to /account?error=. + * + * Each test uses a unique remoteAddress so the 10-req/min/IP cap on + * /api/auth/* doesn't cross-contaminate. + */ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { type FastifyInstance } from 'fastify'; +import { writeFile, readFile } from 'node:fs/promises'; +import { join } from 'node:path'; + +import { buildApp } from '../src/app.js'; +import { issueSession } from '../src/auth/jwt.js'; +import { verifyOAuthSession } from '../src/auth/oauth-session-cookie.js'; +import { createFullDataRepo, createPrivateStorageDir } from './helpers/test-full-repo.js'; +import { seedRawToml } from './helpers/seed-fixtures.js'; +import { createGitHubMock } from './helpers/mocks.js'; + +const JWT_KEY = 'test-jwt-signing-key-at-least-32-chars!!'; +const GH_CLIENT_ID = 'test-client-id'; +const GH_CLIENT_SECRET = 'test-client-secret'; + +let testIpCounter = 0; +function nextTestIp(): string { + testIpCounter += 1; + return `10.7.${Math.floor(testIpCounter / 250)}.${testIpCounter % 250}`; +} + +interface SeedPersonOpts { + readonly githubUserId?: number; + readonly githubLogin?: string; + readonly githubLinkedAt?: string; +} + +async function seedPerson( + repoDir: string, + slug: string, + id: string, + opts: SeedPersonOpts = {}, +): Promise { + const lines = [ + `id = "${id}"`, + `slug = "${slug}"`, + `fullName = "Test ${slug}"`, + `accountLevel = "user"`, + `createdAt = "2026-05-01T00:00:00Z"`, + `updatedAt = "2026-05-01T00:00:00Z"`, + ]; + if (opts.githubUserId !== undefined) lines.push(`githubUserId = ${opts.githubUserId}`); + if (opts.githubLogin !== undefined) lines.push(`githubLogin = "${opts.githubLogin}"`); + if (opts.githubLinkedAt !== undefined) lines.push(`githubLinkedAt = "${opts.githubLinkedAt}"`); + await seedRawToml(repoDir, `people/${slug}.toml`, lines.join('\n'), `seed person ${slug}`); +} + +async function seedPrivateProfile( + privatePath: string, + personId: string, + email: string, +): Promise { + const filePath = join(privatePath, 'profiles.jsonl'); + const profile = { + personId, + email: email.toLowerCase(), + emailRefreshedAt: '2026-05-01T00:00:00Z', + newsletter: null, + updatedAt: '2026-05-01T00:00:00Z', + }; + let content = ''; + try { + content = await readFile(filePath, 'utf8'); + } catch { + // first write + } + await writeFile(filePath, content + JSON.stringify(profile) + '\n'); +} + +async function buildTestApp( + dataPath: string, + privatePath: string, +): Promise { + return buildApp({ + serverOptions: { logger: false }, + overrideEnv: { + CFP_DATA_REPO_PATH: dataPath, + STORAGE_BACKEND: 'filesystem', + CFP_PRIVATE_STORAGE_PATH: privatePath, + CFP_JWT_SIGNING_KEY: JWT_KEY, + GITHUB_OAUTH_CLIENT_ID: GH_CLIENT_ID, + GITHUB_OAUTH_CLIENT_SECRET: GH_CLIENT_SECRET, + NODE_ENV: 'test', + }, + }); +} + +describe('POST /api/auth/link-github', () => { + let dataRepo: { path: string; cleanup: () => Promise }; + let privateStore: { path: string; cleanup: () => Promise }; + let app: FastifyInstance; + const unlinkedPersonId = '01951a3c-0000-7000-8000-0000ddddddd1'; + const linkedPersonId = '01951a3c-0000-7000-8000-0000ddddddd2'; + + beforeAll(async () => { + dataRepo = await createFullDataRepo(); + privateStore = await createPrivateStorageDir(); + await seedPerson(dataRepo.path, 'unlinked-user', unlinkedPersonId); + await seedPrivateProfile(privateStore.path, unlinkedPersonId, 'unlinked@example.com'); + await seedPerson(dataRepo.path, 'already-linked', linkedPersonId, { + githubUserId: 4242, + githubLogin: 'already-linked-gh', + githubLinkedAt: '2026-04-01T00:00:00Z', + }); + await seedPrivateProfile(privateStore.path, linkedPersonId, 'linked@example.com'); + app = await buildTestApp(dataRepo.path, privateStore.path); + }, 60_000); + + afterAll(async () => { + await app.close(); + await dataRepo.cleanup(); + await privateStore.cleanup(); + }); + + it('returns 401 unauthenticated when no session cookie is present', async () => { + const res = await app.inject({ + method: 'POST', + url: '/api/auth/link-github', + remoteAddress: nextTestIp(), + }); + expect(res.statusCode).toBe(401); + }); + + it('redirects to /account?error=github_already_linked when the caller already has a GitHub link', async () => { + const { access } = await issueSession(linkedPersonId, 'user', JWT_KEY, { + loginMethod: 'github', + }); + const res = await app.inject({ + method: 'POST', + url: '/api/auth/link-github', + remoteAddress: nextTestIp(), + cookies: { cfp_session: access }, + }); + expect(res.statusCode).toBe(302); + expect(String(res.headers['location'])).toBe('/account?error=github_already_linked'); + }); + + it('sets link-mode oauth-session cookie + state cookie + 302s to GitHub authorize', async () => { + const { access } = await issueSession(unlinkedPersonId, 'user', JWT_KEY, { + loginMethod: 'legacy_password', + }); + const res = await app.inject({ + method: 'POST', + url: '/api/auth/link-github', + remoteAddress: nextTestIp(), + cookies: { cfp_session: access }, + }); + expect(res.statusCode).toBe(302); + const location = String(res.headers['location']); + expect(location.startsWith('https://github.com/login/oauth/authorize')).toBe(true); + + const setCookies = res.headers['set-cookie']; + const cookies = Array.isArray(setCookies) ? setCookies : [String(setCookies ?? '')]; + const stateCookie = cookies.find((c) => c.startsWith('cfp_oauth_state=')); + const sessionCookie = cookies.find((c) => c.startsWith('cfp_oauth_session=')); + expect(stateCookie).toBeDefined(); + expect(sessionCookie).toBeDefined(); + + const sessionValue = sessionCookie!.split(';')[0]!.replace('cfp_oauth_session=', ''); + const claims = await verifyOAuthSession(sessionValue, JWT_KEY); + expect(claims.mode).toBe('link'); + expect(claims.linkPersonId).toBe(unlinkedPersonId); + expect(claims.return).toBe('/account'); + }); +}); + +// Helper for the callback tests below: drive a real link-mode start flow +// so the cookies + state + verifier are all consistent with what the route +// expects. +async function startLinkFlow( + app: FastifyInstance, + personId: string, + ip: string, +): Promise<{ state: string; stateCookie: string; oauthSessionCookie: string }> { + const { access } = await issueSession(personId, 'user', JWT_KEY, { + loginMethod: 'legacy_password', + }); + const res = await app.inject({ + method: 'POST', + url: '/api/auth/link-github', + remoteAddress: ip, + cookies: { cfp_session: access }, + }); + if (res.statusCode !== 302) { + throw new Error(`startLinkFlow: expected 302, got ${res.statusCode}`); + } + const url = new URL(String(res.headers['location'])); + const state = url.searchParams.get('state'); + const setCookies = res.headers['set-cookie']; + const cookies = Array.isArray(setCookies) ? setCookies : [String(setCookies ?? '')]; + const stateCookie = cookies.find((c) => c.startsWith('cfp_oauth_state='))!; + const sessionCookie = cookies.find((c) => c.startsWith('cfp_oauth_session='))!; + return { + state: state!, + stateCookie: stateCookie.split(';')[0]!.replace('cfp_oauth_state=', ''), + oauthSessionCookie: sessionCookie.split(';')[0]!.replace('cfp_oauth_session=', ''), + }; +} + +describe('GET /api/auth/github/callback — link mode', () => { + let dataRepo: { path: string; cleanup: () => Promise }; + let privateStore: { path: string; cleanup: () => Promise }; + let app: FastifyInstance; + const mock = createGitHubMock(); + const linkingPersonId = '01951a3c-0000-7000-8000-0000eeeeeee1'; + const otherPersonId = '01951a3c-0000-7000-8000-0000eeeeeee2'; + + beforeAll(async () => { + mock.server.listen({ onUnhandledRequest: 'error' }); + dataRepo = await createFullDataRepo(); + privateStore = await createPrivateStorageDir(); + // Person doing the linking — has a legacy credential, no GitHub link. + await seedPerson(dataRepo.path, 'linking-user', linkingPersonId); + await seedPrivateProfile(privateStore.path, linkingPersonId, 'linking@example.com'); + // Another Person already bound to GitHub id 55555 — collision target. + await seedPerson(dataRepo.path, 'other-user', otherPersonId, { + githubUserId: 55555, + githubLogin: 'someone-else-gh', + githubLinkedAt: '2026-04-01T00:00:00Z', + }); + await seedPrivateProfile(privateStore.path, otherPersonId, 'other@example.com'); + + app = await buildTestApp(dataRepo.path, privateStore.path); + + mock.setTokenResponse({ + access_token: 'gho_link_test', + token_type: 'bearer', + scope: 'read:user,user:email', + }); + }, 60_000); + + afterAll(async () => { + mock.server.close(); + await app.close(); + await dataRepo.cleanup(); + await privateStore.cleanup(); + }); + + it('redirects /account?error=github_id_in_use_elsewhere when gh.id is bound to a different Person', async () => { + mock.setGitHubUser({ id: 55555, login: 'someone-else-gh', name: 'Other', avatar_url: 'x' }); + mock.setGitHubEmails([ + { email: 'other@example.com', primary: true, verified: true, visibility: 'public' }, + ]); + const ip = nextTestIp(); + const flow = await startLinkFlow(app, linkingPersonId, ip); + const res = await app.inject({ + method: 'GET', + url: `/api/auth/github/callback?code=abc&state=${encodeURIComponent(flow.state)}`, + remoteAddress: ip, + cookies: { + cfp_oauth_state: flow.stateCookie, + cfp_oauth_session: flow.oauthSessionCookie, + }, + }); + expect(res.statusCode).toBe(302); + expect(String(res.headers['location'])).toBe('/account?error=github_id_in_use_elsewhere'); + // Did NOT mutate the linking Person + const person = app.inMemoryState.people.get(linkingPersonId); + expect(person?.githubUserId).toBeUndefined(); + }); + + it('links the calling Person to GitHub and redirects /account?linked=github', async () => { + mock.setGitHubUser({ id: 77777, login: 'linking-user-gh', name: 'Linking User', avatar_url: 'x' }); + mock.setGitHubEmails([ + { email: 'linking-gh@example.com', primary: true, verified: true, visibility: 'public' }, + ]); + const ip = nextTestIp(); + const flow = await startLinkFlow(app, linkingPersonId, ip); + const res = await app.inject({ + method: 'GET', + url: `/api/auth/github/callback?code=abc&state=${encodeURIComponent(flow.state)}`, + remoteAddress: ip, + cookies: { + cfp_oauth_state: flow.stateCookie, + cfp_oauth_session: flow.oauthSessionCookie, + }, + }); + expect(res.statusCode).toBe(302); + expect(String(res.headers['location'])).toBe('/account?linked=github'); + + // Person mutated: gh fields populated + const person = app.inMemoryState.people.get(linkingPersonId); + expect(person?.githubUserId).toBe(77777); + expect(person?.githubLogin).toBe('linking-user-gh'); + expect(person?.githubLinkedAt).toBeDefined(); + + // PrivateProfile email NOT auto-refreshed (deferred in v1) + const profile = await app.store.private.getProfile(linkingPersonId); + expect(profile?.email).toBe('linking@example.com'); + + // No new session cookies — the user was already signed-in. + const setCookies = res.headers['set-cookie']; + const cookies = Array.isArray(setCookies) ? setCookies : [String(setCookies ?? '')]; + const newSession = cookies.find( + (c) => c.startsWith('cfp_session=') && !c.startsWith('cfp_session=;'), + ); + expect(newSession).toBeUndefined(); + }); +}); diff --git a/apps/web/src/hooks/useAuth.tsx b/apps/web/src/hooks/useAuth.tsx index 0a0327d..055384d 100644 --- a/apps/web/src/hooks/useAuth.tsx +++ b/apps/web/src/hooks/useAuth.tsx @@ -10,6 +10,8 @@ import { export type AccountLevel = 'anonymous' | 'user' | 'staff' | 'administrator'; +export type LoginMethod = 'github' | 'legacy_password' | 'password_reset'; + export interface AuthPerson { id: string; slug: string; @@ -23,6 +25,10 @@ export interface AuthState { person: AuthPerson | null; /** true while the initial /api/auth/me fetch is in flight */ loading: boolean; + /** Whether the current Person has a GitHub identity bound. False when anonymous. */ + hasGitHubLink: boolean; + /** How the current session was minted. null for anonymous or pre-loginMethod sessions. */ + lastLoginMethod: LoginMethod | null; /** Reload auth state from the server */ reload: () => Promise; /** Sign out: calls POST /api/auth/logout then clears state */ @@ -35,35 +41,53 @@ interface MeEnvelope { data?: { person?: AuthPerson | null; accountLevel?: AccountLevel; + hasGitHubLink?: boolean; + lastLoginMethod?: LoginMethod | null; }; } -async function fetchMe(): Promise { +interface MeSnapshot { + person: AuthPerson | null; + hasGitHubLink: boolean; + lastLoginMethod: LoginMethod | null; +} + +const ANON_SNAPSHOT: MeSnapshot = { + person: null, + hasGitHubLink: false, + lastLoginMethod: null, +}; + +async function fetchMe(): Promise { try { const res = await fetch('/api/auth/me', { credentials: 'include' }); if (!res.ok) { // 401 = anonymous, 404 = not yet implemented — both mean no session - return null; + return ANON_SNAPSHOT; } const json = (await res.json()) as MeEnvelope; - return json.data?.person ?? null; + return { + person: json.data?.person ?? null, + hasGitHubLink: json.data?.hasGitHubLink ?? false, + lastLoginMethod: json.data?.lastLoginMethod ?? null, + }; } catch { // Network error — treat as anonymous, don't throw - return null; + return ANON_SNAPSHOT; } } export function AuthProvider({ children }: { children: ReactNode }) { - const [person, setPerson] = useState(null); + const [snapshot, setSnapshot] = useState(ANON_SNAPSHOT); const [loading, setLoading] = useState(true); // Use a ref to track if we're mounted so we don't setState after unmount const mountedRef = useRef(true); const reload = useCallback(async () => { setLoading(true); - const p = await fetchMe(); + const snap = await fetchMe(); if (mountedRef.current) { - setPerson(p); + setSnapshot(snap); setLoading(false); } }, []); @@ -78,7 +102,7 @@ export function AuthProvider({ children }: { children: ReactNode }) { // Ignore network errors on logout } if (mountedRef.current) { - setPerson(null); + setSnapshot(ANON_SNAPSHOT); } }, []); @@ -87,9 +111,9 @@ export function AuthProvider({ children }: { children: ReactNode }) { // Kick off initial auth check; all setState calls happen in the async // callback, not synchronously in the effect body. fetchMe() - .then((p) => { + .then((snap) => { if (mountedRef.current) { - setPerson(p); + setSnapshot(snap); setLoading(false); } }) @@ -105,7 +129,16 @@ export function AuthProvider({ children }: { children: ReactNode }) { }, []); return ( - + {children} ); diff --git a/apps/web/src/screens/Account.tsx b/apps/web/src/screens/Account.tsx index bcac97b..b808ffd 100644 --- a/apps/web/src/screens/Account.tsx +++ b/apps/web/src/screens/Account.tsx @@ -1,5 +1,5 @@ import { useEffect, useState } from 'react'; -import { Link, useNavigate } from 'react-router'; +import { Link, useNavigate, useSearchParams } from 'react-router'; import { useQuery, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner'; import { Button } from '@/components/ui/button'; @@ -25,10 +25,40 @@ function parseUA(ua: string): string { return os ? `${browser} on ${os}` : browser; } +const LINK_GITHUB_ERROR_MESSAGES: Record = { + github_already_linked: 'Your account is already connected to GitHub.', + github_id_in_use_elsewhere: + 'That GitHub account is already connected to a different Code for Philly account. Email accounts@codeforphilly.org if this is a mistake.', + github_unreachable: 'We could not reach GitHub. Please try again in a moment.', + oauth_state_mismatch: 'Something went wrong with the GitHub link flow. Please try again.', + oauth_session_invalid: 'Something went wrong with the GitHub link flow. Please try again.', +}; + export function Account() { - const { person, loading, signOut, reload } = useAuth(); + const { person, loading, signOut, reload, hasGitHubLink, lastLoginMethod } = useAuth(); const navigate = useNavigate(); const queryClient = useQueryClient(); + const [searchParams, setSearchParams] = useSearchParams(); + const [bannerDismissed, setBannerDismissed] = useState(false); + + // Toast on /account?linked=github or ?error= from the link-flow + // callback, then strip the param so reloading doesn't re-toast. + useEffect(() => { + const linked = searchParams.get('linked'); + const errorCode = searchParams.get('error'); + if (linked === 'github') { + toast.success('GitHub account connected.'); + void reload(); + const next = new URLSearchParams(searchParams); + next.delete('linked'); + setSearchParams(next, { replace: true }); + } else if (errorCode && errorCode in LINK_GITHUB_ERROR_MESSAGES) { + toast.error(LINK_GITHUB_ERROR_MESSAGES[errorCode]!); + const next = new URLSearchParams(searchParams); + next.delete('error'); + setSearchParams(next, { replace: true }); + } + }, [searchParams, setSearchParams, reload]); const sessionsQ = useQuery({ queryKey: ['auth-sessions'], @@ -85,17 +115,52 @@ export function Account() { const sessions = sessionsQ.data?.data ?? []; + const showConnectGitHubBanner = + !bannerDismissed && + !hasGitHubLink && + (lastLoginMethod === 'legacy_password' || lastLoginMethod === 'password_reset'); + return (

Account Settings

+ {showConnectGitHubBanner && ( +
+
+

Connect your GitHub account

+

+ Faster sign-in next time, and one less password to remember. Code + for Philly plans to retire password sign-in eventually — link + GitHub now to stay ahead. +

+
+
+ +
+ +
+ )} + {/* Identity */} Identity - Your sign-in identity is sourced from GitHub. To change your name or - email, update them on GitHub and sign back in. + {hasGitHubLink + ? 'Your sign-in identity is sourced from GitHub. To change your name or email, update them on GitHub and sign back in.' + : 'Connect a GitHub account to use GitHub sign-in. You can still use your existing password until you do.'} @@ -103,18 +168,26 @@ export function Account() {
GitHub
- Connected — primary identity + {hasGitHubLink ? 'Connected — primary identity' : 'Not connected'}
- + {hasGitHubLink ? ( + + ) : ( +
+ +
+ )}
Slack
diff --git a/apps/web/tests/Account.test.tsx b/apps/web/tests/Account.test.tsx new file mode 100644 index 0000000..9c98a6a --- /dev/null +++ b/apps/web/tests/Account.test.tsx @@ -0,0 +1,157 @@ +/** + * Account screen tests focused on the phase-D additions: + * - Connect-GitHub banner renders only for legacy-credential users + * who haven't linked yet. + * - Identity card swaps "Manage on GitHub" for "Connect GitHub" when + * hasGitHubLink is false. + * - Banner dismiss button hides it for the rest of the session. + * + * The existing Account page predates this test file — these tests focus + * narrowly on the banner + identity-card branches and leave the + * newsletter / sessions / claim-legacy regions alone. + */ +import { describe, expect, it, vi, afterEach } from 'vitest'; +import { fireEvent, screen, waitFor } from '@testing-library/react'; +import { renderScreen, mockOk } from './test-utils.js'; +import { Account } from '../src/screens/Account.js'; +import { AuthProvider } from '../src/hooks/useAuth.js'; + +interface MeShape { + person: { id: string; slug: string; fullName: string; accountLevel: string; avatarUrl: string | null } | null; + accountLevel: string; + hasGitHubLink: boolean; + lastLoginMethod: 'github' | 'legacy_password' | 'password_reset' | null; +} + +function mockApi(me: MeShape): void { + vi.spyOn(globalThis, 'fetch').mockImplementation(((input: string) => { + if (input.startsWith('/api/auth/me')) { + return Promise.resolve( + new Response(JSON.stringify(mockOk(me)), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ); + } + if (input.startsWith('/api/auth/sessions')) { + return Promise.resolve( + new Response(JSON.stringify(mockOk([])), { + status: 200, + headers: { 'content-type': 'application/json' }, + }), + ); + } + return Promise.resolve(new Response(null, { status: 404 })); + }) as typeof fetch); +} + +const legacyPerson: MeShape = { + person: { + id: '01951a3c-0000-7000-8000-0000ffffff01', + slug: 'legacy-user', + fullName: 'Legacy User', + accountLevel: 'user', + avatarUrl: null, + }, + accountLevel: 'user', + hasGitHubLink: false, + lastLoginMethod: 'legacy_password', +}; + +const githubPerson: MeShape = { + person: { + id: '01951a3c-0000-7000-8000-0000ffffff02', + slug: 'gh-user', + fullName: 'GH User', + accountLevel: 'user', + avatarUrl: null, + }, + accountLevel: 'user', + hasGitHubLink: true, + lastLoginMethod: 'github', +}; + +function render() { + return renderScreen( + + + , + { initialEntries: ['/account'] }, + ); +} + +describe('Account — Connect-GitHub banner', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('renders for a legacy-password user with no GitHub link', async () => { + mockApi(legacyPerson); + render(); + await waitFor(() => { + expect( + screen.getByRole('region', { name: /connect github/i }), + ).toBeInTheDocument(); + }); + // Banner has a Connect button + a Dismiss button + const region = screen.getByRole('region', { name: /connect github/i }); + expect(region.querySelector('form[action="/api/auth/link-github"]')).not.toBeNull(); + expect(screen.getByRole('button', { name: /dismiss/i })).toBeInTheDocument(); + }); + + it('does not render for a github-signed-in user', async () => { + mockApi(githubPerson); + render(); + // Wait for the Identity card to render — that proves the page is past loading. + await waitFor(() => { + expect(screen.getByText(/connected — primary identity/i)).toBeInTheDocument(); + }); + expect( + screen.queryByRole('region', { name: /connect github/i }), + ).not.toBeInTheDocument(); + }); + + it('dismiss button hides the banner for the rest of the session', async () => { + mockApi(legacyPerson); + render(); + const dismissBtn = await screen.findByRole('button', { name: /dismiss/i }); + fireEvent.click(dismissBtn); + await waitFor(() => { + expect( + screen.queryByRole('region', { name: /connect github/i }), + ).not.toBeInTheDocument(); + }); + }); +}); + +describe('Account — Identity card', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('shows the "Connect GitHub" form when hasGitHubLink is false', async () => { + mockApi(legacyPerson); + render(); + await waitFor(() => { + expect(screen.getByText(/not connected/i)).toBeInTheDocument(); + }); + // Two forms post to the link endpoint: banner + identity card + const forms = document.querySelectorAll('form[action="/api/auth/link-github"]'); + expect(forms.length).toBeGreaterThanOrEqual(2); + }); + + it('shows the "Manage on GitHub" link when hasGitHubLink is true', async () => { + mockApi(githubPerson); + render(); + await waitFor(() => { + expect(screen.getByText(/connected — primary identity/i)).toBeInTheDocument(); + }); + expect( + screen.getByRole('link', { name: /manage on github/i }), + ).toHaveAttribute('href', 'https://github.com/settings'); + // No link-github form when already connected + expect( + document.querySelectorAll('form[action="/api/auth/link-github"]').length, + ).toBe(0); + }); +}); From 033ac6d07d71705201090973bf4e08a4c786e24f Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Sun, 31 May 2026 16:30:33 -0400 Subject: [PATCH 2/3] chore(plans): open login-migration-impl-phase-d (in-progress) Final phase of the login-migration track. Will flip to done in a closeout commit once the PR opens. Co-Authored-By: Claude Opus 4.7 (1M context) --- plans/login-migration-impl-phase-d.md | 145 ++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 plans/login-migration-impl-phase-d.md diff --git a/plans/login-migration-impl-phase-d.md b/plans/login-migration-impl-phase-d.md new file mode 100644 index 0000000..b5ae210 --- /dev/null +++ b/plans/login-migration-impl-phase-d.md @@ -0,0 +1,145 @@ +--- +status: in-progress +depends: [login-migration-impl-phase-c] +specs: + - specs/api/auth.md + - specs/behaviors/account-migration.md + - specs/screens/account.md +issues: [] +pr: +--- + +# Plan: login-migration impl — phase D (link-github) + +## Scope + +Final phase of the [login-migration-strategy](./login-migration-strategy.md) implementation track. Implements `POST /api/auth/link-github` + the link-mode GitHub OAuth callback variant + the `/account` banner that nags legacy-password users to bind a GitHub identity. + +What ships: + +- **`POST /api/auth/link-github`** — auth-required; sets a link-mode `cfp_oauth_session` cookie and redirects to GitHub OAuth. +- **GitHub callback branches on mode.** Existing `mode = 'login'` path unchanged. New `mode = 'link'` path looks up the GitHub identity, rejects 409 `github_already_linked` / 409 `github_id_in_use_elsewhere`, mutates the Person, redirects to `/account?linked=github` (or 302-error to `/account?error=`). +- **`/account` banner.** Persistent, dismissible-per-session banner above the cards when `hasGitHubLink === false` and `lastLoginMethod` is `'legacy_password'` or `'password_reset'`. CTA: "Connect GitHub" → POST link-github. +- **Identity card update.** Replaces the hardcoded "GitHub — Connected" with conditional rendering: connected when `hasGitHubLink`, otherwise a "Connect GitHub" button + a short explainer. + +## Implements + +- [api/auth.md](../specs/api/auth.md) — `POST /api/auth/link-github` endpoint + callback's link-mode error codes. +- [behaviors/account-migration.md](../specs/behaviors/account-migration.md) — the nag-toward-linking story now has a UI surface. +- [screens/account.md](../specs/screens/account.md) — banner + identity-card linking flow. + +## Approach + +### 1. Extend `OAuthSessionClaims` with mode + linkPersonId + +`apps/api/src/auth/oauth-session-cookie.ts` — add two optional fields: + +```ts +export interface OAuthSessionClaims { + state: string; + codeVerifier: string; + return: string; + mode?: 'login' | 'link'; // default 'login' for back-compat + linkPersonId?: string; // present iff mode === 'link' +} +``` + +`signOAuthSession` writes them when set; `verifyOAuthSession` parses them back, defaulting `mode` to `'login'` when absent. Existing tests + cookies issued by `/api/auth/github/start` keep working unchanged. + +### 2. `POST /api/auth/link-github` route + +In `apps/api/src/routes/auth.ts`. Auth-required (uses `requireAuth`). Pipeline: + +1. Validate session → personId. +2. Fast-fail if the Person already has a GitHub link (409 `github_already_linked`). +3. Generate state + PKCE; sign an oauth-session cookie with `mode: 'link'` + `linkPersonId: personId`. +4. Set state + session cookies; redirect to `buildAuthorizeUrl(...)`. + +Body is empty. The route returns 302. Rate-limit covered by the global `/api/auth/*` 10/min/IP cap. + +### 3. GitHub callback link-mode branch + +The route handler at `/api/auth/github/callback` already verifies the state + oauth-session cookie. New code: if `sessionClaims.mode === 'link'`, the entire `completeCallback(...)` path is skipped (no session minting / no Person matching). Instead, a new `completeLinkCallback(...)` helper: + +1. Exchange code → token (same `exchangeCodeForToken`). +2. Fetch identity (same `fetchGitHubUser` + `fetchGitHubEmails` + `resolveIdentitySnapshot`). +3. Look up the linking Person. If already linked → `github_already_linked`. +4. Search `inMemoryState.personIdByGithubUserId` for `String(identity.id)`. If found on a *different* person → `github_id_in_use_elsewhere`. +5. Transact a Person mutation: `githubUserId`, `githubLogin`, `githubLinkedAt = now`. Optionally refresh `PrivateProfile.email` if the GitHub primary email differs and the user hasn't customized — for v1, **do not** refresh the email automatically; the spec says "only if the user consents at the link-confirmation screen" and we don't have that screen yet. Future ticket. +6. Apply stateApply; redirect to `/account?linked=github` (success) or `/account?error=`. + +The error redirect target differs from the login path (`/login?error=...` for login, `/account?error=...` for link) because the user is signed-in throughout. + +### 4. SPA — `api.auth.linkGitHub()` helper + +A thin wrapper that POSTs to `/api/auth/link-github` and *follows the redirect* in the browser. Since `POST` → 302 doesn't follow naturally in fetch, the cleanest UX is to render this as a form submission (or simply `window.location.assign('/api/auth/link-github')` + accept the implied "this requires a POST" → use a small hidden form). Actually simpler: the spec says the route is `POST /api/auth/link-github`; we render an HTML `
` and submit it on button click. That hits the route as a real form post, the route 302s, the browser follows → GitHub. Round-trip without JS-fetch ceremony. + +### 5. `/account` banner + +In `apps/web/src/screens/Account.tsx`. New component `` rendered above the cards when: + +- `person !== null` and `auth.hasGitHubLink === false` and `auth.lastLoginMethod !== 'github'`. + +Contents: + +- One-sentence pitch: "Connect your GitHub account for faster sign-in next time — and so we can sunset password sign-in eventually." +- Primary CTA: "Connect GitHub" (a `` POST to `/api/auth/link-github`). +- Dismissable for the session via `useState`; persistence across reloads is not v1 (the spec says persistent, not dismissable-forever). + +Also surface success via `?linked=github` toast. + +Identity card update: render "GitHub: not connected" + a small "Connect GitHub" button as the secondary entry-point when `hasGitHubLink` is false. + +### 6. Extend `AuthState` + +`useAuth()` currently exposes `person`, `loading`, `reload`, `signOut`. Add `hasGitHubLink: boolean` and `lastLoginMethod: 'github' | 'legacy_password' | 'password_reset' | null` so the banner + identity card can branch without an extra fetch. Wire from `/api/auth/me` response (already returned by the API since phase B). + +### 7. Tests + +- **`apps/api/tests/auth-link-github.test.ts` (new)** — 5 cases: + - 401 for unauthenticated POST. + - 409 `github_already_linked` when the calling person already has a GitHub link. + - Happy path: sets oauth-session cookie with `mode = 'link'` + `linkPersonId`, sets state cookie, redirects to GitHub authorize URL. + - Callback link-mode: rejects 409 `github_id_in_use_elsewhere` when GitHub identity matches a different Person. + - Callback link-mode happy path: mutates Person.githubUserId etc., redirects to `/account?linked=github`. +- **`apps/web/tests/Account.test.tsx` (new — first Account screen test file)** — 3 cases: + - Banner renders for legacy-password user with `hasGitHubLink: false`. + - Banner does NOT render for github-signed-in user. + - Banner dismiss button hides it for the session. + +## Validation + +- [x] `POST /api/auth/link-github` route requires auth, sets link-mode cookie, redirects to GitHub. +- [x] Callback recognizes `mode = 'link'`, branches into link path, mutates Person on success, redirects to `/account?linked=github`. +- [x] Conflict cases (`github_already_linked`, `github_id_in_use_elsewhere`) return 302 to `/account?error=`. +- [x] `/account` banner renders only when `hasGitHubLink === false` AND `lastLoginMethod` ∈ `{'legacy_password', 'password_reset'}`. +- [x] Identity card on `/account` shows "Connect GitHub" CTA when `hasGitHubLink` is false. +- [x] `useAuth()` exposes `hasGitHubLink` + `lastLoginMethod`. +- [x] `npm run type-check && npm run lint` clean. +- [x] 5 new API tests pass; 5 new web tests pass; full sweep (api 387 + web 68 + shared 75) clean. + +## Risks / unknowns + +- **PrivateProfile email refresh on link is skipped in v1.** The spec calls for a consent toggle at the link confirmation screen ("keep current email" vs "use GitHub email"). v1 ships without the toggle and without the auto-refresh — the link only updates the Person's GitHub fields. The email-refresh path is deferred to a future ticket; legacy-imported emails stay until the user edits them on profile-edit. +- **Conflict on `github_id_in_use_elsewhere` is staff-mediated.** The spec doesn't define a self-service merge flow; the user just sees the error code and has to email staff. v1 surfaces the error; staff merge tooling is out of scope. +- **Older sessions without `loginMethod` claim.** Pre-phase-B sessions report `lastLoginMethod: null`. For the banner predicate, `null` is treated as "don't show" — these are GitHub-linked anyway (the only kind issued before phase B), so the identity card already says "connected." Benign. +- **Banner dismissal is session-only.** No localStorage / cookie persistence. Acceptable v1 — the banner exists to nag, and a per-tab dismiss is enough relief; rendering it again next session is a feature, not a bug. + +## Notes + +Shipped clean — all 5 API tests + 5 web tests pass on first full sweep. Type-check + lint green. + +Surprises: + +- **The route is a POST with no body**, which is a slightly awkward affordance from the SPA: we render it as an HTML `` and let the browser submit + follow the 302. The alternative (fetch with `redirect: 'manual'` + JS-side `window.location.assign(...)`) was strictly worse — same number of round-trips, more code, and no CSRF win since the cookie does the gating already. Form post matches the spec semantics directly. +- **`hasGitHubLink` derivation lives in two places.** The API computes it from `Person.githubUserId !== null` in `/api/auth/me`. The SPA passes it through `useAuth()` from the `me` response. Resist the urge to also derive it client-side from `person.githubUserId` — that field isn't in the SPA's `AuthPerson` shape, and pulling it through would widen the auth surface for no benefit. +- **OAuth-session cookie schema is forward-extensible.** Existing pre-link-flow cookies don't carry `mode` or `linkPersonId`; the verifier defaults `mode` to `'login'`. Worth noting because it sets a precedent: future flow modes (e.g., admin-impersonate, OAuth scope-upgrade) can ride the same cookie with no compatibility shim. The 10-minute TTL bounds the back-compat window anyway. +- **No CardTitle role=heading in shadcn.** First test pass used `getByRole('heading', { name: /identity/i })` — shadcn's CardTitle renders as a div by default. Switched the wait-for selector to text-based. Worth remembering for future Account tests: shadcn Cards aren't semantic headings unless you wrap them. +- **`completeLinkCallback` is intentionally separate from `completeCallback`.** Sharing code would mean threading a `mode` discriminator through identity-resolution, session-mint, and notifier logic that aren't relevant to linking. The two pipelines share only the prefix (token exchange + identity fetch); after that they diverge entirely. Easier to keep them parallel. + +## Follow-ups + +- **Email-refresh consent toggle on link.** Spec calls for a confirmation screen that lets the user opt to swap their on-file email to GitHub's primary. v1 skips both the screen and the auto-swap. *Tracked as* — a follow-up issue file when the SPA gets the screen design. +- **Self-service GitHub unlink.** Not in v1 per spec. *None* — staff-mediated only. +- **Sunset commitment for password sign-in.** The strategy plan deferred this; once the coverage metric (legacy-password users with linked GitHub) crosses some threshold, we should publish a sunset date. *None* — wait for the data. +- **Login-migration track complete.** Phases A-D shipped: verifier (A), legacy password login route (B), password reset (C), link-github + banner (D). The track is **closed** as a feature initiative; future work (sunset, unlink, email-consent) gets its own plans. From 5098d32c1b434b818777bcda9e6f7e999c34d1c1 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Sun, 31 May 2026 16:31:14 -0400 Subject: [PATCH 3/3] chore(plans): close out login-migration-impl-phase-d (PR #121) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All validation items ticked, PR opened. Status: in-progress → done. This is the final phase of the login-migration impl track. Co-Authored-By: Claude Opus 4.7 (1M context) --- plans/login-migration-impl-phase-d.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plans/login-migration-impl-phase-d.md b/plans/login-migration-impl-phase-d.md index b5ae210..db53d6a 100644 --- a/plans/login-migration-impl-phase-d.md +++ b/plans/login-migration-impl-phase-d.md @@ -1,12 +1,12 @@ --- -status: in-progress +status: done depends: [login-migration-impl-phase-c] specs: - specs/api/auth.md - specs/behaviors/account-migration.md - specs/screens/account.md issues: [] -pr: +pr: 121 --- # Plan: login-migration impl — phase D (link-github)