perf(auth): resolve the caller in one query instead of two - #372
Open
nourshoreibah wants to merge 2 commits into
Open
perf(auth): resolve the caller in one query instead of two#372nourshoreibah wants to merge 2 commits into
nourshoreibah wants to merge 2 commits into
Conversation
Every guarded request paid two strictly serial round trips before the
handler ran: read branch.users by cognito_sub, then read
branch.project_memberships by the user_id the first query returned. The
second could not start until the first finished, so at 2-5ms RTT that was
a 4-10ms latency floor on every authenticated call in all six lambdas.
GET /auth/me paid three, because it re-read the identity row by the same
key for a different column list.
authenticateRequest now LEFT JOINs the memberships into the identity
query and selects the union of the columns both callers need, so the
memberships and the /auth/me payload arrive with the identity.
loadRbacSubject assembles the subject from what it was handed and only
queries for a context that arrived without memberships -- which is how
every lambda's tests build one, so the six services' wiring is unchanged.
Emitted SQL, verified against a live Postgres:
before, guarded request (2)
select * from "branch"."users" where "cognito_sub" = $1
select "project_id", "role" from "branch"."project_memberships"
where "user_id" = $1
before, GET /auth/me (3) -- the two above, plus
select "user_id", "cognito_sub", "email", "name", "is_admin",
"profile_image" from "branch"."users" where "cognito_sub" = $1
after, both (1)
select "u"."user_id", "u"."cognito_sub", "u"."email", "u"."name",
"u"."is_admin", "u"."profile_image", "pm"."project_id", "pm"."role"
from "branch"."users" as "u"
left join "branch"."project_memberships" as "pm"
on "pm"."user_id" = "u"."user_id"
where "u"."cognito_sub" = $1
LEFT and not inner: a user with no memberships must still authenticate.
That user comes back as one row with NULL membership columns, which is
dropped rather than turned into a membership on project null. Token
verification still happens before any query, the query still sits outside
the try/catch that handles a bad token so a DB outage stays a 500 rather
than logging everyone out, a Cognito identity with no branch.users row is
still unauthenticated, and branch.users.is_admin is still the only source
of admin.
email provenance is unchanged and still deliberately split:
AuthenticatedUser.email is the JWT claim, AuthenticatedUser.dbUser.email
is the column, and GET /auth/me reports the column.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
The problem
Every guarded request paid two strictly serial database round trips before the
handler ran, and nothing was cached:
authenticateRequestreadbranch.usersbycognito_subto turn the token'ssubinto an identity.loadRbacSubjectthen readbranch.project_membershipsbyuser_id— which itcould not do until step 1 came back, because step 1 is where
user_idcomes from.Serial, not concurrent: at 2-5 ms RTT that is a 4-10 ms latency floor on every
authenticated call, in all six lambdas.
GET /auth/mepaid three. On top of the two above,handleMere-read the samebranch.usersrow by the same key for a different column list.The change
authenticateRequestLEFT JOINs the memberships into the identity query and selectsthe union of the columns both callers need. The memberships and the
/auth/mepayload now arrive with the identity.
loadRbacSubjectassembles the subject from the context it was handed and onlyqueries when the memberships were not loaded — which is how every lambda's tests
build a context, so the six services'
resolveAuthwiring is untouched.Emitted SQL
Captured from Kysely's
loghook against a live Postgres, running the realresolveAuthand the realhandlerwith only the JWT verifier stubbed.Before — guarded request, 2 statements:
Before —
GET /auth/me, 3 statements (the two above, plus):After — both, 1 statement:
GET /auth/meThe resolved subject is byte-identical before and after in every probed case —
one membership, zero memberships, three memberships — and so is the
/auth/meresponse body.
Preserved semantics
{ isAuthenticated: false }.Verification still happens before the query. Two tests assert the query's
executewas never called: one with noAuthorizationheader, one whereverifyrejects.LEFT JOIN, not inner. The db stub inauthenticate.test.tsthrows ifinnerJoinis ever called, and a test asserts the exact join arguments. Aprobe against a real user with no memberships authenticates and yields
memberProjectIds: [].NULL membership columns, dropped rather than emitted as a membership on project
null. Covered by a zero-membership test and a three-membership test(
memberProjectIds: [1,2,3],directorProjectIds: [1,3]), plus the liveprobes of both shapes.
branch.users→{ isAuthenticated: false },keeping the
console.warn.rows[0]being undefined is exactly "no user row",because the LEFT JOIN guarantees at least one row for a user that exists. The
test asserts both the result and the warning.
emailprovenance stays split, deliberately.AuthenticatedUser.emailisstill
payload.email(the JWT claim); the newAuthenticatedUser.dbUser.emailis the column, and
GET /auth/mereports the column. A test feeds adeliberately different claim and column and asserts the column is what ships;
the live probe shows the same (claim
claim@branch.org, responseashley@branch.org).handles token failure — that block only wraps
verify. The regression test fromPR fix(preview): point preview lambdas at the branch RDS, not DBInstances[0] #316 still asserts the rejection propagates rather than becoming
unauthenticated.
isAuthor/isSelfon a non-null id, andbuildSubjectstill returnsANONYMOUSfor a user with nouserId.preloadedSubjectreturnsnull(not asubject) for an id-less context, so no nullable-id path was added.
is_adminis still the only source of admin. Derived once, asis_admin === true, and used for bothuser.isAdminanddbUser.isAdmin.Director is still purely "holds a Director/Admin membership on >= 1 project".
The "does NOT promote a member of the Cognito Admins group" regression test is
unchanged and passing, and the
is_admincoercion table now asserts both fields.verifiersingleton is untouched — still module scope, still lazilybuilt, so
aws-jwt-verifykeeps caching the JWKS across warm invocations.Rebased onto #353, so
GET /auth/mestill presigns the avatar key throughresolveProfileImagebefore returning it — that step is unchanged, it just readsthe column off the auth context now instead of from its own query.
Tests
Run against a throwaway
postgres:16-alpine, not the shared instance.shared/rbacshared/lambda-authshared/lambda-httplambdas/authlambdas/userslambdas/projectslambdas/donorslambdas/expenditureslambdas/reportsThe 4 failures are
auth.e2e.test.ts(3) and the donorshealth test(1). Bothfetchhttp://localhost:3000, which only exists understart-server-and-test;they fail identically on unmodified
main. 652 of 656 tests pass.Two run notes, both pre-existing:
npm testinlambdas/projectshangs in itsstart-server-and-testwrapper, and since #365 setidleTimeoutMillis: 0on thepg pool jest holds an open socket after the last test, so these were run as
npx jest --ci --forceExit.npx tsc --noEmitis clean in all six lambdas and inshared/rbac,shared/lambda-auth,shared/lambda-http.🤖 Generated with Claude Code