From 9de092da94bebd33301844734d49dd49d48c5a6c Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Sun, 23 Aug 2026 13:44:16 -0400 Subject: [PATCH 1/2] perf(auth): resolve the caller in one query instead of two 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) --- apps/backend/lambdas/auth/controllers/auth.ts | 31 ++- .../lambdas/auth/test/auth.login.unit.test.ts | 91 ++++++--- shared/lambda-auth/src/authenticate.ts | 85 +++++++- shared/lambda-auth/src/index.ts | 2 +- shared/lambda-auth/src/rbac.ts | 28 ++- shared/lambda-auth/src/types.ts | 7 +- shared/lambda-auth/test/authenticate.test.ts | 188 +++++++++++++++--- shared/lambda-auth/test/rbac.test.ts | 95 +++++++++ shared/lambda-http/src/authz.ts | 6 + shared/types/auth-types.d.ts | 39 ++++ 10 files changed, 486 insertions(+), 86 deletions(-) create mode 100644 shared/lambda-auth/test/rbac.test.ts diff --git a/apps/backend/lambdas/auth/controllers/auth.ts b/apps/backend/lambdas/auth/controllers/auth.ts index 1b4e7ae4..dc05cd36 100644 --- a/apps/backend/lambdas/auth/controllers/auth.ts +++ b/apps/backend/lambdas/auth/controllers/auth.ts @@ -9,8 +9,6 @@ import { } from '@aws-sdk/client-cognito-identity-provider'; import { json, parseBody } from '@branch/lambda-http'; import type { RouteHandler } from '@branch/lambda-http'; -import { authenticateRequest } from '../auth'; -import db from '../db'; import { resolveProfileImage } from '../photos'; import { cognitoClient, @@ -192,21 +190,22 @@ export async function handleRefresh(event: any): Promise * 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. + * + * Those Postgres columns arrive on the auth context: authentication reads them + * in the same query that resolves the identity, so this handler adds no query of + * its own. It used to cost three round trips (identity, memberships, then this + * re-read of the identity row) and now costs one. */ export const handleMe: RouteHandler = async ({ auth }) => { // `access: 'authenticated'` on the route means dispatch has already verified // the session and loaded the subject; re-doing either here would be a second // token verification and a second memberships query per request. - const user = auth.context.user; - if (!user) { - return json(401, { message: 'Authentication required' }); - } - - const me = await db - .selectFrom('branch.users') - .where('cognito_sub', '=', user.cognitoSub) - .select(['user_id', 'cognito_sub', 'email', 'name', 'is_admin', 'profile_image']) - .executeTakeFirst(); + // + // `user.dbUser` is the branch.users row that authentication read, so this + // endpoint no longer re-reads it by the same key for a different column list. + // Note `dbUser.email` is the column, deliberately not `user.email`, which is + // the token claim and is absent from a Cognito access token. + const me = auth.context.user?.dbUser; // Defensive: authenticateRequest already rejects a token whose sub has no row, // so this is unreachable today. Kept so a future refactor cannot turn a @@ -218,12 +217,12 @@ export const handleMe: RouteHandler = async ({ auth }) => { } return json(200, { - userId: me.user_id, - cognitoSub: me.cognito_sub, + userId: me.userId, + cognitoSub: me.cognitoSub, email: me.email, name: me.name, - isAdmin: me.is_admin === true, - profileImage: await resolveProfileImage(me.profile_image), + isAdmin: me.isAdmin, + profileImage: await resolveProfileImage(me.profileImage), // The RBAC subject travels with identity so the browser evaluates the same // policy against the same facts the lambdas used. Without it the frontend // would have to re-derive "is a director" and "which projects am I on" diff --git a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts index 201ac703..e63481a5 100644 --- a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts +++ b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts @@ -421,32 +421,38 @@ describe('GET /me', () => { expect(mockExecuteTakeFirst).not.toHaveBeenCalled(); }); - it('returns 401 when the token verifies but no branch.users row exists', async () => { + it('returns 401 when the context carries no branch.users row', async () => { + // Unreachable today -- authentication rejects a token whose sub has no row. + // The guard keeps a future refactor from turning a missing row into a 500, + // and 401-not-404 keeps /me from being a user-existence oracle. mockAuthenticateRequest.mockResolvedValue({ isAuthenticated: true, user: { cognitoSub: 'sub-1', isAdmin: false }, }); - mockExecuteTakeFirst.mockResolvedValue(undefined); const res = await handler(event('/me', 'GET')); expect(res.statusCode).toBe(401); }); - it('sources isAdmin from the database row, not the auth context', async () => { - // Regression guard: /auth/me is the only place the frontend can learn - // isAdmin, and it must reflect branch.users rather than any token claim. + it('answers from the row authentication already read, with no query of its own', async () => { + // Was three round trips: identity, memberships, then this handler + // re-reading the identity row by the same key for a different column list. mockAuthenticateRequest.mockResolvedValue({ isAuthenticated: true, - user: { cognitoSub: 'sub-1', isAdmin: false }, - }); - mockExecuteTakeFirst.mockResolvedValue({ - user_id: 7, - cognito_sub: 'sub-1', - email: 'a@b.com', - name: 'Ada', - is_admin: true, - profile_image: null, + user: { + cognitoSub: 'sub-1', + userId: 7, + isAdmin: true, + dbUser: { + userId: 7, + cognitoSub: 'sub-1', + email: 'a@b.com', + name: 'Ada', + isAdmin: true, + profileImage: 'https://s3/pic.png', + }, + }, }); const res = await handler(event('/me', 'GET', undefined, { Authorization: 'Bearer t' })); @@ -458,30 +464,65 @@ describe('GET /me', () => { email: 'a@b.com', name: 'Ada', isAdmin: true, - profileImage: null, + profileImage: 'https://s3/pic.png', // The authorization subject rides along with identity so the browser can // evaluate @branch/rbac without a second round trip. rbac: mockSubject, }); + expect(mockExecuteTakeFirst).not.toHaveBeenCalled(); }); - it('coerces a non-boolean is_admin to false', async () => { + it('reports the email column, not the token email claim', async () => { + // The two fields are populated from different places on purpose: an access + // token's claim can be stale or absent, branch.users.email cannot. mockAuthenticateRequest.mockResolvedValue({ isAuthenticated: true, - user: { cognitoSub: 'sub-1', isAdmin: true }, + user: { + cognitoSub: 'sub-1', + userId: 7, + email: 'stale-claim@b.com', + isAdmin: false, + dbUser: { + userId: 7, + cognitoSub: 'sub-1', + email: 'column@b.com', + name: 'Ada', + isAdmin: false, + profileImage: null, + }, + }, }); - mockExecuteTakeFirst.mockResolvedValue({ - user_id: 7, - cognito_sub: 'sub-1', - email: 'a@b.com', - name: 'Ada', - is_admin: null, - profile_image: null, + + const res = await handler(event('/me', 'GET')); + + expect(JSON.parse(res.body).email).toBe('column@b.com'); + }); + + it('sources isAdmin from the database row', async () => { + // Regression guard: /auth/me is the only place the frontend can learn + // isAdmin, and it must reflect branch.users rather than any token claim. + // Production derives both fields below from the same column, so they cannot + // really disagree -- forcing them apart pins down which one is reported. + mockAuthenticateRequest.mockResolvedValue({ + isAuthenticated: true, + user: { + cognitoSub: 'sub-1', + userId: 7, + isAdmin: false, + dbUser: { + userId: 7, + cognitoSub: 'sub-1', + email: 'a@b.com', + name: 'Ada', + isAdmin: true, + profileImage: null, + }, + }, }); const res = await handler(event('/me', 'GET')); - expect(JSON.parse(res.body).isAdmin) .toBe(false); + expect(JSON.parse(res.body).isAdmin).toBe(true); }); }); diff --git a/shared/lambda-auth/src/authenticate.ts b/shared/lambda-auth/src/authenticate.ts index d0941119..23fa96d8 100644 --- a/shared/lambda-auth/src/authenticate.ts +++ b/shared/lambda-auth/src/authenticate.ts @@ -1,5 +1,5 @@ import { CognitoJwtVerifier } from 'aws-jwt-verify'; -import type { AuthContext, AuthenticatedUser } from './types'; +import type { AuthContext, AuthenticatedUser, AuthMembership } from './types'; // Minimal structural type — avoids a hard dependency on kysely at compile time. // Parameter typed as `any` so Kysely's constrained overload is assignable. @@ -42,6 +42,35 @@ export function extractToken(event: any): string | null { return authHeader; } +/** + * One row of the identity query. The LEFT JOIN fans the single `branch.users` + * row out to one row per membership, so the user columns repeat and the + * membership columns are NULL for a user who holds none. + */ +interface CallerRow { + user_id: number; + cognito_sub: string | null; + email: string; + name: string; + is_admin: boolean | null; + profile_image: string | null; + project_id: number | null; + role: string | null; +} + +/** + * Resolve the caller in a single round trip: verify the token, then read the + * `branch.users` row *and* its memberships with one LEFT JOIN. + * + * It used to be two strictly serial queries — identity, then memberships keyed + * on the user_id the first one returned — which put two RTTs in front of every + * guarded request in all six lambdas. The join is LEFT because a user with no + * memberships must still authenticate; an inner join would sign them out. + * + * The columns are the union of what authentication and `GET /auth/me` need, so + * `/auth/me` can answer from this context instead of re-reading the same row by + * the same key. + */ export async function authenticateRequest( db: QueryableDb, event: any, @@ -63,12 +92,25 @@ export async function authenticateRequest( // Uncaught on purpose: a DB outage is not a 401. Catching it hid an unreachable // RDS behind "Authentication required" and logged users out. Handlers map to 500. - const dbUser = await db - .selectFrom('branch.users') - .where('cognito_sub', '=', payload.sub) - .selectAll() - .executeTakeFirst(); + const rows: CallerRow[] = await db + .selectFrom('branch.users as u') + .leftJoin('branch.project_memberships as pm', 'pm.user_id', 'u.user_id') + .where('u.cognito_sub', '=', payload.sub) + .select([ + 'u.user_id', + 'u.cognito_sub', + 'u.email', + 'u.name', + 'u.is_admin', + 'u.profile_image', + 'pm.project_id', + 'pm.role', + ]) + .execute(); + // No rows at all means no `branch.users` row — the LEFT JOIN guarantees at + // least one row for a user that exists, membership or not. + const dbUser = rows[0]; if (!dbUser) { console.warn( 'User authenticated with Cognito but not found in database:', @@ -77,18 +119,47 @@ export async function authenticateRequest( return { isAuthenticated: false }; } + const isAdmin = dbUser.is_admin === true; + const user: AuthenticatedUser = { cognitoSub: payload.sub, userId: dbUser.user_id, email: payload.email as string | undefined, - isAdmin: dbUser.is_admin === true, + isAdmin, // Informational only. We deliberately do NOT promote on a Cognito // "Admins" group: branch.users.is_admin is the single source of truth. // A second source would make demotion via PATCH /users/{userId} silently // ineffective, nothing in this codebase writes group membership, and no // aws_cognito_user_group is defined in infrastructure/aws/cognito.tf. cognitoGroups: payload['cognito:groups'] as string[] | undefined, + // The same row, under the names `GET /auth/me` reports. `email` above stays + // the token claim; this one is the column. + dbUser: { + userId: dbUser.user_id, + cognitoSub: dbUser.cognito_sub, + email: dbUser.email, + name: dbUser.name, + isAdmin, + profileImage: dbUser.profile_image, + }, + memberships: collectMemberships(rows), }; return { user, isAuthenticated: true }; } + +/** + * Dedupe the fanned-out rows down to the memberships. + * + * A user with no memberships arrives as exactly one row whose membership + * columns are NULL; dropping those is what keeps the join from inventing a + * phantom membership on a null project. + */ +function collectMemberships(rows: readonly CallerRow[]): AuthMembership[] { + const memberships: AuthMembership[] = []; + for (const row of rows) { + if (row.project_id == null || row.role == null) continue; + memberships.push({ project_id: row.project_id, role: row.role }); + } + return memberships; +} diff --git a/shared/lambda-auth/src/index.ts b/shared/lambda-auth/src/index.ts index cea45141..5574d4c9 100644 --- a/shared/lambda-auth/src/index.ts +++ b/shared/lambda-auth/src/index.ts @@ -1,6 +1,6 @@ export * from './types'; export { extractToken, authenticateRequest } from './authenticate'; -export { loadRbacSubject } from './rbac'; +export { loadRbacSubject, preloadedSubject } from './rbac'; // The policy itself is re-exported so a lambda needs one import for auth and // authorization, the same way this package already re-exports the auth DTOs. export * from '@branch/rbac'; diff --git a/shared/lambda-auth/src/rbac.ts b/shared/lambda-auth/src/rbac.ts index ecdeb9cc..41eebaae 100644 --- a/shared/lambda-auth/src/rbac.ts +++ b/shared/lambda-auth/src/rbac.ts @@ -8,9 +8,28 @@ interface QueryableDb { } /** - * Build the authorization subject for an authenticated request: one query at the - * edge, because nearly every rule needs the caller's memberships. The result is - * also what `GET /auth/me` ships to the browser. + * The subject for a caller whose memberships `authenticateRequest` already + * joined in, or `null` when they were not loaded and somebody has to query. + * + * `null` and "member of nothing" are different answers, which is why this reads + * the presence of the array rather than its length: a hand-built context (every + * lambda's tests build one) must still fall through to `loadRbacSubject`. + */ +export function preloadedSubject(authContext: AuthContext): RbacSubject | null { + if (!authContext.isAuthenticated || !authContext.user?.userId) return null; + const memberships = authContext.user.memberships; + if (!memberships) return null; + return buildSubject(authContext.user, memberships); +} + +/** + * Build the authorization subject for an authenticated request by reading the + * memberships from Postgres, because nearly every rule needs them. The result + * is also what `GET /auth/me` ships to the browser. + * + * `authenticateRequest` now fetches the same rows in the query that resolves the + * identity, so the request path takes `preloadedSubject` and never gets here. + * This remains the loader for a context that arrived without them. * * The assembly lives in `buildSubject` in @branch/rbac; this is the "read it * from Postgres" half. @@ -22,6 +41,9 @@ export async function loadRbacSubject( const userId = authContext.user?.userId; if (!authContext.isAuthenticated || !userId) return ANONYMOUS; + const preloaded = preloadedSubject(authContext); + if (preloaded) return preloaded; + const memberships: MembershipRow[] = await db .selectFrom('branch.project_memberships') .where('user_id', '=', userId) diff --git a/shared/lambda-auth/src/types.ts b/shared/lambda-auth/src/types.ts index 2f815a6f..cf085392 100644 --- a/shared/lambda-auth/src/types.ts +++ b/shared/lambda-auth/src/types.ts @@ -1,3 +1,8 @@ // Declared once in @branch/types and re-exported here so runtime consumers get // the DTOs from the package that produces them. -export type { AuthContext, AuthenticatedUser } from '@branch/types'; +export type { + AuthContext, + AuthenticatedUser, + AuthenticatedDbUser, + AuthMembership, +} from '@branch/types'; diff --git a/shared/lambda-auth/test/authenticate.test.ts b/shared/lambda-auth/test/authenticate.test.ts index 5de4b350..62cc8aa0 100644 --- a/shared/lambda-auth/test/authenticate.test.ts +++ b/shared/lambda-auth/test/authenticate.test.ts @@ -7,17 +7,66 @@ jest.mock('aws-jwt-verify', () => ({ }, })); -/** Minimal Kysely-shaped stub: selectFrom().where().selectAll().executeTakeFirst() */ -function makeDb(row: unknown) { - const executeTakeFirst = jest.fn().mockResolvedValue(row); +/** A `branch.users` row as the identity query selects it, membership columns included. */ +function userRow(over: Record = {}) { return { - db: { - selectFrom: () => ({ - where: () => ({ selectAll: () => ({ executeTakeFirst }) }), - }), + user_id: 7, + cognito_sub: 'sub-1', + email: 'row@b.com', + name: 'Ada', + is_admin: false, + profile_image: null, + project_id: null, + role: null, + ...over, + }; +} + +/** + * Minimal Kysely-shaped stub for the one identity query: + * selectFrom().leftJoin().where().select().execute() + * + * Every call is recorded so a test can assert the join is a LEFT join keyed on + * cognito_sub -- an inner join here would sign out a user with no memberships. + * `innerJoin` throws rather than returning a chain, so a regression cannot pass + * quietly. + */ +function makeDb(rows: unknown[]) { + const execute = jest.fn().mockResolvedValue(rows); + const calls: { + from?: unknown; + leftJoin?: unknown[]; + where?: unknown[]; + select?: unknown; + } = {}; + + const tail = { + where: (...args: unknown[]) => { + calls.where = args; + return tail; + }, + select: (columns: unknown) => { + calls.select = columns; + return { execute }; }, - executeTakeFirst, }; + + const db = { + selectFrom: (table: unknown) => { + calls.from = table; + return { + leftJoin: (...args: unknown[]) => { + calls.leftJoin = args; + return tail; + }, + innerJoin: () => { + throw new Error('inner join would sign out a user with no memberships'); + }, + }; + }, + }; + + return { db, execute, calls }; } function bearerEvent(token: string) { @@ -88,61 +137,141 @@ describe('extractToken', () => { describe('authenticateRequest', () => { it('returns unauthenticated without verifying when no token is present', async () => { const { authenticateRequest } = await loadModule(); - const { db } = makeDb(undefined); + const { db, execute } = makeDb([]); await expect(authenticateRequest(db, { headers: {} })).resolves.toEqual({ isAuthenticated: false, }); expect(mockVerify).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); }); it('returns unauthenticated when verification rejects', async () => { mockVerify.mockRejectedValue(new Error('expired')); const { authenticateRequest } = await loadModule(); - const { db, executeTakeFirst } = makeDb(undefined); + const { db, execute } = makeDb([]); await expect(authenticateRequest(db, bearerEvent('bad'))).resolves.toEqual({ isAuthenticated: false, }); - expect(executeTakeFirst).not.toHaveBeenCalled(); + expect(execute).not.toHaveBeenCalled(); }); it('returns unauthenticated when the token is valid but no branch.users row matches', async () => { mockVerify.mockResolvedValue({ sub: 'orphan-sub' }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb(undefined); + const { db } = makeDb([]); await expect(authenticateRequest(db, bearerEvent('good'))).resolves.toEqual({ isAuthenticated: false, }); + expect(console.warn).toHaveBeenCalledWith( + 'User authenticated with Cognito but not found in database:', + 'orphan-sub', + ); }); it('builds the auth context from the DB row', async () => { - mockVerify.mockResolvedValue({ sub: 'sub-1', email: 'a@b.com' }); + mockVerify.mockResolvedValue({ sub: 'sub-1', email: 'claim@b.com' }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: true }); + const { db } = makeDb([userRow({ is_admin: true })]); await expect(authenticateRequest(db, bearerEvent('good'))).resolves.toEqual({ isAuthenticated: true, user: { cognitoSub: 'sub-1', userId: 7, - email: 'a@b.com', + // The JWT claim, deliberately not the column -- see dbUser.email. + email: 'claim@b.com', isAdmin: true, cognitoGroups: undefined, + dbUser: { + userId: 7, + cognitoSub: 'sub-1', + email: 'row@b.com', + name: 'Ada', + isAdmin: true, + profileImage: null, + }, + memberships: [], }, }); }); + it('resolves identity and memberships in ONE left-joined query', async () => { + // The two used to be strictly serial: identity, then memberships keyed on + // the user_id it returned. That was 2 RTTs on every guarded request. + mockVerify.mockResolvedValue({ sub: 'sub-1' }); + const { authenticateRequest } = await loadModule(); + const { db, execute, calls } = makeDb([userRow()]); + + await authenticateRequest(db, bearerEvent('good')); + + expect(execute).toHaveBeenCalledTimes(1); + expect(calls.from).toBe('branch.users as u'); + expect(calls.leftJoin).toEqual([ + 'branch.project_memberships as pm', + 'pm.user_id', + 'u.user_id', + ]); + expect(calls.where).toEqual(['u.cognito_sub', '=', 'sub-1']); + // The union of what authentication and GET /auth/me need, so /auth/me does + // not re-read the same row for a different column list. + expect(calls.select).toEqual([ + 'u.user_id', + 'u.cognito_sub', + 'u.email', + 'u.name', + 'u.is_admin', + 'u.profile_image', + 'pm.project_id', + 'pm.role', + ]); + }); + + it('authenticates a user with no memberships and invents none', async () => { + // The LEFT JOIN hands back one row with NULL membership columns. An inner + // join would return nothing and sign this user out; a missing NULL check + // would hand the policy a membership on project `null`. + mockVerify.mockResolvedValue({ sub: 'sub-1' }); + const { authenticateRequest } = await loadModule(); + const { db } = makeDb([userRow({ project_id: null, role: null })]); + + const ctx = await authenticateRequest(db, bearerEvent('good')); + + expect(ctx.isAuthenticated).toBe(true); + expect(ctx.user?.memberships).toEqual([]); + }); + + it('collects one membership per joined row and dedupes the identity', async () => { + mockVerify.mockResolvedValue({ sub: 'sub-1' }); + const { authenticateRequest } = await loadModule(); + const { db } = makeDb([ + userRow({ project_id: 1, role: 'Director' }), + userRow({ project_id: 2, role: 'Student' }), + userRow({ project_id: 3, role: 'Admin' }), + ]); + + const ctx = await authenticateRequest(db, bearerEvent('good')); + + expect(ctx.user?.userId).toBe(7); + expect(ctx.user?.memberships).toEqual([ + { project_id: 1, role: 'Director' }, + { project_id: 2, role: 'Student' }, + { project_id: 3, role: 'Admin' }, + ]); + }); + it.each([[false], [null], [undefined], ['true']])( 'treats is_admin %p as not-admin (strict === true only)', async (isAdminValue) => { mockVerify.mockResolvedValue({ sub: 'sub-1' }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: isAdminValue }); + const { db } = makeDb([userRow({ is_admin: isAdminValue })]); const ctx = await authenticateRequest(db, bearerEvent('good')); expect(ctx.user?.isAdmin).toBe(false); + expect(ctx.user?.dbUser?.isAdmin).toBe(false); }, ); @@ -152,7 +281,7 @@ describe('authenticateRequest', () => { // silently ineffective. cognitoGroups stays populated but informational. mockVerify.mockResolvedValue({ sub: 'sub-1', 'cognito:groups': ['Admins'] }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: false }); + const { db } = makeDb([userRow({ is_admin: false })]); const ctx = await authenticateRequest(db, bearerEvent('good')); expect(ctx.isAuthenticated).toBe(true); @@ -163,7 +292,7 @@ describe('authenticateRequest', () => { it('verifies with tokenUse "access" and the configured client id', async () => { mockVerify.mockResolvedValue({ sub: 'sub-1' }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: false }); + const { db } = makeDb([userRow()]); await authenticateRequest(db, bearerEvent('good')); expect(mockCreate).toHaveBeenCalledWith({ @@ -178,7 +307,7 @@ describe('authenticateRequest', () => { process.env.COGNITO_APP_CLIENT_ID = 'legacy-client'; mockVerify.mockResolvedValue({ sub: 'sub-1' }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: false }); + const { db } = makeDb([userRow()]); await authenticateRequest(db, bearerEvent('good')); expect(mockCreate).toHaveBeenCalledWith( @@ -191,7 +320,7 @@ describe('authenticateRequest', () => { delete process.env.COGNITO_APP_CLIENT_ID; mockVerify.mockResolvedValue({ sub: 'sub-1' }); const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: false }); + const { db } = makeDb([userRow()]); await authenticateRequest(db, bearerEvent('good')); expect(mockCreate).toHaveBeenCalledWith(expect.objectContaining({ clientId: null })); @@ -201,7 +330,7 @@ describe('authenticateRequest', () => { // Swallowing this gave blanket silent 401s across all six lambdas. delete process.env.COGNITO_USER_POOL_ID; const { authenticateRequest } = await loadModule(); - const { db } = makeDb({ user_id: 7, is_admin: true }); + const { db } = makeDb([userRow({ is_admin: true })]); await expect(authenticateRequest(db, bearerEvent('good'))).rejects.toThrow( 'COGNITO_USER_POOL_ID', @@ -210,20 +339,13 @@ describe('authenticateRequest', () => { }); it('propagates a database failure instead of reporting it as unauthenticated', async () => { - // Regression guard for the preview-env outage (PR #316). + // Regression guard for the preview-env outage (PR #316). The query stays + // outside the try/catch that handles a bad token, so an unreachable RDS + // surfaces as a 500 rather than logging everyone out. mockVerify.mockResolvedValue({ sub: 'sub-1' }); const { authenticateRequest } = await loadModule(); - const db = { - selectFrom: () => ({ - where: () => ({ - selectAll: () => ({ - executeTakeFirst: jest - .fn() - .mockRejectedValue(new Error('timeout exceeded when trying to connect')), - }), - }), - }), - }; + const { db, execute } = makeDb([]); + execute.mockRejectedValue(new Error('timeout exceeded when trying to connect')); await expect(authenticateRequest(db, bearerEvent('good'))).rejects.toThrow( 'timeout exceeded when trying to connect', diff --git a/shared/lambda-auth/test/rbac.test.ts b/shared/lambda-auth/test/rbac.test.ts new file mode 100644 index 00000000..f1fc5fd8 --- /dev/null +++ b/shared/lambda-auth/test/rbac.test.ts @@ -0,0 +1,95 @@ +import { ANONYMOUS } from '@branch/rbac'; +import type { AuthContext } from '../src/types'; +import { loadRbacSubject, preloadedSubject } from '../src/rbac'; + +/** Kysely-shaped stub for the memberships fallback query. */ +function makeDb(rows: unknown[]) { + const execute = jest.fn().mockResolvedValue(rows); + const db = { + selectFrom: () => ({ + where: () => ({ select: () => ({ execute }) }), + }), + }; + return { db, execute }; +} + +const context = (over: Partial> = {}): AuthContext => ({ + isAuthenticated: true, + user: { cognitoSub: 'sub-1', userId: 7, isAdmin: false, ...over }, +}); + +describe('preloadedSubject', () => { + it('builds the subject from memberships the identity query already fetched', () => { + const subject = preloadedSubject( + context({ + memberships: [ + { project_id: 1, role: 'Director' }, + { project_id: 2, role: 'Student' }, + ], + }), + ); + + expect(subject).toEqual({ + userId: 7, + isAdmin: false, + memberProjectIds: [1, 2], + directorProjectIds: [1], + }); + }); + + it('treats an empty array as "member of nothing", not as "not loaded"', () => { + // A user with no memberships still authenticates (the join is LEFT), and + // must not trigger a second query to rediscover that. + expect(preloadedSubject(context({ memberships: [] }))).toEqual({ + userId: 7, + isAdmin: false, + memberProjectIds: [], + directorProjectIds: [], + }); + }); + + it('returns null when memberships were never loaded', () => { + expect(preloadedSubject(context())).toBeNull(); + }); + + it('returns null for an unauthenticated or id-less context', () => { + expect(preloadedSubject({ isAuthenticated: false })).toBeNull(); + expect( + preloadedSubject({ + isAuthenticated: true, + user: { cognitoSub: 'sub-1', isAdmin: false, memberships: [] }, + }), + ).toBeNull(); + }); +}); + +describe('loadRbacSubject', () => { + it('costs no query when authentication already joined the memberships in', async () => { + const { db, execute } = makeDb([]); + + const subject = await loadRbacSubject( + db, + context({ memberships: [{ project_id: 4, role: 'Admin' }] }), + ); + + expect(execute).not.toHaveBeenCalled(); + expect(subject.memberProjectIds).toEqual([4]); + expect(subject.directorProjectIds).toEqual([4]); + }); + + it('queries when handed a context that carries no memberships', async () => { + const { db, execute } = makeDb([{ project_id: 9, role: 'Student' }]); + + const subject = await loadRbacSubject(db, context()); + + expect(execute).toHaveBeenCalledTimes(1); + expect(subject.memberProjectIds).toEqual([9]); + }); + + it('is ANONYMOUS, and silent, for an unauthenticated caller', async () => { + const { db, execute } = makeDb([]); + + await expect(loadRbacSubject(db, { isAuthenticated: false })).resolves.toBe(ANONYMOUS); + expect(execute).not.toHaveBeenCalled(); + }); +}); diff --git a/shared/lambda-http/src/authz.ts b/shared/lambda-http/src/authz.ts index d098528b..c4be4bdd 100644 --- a/shared/lambda-http/src/authz.ts +++ b/shared/lambda-http/src/authz.ts @@ -31,6 +31,12 @@ export function requirePermission( /** * Bind a service's db-scoped authenticate + subject loader into the * `resolveAuth` shape `dispatch` expects. + * + * Two halves, one round trip: `authenticate` joins the caller's memberships into + * the identity query, so `loadSubject` (`loadRbacSubject`) assembles them from + * the context it is handed rather than querying again. It stays a separate + * argument because a caller that builds a context by hand -- every lambda's + * tests do -- still needs something that can go and read them. */ export function createAuthResolver( authenticate: (event: any) => Promise, diff --git a/shared/types/auth-types.d.ts b/shared/types/auth-types.d.ts index 8d9cf35b..d4aa3e67 100644 --- a/shared/types/auth-types.d.ts +++ b/shared/types/auth-types.d.ts @@ -3,12 +3,51 @@ * rather than declaring its own copy. */ +/** + * One `branch.project_memberships` row as authentication reads it. + * + * Structurally identical to `MembershipRow` in @branch/rbac on purpose -- it is + * the same row -- but spelled here because @branch/types declares no + * dependencies at all and so cannot import it. Keep the two in step. + */ +export interface AuthMembership { + project_id: number; + role: string; +} + +/** + * The `branch.users` row the caller was authenticated against. + * + * Its own object rather than flattened onto `AuthenticatedUser` because the + * provenance differs and callers depend on which one they read: + * `AuthenticatedUser.email` is the JWT claim (a Cognito *access* token carries + * none), `dbUser.email` is the column. `GET /auth/me` reports the column. + */ +export interface AuthenticatedDbUser { + userId: number; + cognitoSub: string | null; + email: string; + name: string; + isAdmin: boolean; + profileImage: string | null; +} + export interface AuthenticatedUser { cognitoSub: string; userId?: number; + /** The JWT `email` claim. Not the same field as `dbUser.email`. */ email?: string; isAdmin: boolean; cognitoGroups?: string[]; + /** Set whenever `AuthContext.isAuthenticated` is true. */ + dbUser?: AuthenticatedDbUser; + /** + * Every membership this user holds, when the identity query already fetched + * them (it joins them in). Absent means "not loaded", never "none": a user + * with no memberships gets an empty array, so a caller can tell the two apart + * instead of querying again to find out. + */ + memberships?: readonly AuthMembership[]; } export interface AuthContext { From a376dccb44619060a9e6c5569bb396413e2db472 Mon Sep 17 00:00:00 2001 From: nourshoreibah Date: Mon, 24 Aug 2026 22:18:57 -0400 Subject: [PATCH 2/2] docs(auth): drop the comment blocks flagged in review 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) --- apps/backend/lambdas/auth/controllers/auth.ts | 10 ---------- apps/backend/lambdas/auth/test/auth.login.unit.test.ts | 4 ---- 2 files changed, 14 deletions(-) diff --git a/apps/backend/lambdas/auth/controllers/auth.ts b/apps/backend/lambdas/auth/controllers/auth.ts index dc05cd36..70f9c7f6 100644 --- a/apps/backend/lambdas/auth/controllers/auth.ts +++ b/apps/backend/lambdas/auth/controllers/auth.ts @@ -190,21 +190,11 @@ export async function handleRefresh(event: any): Promise * 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. - * - * Those Postgres columns arrive on the auth context: authentication reads them - * in the same query that resolves the identity, so this handler adds no query of - * its own. It used to cost three round trips (identity, memberships, then this - * re-read of the identity row) and now costs one. */ export const handleMe: RouteHandler = async ({ auth }) => { // `access: 'authenticated'` on the route means dispatch has already verified // the session and loaded the subject; re-doing either here would be a second // token verification and a second memberships query per request. - // - // `user.dbUser` is the branch.users row that authentication read, so this - // endpoint no longer re-reads it by the same key for a different column list. - // Note `dbUser.email` is the column, deliberately not `user.email`, which is - // the token claim and is absent from a Cognito access token. const me = auth.context.user?.dbUser; // Defensive: authenticateRequest already rejects a token whose sub has no row, diff --git a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts index e63481a5..e5e114a7 100644 --- a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts +++ b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts @@ -499,10 +499,6 @@ describe('GET /me', () => { }); it('sources isAdmin from the database row', async () => { - // Regression guard: /auth/me is the only place the frontend can learn - // isAdmin, and it must reflect branch.users rather than any token claim. - // Production derives both fields below from the same column, so they cannot - // really disagree -- forcing them apart pins down which one is reported. mockAuthenticateRequest.mockResolvedValue({ isAuthenticated: true, user: {