feat(auth): wire auth end to end across frontend, backend and Cognito - #289
Merged
Conversation
Auth was half-built on both sides of the wire and the halves did not meet.
Frontend had no route guard of any kind -- no middleware, no wrapper, and not
one page read `isAuthenticated`. The root route rendered `<NavBar role="admin">`
unconditionally, so opening the site showed the admin shell to anonymous
visitors. `AuthContext` was fully implemented and consumed by nothing, which is
why CI stayed green.
Login could also hang. `handleLogin` drove `amazon-cognito-identity-js` with
only onSuccess/onFailure/newPasswordRequired callbacks, so any other Cognito
challenge resolved no promise and the lambda ran to its 30s timeout.
NEW_PASSWORD_REQUIRED returned 403 without a Session and had no follow-up
endpoint, making it a dead end.
Frontend
- Add AuthGate, mounted once in providers.tsx. Static export means middleware
would never run, so guarding is client-side. lib/routes.ts is
protected-by-default: a new page is gated without opting in.
- Rewrite the root route to route by session, and absorb the CloudFront SPA
fallback (403/404 -> /index.html) with a sessionStorage loop breaker.
- Add the /dashboard and /projects pages the Navbar always linked to, and drop
the /profile and /logout links that pointed nowhere.
- Bootstrap the session from GET /auth/me instead of trusting a decoded ID
token, so a revoked session no longer looks signed in.
- Add ApiError (carrying status), lib/authTokens.ts as the sole owner of token
storage, and lib/authClient.ts with single-flight refresh plus one 401 retry.
Components now use useApi() rather than threading a token through props; the
old `localStorage.getItem(...) ?? ''` pattern silently sent unauthenticated
requests.
- Schedule refresh from the token exp, so sessions stop breaking at one hour.
- Derive the Navbar role and Header identity from the real session.
- Handle NEW_PASSWORD_REQUIRED at login, and stop reporting all failures as bad
credentials. Throw instead of persisting `undefined` when a response carries
no tokens.
- Stop showing "Password Changed" when the reset request failed.
Backend
- Replace the SRP library with InitiateAuth/RespondToAuthChallenge on
USER_PASSWORD_AUTH. The SDK returns Session as an opaque string, which is what
makes a stateless /auth/respond-challenge possible, and an unrecognised
ChallengeName becomes a value rather than a hang.
- Add POST /auth/refresh, POST /auth/respond-challenge and GET /auth/me. Adding
a challenge type is now a row in CHALLENGE_SPECS, so enabling MFA is a
configuration change rather than a code change.
- Treat a branch.users row with a NULL cognito_sub as a pending invitation and
claim it on register, never touching is_admin. The seeded admins could not
previously sign in at all.
- Make branch.users.is_admin the single source of truth by dropping the dead
Cognito "Admins" group promotion.
- Fix PATCH /users/{userId} self-escalation: is_admin is admin-only.
- Gate DELETE /projects/{id}, which had no authorization check behind a stale
TODO referencing an already-merged ticket.
- Advertise PATCH in Allow-Methods, without which PATCH /users/{userId} was
unreachable from a browser.
Infra
- Declare COGNITO_USER_POOL_ID, COGNITO_CLIENT_ID and REPORTS_BUCKET_NAME in the
lambda environment block. They existed only as hand-set console values, so the
next apply would have removed them and silently 401'd every authenticated
request across all six lambdas.
- Add a pool-scoped cognito-idp policy for the registration-rollback path.
- Set advanced_security_mode to AUDIT: every sign-in is proxied through the auth
lambda, so adaptive auth was risk-scoring one shared ENI address.
- Add the cognito_user_pool_id and cognito_client_id outputs the docs referenced
but which did not exist.
Tests
- shared/lambda-auth had none; add 42, including guards against re-adding the
group promotion and against a missing pool id throwing rather than degrading.
- Cover every Cognito SDK interaction in the auth lambda, including a
SOFTWARE_TOKEN_MFA case with a short timeout so the hang cannot come back.
- Add frontend coverage for the guard, refresh, route policy and reset flow, and
run the shared package in CI, which previously never executed its tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Auto-formatted .tf files with terraform fmt - Updated README.md with terraform-docs Co-authored-by: nourshoreibah <nourshoreibah@users.noreply.github.com>
Co-authored-by: nourshoreibah <nourshoreibah@users.noreply.github.com>
Contributor
|
Database Types Auto-Regenerated The database schema has changed and the shared TypeScript definitions were regenerated:
All lambdas consume these types through the |
nourshoreibah
added
the
test-environment
Contributor
|
🌿 ⏳ Creating preview environment… (logs) |
Contributor
|
Database Types Check Complete The database schema files were modified, but the regenerated TypeScript types are identical to the existing ones. No changes were needed and the type definitions are already up to date. |
Contributor
Terraform Plan 📖
|
Contributor
🌿 Preview environment — ready ✅Open: https://d3nmtjoh6ir9ym.cloudfront.net/pr-289/ Shared RDS + Cognito (prod data); DB migrations are not applied here. New commits update this environment in place — a note is posted here on each update. Remove the |
nourshoreibah
marked this pull request as ready for review
July 28, 2026 01:45
Contributor
|
🌿 Preview environment torn down 🧹 — the stack for this PR has been destroyed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Auth was half-built on both sides of the wire and the halves didn't meet.
The reported symptom — opening the site lands you on a dashboard as if logged in — was
apps/frontend/src/app/page.tsx, which rendered<NavBar role="admin" />with no auth check. It wasn't a faulty redirect; no redirect was ever written. There were zero route guards in the entire frontend — nomiddleware.ts, noProtectedRoute, and not one page readisAuthenticated.AuthContextwas fully implemented and consumed by nothing, which is exactly why CI stayed green.Login could hang.
handleLogindroveamazon-cognito-identity-jswith onlyonSuccess/onFailure/newPasswordRequiredcallbacks. Any other challenge (SOFTWARE_TOKEN_MFA,SMS_MFA,MFA_SETUP,SELECT_MFA_TYPE) resolved no promise at all, so the lambda ran to its 30s timeout.NEW_PASSWORD_REQUIREDreturned 403 without aSessionand had no follow-up endpoint — an unrecoverable dead end. Turning MFA on would have broken login.Sessions died silently at one hour. No
/auth/refreshexisted;InitiateAuthCommandwas imported and never called. The frontend storedbranch_refresh_tokenand never read it. Nothing handled a 401.Anyone could create an account.
POST /auth/registeris public and unauthenticated, and when nobranch.usersrow matched the email it inserted one. This predates the branch, but it mattered more thanis_admin: falsesuggests: four list endpoints authorize onisAuthenticatedalone with no project scoping, so a self-registered stranger could read the full donor list and financial history.infrastructure/aws/lambda.tfdeclared a completeenvironmentblock containing onlyNODE_ENV+DB_*, withignore_changeslisting justs3_key. The Cognito IDs exist in production solely as hand-set console values (proven bypreview-env.yml:120, which reads them off the livebranch-authfunction). The nextterraform applyofinfrastructure/awswould have deleted them — and becauseauthenticate.tsthrows inside atry, that degrades to blanket silent 401s across all six lambdas, not a loud failure.Before applying, record the live values and confirm the plan matches:
If they differ, the pool in state is not the pool in the console — stop and reconcile.
Two facts that shaped the design
is_adminis not a JWT claim. It lives only in Postgres; there is no pre-token-generation trigger, and a Cognito access token carries noemailornameeither. Hence the newGET /auth/me— the frontend cannot decode its way to identity or role. This is whyNavbar.tsxhardcodedrole="admin".middleware.tswould never run. All guarding is client-side.Account provisioning is now invitation-only
Registration can no longer create a
branch.usersrow, only claim one an admin already approved:POST /users— a row withcognito_sub = NULL.POST /auth/registers with that email, which claims the row (settingcognito_sub, never touchingis_admin).POST /auth/verify-emailwith the emailed code, thenPOST /auth/login.An email with no pending invitation gets
403 INVITATION_REQUIRED; an already-claimed one gets 409. The 403 is deliberately identical whether or not the address exists, so the endpoint can't be used to enumerate staff emails.The DB row is the real control rather than the pool configuration —
authenticateRequestrejects any Cognito identity whosesubhas no row, so an identity created out of band stays inert.AdminCreateUseralso works as an invitation path: it yieldsNEW_PASSWORD_REQUIRED, which the login page now handles.Changes
Frontend —
AuthGatemounted once inproviders.tsx;lib/routes.tsis protected-by-default, so a new page is gated without anyone opting in. New/dashboardand/projectspages the Navbar always linked to.ApiErrorcarrying status,authTokens.tsas sole owner of storage,authClient.tswith single-flight refresh + one 401 retry,useApi()replacing token-through-props. Session bootstraps from/auth/me; refresh is scheduled from tokenexp.Backend — SRP library replaced with
InitiateAuth/RespondToAuthChallengeonUSER_PASSWORD_AUTH(the SDK'sSessionis an opaque string that survives across invocations, which is what makes a stateless/auth/respond-challengepossible). New/auth/refresh,/auth/respond-challenge,/auth/me. Claim-on-register so the seeded admins can finally sign in. Adding a challenge type is now one row inCHALLENGE_SPECS.Authorization fixes —
PATCH /users/{userId}self-escalation (any user could POST{isAdmin: true}to their own id);DELETE /projects/{id}had no check at all behind a stale TODO citing an already-merged ticket;PATCHmissing fromAllow-Methods, which madePATCH /users/{userId}unreachable from a browser.Infra — Cognito IDs into the lambda env, pool-scoped
cognito-idppolicy for the registration-rollback path,advanced_security_mode→AUDIT(all sign-ins arrive from one Lambda ENI, so adaptive auth was risk-scoring a shared address), and the two outputs the docs already told operators to use but which didn't exist.Decisions worth a look
mfa_configurationstaysOFF, but login can no longer hang and every challenge name is plumbed through. Enabling MFA becomes a Terraform change.Adminsgroup promotion dropped. Postgresis_adminis now the single source of truth — two sources would make demotion viaPATCH /users/{userId}silently ineffective. The branch was 100% dead (no such group exists).register/verifyEmail/resendCoderemoved from the context; the backend routes stay for the invitation-claim flow. Rationale is recorded in a comment so nobody re-adds them.Verification
tscclean, eslint clean, production build emits/dashboard+/projectsshared/lambda-authfmt -checkandvalidatepassRegression guards worth calling out:
SOFTWARE_TOKEN_MFAreturns 200 under an explicit 2s jest timeout so the hang fails fast rather than stalling the suite; an invented challenge name proves an unknown challenge can't hang; a seeded ID token claimingis_admin: trueagainst/auth/mesayingfalsemust yieldfalse; five concurrent 401s trigger exactly one/auth/refresh; an uninvited registration creates neither a Cognito user nor a DB row.Not run locally: the auth e2e suite. It hardcodes
localhost:3000and that port is occupied on my machine, so it returns 404 rather than connecting. Its three original cases were invalidated by invitation-only (two inserted rows without acognito_sub, which now reads as a pending invitation rather than a conflict, and one created an account from nothing); all four have been rewritten and typecheck, but please confirm they pass in CI, which runs on a clean runner.Still needs a real Cognito user, run manually once:
NEW_PASSWORD_REQUIREDend to end (admin-create-user --temporary-password … --message-action SUPPRESS),AdminDeleteUserIAM post-apply, and refresh across the real one-hour boundary. Verification emails are capped at 50/day pool-wide — don't loop registration tests.Out of scope (worth tickets)
GET /expenditures,/reports,/donors,/donationshave no project scoping — any authenticated user reads every row. Invitation-only removes the anonymous path to that data, but this should still be fixed; deliberately left to its own PR.POST /donorsreturns 201 without writing anything.api_gateway.tfgrants onlyGETon bare/donorsand/reportswhile the handlers servePOSTthere.branch-lambda-rolehas nos3:PutObject, so report generation is likely failingAccessDenied— same root-cause class as the Cognito IAM gap fixed here.lambda.zipbuild artifacts aren't gitignored; worth adding*.zip.🤖 Generated with Claude Code