Skip to content

Commit 0ceaed8

Browse files
chore(plans): mark github-oauth done (PR #41)
1 parent cd7b079 commit 0ceaed8

2 files changed

Lines changed: 29 additions & 11 deletions

File tree

‎plans/account-claim.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ Audit-logged via commit trailers (`Action: account-claim.approve`, etc.).
131131

132132
### Screens (`apps/web/src/pages/AccountClaim*.tsx`)
133133

134-
- `/account-claim` — main entry, fetches candidates, routes by N=0/1/N
134+
- `/account-claim` — main entry, fetches candidates, routes by N=0/1/N. **Replaces the placeholder shipped by [`github-oauth`](github-oauth.md)** at `apps/web/src/pages/AccountClaimPlaceholder.tsx`; the existing route in `apps/web/src/App.tsx` should be repointed to the new screen and the placeholder file deleted as part of this plan.
135135
- `/account-claim/by-password` — username + password form
136136
- `/account-claim/request-staff-review` — evidence textarea, submit
137137
- `/account/claim-legacy` — post-onboarding search box → either auto-resolve (email match found, similar confirmation) or "submit for staff review" with the merge framing
@@ -155,6 +155,7 @@ A staff-only screen at `/staff/account-claim` listing open requests with approve
155155
- [ ] No PII appears in any commit message body or trailer (verified by inspecting commits produced by the test)
156156
- [ ] Anti-enumeration: error responses for unknown slugs are indistinguishable from wrong-password / not-yet-existed
157157
- [ ] Tests cover each path with the GitHub mocks + the test-harness stores
158+
- [ ] `AccountClaimPlaceholder` from [`github-oauth`](github-oauth.md) is removed and its `/account-claim` route now points at the real screen
158159

159160
## Risks / unknowns
160161

‎plans/github-oauth.md‎

Lines changed: 27 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
---
2-
status: in-progress
2+
status: done
33
depends: [write-api]
44
specs:
55
- specs/api/auth.md
66
- specs/behaviors/account-migration.md
77
- specs/screens/login.md
88
issues: []
9+
pr: 41
910
---
1011

1112
# Plan: GitHub OAuth
@@ -129,15 +130,15 @@ Per [api/auth.md](../specs/api/auth.md): redirect to `/login?error=<code>` on ea
129130

130131
## Validation
131132

132-
- [ ] OAuth happy path: never-seen-this-user → clicks Sign in with GitHub → GitHub auth → callback → fresh Person + PrivateProfile created → session issued → redirected to `/`
133-
- [ ] OAuth returning user: a Person with `githubUserId` set → callback → session issued → no Person/PrivateProfile mutations beyond email refresh + githubLogin refresh
134-
- [ ] OAuth with candidates: a Person without `githubUserId` exists whose PrivateProfile.email matches a GitHub-verified email → callback → claim-pending JWT issued → redirected to `/account-claim` placeholder
135-
- [ ] CSRF: tampering with the `state` query param → 401 `oauth_state_mismatch`
136-
- [ ] PKCE: GitHub returns an error → handled gracefully → redirected to `/login?error=…`
137-
- [ ] User denies on GitHub (`error=access_denied`) → redirected to `/login?error=access_denied` with the documented message
138-
- [ ] GitHub returns no verified emails → redirected to `/login?error=email_unverified` with the documented help message
139-
- [ ] `cfp_oauth_session` cookie expires after 10 minutes; expired sessions fail with `oauth_session_invalid`
140-
- [ ] Tests: mock GitHub via the test-harness mocks; cover each outcome (existing / fresh / candidates) + each error mode
133+
- [x] OAuth happy path: never-seen-this-user → clicks Sign in with GitHub → GitHub auth → callback → fresh Person + PrivateProfile created → session issued → redirected to `/`
134+
- [x] OAuth returning user: a Person with `githubUserId` set → callback → session issued → no Person/PrivateProfile mutations beyond email refresh + githubLogin refresh
135+
- [x] OAuth with candidates: a Person without `githubUserId` exists whose PrivateProfile.email matches a GitHub-verified email → callback → claim-pending JWT issued → redirected to `/account-claim` placeholder
136+
- [x] CSRF: tampering with the `state` query param → 401 `oauth_state_mismatch`
137+
- [x] PKCE: GitHub returns an error → handled gracefully → redirected to `/login?error=…`
138+
- [x] User denies on GitHub (`error=access_denied`) → redirected to `/login?error=access_denied` with the documented message
139+
- [x] GitHub returns no verified emails → redirected to `/login?error=email_unverified` with the documented help message
140+
- [x] `cfp_oauth_session` cookie expires after 10 minutes; expired sessions fail with `oauth_session_invalid`
141+
- [x] Tests: mock GitHub via the test-harness mocks; cover each outcome (existing / fresh / candidates) + each error mode
141142

142143
## Risks / unknowns
143144

@@ -146,3 +147,19 @@ Per [api/auth.md](../specs/api/auth.md): redirect to `/login?error=<code>` on ea
146147
- **`gh.login` slugify collisions.** `kebab-case-this-name` might already be taken by an existing legacy slug. The dedup-with-`-2`/`-3` is a clean fallback but the resulting fresh slug could look weird (`jane-doe-3`). Acceptable for v1; staff can rename later if needed.
147148

148149
## Notes
150+
151+
- **Hand-rolled PKCE over `@fastify/oauth2`.** Verifier = base64url(32 random bytes); challenge = base64url(sha256(verifier)). Keeps the dependency surface small and the flow legible. The carry-state cookie is a signed JWT (10 min) carrying `{ state, codeVerifier, return }` so the verifier survives the GitHub round-trip without server-side state.
152+
- **State + session cookies both scoped to `/api/auth`.** Tightens blast radius vs `Path=/` and keeps them out of every other request's cookie jar. Cleared on every callback regardless of outcome.
153+
- **Redirect-for-every-error.** The spec lists 401/403/502 status codes for some OAuth error modes; the github-oauth flow is browser-driven so the implementation always redirects to `/login?error=<code>`. Validation criterion #4's "401" wording reflects the spec's status code; the implemented behavior (redirect carrying the same code) was the plan's explicit choice. See follow-up #42.
154+
- **`callbackRedirectUri` is derived from the inbound request** (honoring `X-Forwarded-Proto` and `X-Forwarded-Host`) so dev/staging/prod each round-trip to themselves without an env var per environment. The deployed OAuth Apps still need their callback URLs registered at github.com/settings/developers.
155+
- **Fresh-user slug derivation.** `slugifyGitHubLogin(login, ghId)` lowercases, handles reserved-slug collision (`user-<login>`), and falls back to `user-<gh-id>` if both shape and reservation lose. `ensureUniqueSlug` then dedupes with `-2`/`-3` against the in-memory `personIdBySlug` index.
156+
- **Fresh-user transaction uses `writeOrder: 'private-first'`.** If the private-profile flush fails, the public Person commit never lands — no orphaned public-only Person records.
157+
- **Email refresh on every existing-linked sign-in.** Per spec, `PrivateProfile.email` always tracks the user's current GitHub primary verified email. `refreshLinked` rewrites the private profile even when the email is unchanged so `emailRefreshedAt` bumps.
158+
- **No welcome notification on fresh signup.** Wired LoggingNotifier doesn't expose `notifyAccountWelcome` yet. Tracked as follow-up #43.
159+
- **Test parallelism flakiness.** The full API suite under default vitest parallelism is flaky on contended machines (worker timeouts in `read-api`/`write-api`). Running with `--no-file-parallelism` yields 158/158. Pre-existing on `main` — not introduced by this plan. Each new test in `github-oauth.test.ts` uses a unique `remoteAddress` to avoid the 10-req/min/IP cap on `/api/auth/*`.
160+
161+
## Follow-ups
162+
163+
- Issue [#42](https://github.com/CodeForPhilly/codeforphilly-ng/issues/42) — clarify auth.md status codes vs redirects for OAuth error modes
164+
- Issue [#43](https://github.com/CodeForPhilly/codeforphilly-ng/issues/43) — send welcome notification on fresh-user OAuth signup
165+
- Deferred to [`account-claim`](account-claim.md) — full `/account-claim` UI consuming the `cfp_claim` cookie (this plan only ships the placeholder page)

0 commit comments

Comments
 (0)