diff --git a/apps/web/auth.ts b/apps/web/auth.ts index 6dbb286..0dfce13 100644 --- a/apps/web/auth.ts +++ b/apps/web/auth.ts @@ -1,7 +1,6 @@ import NextAuth from "next-auth"; import { env } from "@/lib/env"; -import { db } from "@/lib/db/client"; -import { users } from "@/lib/db/schema"; +import { oidClaim, resolveUserId } from "@/lib/auth/identity"; import { syncUserGroupsFromClaim } from "@/lib/auth/sync-groups"; /** @@ -41,27 +40,18 @@ export const { auth, handlers, signIn, signOut } = NextAuth({ const iss = (profile.iss as string | undefined) ?? env().OIDC_ISSUER; if (!sub) throw new Error("OIDC profile missing `sub` claim"); - const row = await db - .insert(users) - .values({ - oidcSub: sub, - oidcIss: iss, - email: profile.email ?? null, - name: profile.name ?? null, - picture: (profile.picture as string | undefined) ?? null, - }) - .onConflictDoUpdate({ - target: [users.oidcIss, users.oidcSub], - set: { - email: profile.email ?? null, - name: profile.name ?? null, - picture: (profile.picture as string | undefined) ?? null, - lastSeenAt: new Date(), - }, - }) - .returning({ id: users.id }); + // Shared with the MCP path (lib/mcp/context.ts). On EntraID the `oid` + // claim is what keeps the two surfaces resolving to one account — + // `sub` differs per app registration there. See lib/auth/identity.ts. + const userId = await resolveUserId({ + iss, + sub, + oid: oidClaim(profile), + email: profile.email ?? null, + name: profile.name ?? null, + picture: (profile.picture as string | undefined) ?? null, + }); - const userId = row[0]?.id; token.userId = userId; token.sub = sub; token.iss = iss; @@ -71,10 +61,15 @@ export const { auth, handlers, signIn, signOut } = NextAuth({ // wipes the user's existing memberships, which is the conservative // choice (don't keep stale grants alive if the IdP stopped // asserting them). + // + // The whole profile goes in, not just `profile.groups`: an absent + // claim means one thing on its own and something else entirely next + // to EntraID's overage markers, and only the second case must abort. + // A GroupsOverageError thrown here fails the sign-in, which is the + // intent — it leaves the user's existing memberships untouched + // instead of silently deleting them. if (userId) { - // `profile.groups` is untyped at the next-auth boundary — coerce. - const claimGroups = (profile as { groups?: unknown }).groups; - await syncUserGroupsFromClaim(userId, iss, claimGroups); + await syncUserGroupsFromClaim(userId, iss, profile); } } return token; diff --git a/apps/web/drizzle/0005_user_oid.sql b/apps/web/drizzle/0005_user_oid.sql new file mode 100644 index 0000000..ab9d29c --- /dev/null +++ b/apps/web/drizzle/0005_user_oid.sql @@ -0,0 +1,29 @@ +-- EntraID identity: key users on `oid` when the IdP emits it. +-- +-- EntraID's `sub` is a PAIRWISE identifier — derived from the token recipient, +-- so the Web UI app registration and the MCP app registration hand out +-- different `sub` values for the same person. Both `auth.ts` and +-- `lib/mcp/context.ts` upsert on (oidc_iss, oidc_sub), so on EntraID one human +-- resolves to two rows: they sign into the Web UI, connect an MCP client, and +-- land in an empty account with their memories nowhere to be seen. Nothing +-- errors, which is what makes it dangerous. +-- +-- `oid` is the user's directory object id, which Microsoft documents as +-- constant for a user across every application in a tenant. Recording it gives +-- us a key that holds across both surfaces. +-- +-- Nullable on purpose: Authentik, Keycloak and Okta emit no `oid`, and there +-- `sub` is already application-independent. Those deployments keep using +-- (oidc_iss, oidc_sub) and are untouched by this migration. + +ALTER TABLE "users" ADD COLUMN "oidc_oid" text; + +-- Partial index. Every non-EntraID row holds NULL here; a plain unique index +-- would treat those as colliding and permit exactly one such user. +CREATE UNIQUE INDEX "users_iss_oid_uq" + ON "users" ("oidc_iss", "oidc_oid") + WHERE "oidc_oid" IS NOT NULL; + +-- No backfill. `oid` is only knowable from a token, so existing rows adopt +-- theirs on the owner's next sign-in (see `adoptLegacyRow` in +-- lib/auth/identity.ts). Backfilling would mean guessing. diff --git a/apps/web/lib/auth/identity.test.ts b/apps/web/lib/auth/identity.test.ts new file mode 100644 index 0000000..8cbb57d --- /dev/null +++ b/apps/web/lib/auth/identity.test.ts @@ -0,0 +1,165 @@ +import { afterAll, beforeEach, describe, expect, test } from "vitest"; + +/** + * Identity resolution across the two surfaces. + * + * The bug these guard against is silent: on EntraID the Web UI and the MCP + * endpoint see different `sub` values for the same person, both code paths + * UPSERT rather than fail, and the result is two accounts — the user signs in, + * connects an MCP client, and finds their memories gone. Nothing errors, so + * only a test that asserts "same person ⇒ same row id" catches it. + */ +const { db, pg } = await import("@/lib/db/client"); +const { users } = await import("@/lib/db/schema"); +const { resolveUserId, oidClaim } = await import("@/lib/auth/identity"); +const { eq } = await import("drizzle-orm"); + +/** `noUncheckedIndexedAccess` is on; narrow once rather than at every use. */ +function first(rows: T[]): T { + const row = rows[0]; + if (!row) throw new Error("expected at least one row"); + return row; +} + +const ISS = "https://login.microsoftonline.com/test-tenant/v2.0"; + +/** Distinct `sub` values, as EntraID's pairwise identifiers would be. */ +const WEB_SUB = "pairwise-sub-for-web-registration"; +const MCP_SUB = "pairwise-sub-for-mcp-registration"; +const OID = "00000000-1111-2222-3333-444444444444"; + +function claims(overrides: Record = {}) { + return { + iss: ISS, + sub: WEB_SUB, + oid: OID, + email: "person@example.com", + name: "Person", + picture: null, + ...overrides, + } as Parameters[0]; +} + +beforeEach(async () => { + await db.delete(users).where(eq(users.oidcIss, ISS)); +}); + +afterAll(async () => { + await db.delete(users).where(eq(users.oidcIss, ISS)); + await pg.end(); +}); + +describe("oidClaim", () => { + test("reads a string oid", () => { + expect(oidClaim({ oid: OID })).toBe(OID); + }); + + test("is null when absent, blank, or not a string", () => { + expect(oidClaim({})).toBeNull(); + expect(oidClaim({ oid: " " })).toBeNull(); + expect(oidClaim({ oid: 42 })).toBeNull(); + expect(oidClaim(null)).toBeNull(); + }); +}); + +describe("resolveUserId with an oid (EntraID)", () => { + test("both surfaces resolve to ONE row despite different subs", async () => { + const fromWeb = await resolveUserId(claims({ sub: WEB_SUB })); + const fromMcp = await resolveUserId(claims({ sub: MCP_SUB })); + + expect(fromMcp).toBe(fromWeb); + + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(1); + }); + + test("does not rewrite oidc_sub once the row exists", async () => { + await resolveUserId(claims({ sub: WEB_SUB })); + await resolveUserId(claims({ sub: MCP_SUB })); + + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + // Whichever arrived first stays put; flip-flopping it on every request + // could collide with the (iss, sub) unique index. + expect(first(rows).oidcSub).toBe(WEB_SUB); + expect(first(rows).oidcOid).toBe(OID); + }); + + test("adopts a pre-migration row instead of stranding it", async () => { + // A deployment that signed this person in before 0005 ran: correct sub, + // no oid recorded. + const legacy = first( + await db + .insert(users) + .values({ oidcIss: ISS, oidcSub: WEB_SUB, email: "old@example.com" }) + .returning({ id: users.id }), + ); + + const resolved = await resolveUserId(claims({ sub: WEB_SUB })); + + expect(resolved).toBe(legacy.id); + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(1); + expect(first(rows).oidcOid).toBe(OID); + }); + + test("refreshes profile fields on an existing row", async () => { + await resolveUserId(claims({ name: "Old Name" })); + await resolveUserId(claims({ sub: MCP_SUB, name: "New Name" })); + + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(first(rows).name).toBe("New Name"); + }); + + test("concurrent first-contact from both surfaces yields one row", async () => { + const [a, b] = await Promise.all([ + resolveUserId(claims({ sub: WEB_SUB })), + resolveUserId(claims({ sub: MCP_SUB })), + ]); + + expect(a).toBe(b); + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(1); + }); + + test("different people in one tenant stay separate", async () => { + const one = await resolveUserId(claims()); + const two = await resolveUserId( + claims({ sub: "other-sub", oid: "99999999-1111-2222-3333-444444444444" }), + ); + + expect(two).not.toBe(one); + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(2); + }); +}); + +describe("resolveUserId without an oid (Authentik and friends)", () => { + test("keys on (iss, sub) exactly as before", async () => { + const initial = await resolveUserId(claims({ oid: null })); + const again = await resolveUserId(claims({ oid: null, name: "Renamed" })); + + expect(again).toBe(initial); + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(1); + expect(first(rows).oidcOid).toBeNull(); + expect(first(rows).name).toBe("Renamed"); + }); + + test("distinct subs are distinct people", async () => { + await resolveUserId(claims({ oid: null, sub: "a" })); + await resolveUserId(claims({ oid: null, sub: "b" })); + + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(2); + }); + + test("several oid-less users coexist — the unique index is partial", async () => { + // A non-partial unique index on (iss, oid) would allow exactly one NULL + // pair and reject everyone after the first. + for (const sub of ["u1", "u2", "u3"]) { + await resolveUserId(claims({ oid: null, sub })); + } + const rows = await db.select().from(users).where(eq(users.oidcIss, ISS)); + expect(rows).toHaveLength(3); + }); +}); diff --git a/apps/web/lib/auth/identity.ts b/apps/web/lib/auth/identity.ts new file mode 100644 index 0000000..eacf6a1 --- /dev/null +++ b/apps/web/lib/auth/identity.ts @@ -0,0 +1,125 @@ +import { and, eq, isNull } from "drizzle-orm"; +import { db } from "@/lib/db/client"; +import { users } from "@/lib/db/schema"; + +/** + * Resolving OIDC claims to the internal `users.id`. + * + * Both surfaces come through here — the Web UI (`auth.ts`) and the MCP + * endpoint (`lib/mcp/context.ts`) — and that is the point. They each used to + * carry their own copy of this upsert, which is how the two drifted into + * disagreeing about who a user is. + * + * ## Why `oid` exists here + * + * Authentik's `sub` is `user.uid`, identical across every provider, so + * (iss, sub) identifies a person. EntraID's `sub` is PAIRWISE: Microsoft + * derives it from the token recipient, so the Web UI app registration and the + * MCP app registration emit different `sub` values for the same human. Keyed + * on `sub`, that person gets two rows — they sign into the Web UI, connect + * Claude Code, and find an empty account. Both paths upsert, so nothing + * errors; the split is completely silent. + * + * `oid` is the directory object id, which Microsoft documents as constant for + * a user across every application in a tenant. When it's present it wins. + * When it's absent (Authentik, Keycloak, Okta) behaviour is exactly as before. + */ + +/** Extract a usable EntraID `oid` claim, or null on IdPs that don't emit one. */ +export function oidClaim(claims: unknown): string | null { + const raw = (claims as { oid?: unknown } | null | undefined)?.oid; + if (typeof raw !== "string") return null; + const trimmed = raw.trim(); + return trimmed.length > 0 ? trimmed : null; +} + +export interface IdentityInput { + iss: string; + sub: string; + /** EntraID object id, or null. */ + oid: string | null; + email: string | null; + name: string | null; + picture: string | null; +} + +/** Postgres unique-violation. */ +function isUniqueViolation(err: unknown): boolean { + return (err as { code?: unknown } | null)?.code === "23505"; +} + +/** + * Resolve (creating if needed) the `users` row for a set of verified claims. + * + * Resolution order when `oid` is present: + * + * 1. a row already keyed on this `oid` — the steady state + * 2. a pre-`oid` row for the same (iss, sub), which gets its `oid` + * backfilled in place. This is how a deployment that ran before the + * 0005 migration keeps its accounts instead of stranding them. + * 3. insert + * + * Without `oid` this collapses to the original (iss, sub) upsert. + */ +export async function resolveUserId(input: IdentityInput): Promise { + const { iss, sub, oid, email, name, picture } = input; + const profile = { email, name, picture, lastSeenAt: new Date() }; + + if (oid) { + // 1. Steady state. + // + // Deliberately does NOT touch `oidc_sub`. The stored value is whichever + // app registration this person first arrived through; rewriting it on + // every request would flip it back and forth between the Web and MCP + // values and could collide with the (iss, sub) unique index. + const byOid = await db + .update(users) + .set(profile) + .where(and(eq(users.oidcIss, iss), eq(users.oidcOid, oid))) + .returning({ id: users.id }); + if (byOid[0]) return byOid[0].id; + + // 2. Adopt a row created before `oid` was recorded. + const adopted = await db + .update(users) + .set({ ...profile, oidcOid: oid }) + .where( + and(eq(users.oidcIss, iss), eq(users.oidcSub, sub), isNull(users.oidcOid)), + ) + .returning({ id: users.id }); + if (adopted[0]) return adopted[0].id; + } + + // 3. Insert. The conflict target stays (iss, sub) because that is the index + // every row has; a concurrent writer racing us on `oid` instead is caught + // below. + try { + const inserted = await db + .insert(users) + .values({ oidcIss: iss, oidcSub: sub, oidcOid: oid, email, name, picture }) + .onConflictDoUpdate({ + target: [users.oidcIss, users.oidcSub], + set: profile, + }) + .returning({ id: users.id }); + if (inserted[0]) return inserted[0].id; + } catch (err) { + // Two requests for the same person arriving together through DIFFERENT + // app registrations: same `oid`, different `sub`, so the (iss, sub) + // conflict target doesn't fire and the partial (iss, oid) index rejects + // the loser. Fall through and read the winner's row. + if (!isUniqueViolation(err)) throw err; + } + + const existing = await db + .select({ id: users.id }) + .from(users) + .where( + oid + ? and(eq(users.oidcIss, iss), eq(users.oidcOid, oid)) + : and(eq(users.oidcIss, iss), eq(users.oidcSub, sub)), + ) + .limit(1); + if (!existing[0]) throw new Error("user upsert failed and not found on re-read"); + return existing[0].id; +} diff --git a/apps/web/lib/auth/jwt.test.ts b/apps/web/lib/auth/jwt.test.ts new file mode 100644 index 0000000..5105e21 --- /dev/null +++ b/apps/web/lib/auth/jwt.test.ts @@ -0,0 +1,226 @@ +import { createServer, type Server } from "node:http"; +import { AddressInfo } from "node:net"; +import { SignJWT, exportJWK, generateKeyPair } from "jose"; +import type { JWK, KeyLike } from "jose"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; + +/** + * The JWKS location is discovered, not assumed. + * + * `${issuer}/jwks/` used to be hardcoded here. That is an Authentik + * convention — EntraID serves its keys at + * `https://login.microsoftonline.com/{tenant}/discovery/v2.0/keys`, so on + * EntraID the hardcoded path 404s and NO MCP token can ever verify. These + * tests pin the three behaviours that make the discovery path safe to ship: + * discovery is honoured, failure degrades to the old path rather than to a + * broken deployment, and it happens once rather than per request. + * + * A real loopback HTTP server is used rather than a `fetch` mock because jose + * fetches the key set through `node:http` directly, not through global + * `fetch` — a mocked `fetch` would silently never be consulted for the JWKS + * request, and the assertion about *which* URL was used would prove nothing. + */ + +const TENANT = "11111111-2222-3333-4444-555555555555"; +const AUDIENCE = "99999999-8888-7777-6666-555555555555"; + +/** Paths the fake IdP was asked for, in order. */ +let requested: string[] = []; +/** Response the fake IdP gives for the discovery document. */ +let discoveryResponse: { status: number; body: string }; +let server: Server; +let origin: string; +let privateKey: KeyLike; +let publicJwk: JWK; + +/** Authentik-shaped issuer: application-scoped path, trailing slash. */ +function authentikIssuer(): string { + return `${origin}/application/o/shared-memory-mcp/`; +} + +/** EntraID-shaped issuer: tenant-scoped, no trailing slash. */ +function entraIssuer(): string { + return `${origin}/${TENANT}/v2.0`; +} + +/** Where an EntraID discovery document points for keys. */ +function entraKeysPath(): string { + return `/${TENANT}/discovery/v2.0/keys`; +} + +beforeEach(async () => { + const pair = await generateKeyPair("RS256"); + privateKey = pair.privateKey; + publicJwk = { ...(await exportJWK(pair.publicKey)), kid: "test-key", alg: "RS256", use: "sig" }; + requested = []; + + server = createServer((req, res) => { + requested.push(req.url ?? ""); + if (req.url?.endsWith("/.well-known/openid-configuration")) { + res.writeHead(discoveryResponse.status, { "content-type": "application/json" }); + res.end(discoveryResponse.body); + return; + } + // Every other path is treated as a key set endpoint. Which path the + // request actually arrived on is the thing under test. + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ keys: [publicJwk] })); + }); + + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + origin = `http://127.0.0.1:${(server.address() as AddressInfo).port}`; +}); + +afterEach(async () => { + // The key set is memoised on globalThis, which survives vi.resetModules(). + // Without clearing it, the second test in this file would silently reuse + // the first test's resolution and assert nothing. + delete (globalThis as Record).__sharedMemoryJwks; + delete (globalThis as Record).__sharedMemoryJwksRetryAt; + delete process.env.OIDC_ISSUER_MCP; + delete process.env.OIDC_AUDIENCE; + await new Promise((resolve) => server.close(() => resolve())); +}); + +/** + * Import `authenticateBearer` fresh so it observes the env vars this test set + * (lib/env.ts caches its parse in a module singleton). + */ +async function loadAuthenticateBearer() { + vi.resetModules(); + const mod = await import("@/lib/auth/jwt"); + return mod.authenticateBearer; +} + +async function signAccessToken(issuer: string): Promise { + return new SignJWT({ groups: ["memory-users"] }) + .setProtectedHeader({ alg: "RS256", kid: "test-key" }) + .setIssuer(issuer) + .setAudience(AUDIENCE) + .setSubject("user-object-id") + .setIssuedAt() + .setExpirationTime("5m") + .sign(privateKey); +} + +function discoveryPathsSeen(): string[] { + return requested.filter((p) => p.endsWith("/.well-known/openid-configuration")); +} + +describe("MCP JWKS resolution", () => { + test("uses the jwks_uri from discovery, so EntraID's key endpoint is reached", async () => { + process.env.OIDC_ISSUER_MCP = entraIssuer(); + process.env.OIDC_AUDIENCE = AUDIENCE; + discoveryResponse = { + status: 200, + // Shape of https://login.microsoftonline.com/{tenant}/v2.0/.well-known/openid-configuration + body: JSON.stringify({ + issuer: entraIssuer(), + jwks_uri: `${origin}${entraKeysPath()}`, + token_endpoint: `${origin}/${TENANT}/oauth2/v2.0/token`, + }), + }; + + const authenticateBearer = await loadAuthenticateBearer(); + const claims = await authenticateBearer(`Bearer ${await signAccessToken(entraIssuer())}`); + + expect(claims.sub).toBe("user-object-id"); + expect(requested).toContain(entraKeysPath()); + // The Authentik convention must NOT have been tried. + expect(requested).not.toContain(`/${TENANT}/v2.0/jwks/`); + }); + + test("tolerates a trailing slash on the issuer while still honouring discovery", async () => { + // acceptedIssuers() takes both spellings; discovery must not regress that. + process.env.OIDC_ISSUER_MCP = `${entraIssuer()}/`; + process.env.OIDC_AUDIENCE = AUDIENCE; + discoveryResponse = { + status: 200, + body: JSON.stringify({ jwks_uri: `${origin}${entraKeysPath()}` }), + }; + + const authenticateBearer = await loadAuthenticateBearer(); + // Token carries the un-slashed spelling; the env var carries the slashed one. + const claims = await authenticateBearer(`Bearer ${await signAccessToken(entraIssuer())}`); + + expect(claims.sub).toBe("user-object-id"); + expect(requested).toContain(entraKeysPath()); + }); + + test("falls back to ${issuer}/jwks/ when discovery is unreachable", async () => { + process.env.OIDC_ISSUER_MCP = authentikIssuer(); + process.env.OIDC_AUDIENCE = AUDIENCE; + discoveryResponse = { status: 500, body: "upstream exploded" }; + + const authenticateBearer = await loadAuthenticateBearer(); + const claims = await authenticateBearer(`Bearer ${await signAccessToken(authentikIssuer())}`); + + // Existing Authentik deployments keep working with no discovery document. + expect(claims.sub).toBe("user-object-id"); + expect(requested).toContain("/application/o/shared-memory-mcp/jwks/"); + }); + + test("falls back to ${issuer}/jwks/ when discovery omits jwks_uri", async () => { + process.env.OIDC_ISSUER_MCP = authentikIssuer(); + process.env.OIDC_AUDIENCE = AUDIENCE; + // 200 OK, valid JSON, no usable key set pointer — the malformed case that + // a naive `doc.jwks_uri` read would turn into `new URL(undefined)`. + discoveryResponse = { status: 200, body: JSON.stringify({ issuer: authentikIssuer() }) }; + + const authenticateBearer = await loadAuthenticateBearer(); + const claims = await authenticateBearer(`Bearer ${await signAccessToken(authentikIssuer())}`); + + expect(claims.sub).toBe("user-object-id"); + expect(requested).toContain("/application/o/shared-memory-mcp/jwks/"); + }); + + test("falls back when discovery returns non-JSON", async () => { + process.env.OIDC_ISSUER_MCP = authentikIssuer(); + process.env.OIDC_AUDIENCE = AUDIENCE; + discoveryResponse = { status: 200, body: "login page" }; + + const authenticateBearer = await loadAuthenticateBearer(); + const claims = await authenticateBearer(`Bearer ${await signAccessToken(authentikIssuer())}`); + + expect(claims.sub).toBe("user-object-id"); + expect(requested).toContain("/application/o/shared-memory-mcp/jwks/"); + }); + + test("discovers once across many verifications, not once per request", async () => { + // The MCP endpoint verifies a token on essentially every request. A + // discovery fetch per request would add a round-trip to every tool call. + process.env.OIDC_ISSUER_MCP = entraIssuer(); + process.env.OIDC_AUDIENCE = AUDIENCE; + discoveryResponse = { + status: 200, + body: JSON.stringify({ jwks_uri: `${origin}${entraKeysPath()}` }), + }; + + const authenticateBearer = await loadAuthenticateBearer(); + for (let i = 0; i < 3; i++) { + await authenticateBearer(`Bearer ${await signAccessToken(entraIssuer())}`); + } + + expect(discoveryPathsSeen()).toHaveLength(1); + }); + + test("concurrent cold requests share a single discovery fetch", async () => { + // Caching the settled value rather than the in-flight promise would let + // every request that arrives before the first one resolves start its own + // discovery fetch — a thundering herd at process start. + process.env.OIDC_ISSUER_MCP = entraIssuer(); + process.env.OIDC_AUDIENCE = AUDIENCE; + discoveryResponse = { + status: 200, + body: JSON.stringify({ jwks_uri: `${origin}${entraKeysPath()}` }), + }; + + const authenticateBearer = await loadAuthenticateBearer(); + const token = await signAccessToken(entraIssuer()); + await Promise.all( + Array.from({ length: 5 }, () => authenticateBearer(`Bearer ${token}`)), + ); + + expect(discoveryPathsSeen()).toHaveLength(1); + }); +}); diff --git a/apps/web/lib/auth/jwt.ts b/apps/web/lib/auth/jwt.ts index 3b04423..f404c34 100644 --- a/apps/web/lib/auth/jwt.ts +++ b/apps/web/lib/auth/jwt.ts @@ -2,13 +2,14 @@ import { createRemoteJWKSet, jwtVerify, errors as joseErrors } from "jose"; import type { JWTPayload } from "jose"; import { env } from "@/lib/env"; import { CLI_TOKEN_KID, tokenKid, verifyCliToken } from "./cli-token"; +import { detectGroupsOverage } from "./sync-groups"; /** * Authenticates a bearer token presented to the MCP endpoint. Two token * kinds are accepted, dispatched by the JWT `kid` header: * - * - Authentik-issued OIDC access tokens (any kid) — verified against - * Authentik's JWKS over the network. + * - IdP-issued OIDC access tokens (any kid) — verified against the + * issuer's JWKS over the network, located via OIDC discovery. * - CLI tokens minted at /connect (kid="cli-v1") — verified locally * with the HMAC CLI_TOKEN_SECRET. * @@ -18,8 +19,25 @@ import { CLI_TOKEN_KID, tokenKid, verifyCliToken } from "./cli-token"; * This is distinct from the NextAuth session cookie path used by the Web UI. */ +type JwkSet = ReturnType; + type GlobalWithJwks = typeof globalThis & { - __sharedMemoryJwks?: ReturnType; + /** + * Resolved key set, cached as a *promise* rather than a value. + * + * Resolution now involves a network round-trip (OIDC discovery), and the + * MCP endpoint verifies a token on essentially every request. Caching the + * settled value would leave a window in which N concurrent cold requests + * each start their own discovery fetch; caching the in-flight promise means + * the first caller does the work and everyone else awaits the same result. + */ + __sharedMemoryJwks?: Promise; + /** + * Epoch ms after which discovery should be re-attempted, set only when we + * had to fall back (see `jwks()`). Undefined means the cached set came from + * a successful discovery and is good indefinitely. + */ + __sharedMemoryJwksRetryAt?: number; }; const g = globalThis as GlobalWithJwks; @@ -27,6 +45,10 @@ const g = globalThis as GlobalWithJwks; * Issuer of MCP access tokens. The MCP endpoint is a separate application in * the IdP from the Web UI, and Authentik stamps each token with its own * application slug, so this is NOT interchangeable with OIDC_ISSUER. + * + * Not every IdP works that way: EntraID has one issuer per tenant regardless + * of how many app registrations you create, so OIDC_ISSUER_MCP is left unset + * there and this falls through to OIDC_ISSUER. */ export function mcpIssuer(): string { return env().OIDC_ISSUER_MCP ?? env().OIDC_ISSUER; @@ -50,16 +72,88 @@ function acceptedIssuers(): [string, string] { return [bare, `${bare}/`]; } -function jwks() { - if (g.__sharedMemoryJwks) return g.__sharedMemoryJwks; - // Authentik discovery is at `${issuer}/.well-known/openid-configuration`; - // the JWKS URI is normally `${issuer}/jwks/` or `${issuer}/.well-known/jwks.json`. - // Authentik canonically serves `${issuer}/jwks/`. - const url = new URL(`${mcpIssuer().replace(/\/$/, "")}/jwks/`); - g.__sharedMemoryJwks = createRemoteJWKSet(url, { - cacheMaxAge: 10 * 60 * 1000, // 10 min - cooldownDuration: 30 * 1000, - }); +const JWKS_OPTIONS = { + cacheMaxAge: 10 * 60 * 1000, // 10 min + cooldownDuration: 30 * 1000, +} as const; + +/** How long to keep serving a fallback key set before retrying discovery. */ +const DISCOVERY_RETRY_COOLDOWN_MS = 60 * 1000; + +/** Discovery can hang; every MCP request waits on it, so bound it. */ +const DISCOVERY_TIMEOUT_MS = 5 * 1000; + +/** + * The pre-discovery convention: `${issuer}/jwks/`. + * + * This is Authentik's canonical JWKS path and was hardcoded here. It stays as + * the fallback so that a deployment whose discovery document is unreachable + * behaves exactly as it did before this change. + */ +function fallbackJwksUri(): string { + return `${mcpIssuer().replace(/\/$/, "")}/jwks/`; +} + +/** + * Read `jwks_uri` out of the MCP issuer's OIDC discovery document. + * + * `${issuer}/jwks/` is an Authentik convention, not a standard — RFC 8414 + * says the key set lives wherever `jwks_uri` points, and providers disagree + * wildly. EntraID serves keys at + * `https://login.microsoftonline.com/{tenant}/discovery/v2.0/keys`, nowhere + * near `${issuer}/jwks/`, so with the path hardcoded every EntraID-issued MCP + * token fails verification with a 404 on the key set — authentication is + * simply impossible, not merely misconfigured. Ask the issuer where its keys + * are instead of guessing. + * + * Returns null (never throws) on any failure, so the caller can fall back. + */ +async function discoverJwksUri(): Promise { + const url = `${mcpIssuer().replace(/\/$/, "")}/.well-known/openid-configuration`; + try { + const res = await fetch(url, { + headers: { accept: "application/json" }, + signal: AbortSignal.timeout(DISCOVERY_TIMEOUT_MS), + }); + if (!res.ok) return null; + const doc: unknown = await res.json(); + const uri = (doc as { jwks_uri?: unknown } | null)?.jwks_uri; + if (typeof uri !== "string" || uri.trim().length === 0) return null; + // A malformed jwks_uri must not blow up the request path. + new URL(uri); + return uri; + } catch { + return null; + } +} + +/** + * The key set MCP access tokens are verified against, resolved once per + * process. + * + * Async because discovery is a network call. The cached promise is installed + * synchronously — before the first `await` inside the IIFE runs — so + * concurrent callers always join the existing resolution rather than racing + * to start their own. + * + * When discovery fails we serve the legacy fallback but arm a retry: a single + * blip at process start would otherwise pin the wrong URL for the lifetime of + * the container, which on EntraID means MCP auth stays broken until someone + * restarts it. The cooldown keeps a persistently-unreachable discovery + * endpoint from being hit on every request. + */ +function jwks(): Promise { + const retryAt = g.__sharedMemoryJwksRetryAt; + const dueForRetry = retryAt !== undefined && Date.now() >= retryAt; + if (g.__sharedMemoryJwks && !dueForRetry) return g.__sharedMemoryJwks; + + g.__sharedMemoryJwksRetryAt = undefined; + g.__sharedMemoryJwks = (async () => { + const discovered = await discoverJwksUri(); + if (discovered) return createRemoteJWKSet(new URL(discovered), JWKS_OPTIONS); + g.__sharedMemoryJwksRetryAt = Date.now() + DISCOVERY_RETRY_COOLDOWN_MS; + return createRemoteJWKSet(new URL(fallbackJwksUri()), JWKS_OPTIONS); + })(); return g.__sharedMemoryJwks; } @@ -143,7 +237,7 @@ export async function authenticateBearer(authHeader: string | null): Promise { + const rows = await db + .select({ name: groups.name }) + .from(userGroups) + .innerJoin(groups, eq(userGroups.groupId, groups.id)) + .where(eq(userGroups.userId, userId)); + return rows.map((r) => r.name).sort(); +} + +beforeEach(async () => { + await db.delete(users).where(eq(users.oidcIss, ISS)); + await db.delete(groups).where(eq(groups.oidcIss, ISS)); + const rows = await db + .insert(users) + .values({ oidcIss: ISS, oidcSub: "sync-test-sub" }) + .returning({ id: users.id }); + const row = rows[0]; + if (!row) throw new Error("failed to seed test user"); + userId = row.id; +}); + +afterAll(async () => { + await db.delete(users).where(eq(users.oidcIss, ISS)); + await db.delete(groups).where(eq(groups.oidcIss, ISS)); + await pg.end(); +}); + +describe("detectGroupsOverage", () => { + test("spots the JWT overage pointer", () => { + expect( + detectGroupsOverage({ + _claim_names: { groups: "src1" }, + _claim_sources: { src1: { endpoint: "https://graph.windows.net/x" } }, + }), + ).toBe(true); + }); + + test("spots the implicit-flow indicator", () => { + expect(detectGroupsOverage({ hasgroups: true })).toBe(true); + }); + + test("is false for ordinary claims", () => { + expect(detectGroupsOverage({ groups: ["a"] })).toBe(false); + expect(detectGroupsOverage({})).toBe(false); + expect(detectGroupsOverage(null)).toBe(false); + // A _claim_names for some OTHER claim is not a groups overage. + expect(detectGroupsOverage({ _claim_names: { roles: "src1" } })).toBe(false); + expect(detectGroupsOverage({ hasgroups: false })).toBe(false); + }); +}); + +describe("syncUserGroupsFromClaim", () => { + test("stores the claim's names", async () => { + await syncUserGroupsFromClaim(userId, ISS, { groups: ["eng", "ops"] }); + expect(await memberships()).toEqual(["eng", "ops"]); + }); + + test("an empty claim really does clear memberships", async () => { + await syncUserGroupsFromClaim(userId, ISS, { groups: ["eng"] }); + await syncUserGroupsFromClaim(userId, ISS, { groups: [] }); + expect(await memberships()).toEqual([]); + }); + + test("an absent claim clears memberships", async () => { + // Unchanged behaviour: the IdP has stopped asserting groups, so we stop + // honouring them rather than keeping stale grants alive. + await syncUserGroupsFromClaim(userId, ISS, { groups: ["eng"] }); + await syncUserGroupsFromClaim(userId, ISS, {}); + expect(await memberships()).toEqual([]); + }); + + test("overage throws instead of clearing", async () => { + await syncUserGroupsFromClaim(userId, ISS, { groups: ["eng", "ops"] }); + + await expect( + syncUserGroupsFromClaim(userId, ISS, { + _claim_names: { groups: "src1" }, + _claim_sources: { src1: { endpoint: "https://graph.windows.net/x" } }, + }), + ).rejects.toBeInstanceOf(GroupsOverageError); + + // The whole point: the snapshot survives, so the operator can fix the IdP + // and the user comes back with their access intact. + expect(await memberships()).toEqual(["eng", "ops"]); + }); + + test("the overage error names the fix", async () => { + let err: Error | null = null; + try { + await syncUserGroupsFromClaim(userId, ISS, { hasgroups: true }); + } catch (e) { + err = e as Error; + } + + expect(err).toBeInstanceOf(GroupsOverageError); + expect(err?.message).toContain("groupMembershipClaims"); + expect(err?.message).toContain("ApplicationGroup"); + }); + + test("non-string entries are dropped, names are de-duplicated", async () => { + await syncUserGroupsFromClaim(userId, ISS, { + groups: ["eng", 7, null, " eng ", "ops"], + }); + expect(await memberships()).toEqual(["eng", "ops"]); + }); +}); diff --git a/apps/web/lib/auth/sync-groups.ts b/apps/web/lib/auth/sync-groups.ts index e3e342c..ec4d906 100644 --- a/apps/web/lib/auth/sync-groups.ts +++ b/apps/web/lib/auth/sync-groups.ts @@ -2,34 +2,99 @@ import { and, eq, notInArray, sql } from "drizzle-orm"; import { db } from "@/lib/db/client"; import { groups, userGroups } from "@/lib/db/schema"; +/** + * Raised when the IdP signals that it holds group memberships it declined to + * enumerate (EntraID's "groups overage"). Callers must abort — see + * `detectGroupsOverage` for why this cannot be treated as "no groups". + */ +export class GroupsOverageError extends Error { + constructor() { + super( + "OIDC groups overage: the identity provider signalled group membership " + + "it did not enumerate, so the user's groups cannot be determined. On " + + "EntraID, set the app registration's `groupMembershipClaims` to " + + '"ApplicationGroup" (portal: "Groups assigned to the application") and ' + + "assign the groups you share projects with. See docs/oidc-entra-id.md " + + "§10b.", + ); + this.name = "GroupsOverageError"; + } +} + +/** + * Does this token say "there are groups, but I'm not listing them"? + * + * EntraID stops emitting `groups` past 200 entries in a JWT (150 in SAML, 5 in + * implicit flow) and substitutes a pointer: + * + * "_claim_names": { "groups": "src1" }, + * "_claim_sources": { "src1": { "endpoint": "https://graph.windows.net/…" } } + * + * or, for implicit flow, `"hasgroups": true`. + * + * This is NOT a truncated list — it is no list at all, and it is materially + * different from "this user belongs to zero groups". Conflating the two is + * what made this dangerous: the absent-claim branch below deletes every one of + * the user's memberships, so a user crossing the 200-group line would silently + * lose access to every shared project on both surfaces, with no error raised + * anywhere. + * + * We refuse instead. Group state gates `readableProjectIds` / `canWriteProject`, + * and granting or revoking access on state we know we don't have is guesswork + * either way. Failing loudly destroys nothing and names its own fix. + */ +export function detectGroupsOverage(claims: unknown): boolean { + const c = claims as + | { _claim_names?: unknown; hasgroups?: unknown } + | null + | undefined; + if (!c || typeof c !== "object") return false; + if (c.hasgroups === true) return true; + const names = c._claim_names; + return ( + typeof names === "object" && + names !== null && + "groups" in (names as Record) + ); +} + /** * Sync a user's group memberships from the OIDC `groups` claim on sign-in. * + * Takes the whole claims object, not just the claim value, because deciding + * what an absent `groups` means requires seeing the overage markers that sit + * beside it. + * * Claim shape: `string[]`. Authentik emits group *names* directly here; - * Keycloak and Okta likewise (with the right mappers configured). EntraID, - * when correctly configured per README, emits names too — but the default - * "groups" optional-claim variant emits object-id GUIDs instead, and if the - * user is in too many groups EntraID switches to a "groups overage" - * indicator (no group list at all). We take the conservative path: + * Keycloak and Okta likewise (with the right mappers configured). EntraID + * emits object-id GUIDs by default — `cloud_displayname` gets you names, but + * only under `groupMembershipClaims: "ApplicationGroup"`, and only for + * directly assigned groups. See docs/oidc-entra-id.md §10a. * * - whatever strings appear in the claim are treated as names verbatim * and stored as-is. If your IdP emits GUIDs, the UI will show GUIDs; * fix it at the IdP layer (we don't attempt resolution). * - if the claim is missing/empty, the user is treated as having zero - * groups and all existing memberships are deleted. - * - groups overage (where EntraID emits `_claim_names.groups` instead of - * `groups`) is not handled in v1 — the user appears as having no - * groups. Documented limit; revisit if it bites someone. + * groups and all existing memberships are deleted. That is the + * conservative reading: don't keep stale grants alive once the IdP has + * stopped asserting them. + * - if the IdP signals an overage, we throw rather than apply either + * reading. See `detectGroupsOverage`. * * The whole operation runs in a single transaction so the membership * snapshot is atomic (no window where a user partially has new memberships * and still has stale ones). + * + * @throws {GroupsOverageError} when the claims carry an overage indicator. */ export async function syncUserGroupsFromClaim( userId: string, oidcIss: string, - rawClaim: unknown, + claims: unknown, ): Promise { + if (detectGroupsOverage(claims)) throw new GroupsOverageError(); + + const rawClaim = (claims as { groups?: unknown } | null | undefined)?.groups; const names = normalizeGroupsClaim(rawClaim); await db.transaction(async (tx) => { diff --git a/apps/web/lib/db/schema.ts b/apps/web/lib/db/schema.ts index 4db9a24..8c298cd 100644 --- a/apps/web/lib/db/schema.ts +++ b/apps/web/lib/db/schema.ts @@ -53,6 +53,21 @@ export const users = pgTable( oidcSub: text("oidc_sub").notNull(), // OIDC `iss` so we can disambiguate if we ever federate. oidcIss: text("oidc_iss").notNull(), + /** + * EntraID `oid` — the user's directory object id. + * + * Null on IdPs that don't emit it (Authentik, Keycloak, Okta), where + * `sub` is already stable across applications and remains the key. + * + * EntraID's `sub` is PAIRWISE: it is derived from the token recipient, + * so the Web UI app registration and the MCP app registration produce + * different `sub` values for the same human. Keying on `sub` there + * silently creates two accounts for one person — sign in on the web, + * connect an MCP client, find an empty account. `oid` is the identifier + * Microsoft documents as constant for a user across every application in + * a tenant, so it takes precedence whenever it's present. + */ + oidcOid: text("oidc_oid"), email: text("email"), name: text("name"), picture: text("picture"), @@ -61,6 +76,11 @@ export const users = pgTable( }, (t) => ({ uniqueIss: uniqueIndex("users_iss_sub_uq").on(t.oidcIss, t.oidcSub), + // Partial: rows from IdPs that emit no `oid` all hold NULL here, and a + // plain unique index would collapse them into a single allowed row. + uniqueOid: uniqueIndex("users_iss_oid_uq") + .on(t.oidcIss, t.oidcOid) + .where(sql`${t.oidcOid} IS NOT NULL`), }), ); diff --git a/apps/web/lib/mcp/context.ts b/apps/web/lib/mcp/context.ts index e0eb6fc..1e7e933 100644 --- a/apps/web/lib/mcp/context.ts +++ b/apps/web/lib/mcp/context.ts @@ -1,6 +1,7 @@ import { db } from "@/lib/db/client"; -import { users, groups, userGroups } from "@/lib/db/schema"; -import { and, eq } from "drizzle-orm"; +import { groups, userGroups } from "@/lib/db/schema"; +import { eq } from "drizzle-orm"; +import { oidClaim, resolveUserId } from "@/lib/auth/identity"; import type { AuthenticatedClaims } from "@/lib/auth/jwt"; /** @@ -55,37 +56,18 @@ export async function userContextFromClaims( const name = (claims.name as string | undefined) ?? null; const picture = (claims.picture as string | undefined) ?? null; - const row = await db - .insert(users) - .values({ - oidcSub: claims.sub, - oidcIss: claims.iss, - email, - name, - picture, - }) - .onConflictDoUpdate({ - target: [users.oidcIss, users.oidcSub], - set: { - email, - name, - picture, - lastSeenAt: new Date(), - }, - }) - .returning({ id: users.id }); - - let userId = row[0]?.id; - if (!userId) { - // Race against another upsert — fall back to a select. - const existing = await db - .select({ id: users.id }) - .from(users) - .where(and(eq(users.oidcIss, claims.iss), eq(users.oidcSub, claims.sub))) - .limit(1); - if (!existing[0]) throw new Error("user upsert failed and not found on re-read"); - userId = existing[0].id; - } + // Shared with the Web UI sign-in path (auth.ts). Keeping one resolver is + // what stops the two surfaces disagreeing about who a user is — on EntraID + // they see different `sub` values for the same person and would otherwise + // each create their own account. See lib/auth/identity.ts. + const userId = await resolveUserId({ + iss: claims.iss, + sub: claims.sub, + oid: oidClaim(claims), + email, + name, + picture, + }); // OIDC bearer tokens carry a `groups` claim (when the IdP is configured to // emit it). CLI tokens never do — they go through verifyCliToken which