Skip to content

perf(auth): resolve the caller in one query instead of two - #372

Merged
nourshoreibah merged 4 commits into
mainfrom
perf/auth-single-query
Aug 25, 2026
Merged

perf(auth): resolve the caller in one query instead of two#372
nourshoreibah merged 4 commits into
mainfrom
perf/auth-single-query

Conversation

@nourshoreibah

Copy link
Copy Markdown
Collaborator

The problem

Every guarded request paid two strictly serial database round trips before the
handler ran, and nothing was cached:

  1. authenticateRequest read branch.users by cognito_sub to turn the token's
    sub into an identity.
  2. loadRbacSubject then read branch.project_memberships by user_id — which it
    could not do until step 1 came back, because step 1 is where user_id comes 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/me paid three. On top of the two above, handleMe re-read the same
branch.users row by the same key for a different column list.

The change

authenticateRequest LEFT JOINs the memberships into the identity query and selects
the union of the columns both callers need. The memberships and the /auth/me
payload now arrive with the identity.

loadRbacSubject assembles the subject from the context it was handed and only
queries when the memberships were not loaded — which is how every lambda's tests
build a context, so the six services' resolveAuth wiring is untouched.

Emitted SQL

Captured from Kysely's log hook against a live Postgres, running the real
resolveAuth and the real handler with only the JWT verifier stubbed.

Before — guarded request, 2 statements:

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 statements (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 statement:

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
before after
guarded request 2 1
GET /auth/me 3 1

The resolved subject is byte-identical before and after in every probed case —
one membership, zero memberships, three memberships — and so is the /auth/me
response body.

Preserved semantics

  1. No token, or an unverifiable token → zero DB queries, { isAuthenticated: false }.
    Verification still happens before the query. Two tests assert the query's
    execute was never called: one with no Authorization header, one where
    verify rejects.
  2. LEFT JOIN, not inner. The db stub in authenticate.test.ts throws if
    innerJoin is ever called, and a test asserts the exact join arguments. A
    probe against a real user with no memberships authenticates and yields
    memberProjectIds: [].
  3. Row-per-membership fan-out. N memberships → N rows; zero → one row with
    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 live
    probes of both shapes.
  4. In Cognito but absent from 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.
  5. email provenance stays split, deliberately. AuthenticatedUser.email is
    still payload.email (the JWT claim); the new AuthenticatedUser.dbUser.email
    is the column, and GET /auth/me reports the column. A test feeds a
    deliberately different claim and column and asserts the column is what ships;
    the live probe shows the same (claim claim@branch.org, response
    ashley@branch.org).
  6. A DB outage is NOT a 401. The query is still outside the try/catch that
    handles token failure — that block only wraps verify. The regression test from
    PR fix(preview): point preview lambdas at the branch RDS, not DBInstances[0] #316 still asserts the rejection propagates rather than becoming
    unauthenticated.
  7. Identity comparisons. Untouched: everything still goes through
    isAuthor/isSelf on a non-null id, and buildSubject still returns
    ANONYMOUS for a user with no userId. preloadedSubject returns null (not a
    subject) for an id-less context, so no nullable-id path was added.
  8. is_admin is still the only source of admin. Derived once, as
    is_admin === true, and used for both user.isAdmin and dbUser.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_admin coercion table now asserts both fields.
  9. The verifier singleton is untouched — still module scope, still lazily
    built, so aws-jwt-verify keeps caching the JWKS across warm invocations.

Rebased onto #353, so GET /auth/me still presigns the avatar key through
resolveProfileImage before returning it — that step is unchanged, it just reads
the column off the auth context now instead of from its own query.

Tests

Run against a throwaway postgres:16-alpine, not the shared instance.

suite result
shared/rbac 28 passed
shared/lambda-auth 30 passed (was 20; +10 new)
shared/lambda-http 34 passed
lambdas/auth 82 passed, 3 failed
lambdas/users 68 passed
lambdas/projects 115 passed
lambdas/donors 59 passed, 1 failed
lambdas/expenditures 128 passed
lambdas/reports 112 passed

The 4 failures are auth.e2e.test.ts (3) and the donors health test (1). Both
fetch http://localhost:3000, which only exists under start-server-and-test;
they fail identically on unmodified main. 652 of 656 tests pass.

Two run notes, both pre-existing: npm test in lambdas/projects hangs in its
start-server-and-test wrapper, and since #365 set idleTimeoutMillis: 0 on the
pg pool jest holds an open socket after the last test, so these were run as
npx jest --ci --forceExit.

npx tsc --noEmit is clean in all six lambdas and in shared/rbac,
shared/lambda-auth, shared/lambda-http.

🤖 Generated with Claude Code

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>
@nourshoreibah nourshoreibah added the no-review The PR review bot won't run label Aug 23, 2026
* nor name, and is_admin exists only in branch.users -- there is no
* pre-token-generation trigger, so it is not a JWT claim. This endpoint is the
* only way the frontend can learn whether the caller is an admin.
*

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

.select(['user_id', 'cognito_sub', 'email', 'name', 'is_admin', 'profile_image'])
.executeTakeFirst();
//
// `user.dbUser` is the branch.users row that authentication read, so this

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

});

