diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 32d7740..a0baa68 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -74,7 +74,11 @@ Automatic on push to `main` touching `apps/backend/lambdas/**` or `shared/types/ **`branch.users.is_admin` is the single source of truth for admin.** There is no promotion from a Cognito group, and no pre-token-generation trigger, so `is_admin` is not a JWT claim — `GET /auth/me` is the only way a client can learn it. -**A `branch.users` row with `cognito_sub IS NULL` is a pending invitation**, created by the `db/seed.sql` rows or by admin `POST /users`. `POST /auth/register` claims such a row (setting `cognito_sub`, never touching `is_admin`) instead of returning 409. Registration only 409s when the row is already claimed. +**Account provisioning is invitation-only.** A `branch.users` row with `cognito_sub IS NULL` is a pending invitation, created by `db/seed.sql` or by admin `POST /users` (ADMIN-gated). `POST /auth/register` is public, so it deliberately **cannot create a row** — it only claims an existing invitation, setting `cognito_sub` and never touching `is_admin`. An email with no pending invitation gets 403 `INVITATION_REQUIRED`; an already-claimed one gets 409. + +The 403 is intentionally identical whether or not the address exists, so registration cannot be used to enumerate staff emails. + +This is the real control, not the Cognito pool config: `authenticateRequest` rejects any Cognito identity whose `sub` has no `branch.users` row, so a Cognito user created out of band is inert. Full flow: admin `POST /users` → invitee `POST /auth/register` with that email → `POST /auth/verify-email` with the emailed code → `POST /auth/login`. **Bootstrapping the first admin** is a manual SQL statement in every environment, because `is_admin` can only be set by an existing admin: `make grant-admin EMAIL=…` locally, or the equivalent `UPDATE` against RDS in production. diff --git a/apps/backend/lambdas/auth/handler.ts b/apps/backend/lambdas/auth/handler.ts index b22cd06..8fa37d6 100644 --- a/apps/backend/lambdas/auth/handler.ts +++ b/apps/backend/lambdas/auth/handler.ts @@ -666,7 +666,27 @@ async function handleRegister(event: any): Promise { return json(409, { message: 'User with this email already exists' }); } - const claimingUserId: number | null = existingUser ? existingUser.user_id : null; + // REGISTRATION IS INVITATION-ONLY. This endpoint is public and + // unauthenticated, so without this gate anyone could create a working + // account for themselves. An account is only meaningful once a branch.users + // row exists -- authenticateRequest rejects any Cognito identity whose sub + // has no row -- so refusing to create that row here is the control. + // + // The invitation must be created first by an admin via the ADMIN-gated + // POST /users, which inserts a row with a NULL cognito_sub. + // + // 403 rather than 404: this endpoint must not become an oracle for which + // email addresses have been invited, so the response is deliberately the + // same whether or not the address is known. + if (!existingUser) { + return json(403, { + message: + 'Registration is by invitation only. Ask an administrator to create your account.', + code: 'INVITATION_REQUIRED', + }); + } + + const claimingUserId: number = existingUser.user_id; // Prepare Cognito SignUp parameters const signUpParams: SignUpCommandInput = { @@ -700,7 +720,7 @@ async function handleRegister(event: any): Promise { // SignUp can never hand us a sub. Happens routinely in local dev: `make // down-v` wipes Postgres while the shared dev pool keeps the user. Link // the existing Cognito identity instead of dead-ending on a 409. - if (claimingUserId !== null) { + { try { // AdminGetUser is SigV4-signed and needs cognito-idp:AdminGetUser // (granted in infrastructure/aws/lambda.tf). With no AWS credentials @@ -746,29 +766,19 @@ async function handleRegister(event: any): Promise { // Create user in database, or claim the pending invitation try { - if (claimingUserId !== null) { - // is_admin is deliberately NOT touched: it was set by whoever created the - // invitation (a seed, or an admin via POST /users) and must not be - // settable from a public, unauthenticated endpoint. The cognito_sub IS - // NULL predicate makes a concurrent claim a no-op rather than an - // overwrite; UNIQUE(cognito_sub) is the backstop. - await db - .updateTable('branch.users') - .set({ cognito_sub: cognitoUserSub, name: name.trim() }) - .where('user_id', '=', claimingUserId) - .where('cognito_sub', 'is', null) - .execute(); - } else { - await db - .insertInto('branch.users') - .values({ - cognito_sub: cognitoUserSub, - email: email.toLowerCase(), - name: name.trim(), - is_admin: false, - }) - .execute(); - } + // Claim the invitation. is_admin is deliberately NOT touched: it was set + // by whoever created the invitation (a seed, or an admin via POST /users) + // and must never be settable from a public, unauthenticated endpoint. + // There is no insert path here -- registration cannot mint a new row, only + // claim one an admin already approved. The cognito_sub IS NULL predicate + // makes a concurrent claim a no-op rather than an overwrite; + // UNIQUE(cognito_sub) is the backstop. + await db + .updateTable('branch.users') + .set({ cognito_sub: cognitoUserSub, name: name.trim() }) + .where('user_id', '=', claimingUserId) + .where('cognito_sub', 'is', null) + .execute(); } catch (dbError: any) { console.error('Database insert error:', dbError); @@ -795,7 +805,7 @@ async function handleRegister(event: any): Promise { name: name.trim(), emailVerificationRequired: true, details: 'Please check your email for verification code', - ...(claimingUserId !== null ? { claimed: true } : {}), + claimed: true, }); } catch (error: any) { console.error('Registration error:', error); diff --git a/apps/backend/lambdas/auth/openapi.yaml b/apps/backend/lambdas/auth/openapi.yaml index f95251a..87cd84b 100644 --- a/apps/backend/lambdas/auth/openapi.yaml +++ b/apps/backend/lambdas/auth/openapi.yaml @@ -21,8 +21,13 @@ paths: /register: post: - summary: Register a new user - description: Creates a new user account in Cognito and the database + summary: Claim an invitation and activate an account + description: > + INVITATION-ONLY. This endpoint is public and unauthenticated, so it + cannot mint new accounts: it only activates a branch.users row that an + admin already created via the ADMIN-gated POST /users (such a row has a + NULL cognito_sub). An email with no pending invitation gets 403. + is_admin is never written here. requestBody: required: true content: @@ -87,8 +92,24 @@ paths: message: type: string example: Invalid email format + '403': + description: > + No pending invitation for this email. Deliberately indistinguishable + from the response for an address that is not in the system at all, + so this endpoint cannot be used to enumerate staff emails. + content: + application/json: + schema: + type: object + properties: + message: + type: string + example: Registration is by invitation only. Ask an administrator to create your account. + code: + type: string + example: INVITATION_REQUIRED '409': - description: User already exists + description: The invitation has already been claimed content: application/json: schema: diff --git a/apps/backend/lambdas/auth/test/auth.e2e.test.ts b/apps/backend/lambdas/auth/test/auth.e2e.test.ts index f609ef6..cc910f4 100644 --- a/apps/backend/lambdas/auth/test/auth.e2e.test.ts +++ b/apps/backend/lambdas/auth/test/auth.e2e.test.ts @@ -23,7 +23,7 @@ beforeAll(async () => { beforeEach(async () => { const testName = expect.getState().currentTestName; - if (testName && (testName.includes('duplicate') || testName.includes('normalization') || testName.includes('register with valid'))) { + if (testName && (testName.includes('duplicate') || testName.includes('normalization') || testName.includes('uninvited') || testName.includes('seeded admins'))) { const client = await pool.connect(); try { await resetData(client); @@ -38,12 +38,13 @@ afterAll(async () => { }); test("duplicate email returns 409", async () => { - // Insert a user with lowercase email directly in DB + // A CLAIMED row (cognito_sub set) is a genuine conflict. A row without one is + // a pending invitation and would be claimed instead -- see the test below. const client = await pool.connect(); try { await client.query( - 'INSERT INTO branch.users (email, name, is_admin) VALUES ($1, $2, $3)', - ['existing@example.com', 'Existing User', false] + 'INSERT INTO branch.users (email, name, is_admin, cognito_sub) VALUES ($1, $2, $3, $4)', + ['existing@example.com', 'Existing User', false, 'existing-sub-123'] ); } finally { client.release(); @@ -69,8 +70,8 @@ test("email normalization uppercase matches lowercase in DB", async () => { const client = await pool.connect(); try { await client.query( - 'INSERT INTO branch.users (email, name, is_admin) VALUES ($1, $2, $3)', - ['lowercase@example.com', 'Existing User', false] + 'INSERT INTO branch.users (email, name, is_admin, cognito_sub) VALUES ($1, $2, $3, $4)', + ['lowercase@example.com', 'Existing User', false, 'lowercase-sub-123'] ); } finally { client.release(); @@ -91,9 +92,12 @@ test("email normalization uppercase matches lowercase in DB", async () => { expect(body.message).toContain("already exists"); }); -test("register with valid data (requires Cognito)", async () => { +test("uninvited email is refused before Cognito is ever called", async () => { + // Registration is invitation-only: with no branch.users row for this address + // the request must be rejected outright. Previously this created a working + // account for any caller. const uniqueEmail = `test${Date.now()}${Math.random().toString(36).substring(7)}@example.com`; - + const res = await fetch("http://localhost:3000/auth/register", { method: "POST", body: JSON.stringify({ @@ -103,30 +107,44 @@ test("register with valid data (requires Cognito)", async () => { }) }); - if (res.status === 201) { - const body = await res.json(); - expect(body.message).toBe("User registered successfully"); - expect(body.userId).toBeDefined(); - expect(body.email).toBe(uniqueEmail); - expect(body.name).toBe("Test User"); - expect(body.emailVerificationRequired).toBe(true); + expect(res.status).toBe(403); + const body = await res.json(); + expect(body.code).toBe("INVITATION_REQUIRED"); - const client = await pool.connect(); - try { - const result = await client.query( - 'SELECT * FROM branch.users WHERE email = $1', - [uniqueEmail] - ); - expect(result.rows.length).toBe(1); - expect(result.rows[0].cognito_sub).toBe(body.userId); - expect(result.rows[0].is_admin).toBe(false); - } finally { - client.release(); + // And nothing was written. + const client = await pool.connect(); + try { + const { rows } = await client.query( + 'SELECT 1 FROM branch.users WHERE email = $1', + [uniqueEmail] + ); + expect(rows).toHaveLength(0); + } finally { + client.release(); + } +}); + +test("seeded admins are pending invitations, not claimed accounts", async () => { + // The claim path depends on this contract: a seeded admin has a row but no + // Cognito identity, so /auth/register activates it instead of 409ing. Before + // claim-on-register these three could never sign in at all. + const client = await pool.connect(); + try { + const { rows } = await client.query( + 'SELECT email, user_id, is_admin, cognito_sub FROM branch.users ORDER BY user_id' + ); + + expect(rows).toHaveLength(3); + for (const row of rows) { + expect(row.cognito_sub).toBeNull(); + expect(row.is_admin).toBe(true); } - } else if (res.status === 500) { - console.log('Skipping Cognito test - Cognito not configured'); - expect(res.status).toBe(500); - } else { - throw new Error(`Unexpected status code: ${res.status}`); + expect(rows.map((r: { email: string }) => r.email)).toEqual([ + 'ashley@branch.org', + 'renee@branch.org', + 'nour@branch.org', + ]); + } finally { + client.release(); } }); 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 9392b91..357f107 100644 --- a/apps/backend/lambdas/auth/test/auth.login.unit.test.ts +++ b/apps/backend/lambdas/auth/test/auth.login.unit.test.ts @@ -474,6 +474,14 @@ describe('GET /me', () => { describe('POST /register — claim-on-register', () => { const validBody = { email: 'Ashley@branch.org', password: 'Password123', name: 'Ashley' }; + /** An admin-created invitation: the row exists, but no Cognito identity yet. */ + const invitation = { + user_id: 1, + email: 'ashley@branch.org', + cognito_sub: null, + is_admin: true, + }; + it('claims a pending invitation (cognito_sub IS NULL) instead of returning 409', async () => { mockExecuteTakeFirst.mockResolvedValue({ user_id: 1, @@ -521,22 +529,37 @@ describe('POST /register — claim-on-register', () => { expect(mockSend).not.toHaveBeenCalled(); }); - it('inserts a new row when no invitation exists', async () => { + it('refuses to create an account for an uninvited email', async () => { + // Registration is invitation-only. This endpoint is public, so without the + // gate anyone on the internet could mint themselves a working account -- + // and several list endpoints authorize on `isAuthenticated` alone. mockExecuteTakeFirst.mockResolvedValue(undefined); - mockSend.mockResolvedValue({ UserSub: 'new-sub' }); - mockExecute.mockResolvedValue(undefined); const res = await handler(event('/register', 'POST', validBody)); - expect(res.statusCode).toBe(201); - expect(JSON.parse(res.body).claimed).toBeUndefined(); - expect(mockValues).toHaveBeenCalledWith( - expect.objectContaining({ cognito_sub: 'new-sub', is_admin: false }), + expect(res.statusCode).toBe(403); + expect(JSON.parse(res.body).code).toBe('INVITATION_REQUIRED'); + // No Cognito user and no DB row: nothing is created at all. + expect(mockSend).not.toHaveBeenCalled(); + expect(mockValues).not.toHaveBeenCalled(); + expect(mockExecute).not.toHaveBeenCalled(); + }); + + it('does not reveal whether an uninvited email is known', async () => { + // Same response shape for an unknown address as for a known-but-uninvited + // one, so /register cannot be used to enumerate staff email addresses. + mockExecuteTakeFirst.mockResolvedValue(undefined); + + const unknown = await handler( + event('/register', 'POST', { ...validBody, email: 'stranger@example.com' }), ); + + expect(unknown.statusCode).toBe(403); + expect(JSON.parse(unknown.body).message).not.toMatch(/not found|no such|unknown/i); }); it('rolls back the Cognito user when the database write fails', async () => { - mockExecuteTakeFirst.mockResolvedValue(undefined); + mockExecuteTakeFirst.mockResolvedValue(invitation); mockSend .mockResolvedValueOnce({ UserSub: 'new-sub' }) // SignUp .mockResolvedValueOnce({}); // AdminDeleteUser