diff --git a/README.md b/README.md index 1cfc83f..77ba3ef 100644 --- a/README.md +++ b/README.md @@ -249,6 +249,13 @@ shape is the same on any OIDC provider; the UI labels differ: | Redirect URI list | Provider's "Redirect URIs / Origins" | App's "Redirect URIs" | Client's "Valid Redirect URIs" | | Audience claim | Scope mapping or property mapping | "Expose an API" + scope | Client scope with audience mapper | +> **Using Microsoft Entra ID?** The differences are large enough that Entra +> gets its own walkthrough: **[docs/oidc-entra-id.md](docs/oidc-entra-id.md)**. +> It follows the same A/B structure as the steps below, and covers the +> Entra-specific traps — access token version, tenant-specific authority, +> `aud` vs. scope URI, redirect-URI platform type, group GUIDs and overage — +> which otherwise surface only as opaque 401s. + ### A. Web UI provider **Admin → Applications → Providers → Create → OAuth2/OpenID Provider** 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 diff --git a/docs/oidc-entra-id.md b/docs/oidc-entra-id.md new file mode 100644 index 0000000..1676e59 --- /dev/null +++ b/docs/oidc-entra-id.md @@ -0,0 +1,688 @@ +# OIDC provider setup: Microsoft Entra ID + +Companion to the **OIDC provider setup** section in [`README.md`](../README.md), +which walks through Authentik. The structure here deliberately mirrors it — +app registration A (Web UI), app registration B (MCP resource server), env var +mapping, verification — so the two are diffable. Where Entra genuinely differs +from Authentik, the difference is called out rather than smoothed over. + +Everything below assumes a **single-tenant** deployment (`signInAudience` = +"Accounts in this organizational directory only"). Multitenant is possible but +the `iss` verification in `apps/web/lib/auth/jwt.ts` compares against a fixed +string, so it would need code changes — see [Multitenant](#13-multitenant-is-not-supported) +at the end. + +--- + +## 0. Prerequisite: your build must have JWKS discovery + +**Check this first. Nothing else in this document works without it.** + +Until recently `apps/web/lib/auth/jwt.ts` hardcoded the JWKS location as +`${issuer}/jwks/`. That is an *Authentik* convention, not a standard — RFC 8414 +says the key set lives wherever the discovery document's `jwks_uri` points, and +Entra puts it somewhere else entirely: + +| IdP | JWKS URL | +|---|---| +| Authentik | `https://auth.example.com/application/o//jwks/` | +| Entra ID | `https://login.microsoftonline.com//discovery/v2.0/keys` | + +With the path hardcoded, every Entra-issued MCP access token fails verification +because the key set fetch 404s. There is no configuration that works around it; +MCP authentication is simply impossible. + +The current code resolves `jwks_uri` from +`${OIDC_ISSUER_MCP or OIDC_ISSUER}/.well-known/openid-configuration`, caches the +result for the process lifetime, and falls back to `${issuer}/jwks/` only if +discovery is unreachable (so existing Authentik deployments are untouched). + +Confirm your deployment has it before debugging anything else: + +```bash +# Should return the Entra keys endpoint, not a 404. +curl -s "https://login.microsoftonline.com//v2.0/.well-known/openid-configuration" \ + | jq -r .jwks_uri +# → https://login.microsoftonline.com//discovery/v2.0/keys +``` + +If MCP calls 401 with `error_description="verification failed"` and your app +logs show a fetch to `.../v2.0/jwks/`, you are on an older build. + +--- + +## 1. Concepts, Authentik → Entra + +| Concept here | Authentik | Entra ID | +|---|---|---| +| OAuth2 client | Provider + Application | App registration | +| Issuer | Per-application (`.../application/o//`) | **Per-tenant only** — one issuer for the whole directory | +| Audience claim | Scope mapping returning `{"aud": …}` | "Expose an API" scope on the resource app; `aud` is set automatically | +| Redirect URI matching | Regex allowed (any port) | **Exact string match**, with one loopback exception | +| Dynamic client registration | Not implemented | Not implemented | + +The **issuer** row is the one that reshapes the setup. On Authentik, the Web UI +and MCP endpoint are separate applications with separate issuers, which is why +`OIDC_ISSUER_MCP` exists. Entra has exactly one issuer per tenant no matter how +many app registrations you create, so **`OIDC_ISSUER_MCP` is left unset on +Entra** and `mcpIssuer()` falls through to `OIDC_ISSUER`. + +You still create **two app registrations**, for the same reason as on Authentik: +one confidential client for the browser sign-in, one resource server that owns +the audience the MCP endpoint validates. (A third participant — the *public +PKCE client* Claude Code uses — is covered in §4; you can fold it into +registration B or split it out.) + +--- + +## 2. Find your tenant ID and use it explicitly + +Everywhere below, `` is your directory (tenant) GUID, from +**Entra admin center → Overview → Tenant ID**. + +**Do not use the `common` or `organizations` authority.** Their discovery +documents return a *templated* issuer — the literal string, verified live: + +```bash +curl -s https://login.microsoftonline.com/common/v2.0/.well-known/openid-configuration | jq -r .issuer +# → https://login.microsoftonline.com/{tenantid}/v2.0 +``` + +That `{tenantid}` is not a formatting artifact; it is what the endpoint really +returns. `jwt.ts` compares `iss` by exact string (via `acceptedIssuers()`), so +against a templated issuer **no token can ever match** and every MCP call fails +with `claim invalid: iss`. + +Microsoft's documented pattern for multitenant apps is to substitute the token's +`tid` claim into the placeholder and then compare — this app does not do that +(see [Multitenant](#13-multitenant-is-not-supported)). For single-tenant, the fix +is simply to use the tenant-specific authority, whose discovery document +returns a concrete issuer: + +```bash +curl -s "https://login.microsoftonline.com//v2.0/.well-known/openid-configuration" | jq -r .issuer +# → https://login.microsoftonline.com//v2.0 (no trailing slash) +``` + +A verified domain (`contoso.onmicrosoft.com`) also works as the authority +segment — Entra resolves it server-side and returns the GUID form in both +`issuer` and `jwks_uri`. Since the *returned* issuer is what tokens carry, +`OIDC_ISSUER` must be the GUID form regardless of which you typed. + +--- + +## 3. App registration A — Web UI (confidential client) + +**Entra admin center → Entra ID → App registrations → New registration** + +- **Name:** `shared-memory-web` +- **Supported account types:** Accounts in this organizational directory only +- **Redirect URI:** platform **Web**, value: + ``` + https://memory.example.com/api/auth/callback/oidc + ``` + (replace with your `PUBLIC_URL`; the `/oidc` suffix comes from the provider + id in `apps/web/auth.ts` and is not configurable without a code change) + +Register, then collect: + +- **Overview → Application (client) ID** → `.env` as `OIDC_CLIENT_ID_WEB` +- **Certificates & secrets → New client secret** → `.env` as + `OIDC_CLIENT_SECRET_WEB` (copy the *Value*, not the Secret ID; it is shown + once) + +**API permissions:** `openid`, `profile`, `email` are Microsoft Graph delegated +permissions and are present by default via `User.Read`. Add `profile` +explicitly if it is missing — it gates the `oid` and `tid` claims, which +matter for §7. + +No "Expose an API" configuration is needed on this registration. Auth.js only +consumes the ID token here. + +--- + +## 4. App registration B — MCP resource server + +This registration is what `OIDC_AUDIENCE` refers to. It owns the API scope that +Claude Code requests, and the MCP endpoint validates that tokens were minted +for it. + +**App registrations → New registration** + +- **Name:** `shared-memory-mcp` +- **Supported account types:** same as A + +### 4a. Set the access token version — the single most common failure + +**Manage → Manifest**, find and set: + +```json +"api": { + "requestedAccessTokenVersion": 2 +} +``` + +There is no checkbox for this; it is a manifest edit. + +> **Note on the property name.** Older guides (and older versions of this +> project's notes) call this `accessTokenAcceptedVersion` at the top level of +> the manifest. That is the **retired Azure AD Graph** manifest format — +> Microsoft removed it from the portal's manifest editor on 2025-01-07, so you +> will not find that property. The current Microsoft Graph app manifest nests +> it as `api.requestedAccessTokenVersion`. The semantics are identical: +> `null` or `1` → v1.0 tokens, `2` → v2.0 tokens. + +Leave it at the default `null` and Entra issues **v1.0** access tokens, whose +issuer is: + +``` +https://sts.windows.net// ← note the trailing slash +``` + +not `https://login.microsoftonline.com//v2.0`. Verification then +fails with `claim invalid: iss`, and — because everything else in the OAuth +handshake succeeded — it looks like a mysterious 401 rather than a +configuration error. + +The setting lives on the **resource** app and wins over whichever endpoint the +client used: with `requestedAccessTokenVersion: 2`, a client hitting the v1.0 +endpoint still receives a v2.0 access token. + +### 4b. Expose an API + +**Manage → Expose an API** + +1. **Application ID URI** → *Add* → accept the default `api://`. +2. **Add a scope**: + - **Scope name:** `access_as_user` + - **Who can consent:** **Admins and users** (see §9) + - Fill in the admin/user consent display strings; they appear on the consent + prompt. + +The resulting full scope string is `api:///access_as_user`. + +### 4c. The public PKCE client (Claude Code) + +Claude Code is a public client using PKCE. Add a platform to registration B +(or to a third registration if you prefer them separated — then that +registration's client ID is `OIDC_CLIENT_ID_MCP`, and it needs +`api:///access_as_user` under **API permissions**): + +**Manage → Authentication → Add a platform → Mobile and desktop applications** + +> **The platform type is not cosmetic.** A redirect URI registered under the +> **Web** platform classifies the app as a *confidential* client, and the +> token exchange then demands a `client_secret` or `client_assertion` — +> Claude Code has neither, so the flow dies with +> `AADSTS7000218: The request body must contain the following parameter: +> 'client_assertion' or 'client_secret'`. The portal will happily accept +> `http://localhost:33418/callback` as a Web redirect URI, which is what makes +> this trap easy to fall into. **Mobile and desktop applications** +> (`publicClient` in the manifest) is the correct platform. SPA is not an +> option either — Entra rejects SPA redirect URIs for non-SPA flows. + +Under **Custom redirect URIs**, register: + +``` +http://localhost/callback +https://memory.example.com/auth/cli-callback +``` + +The first covers the loopback listener from README → *B. OAuth flow*; the +second is the manual-paste fallback from *C*. "Mobile and desktop +applications" permits arbitrary `https://` URIs alongside the loopback one, so +both live on the same platform. + +**Note the missing port.** Entra ignores the port component when matching +`http://localhost` redirect URIs, so the single registration +`http://localhost/callback` matches `http://localhost:33418/callback`, +`http://localhost:9999/callback`, and any other port. This is Entra's +equivalent of the Authentik regex (`^http://(127\.0\.0\.1|localhost):\d+/.*$`) +the README mentions — users can pick any `--callback-port` without +re-registering. + +Three constraints on that convenience: + +- **The path is still matched exactly.** Registering bare `http://localhost` + does *not* match `http://localhost:33418/callback`. The `/callback` suffix + must be there, and paths are case-sensitive. +- **Do not register several localhost URIs differing only by port.** Entra + picks one arbitrarily when matching. +- **Port-agnostic matching is documented for `localhost` only**, not for + `127.0.0.1` — and the portal text box refuses the `http://127.0.0.1` form + anyway (it requires a manifest edit). Use `localhost`. `[::1]` is not + supported at all. + +You do **not** need to enable **Allow public client flows** +(`allowPublicClient`). That toggle is a *fallback* for flows where Entra can't +infer the client type from a redirect URI — device code, ROPC, Windows +Integrated Auth. Authorization code + PKCE with a registered +mobile-and-desktop redirect URI is inferred correctly without it. (Entra's own +`reply-url` doc says otherwise in one sentence; the manifest reference and the +AADSTS7000218 troubleshooting article agree it is a fallback. Leave it off +unless you hit a problem — Microsoft warns that flipping a confidential client +to public has security implications.) + +**Application (client) ID** of whichever registration Claude Code +authenticates as → `.env` as `OIDC_CLIENT_ID_MCP`. + +--- + +## 5. `OIDC_AUDIENCE` vs. `OIDC_AUDIENCE_SCOPE` + +On Authentik these two look redundant — the scope mapping is named +`aud-shared-memory` and it emits `aud: shared-memory`, so the values track each +other. On Entra they are **necessarily different strings**, and swapping them is +the easiest mistake to make here. + +| Var | What it is | Entra value | +|---|---|---| +| `OIDC_AUDIENCE` | The `aud` claim `jwt.ts` requires on the token | `` — a bare GUID | +| `OIDC_AUDIENCE_SCOPE` | The scope string the *client* asks for, advertised in `/.well-known/oauth-protected-resource` | `api:///access_as_user` | + +Why they differ: the client requests a scope by its full URI +(*Application ID URI* + `/` + scope name), but Entra does not put that URI in the +token. For **v2.0** access tokens it splits the request into `aud` (the API's +**client-ID GUID**) and `scp` (the **short** scope name, `access_as_user`). +Three distinct strings for what feels like one concept. + +> **Do not trust this document — decode a real token.** Microsoft's own +> [access-tokens](https://learn.microsoft.com/en-us/entra/identity-platform/access-tokens) +> page says web APIs "must only accept tokens containing one of their AppId +> URIs as the `aud` claim", which contradicts the authoritative +> [access token claims reference](https://learn.microsoft.com/en-us/entra/identity-platform/access-token-claims-reference) +> ("In v2.0 tokens, this value is always the client ID of the API"). The +> claims reference is correct for v2.0, but given that Microsoft's docs +> disagree with each other, verify empirically — see §8. + +`OIDC_AUDIENCE_SCOPE` **must** be set explicitly on Entra. Left unset, the code +defaults to `aud-${OIDC_AUDIENCE}`, which is an Authentik naming convention and +means nothing to Entra — the client would request a nonexistent scope and the +authorize request fails outright. + +--- + +## 6. `offline_access` + +Set `OIDC_OFFLINE_ACCESS=true` from the start. Unlike Authentik — where you must +first attach an `offline_access` scope mapping to the provider — Entra treats +`offline_access` as one of its well-defined platform scopes (`openid`, `email`, +`profile`, `offline_access`). Nothing to create, and it is **implicitly +granted**: if any delegated permission is consented, `offline_access` is too. + +Two caveats: + +- It must still be *requested* at runtime, which is exactly what + `OIDC_OFFLINE_ACCESS=true` achieves — the flag adds it to `scopes_supported` + in `/.well-known/oauth-protected-resource`, and MCP clients only request + scopes they see advertised there. +- A refresh token comes back only on authorization-code-style flows. That is + what Claude Code uses, so this is satisfied; implicit flow would not be. + +The `.env` comment on this var warns that advertising a scope the IdP doesn't +offer risks `invalid_scope`. On Entra that risk doesn't apply. + +--- + +## 7. Identity: `sub` splits accounts across app registrations + +**Handled as of migration `0005_user_oid.sql`. Read this anyway — it explains +why `oid` is in your database, and what happens if you deploy the migration +late.** + +The app keys the `users` row on `(oidc_iss, oidc_sub)` — see the upsert in +`apps/web/lib/mcp/context.ts` and the one in `apps/web/auth.ts`. On Authentik +that is safe, because Authentik's `sub` is `user.uid`, a user-level value that +is identical across providers. + +Entra's `sub` is a **pairwise identifier**. Microsoft documents it as *"based on +a combination of the token recipient, tenant, and user"* — so the value is +scoped to the app registration in the `aud` position of that particular token: + +- Web UI sign-in → ID token with `aud` = registration **A** → `sub` = *X* +- MCP access token → `aud` = registration **B** → `sub` = *Y* + +*X ≠ Y*, by design, for privacy. `iss` is identical for both (one tenant, one +issuer), so `(iss, sub)` yields **two different keys for the same human**. Both +code paths *upsert* rather than fail, so nothing looks broken: the person signs +into the Web UI, sees their memories, connects Claude Code, and finds an empty +account. Writes land in the second row. + +There is no configuration fix. `sub` is in Entra's restricted claim set (no +claims-mapping policy can alter it), `subject_types_supported` advertises only +`pairwise`, and Microsoft has stated that `sector_identifier_uri` is not used to +generate it. + +**How it's handled.** Identity is keyed on `oid` — the directory object id, +which Microsoft documents as constant for a user across every application in a +tenant (*"all apps get the same `oid` and `tid` claims for a user acting in a +tenant"*). It is emitted by default in v2.0 ID *and* access tokens as long as +the `profile` scope is requested, which it is. + +`apps/web/lib/auth/identity.ts` holds the single resolver both surfaces call. +Resolution order when `oid` is present: + +1. an existing row keyed on `(oidc_iss, oidc_oid)` — the steady state +2. a pre-migration row matching `(oidc_iss, oidc_sub)` with no `oid` yet, which + gets its `oid` backfilled in place +3. insert + +IdPs that emit no `oid` (Authentik, Keycloak, Okta) skip straight to the +original `(iss, sub)` behaviour, unchanged. + +> **One upgrade-ordering caveat.** Step 2 adopts a legacy row by matching +> `sub`, and the only `sub` that can match is the one that created it — the +> **Web UI** one, since MCP auth against Entra was impossible before the JWKS +> fix in §0. So if you already had Entra users signing into the Web UI, have +> them **sign into the Web UI once** after deploying this migration, before +> connecting an MCP client. Connecting MCP first creates a fresh row keyed on +> `oid` and leaves the original stranded, with the memories in it invisible. +> Deployments that have never run Entra are unaffected. + +The single-registration layout (making registration A the resource server too) +also works and needs no migration, but you lose audience separation between the +Web UI and MCP. + +--- + +## 8. Verification + +Run these in order; each one isolates a different failure. + +**1. The issuer is concrete, not templated.** + +```bash +curl -s "https://login.microsoftonline.com//v2.0/.well-known/openid-configuration" \ + | jq '{issuer, jwks_uri}' +``` +`issuer` must be a GUID URL, not `{tenantid}`. Copy it verbatim into +`OIDC_ISSUER`. + +**2. Our metadata advertises the right scopes.** + +```bash +curl -s https://memory.example.com/.well-known/oauth-protected-resource | jq +``` +`scopes_supported` must contain `api:///access_as_user` (not +`aud-…`), plus `offline_access` if you enabled it. `authorization_servers[0]` +must be the tenant-specific v2.0 issuer. + +**3. Decode a real access token.** This is the only step that proves the +`aud`/`iss`/version questions. Get a token (from Claude Code's stored +credentials, or by running the flow manually) and inspect the payload: + +```bash +TOKEN='eyJ...' +echo "$TOKEN" | cut -d. -f2 | tr '_-' '/+' | base64 -d 2>/dev/null | jq '{ver, iss, aud, sub, oid, tid, scp, groups}' +``` + +Expected: + +| Field | Expected value | If wrong | +|---|---|---| +| `ver` | `"2.0"` | `api.requestedAccessTokenVersion` is not `2` (§4a) | +| `iss` | `https://login.microsoftonline.com//v2.0` | v1 token, or `common` authority (§2, §4a) | +| `aud` | ``, a bare GUID | set `OIDC_AUDIENCE` to whatever is actually here (§5) | +| `scp` | `access_as_user` | the scope wasn't requested or consented (§9) | +| `groups` | array of GUIDs, or absent | see §10 | + +**4. Confirm the 401 reason** when something is still wrong — the MCP endpoint +names the failing claim: + +```bash +curl -s -i -H "Authorization: Bearer $TOKEN" https://memory.example.com/api/mcp | head -20 +``` +Look at `WWW-Authenticate`: `error_description="claim invalid: iss"` → +§2/§4a. `"claim invalid: aud"` → §5. `"verification failed"` → JWKS could not +be fetched, §0. + +--- + +## 9. Consent for the API scope + +A custom scope is not inherently admin-only. Two levers decide it: + +- **The scope's own setting.** "Who can consent?" on the scope — **Admins and + users** lets users self-consent; **Admins only** always requires an admin. + Select "Admins and users" (§4b). Microsoft's docs don't state which radio the + portal preselects, so set it deliberately rather than assuming. +- **The tenant's user-consent policy.** The default is *"users are allowed to + consent to applications for permissions that don't require administrator + consent"*, but many tenants tighten this to "verified publishers only" or + disable user consent entirely, in which case an admin must consent regardless + of the scope setting. + +Admin consent becomes **mandatory** if: the scope is "Admins only"; the tenant +policy restricts user consent; or — the one that catches people — the enterprise +application is set to **require user assignment**, which forces admin consent +even when tenant policy would otherwise permit self-consent. + +**To grant it:** App registrations → *the client app* (the one Claude Code uses, +not the API) → **API permissions** → **Grant admin consent for \**. The +button is disabled if you aren't an admin or no permissions are configured. + +Alternatively, suppress the prompt entirely with **pre-authorization**: on +registration B, **Expose an API → Authorized client applications → Add a client +application**, select the MCP client ID and tick `access_as_user`. Consent is +then implicit. Reasonable here, since you control both registrations. + +If you prefer the URL form of admin consent, note it needs the `/v2.0/` segment +and must not use `common`: + +``` +https://login.microsoftonline.com//v2.0/adminconsent + ?client_id= + &scope=api:///access_as_user + &redirect_uri=https://memory.example.com/auth/cli-callback + &state=12345 +``` + +--- + +## 10. Groups + +Group memberships gate access to shared projects (`readableProjectIds` / +`canWriteProject` in `apps/web/lib/mcp/tools.ts` and +`apps/web/lib/memory-mutations.ts`). Entra's groups claim needs care on two +independent axes: **what the values look like**, and **what happens when the +claim goes missing**. + +### 10a. By default you get GUIDs, not names + +Entra emits `groups` as a **JSON array of group object-ID GUIDs**. Not display +names. `apps/web/lib/auth/sync-groups.ts` stores whatever strings arrive +verbatim and makes no attempt to resolve them, so the Web UI will list +memberships like `8f4c…-b21a` and your project ACLs must be written against +those GUIDs. + +There *is* a supported way to get display names for cloud-only groups — +contrary to the older note in `sync-groups.ts`, which says names are available +only for AD-synced groups. That was true of the `sam_account_name` family +(those attributes genuinely exist only on groups synced from on-premises AD via +Entra Connect 1.2.70+), but Entra also has `cloud_displayname`: + +**App registrations → \ → Token configuration → Add groups claim**, select +**Groups assigned to the application**, then tick the cloud-only display name +option. In the manifest: + +```json +"groupMembershipClaims": "ApplicationGroup", +"optionalClaims": { + "accessToken": [ + { "name": "groups", + "additionalProperties": ["cloud_displayname"] } + ], + "idToken": [ + { "name": "groups", + "additionalProperties": ["cloud_displayname"] } + ] +} +``` + +Both collections matter: `idToken` feeds the Web UI sign-in path (`auth.ts`), +`accessToken` feeds the MCP path (`jwt.ts`). Configure only one and the two +surfaces disagree about your group names. + +Constraints, all of them load-bearing: + +- `cloud_displayname` **only works with `groupMembershipClaims: + "ApplicationGroup"`**. Microsoft's stated reason is that group display names + aren't unique, so they only emit them for groups explicitly assigned to the + application. +- Only **directly assigned** groups appear. **Nested groups are excluded.** +- Assign the groups under **Enterprise applications → \ → Users and + groups**, or they simply won't be emitted. +- Microsoft's published `cloud_displayname` examples cover `idToken` and + `saml2Token`; we found no official example pairing it with `accessToken`. + It is a documented-valid collection, but **decode a real access token (§8) + and confirm `groups` contains names before relying on it** rather than + assuming symmetry. + +A claims-mapping policy cannot fix this instead: `groups` is a restricted +claim, so its data source can't be changed and no transformation applies. + +If none of this appeals, Microsoft's own recommendation is to use **app roles** +rather than groups for authorization — but this app reads `groups`, so that +would need a code change. + +### 10b. Groups overage — now refused rather than obeyed + +**This was the sharpest edge in this document. It is now a hard failure with a +readable message, which is a much better outcome than what it used to do.** + +Past a limit, Entra stops emitting `groups` altogether and substitutes an +overage indicator: + +| Token | Limit | What you get past it | +|---|---|---| +| JWT (access + ID) | **200** groups | `groups` absent; `_claim_names` / `_claim_sources` present | +| SAML | 150 groups | same | +| Implicit flow | **5** groups | `"hasgroups": true` | + +The indicator looks like this — note it is *not* a truncated list, it is no +list at all: + +```json +{ + "_claim_names": { "groups": "src1" }, + "_claim_sources": { "src1": { "endpoint": "https://graph.windows.net/…" } } +} +``` + +(That endpoint is an **Azure AD Graph** URL, not Microsoft Graph. Don't follow +it; Microsoft says to construct +`https://graph.microsoft.com/v1.0/users/{id}/getMemberObjects` yourself. +Limits are inclusive of nested groups.) + +**What this used to do.** An absent `groups` claim and a claim saying "zero +groups" were indistinguishable to `normalizeGroupsClaim`, which returned `[]` +for both — and the `names.length === 0` branch **deletes every one of that +user's `user_groups` rows**. So a user crossing 200 groups signed into the Web +UI once and silently lost access to every shared project, on both surfaces, +with no error anywhere. (The MCP path never deleted anything, but it then read +the snapshot the Web sign-in had just emptied.) + +**What happens now.** `detectGroupsOverage` looks for `_claim_names.groups` and +`hasgroups`, and both surfaces refuse the token rather than acting on group +state they know they don't have: + +- **Web sign-in** throws `GroupsOverageError`, which fails the sign-in. Existing + memberships are left completely untouched. +- **MCP** returns 401 with + `error_description="groups overage: IdP did not enumerate group membership …"`. + +The user is blocked until an admin fixes the claim configuration — and then +signs in and finds their access exactly as it was. Nothing to restore, because +nothing was destroyed. Granting access from a stale snapshot, or revoking it on +a claim the IdP never made, are both guesses; refusing is the only honest +answer available. + +An absent `groups` claim with **no** overage marker still clears memberships. +That is unchanged and deliberate: the IdP has genuinely stopped asserting the +groups, so we stop honouring them. + +#### Getting a blocked user back in + +1. App registration → **Token configuration** (or the manifest) → set + `groupMembershipClaims` to **`ApplicationGroup`** — the portal labels this + **"Groups assigned to the application"**. It emits only the groups + explicitly assigned to *this* application, which for a memory server is a + handful, so the 200-group ceiling stops being reachable. Microsoft + recommends it for exactly this reason, and it is the same setting + `cloud_displayname` requires — §10a and §10b have one shared fix. +2. Enterprise applications → your app → **Users and groups** → assign the + groups you actually share projects with. `ApplicationGroup` emits **directly + assigned groups only**; nested and transitive membership is excluded, so + assign the real groups rather than a parent. +3. Confirm the `groups` optional claim is configured for the **access token**, + not only the ID token — the MCP path reads the access token. +4. The user signs in again. Their memberships were never deleted, so their + access returns as it was. + +Leaving `groupMembershipClaims` at `All` or `SecurityGroup` in a large tenant is +what makes this bite in the first place. + +If a group genuinely must exceed the limit, the other way out is **app roles**, +which are app-scoped and never overage — but they arrive in a `roles` claim and +this codebase reads `groups`, so that is a code change, not a config change. + +--- + +## 11. Connecting Claude Code + +Everything in README → **Connecting Claude Code** applies unchanged, with one +Entra-specific confirmation: **the pre-registered client-id path is +mandatory.** + +Entra does not implement RFC 7591 Dynamic Client Registration. Its discovery +document publishes no `registration_endpoint`, it serves no RFC 8414 +authorization-server metadata at all (only OIDC discovery), and it does not +advertise `client_id_metadata_document_supported` — so neither DCR nor the CIMD +mechanism that superseded it in the MCP spec is available. Microsoft states this +plainly in its own MCP guidance ("Microsoft Entra ID doesn't currently support +client registration") and has said it is not on the near-term roadmap. + +Practically, this means: + +- Use the plugin (`plugin/.mcp.json` ships a pre-registered `clientId`), or +- Pass `--client-id ` explicitly on `claude mcp add`. + +A client that expects to self-register will fail. This is the same situation as +Authentik, so the README's guidance needs no adjustment. + +--- + +## 12. Env var reference card + +Straight from Entra's UI labels to `.env` keys. `` is app registration A +(Web UI, §3); `` is app registration B (MCP resource server, §4). + +| `.env` key | Where it comes from in Entra | Example | +|---|---|---| +| `OIDC_ISSUER` | `issuer` from the **tenant-specific** discovery document (§2). Not the authority you typed — the value the endpoint returns. | `https://login.microsoftonline.com//v2.0` | +| `OIDC_ISSUER_MCP` | **Leave unset.** Entra has one issuer per tenant; there is no per-application issuer to point at. `mcpIssuer()` falls back to `OIDC_ISSUER`. | *(unset)* | +| `OIDC_CLIENT_ID_WEB` | `` → **Overview → Application (client) ID** | `1111…-aaaa` | +| `OIDC_CLIENT_SECRET_WEB` | `` → **Certificates & secrets → Client secrets → Value** (not Secret ID; shown once) | `abc8Q~…` | +| `OIDC_CLIENT_ID_MCP` | Client ID of the **public PKCE** registration Claude Code authenticates as (§4c) | `3333…-cccc` | +| `OIDC_AUDIENCE` | `` → **Overview → Application (client) ID**. The bare GUID, *not* the `api://` URI. Confirm by decoding a token (§8). | `2222…-bbbb` | +| `OIDC_AUDIENCE_SCOPE` | `` → **Expose an API** → the scope's full string: Application ID URI + `/` + scope name. Must be set explicitly; the `aud-…` default is Authentik-only. | `api://2222…-bbbb/access_as_user` | +| `OIDC_OFFLINE_ACCESS` | Nothing to configure in Entra — set it to `true` (§6). | `true` | + +`PUBLIC_URL` and the non-OIDC vars are unchanged from the README. + +--- + +## 13. Multitenant is not supported + +`acceptedIssuers()` in `apps/web/lib/auth/jwt.ts` compares `iss` against a +fixed pair of strings (with and without a trailing slash). Multitenant Entra +apps require substituting each token's `tid` claim into the `{tenantid}` +placeholder before comparing, and separately validating the signing key's own +issuer. Neither is implemented. + +Beyond `iss`, multitenant would also need the identity keying in §7 resolved, +since Microsoft is explicit that `oid` and `sub` differ per tenant by design and +that a guest user authenticating in another tenant *"should be treated as if +they're a brand new user to the service."* + +Single-tenant is the supported configuration.