it('sources isAdmin from the database row', async () => {
// Regression guard: /auth/me is the only place the frontend can learn

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete

nourshoreibah and others added 2 commits August 24, 2026 22:19
Removes the three explanatory comments marked for deletion in the PR 372
review: the handleMe doc paragraph about the query count, the dbUser
sourcing note inside the handler, and the regression-guard preamble on
the isAdmin test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@nourshoreibah
nourshoreibah merged commit 1abace3 into main Aug 25, 2026
20 checks passed
@nourshoreibah
nourshoreibah deleted the perf/auth-single-query branch August 25, 2026 02:39
nourshoreibah added a commit that referenced this pull request Aug 25, 2026
These fixtures predate this branch -- they arrived with #372 and only met the
narrowed vocabulary at the rebase. `rbac.test.ts` asserted that a project_id 4
membership makes the caller a director of it, which held only because
DIRECTOR_ROLES used to include Admin; the case is about loadRbacSubject not
issuing a second query, so Director expresses it without relying on the
synonym. The authenticate.test.ts rows are pass-through assertions that were
passing either way, updated so no fixture claims a role the CHECK now rejects.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
nourshoreibah added a commit that referenced this pull request Aug 25, 2026
* feat(rbac): drop the project-scoped Admin role

Admin is a user-level flag. `users.is_admin` is the only thing that grants it,
and `buildSubject` never reads it from a membership -- so the `Admin` value in
`project_memberships.role` only ever meant "director of this project", because
`DIRECTOR_ROLES` held both spellings. It was a synonym for Director that the
new staff picker would have offered as if it meant something more.

`PROJECT_ROLES` is now `Director | Student` and `DIRECTOR_ROLES` is just
`Director`. Everything downstream follows from that one list: the picker's
options, `validateMembers`' allowlist, and the two OpenAPI enums.

The migration is the contract phase 20260812011405 promised and never got. It
rewrites the surviving `Admin` rows to `Director` -- which changes nobody's
permissions, for the reason above -- and narrows the CHECK, which had still
been accepting `PI`, `Accountant` and `Staff` a year after they were renamed.
Deliberately not mapped to `users.is_admin`: that would turn a project-scoped
role into global privilege, which is the bug being removed, not a migration.

Verified against a scratch Postgres: Admin rows become Director and the
constraint then rejects an Admin insert.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* test(lambda-auth): stop seeding memberships with the dropped Admin role

These fixtures predate this branch -- they arrived with #372 and only met the
narrowed vocabulary at the rebase. `rbac.test.ts` asserted that a project_id 4
membership makes the caller a director of it, which held only because
DIRECTOR_ROLES used to include Admin; the case is about loadRbacSubject not
issuing a second query, so Director expresses it without relying on the
synonym. The authenticate.test.ts rows are pass-through assertions that were
passing either way, updated so no fixture claims a role the CHECK now rejects.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-review The PR review bot won't run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant