From 1e71f50e833e79e2b0314d08955fc5bfcaa9fb6a Mon Sep 17 00:00:00 2001 From: Eugene Samotija Date: Fri, 14 Aug 2026 17:34:11 -0400 Subject: [PATCH] =?UTF-8?q?auth:=20additive=20roles=20=E2=80=94=20a=20prin?= =?UTF-8?q?cipal=20keeps=20every=20mapped=20group's=20access?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #28. resolveOidcRole returned ONE role: an explicit users.role_id, else the highest group mapping. Harmless while roles nested by tier (viewer subset of editor subset of admin) because the winner contained the loser. It stops being harmless the moment roles are incomparable, which is exactly what named tool sets (#27) introduce: someone in Techs and Billing would get the higher-tier role and silently lose the other's surface. A Principal now carries every role it holds and the envelope is their UNION: PolicyService.allowsAny evaluates each role on its own grant, default tier and overrides, so being added to a group can only widen access. A per-role deny closes that role's path, not the others' — subtracting for everyone stays the global kill switch's job, which the tests pin. Splitting "the admin chose this" from "this came from a group" needed schema v5: users.role_source marks an explicit override (which still REPLACES groups, that being its purpose) and user_login_roles remembers what the last login's group claims mapped to, since the cookie and gateway-JWT paths see no group claims and previously leaned on the login path writing role_id. Existing non-null role_id rows migrate to role_source='admin' deliberately: on a live deployment they were either set from the Users tab or written back unchanged by the login path, so nobody's access moves on upgrade — db/migrate-v5.test.ts builds a v4 database by hand and asserts exactly that, plus idempotency. Also in this batch, all from today's diagnosis session: - GET /api/me/sessions — my connected clients and whether ANY holds the notification stream, rendered next to Apply now on /me. Without a stream the gateway cannot push list_changed at all, which is precisely when the button is the only cure; better shown than explained. - The /me hint spells out the whole model: a switch bites immediately, the list is fetched at connect time, change first then Apply, and if a client still disagrees it caches above the protocol — new chat or reconnect the connector. - [mcp] logging of tools/list size and tools/call decisions (names and verdicts only, never arguments). "Did the request even reach the gateway?" cost an afternoon today; now it is one grep. - gw_status reported the MODE in its `version` field, so it claimed to be running version "integrated". Both fields now, with a test. - /api/users reports loginRoles and effectiveRoles; /me shows every role. Co-Authored-By: Claude Opus 5 --- CLAUDE.md | 4 +- packages/gateway/public/me.html | 38 +++++- packages/gateway/src/auth/login.test.ts | 7 +- packages/gateway/src/auth/principal.ts | 37 +++++- packages/gateway/src/db/index.ts | 36 +++++- packages/gateway/src/db/migrate-v5.test.ts | 99 +++++++++++++++ packages/gateway/src/db/repo.test.ts | 28 +++- packages/gateway/src/db/repo.ts | 93 +++++++++++--- .../gateway/src/domain/additive-roles.test.ts | 120 ++++++++++++++++++ .../src/domain/policy-narrowing.test.ts | 47 +++---- packages/gateway/src/domain/policy.ts | 20 ++- packages/gateway/src/http/admin-api.ts | 15 ++- packages/gateway/src/http/app.test.ts | 15 +++ packages/gateway/src/http/app.ts | 114 ++++++++--------- packages/gateway/src/http/me-api.ts | 32 ++++- packages/gateway/src/mcp/gateway-server.ts | 17 ++- packages/gateway/src/mcp/self-tools.test.ts | 4 + packages/gateway/src/mcp/self-tools.ts | 6 +- 18 files changed, 599 insertions(+), 133 deletions(-) create mode 100644 packages/gateway/src/db/migrate-v5.test.ts create mode 100644 packages/gateway/src/domain/additive-roles.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 401358f..cb90d14 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,11 +14,11 @@ Self-hosted MCP manager/gateway: one streamable-HTTP `/mcp` endpoint federating - `config.ts` — flags/env + `mspstack.config.json` (`ConfigError`); parses `MCP_TOKENS_` lists (duplicate labels are a `ConfigError` — labels are /me identities), OIDC (`OIDC_ISSUER`/`ENTRA_TENANT_ID` + required `OIDC_AUDIENCE`), `BAO_*` - `db/backup.ts` — online snapshots (`VACUUM INTO`, never a file copy of a live db), retention, optional Azure Blob shipping (lazy SDK, DefaultAzureCredential); scheduler unref'd, off for `:memory:`; `GET/POST /api/backups`; restore is manual (docs/backups.md) - `mcp/self-tools.ts` — admin-only `gw_*` toolset (issue #2): status, list servers/tools (values REDACTED), bulk enable by tier/group, tier overrides, grants by role name, preset install, refresh, backup-now. Hidden from non-admins AND re-checked per call with the unknown-tool wording; `gw` namespace reserved in config.ts; `GATEWAY_SELF_TOOLS=off` disables. Target resolution shared with both HTTP APIs via `domain/tool-targets.ts` -- `db/` — `node:sqlite` schema (roles/upstreams/grants/tool_overrides/tool_settings/users/group_mappings, seeded viewer/editor/admin) + typed `Repo` +- `db/` — `node:sqlite` schema (roles/upstreams/grants/tool_overrides/tool_settings/users/group_mappings/user_login_roles, seeded viewer/editor/admin) + typed `Repo`; migrations are `PRAGMA user_version` blocks and `migrate()` is exported so a hand-built legacy DB can be tested (`db/migrate-v5.test.ts`) - `domain/catalog.ts` — namespacing (`${namespace}_${tool}`, no double-prefix), routing map (no string-splitting), annotation-derived tiers (port of mcp-itglue `tierOf`), `derivedGroupOf(tool)` — the category for the group switches: `_meta.group`/`_meta.toolset` first (how the family servers tag their toolsets), else a bracketed description prefix (`[Identity > …]`, CIPP); an explicit `group_label` always wins - `domain/presets.ts` — one-click upstream presets: builtin family configs (itglue/cwpsa/planner/cipp — full specs incl. BYOK headers, per-user mode, userConnect, personalCredentials, the `auth` mint block) + optional `mspstack.presets.json` (file overrides builtin ids); `{{param}}` templating rendered server-side and validated via `parseUpstreamSpec`; recommended grants by role NAME resolved at install (`GET /api/presets`, `POST /api/presets/:id/install` with `dryRun`). Spec's `personalCredentials` metadata drives the /me guided credential forms (`credentialFields` in `/api/me/access`) - `domain/policy.ts` — `PolicyService`: toolEnabled ∧ (override(allow) ∨ (tier ≤ maxTier ∧ ¬deny)); maxTier = per-upstream grant ?? role default. Same function gates tools/list AND tools/call. `allowsFor(principal, entry)` = envelope ∧ personal prefs (deny rows in `user_prefs`; "enable" deletes the row — narrowing can never widen). Spec `userDefault: "off"` inverts the personal layer for that upstream: nothing is live until an explicit opt-in row exists (per tool or server-wide `''`), still capped by the envelope — for servers with hundreds of tools. `/api/me/access` derives its `enabled` flags from `allowsFor` so the page can't disagree with the boundary -- `auth/` — `static-tokens.ts` (timing-safe bearer match), `oidc.ts` (jose JWKS resource-server verifier for inbound *access* tokens), `login.ts` (interactive login: openid-client cookie+PKCE confidential-client flow consuming an *id-token*; signed identity-only session cookie, HMAC + freshness; `safeReturnTo`), `authz-server.ts` (OAuth AS facade: RFC 8414 metadata, RFC 7591 DCR for public clients, single-use hashed 60s codes + PKCE S256, HS256 gateway JWTs keyed by `GATEWAY_JWT_SECRET` (default derived from `SESSION_SECRET`), rotating refresh tokens — 30d sliding, family-revoked on replay, client-bound consume that can't burn a live token — register rate limit; clients managed via `/api/oauth-clients` + Users tab), `prm.ts` (RFC 9728 doc + WWW-Authenticate; lists the gateway itself as AS when login is configured, else the raw IdP), `directory.ts` (app-only Graph search of Entra users/groups via the login app's own creds — powers the admin UI group-mapping typeahead at `/api/directory/search`; null for non-Entra issuers → UI degrades to paste-an-id), `principal.ts` (session binding key). Four inbound auth paths in `createAuthResolver`: static token, gateway-issued JWT (routed by unverified `iss == PUBLIC_URL`, then fully verified), OIDC bearer, and the cookie session — the cookie/JWT carry only identity and the role is re-resolved every request (persisted at callback via `setUserRole`), so a session id never carries privilege. `loginUpsert()` is shared by the bearer + callback paths so they can't drift. `/oauth/authorize` brokers user auth to Entra by piggybacking the interactive login: the pending request rides in the signed transient cookie and `/auth/callback` mints the code. +- `auth/` — `static-tokens.ts` (timing-safe bearer match), `oidc.ts` (jose JWKS resource-server verifier for inbound *access* tokens), `login.ts` (interactive login: openid-client cookie+PKCE confidential-client flow consuming an *id-token*; signed identity-only session cookie, HMAC + freshness; `safeReturnTo`), `authz-server.ts` (OAuth AS facade: RFC 8414 metadata, RFC 7591 DCR for public clients, single-use hashed 60s codes + PKCE S256, HS256 gateway JWTs keyed by `GATEWAY_JWT_SECRET` (default derived from `SESSION_SECRET`), rotating refresh tokens — 30d sliding, family-revoked on replay, client-bound consume that can't burn a live token — register rate limit; clients managed via `/api/oauth-clients` + Users tab), `prm.ts` (RFC 9728 doc + WWW-Authenticate; lists the gateway itself as AS when login is configured, else the raw IdP), `directory.ts` (app-only Graph search of Entra users/groups via the login app's own creds — powers the admin UI group-mapping typeahead at `/api/directory/search`; null for non-Entra issuers → UI degrades to paste-an-id), `principal.ts` (session binding key; a Principal carries EVERY role it holds and the envelope is their union — `resolveOidcRoles` returns all mapped groups' roles, an explicit `users.role_id` override replaces them, and `user_login_roles` remembers the group-derived set for the cookie/JWT paths that see no group claims). Four inbound auth paths in `createAuthResolver`: static token, gateway-issued JWT (routed by unverified `iss == PUBLIC_URL`, then fully verified), OIDC bearer, and the cookie session — the cookie/JWT carry only identity and the roles are re-resolved every request (group-derived set persisted at callback via `setLoginRoles`), so a session id never carries privilege. `loginUpsert()` is shared by the bearer + callback paths so they can't drift. `/oauth/authorize` brokers user auth to Entra by piggybacking the interactive login: the pending request rides in the signed transient cookie and `/auth/callback` mints the code. - `secrets/` — `SecretStore` interface (scheme-tagged: `bao` | `kv`), `openbao.ts` (KV v2, AppRole or token, 5-min cache), `keyvault.ts` (Azure Key Vault, `DefaultAzureCredential`, lazy SDK import, same 5-min cache; `put(path, field)` writes `path-field`), `memory.ts` (tests). Refs: `bao:path#field` / `kv:secret-name`; env refs: `${VAR}` — all resolved only at upstream connect time. One store at a time (`BAO_ADDR` xor `KEY_VAULT_URI`) - `upstream/connection.ts` — one pooled SDK `Client` per upstream; header/env injection; backoff reconnect (1s→60s) + `onRecovered`; retry-once on dropped transport AND on server-side session expiry (upstream 404 "unknown session" → transparent re-initialize + retry, per MCP spec). Optional spec `auth` block (`oauth2-client-credentials`): the gateway mints the upstream's bearer itself (secret via `${VAR}`/`bao:`/`kv:` ref), caches it, and rebuilds the connection when it nears expiry (60s skew) — for third-party servers that want a finished token, e.g. CIPP behind Easy Auth. Neither secret nor token is ever logged - `upstream/manager.ts` also pools **per-principal links** for `sessionMode:"per-user"` upstreams: spec clone with the caller's credential REFS layered over headers/env (still resolved via the secret store at connect — anti-passthrough intact); catalog discovery stays on the shared link; personal pool flushed on upstream upsert/remove. `requirePersonalCredentials` refuses the shared fallback diff --git a/packages/gateway/public/me.html b/packages/gateway/public/me.html index aff3b12..5dc374f 100644 --- a/packages/gateway/public/me.html +++ b/packages/gateway/public/me.html @@ -164,7 +164,9 @@

Sign in

try { data = await api("/access"); } catch (e) { el.innerHTML = `

${esc(e.message)}

`; return; } - $("#whoami").textContent = data.principal ? `${data.principal.label} · ${data.principal.role}` : ""; + $("#whoami").textContent = data.principal + ? `${data.principal.label} · ${(data.principal.roles ?? [data.principal.role]).join(" + ")}` + : ""; if (!data.servers.length) { el.innerHTML = `

My access

@@ -176,8 +178,15 @@

Sign in

el.innerHTML = `

My access

Turn servers, whole tiers, or individual tools off for your own sessions. You can only narrow what your role already grants — turning something on just removes your personal off-switch.

-

Changes take effect immediately: a tool you switched off is refused even if your client still lists it. The list your client shows is fetched when it connects — if a change doesn't appear there, use Apply now and your clients will reconnect and pick up the current set. -

+
+

A switch here bites immediately. A tool you turned off is refused on the next call, even while your client still shows it in its list — enforcement and display are two different things.

+

The list is fetched when a client connects. Change something, then press Apply now: your live sessions are dropped, your clients reconnect and read the current set. Order matters — change first, apply second.

+

If a tool still doesn't appear, that client is caching the list above the protocol. Then: start a new chat, or turn the connector off and on in your client's settings. Nothing is wrong with your access — check this page, it is the source of truth.

+
+ + +
+
${data.servers.map(s => { const counts = tierCounts(s.tools); const optIn = s.userDefault === "off"; @@ -285,8 +294,9 @@

? `Reconnecting ${r.closed} session${r.closed === 1 ? "" : "s"} — your client will re-read its tools` : "No live sessions to reconnect — your client will read the current set when it connects"); } catch (e) { toast(e.message, true); } - finally { b.disabled = false; } + finally { b.disabled = false; renderSessionState(); } }; + renderSessionState(); el.onchange = async (ev) => { const i = ev.target; @@ -300,6 +310,26 @@

}; } +/** + * Connected clients, and whether any of them holds the notification stream. + * Without a stream the gateway cannot push tools/list_changed at all, which is + * precisely when Apply now is not optional — so say it here instead of leaving + * people to guess why a change didn't show up. + */ +async function renderSessionState() { + const el = $("#session-state"); + if (!el) return; + try { + const r = await api("/sessions"); + const n = r.sessions.length; + el.textContent = n === 0 + ? "no clients connected right now — the next one to connect reads the current set" + : `${n} client${n === 1 ? "" : "s"} connected · ` + (r.notificationStream + ? "live notifications available" + : "no notification channel — press Apply now after a change"); + } catch { el.textContent = ""; } +} + // ── My credentials ── async function renderCredentials() { const el = $("#credentials"); diff --git a/packages/gateway/src/auth/login.test.ts b/packages/gateway/src/auth/login.test.ts index e55079f..38539a0 100644 --- a/packages/gateway/src/auth/login.test.ts +++ b/packages/gateway/src/auth/login.test.ts @@ -202,8 +202,11 @@ describe("interactive login callback → cookie session", () => { const session = cookieFrom(cb, "mspstack_session"); expect(session).toBeTruthy(); - // The role was persisted at callback time. - expect(repo.userBySubject(ISS, "oid-editor")?.roleId).toBe(editorRoleId); + // The group-derived roles were persisted at callback time — as login roles, + // NOT as an explicit override, so an admin's own choice stays theirs (#28). + const user = repo.userBySubject(ISS, "oid-editor")!; + expect(user.roleId).toBeNull(); + expect(repo.loginRoles(user.id).map((r) => r.id)).toEqual([editorRoleId]); // The session cookie now authenticates /api/me (no bearer token at all). const access = await fetch(`${base}/api/me/access`, { headers: { Cookie: session! } }); diff --git a/packages/gateway/src/auth/principal.ts b/packages/gateway/src/auth/principal.ts index 531b2ad..d3273a7 100644 --- a/packages/gateway/src/auth/principal.ts +++ b/packages/gateway/src/auth/principal.ts @@ -6,18 +6,53 @@ import { createHash } from "node:crypto"; +export interface PrincipalRole { + id: number; + name: string; + isAdmin: boolean; +} + export interface Principal { kind: "static" | "oidc" | "dev"; /** Stable identity: static → token label; oidc → `${iss}|${sub}`; dev → "dev". */ subject: string; /** Human-readable, for logs (never a secret). */ label: string; + /** + * Every role held. The envelope is their UNION (issue #28): a user in two + * mapped groups keeps both surfaces, because "we gave you billing access" + * must never take ticket access away. Subtraction lives outside roles — the + * global kill switch and the user's own /me prefs. + */ + roles: PrincipalRole[]; + /** Primary role — most privileged, first in `roles` — for display and messages. */ roleId: number; roleName: string; isAdmin: boolean; } -export const principalKey = (p: Principal): string => `${p.kind}:${p.subject}:${p.roleId}`; +/** Build a Principal's role fields from a "most privileged first" role list. */ +export function withRoles( + base: T, + roles: PrincipalRole[] +): T & Pick { + const primary = roles[0]!; + return { + ...base, + roles, + roleId: primary.id, + roleName: roles.length > 1 ? roles.map((r) => r.name).join("+") : primary.name, + isAdmin: roles.some((r) => r.isAdmin), + }; +} + +/** Role ids, sorted — the identity half of a session binding key. */ +export const roleIdsOf = (p: Principal): number[] => p.roles.map((r) => r.id).sort((a, b) => a - b); + +// A role change mid-session yields a different key, so the existing +// principal-mismatch 403 makes the client reconnect — unchanged behaviour. +export const principalKey = (p: Principal): string => + `${p.kind}:${p.subject}:${roleIdsOf(p).join(",")}`; /** * Identity key for personal state (prefs, registered credentials) — diff --git a/packages/gateway/src/db/index.ts b/packages/gateway/src/db/index.ts index 8d8e327..7de51de 100644 --- a/packages/gateway/src/db/index.ts +++ b/packages/gateway/src/db/index.ts @@ -16,7 +16,8 @@ export function openDatabase(path: string): DatabaseSync { return db; } -function migrate(db: DatabaseSync): void { +/** Exported so migrations can be tested against a hand-built legacy database. */ +export function migrate(db: DatabaseSync): void { const version = (db.prepare("PRAGMA user_version").get() as { user_version: number }) .user_version; @@ -173,4 +174,37 @@ function migrate(db: DatabaseSync): void { db.exec("PRAGMA user_version = 4"); } + + if (version < 5) { + // Additive roles (issue #28). Until now the login path wrote the + // group-derived role into users.role_id, which made "the admin chose this + // role" and "this came from a group" the same row — and forced a single + // winner. Split them: + // + // users.role_id + role_source='admin' → an explicit override, REPLACES groups + // user_login_roles → every role the last login's groups mapped to + // + // Existing non-null role_id rows are marked 'admin' deliberately: on a live + // deployment they were either set from the Users tab or written back + // unchanged by the login path, so treating them as explicit overrides keeps + // everyone's access byte-for-byte. The union only starts to matter for users + // with no override, which is the state a fresh group mapping produces. + db.exec(` + CREATE TABLE IF NOT EXISTS user_login_roles ( + user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, + role_id INTEGER NOT NULL REFERENCES roles(id) ON DELETE CASCADE, + PRIMARY KEY (user_id, role_id) + ); + `); + const columns = db.prepare("PRAGMA table_info(users)").all() as Array<{ name: string }>; + if (!columns.some((c) => c.name === "role_source")) { + db.exec(` + ALTER TABLE users ADD COLUMN role_source TEXT + CHECK (role_source IS NULL OR role_source IN ('admin','login')); + UPDATE users SET role_source = 'admin' WHERE role_id IS NOT NULL; + `); + } + + db.exec("PRAGMA user_version = 5"); + } } diff --git a/packages/gateway/src/db/migrate-v5.test.ts b/packages/gateway/src/db/migrate-v5.test.ts new file mode 100644 index 0000000..a7b49fd --- /dev/null +++ b/packages/gateway/src/db/migrate-v5.test.ts @@ -0,0 +1,99 @@ +/** + * Migration v4 → v5 (additive roles). The interesting case is an EXISTING + * deployment: every user's role_id was written by the login path, and the new + * code reads role_id as "an admin chose this — ignore groups". Get that wrong + * and a live gateway either widens access silently or drops people's roles, so + * build a v4-shaped database by hand and migrate it. + */ + +import { describe, expect, it } from "vitest"; +import { DatabaseSync } from "node:sqlite"; +import { migrate } from "./index.js"; +import { Repo } from "./repo.js"; + +/** The v1 shape of the tables this migration touches, stamped as v4. */ +function legacyDatabase(): DatabaseSync { + const db = new DatabaseSync(":memory:"); + db.exec(` + CREATE TABLE roles ( + id INTEGER PRIMARY KEY, + name TEXT NOT NULL UNIQUE, + default_max_tier TEXT NOT NULL DEFAULT 'none', + is_admin INTEGER NOT NULL DEFAULT 0, + protected INTEGER NOT NULL DEFAULT 0 + ); + CREATE TABLE users ( + id INTEGER PRIMARY KEY, + iss TEXT NOT NULL, + sub TEXT NOT NULL, + email TEXT, + display_name TEXT, + role_id INTEGER REFERENCES roles(id) ON DELETE SET NULL, + last_login_at TEXT, + UNIQUE (iss, sub) + ); + CREATE TABLE group_mappings ( + id INTEGER PRIMARY KEY, + iss TEXT NOT NULL, + claim_value TEXT NOT NULL, + role_id INTEGER NOT NULL REFERENCES roles(id) ON DELETE CASCADE, + UNIQUE (iss, claim_value) + ); + INSERT INTO roles (id, name, default_max_tier, is_admin, protected) VALUES + (1, 'viewer', 'read', 0, 1), (2, 'editor', 'write', 0, 1), + (3, 'admin', 'destructive', 1, 1), (4, 'managers-ro', 'read', 0, 0); + -- one user with a role (the state prod is in) and one without + INSERT INTO users (id, iss, sub, email, role_id) VALUES + (1, 'https://idp', 'has-role', 'daniel@test', 4), + (2, 'https://idp', 'no-role', 'newcomer@test', NULL); + INSERT INTO group_mappings (iss, claim_value, role_id) VALUES ('https://idp', 'g-core', 2); + PRAGMA user_version = 4; + `); + return db; +} + +const roleSource = (db: DatabaseSync, sub: string): string | null => + (db.prepare("SELECT role_source FROM users WHERE sub = ?").get(sub) as { role_source: string | null }) + .role_source; + +describe("migration v4 → v5", () => { + it("keeps an existing role as an explicit override, so nobody's access moves", () => { + const db = legacyDatabase(); + migrate(db); + const repo = new Repo(db); + + // The pre-existing role still resolves and now REPLACES group mappings — + // today's behaviour exactly, even though this user is in a mapped group. + expect(repo.resolveOidcRoles("https://idp", "has-role", ["g-core"]).map((r) => r.name)).toEqual([ + "managers-ro", + ]); + expect(roleSource(db, "has-role")).toBe("admin"); + + // A user without one now picks up every mapped group (the new behaviour). + expect(repo.resolveOidcRoles("https://idp", "no-role", ["g-core"]).map((r) => r.name)).toEqual([ + "editor", + ]); + expect(roleSource(db, "no-role")).toBeNull(); + }); + + it("creates the login-roles table, and re-running changes nothing", () => { + const db = legacyDatabase(); + migrate(db); + const repo = new Repo(db); + repo.setLoginRoles(2, [1, 2]); + expect(repo.loginRoles(2).map((r) => r.name)).toEqual(["editor", "viewer"]); + + // Re-entering the same migration must not throw (duplicate column/table) + // nor disturb stored rows — the ADD COLUMN is guarded by a column check. + db.exec("PRAGMA user_version = 4"); + expect(() => migrate(db)).not.toThrow(); + expect(repo.loginRoles(2)).toHaveLength(2); + expect(roleSource(db, "no-role")).toBeNull(); + }); + + it("reaches v5 on a fresh database too", () => { + const db = new DatabaseSync(":memory:"); + migrate(db); + expect((db.prepare("PRAGMA user_version").get() as { user_version: number }).user_version).toBe(5); + }); +}); diff --git a/packages/gateway/src/db/repo.test.ts b/packages/gateway/src/db/repo.test.ts index 4f88f1d..b29fda4 100644 --- a/packages/gateway/src/db/repo.test.ts +++ b/packages/gateway/src/db/repo.test.ts @@ -99,21 +99,39 @@ describe("Repo", () => { expect(again.email).toBe("a@b.c"); // COALESCE keeps the earlier email }); - it("resolveOidcRole: explicit user role beats group mappings; highest tier mapping wins", () => { + it("resolveOidcRoles: every mapped group counts; an explicit override replaces them", () => { const repo = fresh(); const viewer = repo.roleByName("viewer")!; const editor = repo.roleByName("editor")!; const admin = repo.roleByName("admin")!; const user = repo.upsertUserOnLogin({ iss: "https://idp", sub: "u1" }); + const names = (groups: string[]) => repo.resolveOidcRoles("https://idp", "u1", groups).map((r) => r.name); - expect(repo.resolveOidcRole("https://idp", "u1", ["g1"])).toBeNull(); + expect(names(["g1"])).toEqual([]); repo.setGroupMapping("https://idp", "g1", viewer.id); repo.setGroupMapping("https://idp", "g2", editor.id); - expect(repo.resolveOidcRole("https://idp", "u1", ["g1", "g2"])?.name).toBe("editor"); - + // the UNION, most privileged first — not just the winner (issue #28) + expect(names(["g1", "g2"])).toEqual(["editor", "viewer"]); + expect(names(["g1"])).toEqual(["viewer"]); + // an unmapped group contributes nothing rather than failing the lookup + expect(names(["g1", "nope"])).toEqual(["viewer"]); + + // no groups in hand (cookie / gateway-JWT paths) → what the last login stored + expect(names([])).toEqual([]); + repo.setLoginRoles(user.id, [viewer.id, editor.id]); + expect(names([])).toEqual(["editor", "viewer"]); + // replace-all: losing a group removes its role instead of accumulating + repo.setLoginRoles(user.id, [viewer.id]); + expect(names([])).toEqual(["viewer"]); + + // an explicit override replaces group-derived roles entirely repo.setUserRole(user.id, admin.id); - expect(repo.resolveOidcRole("https://idp", "u1", ["g1"])?.name).toBe("admin"); + expect(names(["g1", "g2"])).toEqual(["admin"]); + expect(repo.userBySubject("https://idp", "u1")?.roleId).toBe(admin.id); + // …and clearing it falls back to the roles the last login remembered + repo.setUserRole(user.id, null); + expect(names([])).toEqual(["viewer"]); }); it("stores and reads back OAuth clients", () => { diff --git a/packages/gateway/src/db/repo.ts b/packages/gateway/src/db/repo.ts index 406e423..fbcb0d2 100644 --- a/packages/gateway/src/db/repo.ts +++ b/packages/gateway/src/db/repo.ts @@ -102,6 +102,10 @@ export interface UserCredentialRow { updatedAt: string; } +/** Ordering helper: "most privileged first" for role lists. */ +const TIER_RANK_SQL = `CASE r.default_max_tier + WHEN 'destructive' THEN 3 WHEN 'write' THEN 2 WHEN 'read' THEN 1 ELSE 0 END`; + export class Repo { constructor(private readonly db: DatabaseSync) {} @@ -423,11 +427,48 @@ export class Repo { ).map(mapUser); } + /** Admin/bootstrap override — REPLACES whatever the user's groups map to. */ setUserRole(userId: number, roleId: number | null): boolean { - const result = this.db.prepare("UPDATE users SET role_id = ? WHERE id = ?").run(roleId, userId); + const result = this.db + .prepare("UPDATE users SET role_id = ?, role_source = ? WHERE id = ?") + .run(roleId, roleId === null ? null : "admin", userId); return result.changes > 0; } + /** + * Remember every role the login's group claims mapped to. Group claims only + * arrive with a fresh IdP token, so the cookie/JWT paths (which see no groups) + * read these rows back instead of re-deriving. Replace-all in one transaction: + * losing a group must remove its role, not accumulate forever. + */ + setLoginRoles(userId: number, roleIds: number[]): void { + this.db.exec("BEGIN"); + try { + this.db.prepare("DELETE FROM user_login_roles WHERE user_id = ?").run(userId); + const insert = this.db.prepare( + "INSERT OR IGNORE INTO user_login_roles (user_id, role_id) VALUES (?, ?)" + ); + for (const roleId of roleIds) insert.run(userId, roleId); + this.db.exec("COMMIT"); + } catch (err) { + this.db.exec("ROLLBACK"); + throw err; + } + } + + loginRoles(userId: number): RoleRow[] { + return ( + this.db + .prepare( + `SELECT r.id, r.name, r.default_max_tier, r.is_admin, r.protected + FROM user_login_roles ulr JOIN roles r ON r.id = ulr.role_id + WHERE ulr.user_id = ? + ORDER BY r.is_admin DESC, ${TIER_RANK_SQL} DESC, r.id` + ) + .all(userId) as Array> + ).map(mapRole); + } + // ── group mappings ── listGroupMappings(): GroupMappingRow[] { @@ -454,25 +495,41 @@ export class Repo { return this.db.prepare("DELETE FROM group_mappings WHERE id = ?").run(id).changes > 0; } - /** Resolve the role for an OIDC login: user override > group mapping (highest tier wins). */ - resolveOidcRole(iss: string, sub: string, groups: string[]): RoleRow | null { + /** + * Every role an OIDC principal holds — the envelope is their UNION, so being + * added to a group can only widen access (issue #28). Order is + * "most privileged first" so callers can take [0] as the primary for display. + * + * - an explicit `users.role_id` override REPLACES group-derived roles (that is + * the point of an override); + * - otherwise every mapped group counts, not just the highest; + * - with no groups in hand (cookie / gateway-JWT paths) the roles remembered + * at the last real login are used. + */ + resolveOidcRoles(iss: string, sub: string, groups: string[]): RoleRow[] { const user = this.userBySubject(iss, sub); - if (user?.roleId != null) return this.roleById(user.roleId); - if (groups.length === 0) return null; + if (user?.roleId != null) { + const role = this.roleById(user.roleId); + return role ? [role] : []; + } + if (groups.length === 0) return user ? this.loginRoles(user.id) : []; + return this.rolesForGroups(iss, groups); + } + + /** Roles mapped from group claims, most privileged first. Override-blind. */ + rolesForGroups(iss: string, groups: string[]): RoleRow[] { + if (groups.length === 0) return []; const placeholders = groups.map(() => "?").join(","); - const row = this.db - .prepare( - `SELECT r.id, r.name, r.default_max_tier, r.is_admin, r.protected - FROM group_mappings gm JOIN roles r ON r.id = gm.role_id - WHERE gm.iss = ? AND gm.claim_value IN (${placeholders}) - ORDER BY r.is_admin DESC, - CASE r.default_max_tier - WHEN 'destructive' THEN 3 WHEN 'write' THEN 2 WHEN 'read' THEN 1 ELSE 0 - END DESC - LIMIT 1` - ) - .get(iss, ...groups) as Record | undefined; - return row ? mapRole(row) : null; + return ( + this.db + .prepare( + `SELECT DISTINCT r.id, r.name, r.default_max_tier, r.is_admin, r.protected + FROM group_mappings gm JOIN roles r ON r.id = gm.role_id + WHERE gm.iss = ? AND gm.claim_value IN (${placeholders}) + ORDER BY r.is_admin DESC, ${TIER_RANK_SQL} DESC, r.id` + ) + .all(iss, ...groups) as Array> + ).map(mapRole); } // ── user prefs (personal narrowing — slice 3) ── diff --git a/packages/gateway/src/domain/additive-roles.test.ts b/packages/gateway/src/domain/additive-roles.test.ts new file mode 100644 index 0000000..e76ae4c --- /dev/null +++ b/packages/gateway/src/domain/additive-roles.test.ts @@ -0,0 +1,120 @@ +/** + * Additive roles (issue #28): a principal holding several roles gets their + * UNION. Being added to a group can only ever widen access — the surprise this + * prevents is "we gave you billing access" silently removing ticket access. + */ + +import { describe, expect, it } from "vitest"; +import type { Tool } from "@modelcontextprotocol/sdk/types.js"; +import { openDatabase } from "../db/index.js"; +import { Repo } from "../db/repo.js"; +import type { Principal } from "../auth/principal.js"; +import { principalKey, withRoles } from "../auth/principal.js"; +import type { CatalogEntry } from "./catalog.js"; +import { PolicyService } from "./policy.js"; + +const tool = (name: string): Tool => ({ name, inputSchema: { type: "object" } }); +const entry = ( + upstreamToolName: string, + tier: "read" | "write" | "destructive", + upstreamId: string +): CatalogEntry => ({ + upstreamId, + namespace: upstreamId, + upstreamToolName, + exposedName: `${upstreamId}_${upstreamToolName}`, + tier, + tool: tool(upstreamToolName), +}); + +/** + * Two deliberately INCOMPARABLE roles, the case "highest tier wins" got wrong: + * techs may write tickets and cannot touch billing; billing may write invoices + * and cannot touch tickets. Neither contains the other. + */ +function setup() { + const repo = new Repo(openDatabase(":memory:")); + const policy = new PolicyService(repo); + const techs = repo.createRole("techs", "none"); + const billing = repo.createRole("billing", "none"); + repo.setGrant(techs.id, "tickets", "write"); + repo.setGrant(billing.id, "invoices", "write"); + + const asRoles = (...roles: Array<{ id: number; name: string }>): Principal => + withRoles( + { kind: "oidc", subject: "https://idp|u1", label: "alice" }, + roles.map((r) => ({ id: r.id, name: r.name, isAdmin: false })) + ); + + return { repo, policy, techs, billing, asRoles }; +} + +const ticket = entry("update_ticket", "write", "tickets"); +const invoice = entry("update_invoice", "write", "invoices"); + +describe("role union", () => { + it("keeps both surfaces where a single winner would drop one", () => { + const { policy, techs, billing, asRoles } = setup(); + + const techOnly = asRoles(techs); + expect(policy.allowsFor(techOnly, ticket)).toBe(true); + expect(policy.allowsFor(techOnly, invoice)).toBe(false); + + // Adding the billing group must ADD invoices without removing tickets. + const both = asRoles(techs, billing); + expect(policy.allowsFor(both, ticket)).toBe(true); + expect(policy.allowsFor(both, invoice)).toBe(true); + expect(policy.envelopeFor(both, [ticket, invoice]).map((e) => e.upstreamId)).toEqual([ + "tickets", + "invoices", + ]); + }); + + it("takes the most permissive ceiling per upstream, not the first role's", () => { + const { repo, policy, techs, billing, asRoles } = setup(); + // techs are capped at read on invoices, billing may write them + repo.setGrant(techs.id, "invoices", "read"); + const both = asRoles(techs, billing); + expect(policy.allowsFor(both, entry("get_invoice", "read", "invoices"))).toBe(true); + expect(policy.allowsFor(both, invoice)).toBe(true); + + // …and a role granted "none" doesn't drag the union down + repo.setGrant(techs.id, "invoices", "none"); + expect(policy.allowsFor(both, invoice)).toBe(true); + }); + + it("a per-role deny closes that role's path only — the kill switch closes all", () => { + const { repo, policy, techs, billing, asRoles } = setup(); + repo.setGrant(billing.id, "tickets", "write"); + repo.setOverride(techs.id, "tickets", "update_ticket", "deny"); + const both = asRoles(techs, billing); + + // still reachable through billing: subtracting for everyone is not a role's job + expect(policy.allows(techs.id, ticket)).toBe(false); + expect(policy.allowsFor(both, ticket)).toBe(true); + + // the global kill switch is the absolute one + repo.upsertToolSetting({ upstreamId: "tickets", toolName: "update_ticket", enabled: false }); + expect(policy.allowsFor(both, ticket)).toBe(false); + }); + + it("an admin role anywhere in the list makes the principal an admin", () => { + const { repo, techs } = setup(); + const adminRole = repo.roleByName("admin")!; + const principal = withRoles({ kind: "oidc", subject: "https://idp|u1", label: "alice" }, [ + { id: techs.id, name: "techs", isAdmin: false }, + { id: adminRole.id, name: "admin", isAdmin: true }, + ]); + expect(principal.isAdmin).toBe(true); + // primary stays the first entry (callers order most-privileged-first) + expect(principal.roleId).toBe(techs.id); + expect(principal.roleName).toBe("techs+admin"); + }); + + it("the session binding key covers the whole role set, order-independently", () => { + const { techs, billing, asRoles } = setup(); + expect(principalKey(asRoles(techs, billing))).toBe(principalKey(asRoles(billing, techs))); + // losing a role changes the key → the existing 403 forces a reconnect + expect(principalKey(asRoles(techs))).not.toBe(principalKey(asRoles(techs, billing))); + }); +}); diff --git a/packages/gateway/src/domain/policy-narrowing.test.ts b/packages/gateway/src/domain/policy-narrowing.test.ts index d177665..3279411 100644 --- a/packages/gateway/src/domain/policy-narrowing.test.ts +++ b/packages/gateway/src/domain/policy-narrowing.test.ts @@ -3,7 +3,7 @@ import type { Tool } from "@modelcontextprotocol/sdk/types.js"; import { openDatabase } from "../db/index.js"; import { Repo } from "../db/repo.js"; import type { Principal } from "../auth/principal.js"; -import { prefsIdentity, principalSlug } from "../auth/principal.js"; +import { prefsIdentity, principalSlug, withRoles } from "../auth/principal.js"; import type { CatalogEntry } from "./catalog.js"; import { PolicyService } from "./policy.js"; @@ -21,14 +21,14 @@ function setup() { const repo = new Repo(openDatabase(":memory:")); const policy = new PolicyService(repo); const editorRole = repo.roleByName("editor")!; - const editor: Principal = { - kind: "oidc", - subject: "https://login.example|ab428be9-1111-2222-3333-444455556666", - label: "alice", - roleId: editorRole.id, - roleName: "editor", - isAdmin: false, - }; + const editor: Principal = withRoles( + { + kind: "oidc", + subject: "https://login.example|ab428be9-1111-2222-3333-444455556666", + label: "alice", + }, + [{ id: editorRole.id, name: "editor", isAdmin: false }] + ); return { repo, policy, editor }; } @@ -144,7 +144,9 @@ describe("personal narrowing (allowsFor = envelope ∧ prefs)", () => { const { repo, policy, editor } = setup(); repo.setUserPref(prefsIdentity(editor), "up1", "get_doc", false); const viewerRole = repo.roleByName("viewer")!; - const sameUserNewRole: Principal = { ...editor, roleId: viewerRole.id, roleName: "viewer" }; + const sameUserNewRole: Principal = withRoles(editor, [ + { id: viewerRole.id, name: "viewer", isAdmin: false }, + ]); expect(policy.allowsFor(sameUserNewRole, entry("get_doc", "read"))).toBe(false); }); }); @@ -185,28 +187,19 @@ describe("denialReason (only self-inflicted denials are nameable)", () => { describe("principalSlug", () => { it("passes Entra OIDs through recognizably", () => { - const p: Principal = { - kind: "oidc", - subject: "https://login.example|AB428BE9-1111-2222-3333-444455556666", - label: "x", - roleId: 1, - roleName: "viewer", - isAdmin: false, - }; + const p: Principal = withRoles({ kind: "oidc", subject: "https://login.example|AB428BE9-1111-2222-3333-444455556666", label: "x" }, [ + { id: 1, name: "viewer", isAdmin: false }, + ]); expect(principalSlug(p)).toBe("ab428be9-1111-2222-3333-444455556666"); }); it("hashes non-KV-safe subjects deterministically", () => { - const p: Principal = { - kind: "oidc", - subject: "user@example.com", - label: "x", - roleId: 1, - roleName: "viewer", - isAdmin: false, - }; + const p: Principal = withRoles({ kind: "oidc", subject: "user@example.com", label: "x" }, [ + { id: 1, name: "viewer", isAdmin: false }, + ]); const slug = principalSlug(p); expect(slug).toMatch(/^[0-9a-f]{16}$/); - expect(principalSlug({ ...p, roleId: 99 })).toBe(slug); // role-independent + // role-independent + expect(principalSlug(withRoles(p, [{ id: 99, name: "admin", isAdmin: true }]))).toBe(slug); }); }); diff --git a/packages/gateway/src/domain/policy.ts b/packages/gateway/src/domain/policy.ts index d40d569..cde03ff 100644 --- a/packages/gateway/src/domain/policy.ts +++ b/packages/gateway/src/domain/policy.ts @@ -65,6 +65,22 @@ export class PolicyService { return [...entries].filter((entry) => this.allows(roleId, entry)); } + /** + * The envelope of a principal holding several roles: the UNION (issue #28). + * Each role is evaluated on its own — its grant, its default tier, its own + * per-tool overrides — so a `deny` override closes that role's path and not + * the others'. Subtracting for everyone is what the kill switch is for. + */ + allowsAny(roleIds: readonly number[], entry: CatalogEntry): boolean { + return roleIds.some((roleId) => this.allows(roleId, entry)); + } + + /** Envelope of every role the principal holds (no personal prefs applied). */ + envelopeFor(principal: Principal, entries: Iterable): CatalogEntry[] { + const roleIds = principal.roles.map((r) => r.id); + return [...entries].filter((entry) => this.allowsAny(roleIds, entry)); + } + /** * Personal narrowing (slice 3): effective = admin envelope ∧ user prefs. * Prefs are deny-only rows (an upstream-wide '' row or a per-tool row), so @@ -72,7 +88,7 @@ export class PolicyService { * Same function gates tools/list and tools/call, like the envelope itself. */ allowsFor(principal: Principal, entry: CatalogEntry): boolean { - if (!this.allows(principal.roleId, entry)) return false; + if (!this.allowsAny(principal.roles.map((r) => r.id), entry)) return false; const who = prefsIdentity(principal); const serverPref = this.repo.userPrefFor(who, entry.upstreamId, ""); const toolPref = this.repo.userPrefFor(who, entry.upstreamId, entry.upstreamToolName); @@ -100,7 +116,7 @@ export class PolicyService { * else stays `"envelope"` and keeps the no-oracle wording. */ denialReason(principal: Principal, entry: CatalogEntry): "allowed" | "envelope" | "personal" | "optIn" { - if (!this.allows(principal.roleId, entry)) return "envelope"; + if (!this.allowsAny(principal.roles.map((r) => r.id), entry)) return "envelope"; if (this.allowsFor(principal, entry)) return "allowed"; return this.repo.getUpstream(entry.upstreamId)?.spec.userDefault === "off" ? "optIn" : "personal"; } diff --git a/packages/gateway/src/http/admin-api.ts b/packages/gateway/src/http/admin-api.ts index a6802bb..2ada367 100644 --- a/packages/gateway/src/http/admin-api.ts +++ b/packages/gateway/src/http/admin-api.ts @@ -506,7 +506,20 @@ export function createAdminRouter(deps: AppDeps, admin: AdminDeps): Router { router.get( "/users", h((_req, res) => { - res.json(repo.listUsers()); + // roleId is the ADMIN override; loginRoles are what the user's groups + // mapped to at their last login. Effective access is the union of + // whichever applies (#28), so show both rather than one "role" column. + res.json( + repo.listUsers().map((user) => { + const loginRoles = repo.loginRoles(user.id).map((r) => r.name); + const override = user.roleId != null ? repo.roleById(user.roleId)?.name ?? null : null; + return { + ...user, + loginRoles, + effectiveRoles: override ? [override] : loginRoles, + }; + }) + ); }) ); diff --git a/packages/gateway/src/http/app.test.ts b/packages/gateway/src/http/app.test.ts index b6600a2..d15af3e 100644 --- a/packages/gateway/src/http/app.test.ts +++ b/packages/gateway/src/http/app.test.ts @@ -583,6 +583,21 @@ describe("session reload", () => { expect(await listFederated("tok-admin", theirs)).toContain("fake_read_thing"); }); + it("shows me my own clients and whether they can be notified at all", async () => { + const sid = await initSession("tok-viewer"); + await initSession("tok-admin"); // someone else's, must not appear + + const mine = (await ( + await fetch(`${base}/api/me/sessions`, { headers: { Authorization: "Bearer tok-viewer" } }) + ).json()) as { sessions: Array<{ sessionId: string; streamOpen: boolean }>; notificationStream: boolean }; + + expect(mine.sessions.map((s) => s.sessionId)).toContain(sid); + expect(mine.sessions.every((s) => s.streamOpen === false)).toBe(true); + // No GET stream anywhere → the gateway cannot push list_changed at all, + // which is what makes "Apply now" the only cure. Say so, don't imply it. + expect(mine.notificationStream).toBe(false); + }); + it("is admin-only on /api/sessions, and targets one session or one principal", async () => { const sid = await initSession("tok-viewer"); diff --git a/packages/gateway/src/http/app.ts b/packages/gateway/src/http/app.ts index 456b5ab..629fbd4 100644 --- a/packages/gateway/src/http/app.ts +++ b/packages/gateway/src/http/app.ts @@ -34,7 +34,7 @@ import type { UpstreamManager } from "../upstream/manager.js"; import type { SecretStore } from "../secrets/store.js"; import type { OidcVerifier, OidcIdentity } from "../auth/oidc.js"; import { authenticateStaticToken, bearerToken } from "../auth/static-tokens.js"; -import { prefsIdentity, principalKey, type Principal } from "../auth/principal.js"; +import { prefsIdentity, principalKey, withRoles, type Principal } from "../auth/principal.js"; import { SESSION_COOKIE, TRANSIENT_COOKIE, @@ -170,7 +170,7 @@ export function loginUpsert( repo: Repo, config: GatewayConfig, identity: OidcIdentity -): { user: UserRow; role: RoleRow | null } { +): { user: UserRow; roles: RoleRow[] } { const user = repo.upsertUserOnLogin({ iss: identity.iss, sub: identity.sub, @@ -189,8 +189,16 @@ export function loginUpsert( } } } - const role = repo.resolveOidcRole(identity.iss, identity.sub, identity.groups); - return { user, role }; + // Group claims only ride on a real IdP token, so remember what they mapped to: + // the cookie and gateway-JWT paths see no groups and read these back. Stored + // even when an explicit override currently wins, so clearing the override + // later falls back to today's groups rather than to nothing. + repo.setLoginRoles( + user.id, + repo.rolesForGroups(identity.iss, identity.groups).map((r) => r.id) + ); + const roles = repo.resolveOidcRoles(identity.iss, identity.sub, identity.groups); + return { user, roles }; } /** Resolve the principal for a request. Exported for tests and the admin API. */ @@ -222,14 +230,9 @@ export function createAuthResolver(deps: AppDeps) { } return { ok: true, - principal: { - kind: "static", - subject: staticEntry.label, - label: staticEntry.label, - roleId: role.id, - roleName: role.name, - isAdmin: role.isAdmin, - }, + principal: withRoles({ kind: "static", subject: staticEntry.label, label: staticEntry.label }, [ + { id: role.id, name: role.name, isAdmin: role.isAdmin }, + ]), }; } @@ -252,19 +255,19 @@ export function createAuthResolver(deps: AppDeps) { } catch (err) { return unauthorized(`Invalid token: ${err instanceof Error ? err.message : String(err)}`); } - const role = repo.resolveOidcRole(identity.iss, identity.sub, []); - if (!role) { + const roles = repo.resolveOidcRoles(identity.iss, identity.sub, []); + if (roles.length === 0) { return { ok: false, status: 403, code: -32003, message: NO_ROLE_MESSAGE }; } const user = repo.userBySubject(identity.iss, identity.sub); - const principal: Principal = { - kind: "oidc", - subject: `${identity.iss}|${identity.sub}`, - label: user?.email ?? user?.displayName ?? identity.sub, - roleId: role.id, - roleName: role.name, - isAdmin: role.isAdmin, - }; + const principal: Principal = withRoles( + { + kind: "oidc", + subject: `${identity.iss}|${identity.sub}`, + label: user?.email ?? user?.displayName ?? identity.sub, + }, + roles + ); cache.set(cacheKey, { principal, expiresAt: Date.now() + CACHE_TTL_MS }); return { ok: true, principal }; } @@ -282,18 +285,14 @@ export function createAuthResolver(deps: AppDeps) { } catch (err) { return unauthorized(`Invalid token: ${err instanceof Error ? err.message : String(err)}`); } - const { role } = loginUpsert(repo, config, identity); - if (!role) { + const { roles } = loginUpsert(repo, config, identity); + if (roles.length === 0) { return { ok: false, status: 403, code: -32003, message: NO_ROLE_MESSAGE }; } - const principal: Principal = { - kind: "oidc", - subject: `${identity.iss}|${identity.sub}`, - label: identity.email ?? identity.sub, - roleId: role.id, - roleName: role.name, - isAdmin: role.isAdmin, - }; + const principal: Principal = withRoles( + { kind: "oidc", subject: `${identity.iss}|${identity.sub}`, label: identity.email ?? identity.sub }, + roles + ); cache.set(cacheKey, { principal, expiresAt: Date.now() + CACHE_TTL_MS }); return { ok: true, principal }; } @@ -307,21 +306,21 @@ export function createAuthResolver(deps: AppDeps) { const cookies = parseCookies(req.headers.cookie); const session = readSessionClaims(cookies[SESSION_COOKIE], config.login.sessionSecret); if (session) { - const role = repo.resolveOidcRole(session.iss, session.sub, []); - if (!role) { + const roles = repo.resolveOidcRoles(session.iss, session.sub, []); + if (roles.length === 0) { return { ok: false, status: 403, code: -32003, message: NO_ROLE_MESSAGE }; } const user = repo.userBySubject(session.iss, session.sub); return { ok: true, - principal: { - kind: "oidc", - subject: `${session.iss}|${session.sub}`, - label: user?.email ?? user?.displayName ?? session.sub, - roleId: role.id, - roleName: role.name, - isAdmin: role.isAdmin, - }, + principal: withRoles( + { + kind: "oidc", + subject: `${session.iss}|${session.sub}`, + label: user?.email ?? user?.displayName ?? session.sub, + }, + roles + ), }; } } @@ -332,14 +331,9 @@ export function createAuthResolver(deps: AppDeps) { if (admin) { return { ok: true, - principal: { - kind: "dev", - subject: "dev", - label: "dev-unauthenticated", - roleId: admin.id, - roleName: admin.name, - isAdmin: true, - }, + principal: withRoles({ kind: "dev", subject: "dev", label: "dev-unauthenticated" }, [ + { id: admin.id, name: admin.name, isAdmin: true }, + ]), }; } } @@ -594,6 +588,7 @@ export function createApp(deps: AppDeps): express.Express { resolveAuth, onPolicyChanged: broadcastVisibility, reloadSessions, + sessionSummaries, }) ); @@ -665,10 +660,10 @@ export function createApp(deps: AppDeps): express.Express { res.status(401).send("Sign-in failed. Please try again."); return; } - const { user, role } = loginUpsert(deps.repo, config, identity); - // Persist the resolved role so the cookie path resolves it from the - // stored user row (approach b) — no privilege ever rides in the cookie. - if (role) deps.repo.setUserRole(user.id, role.id); + // loginUpsert persists the group-derived roles (user_login_roles), which + // is what the cookie path reads back — no privilege ever rides in the + // cookie, and an explicit admin override still wins over them. + loginUpsert(deps.repo, config, identity); // OAuth AS facade: this login was brokering user authentication for an // MCP client — mint a single-use code bound to the pending request and // bounce to the client's registered redirect_uri (validated at @@ -883,14 +878,9 @@ export function createApp(deps: AppDeps): express.Express { const sessionPrincipal = (req: Request): Principal | null => { const claims = readSessionClaims(parseCookies(req.headers.cookie)[SESSION_COOKIE], login.sessionSecret); if (!claims) return null; - return { - kind: "oidc", - subject: `${claims.iss}|${claims.sub}`, - label: claims.sub, - roleId: 0, - roleName: "", - isAdmin: false, - }; + return withRoles({ kind: "oidc", subject: `${claims.iss}|${claims.sub}`, label: claims.sub }, [ + { id: 0, name: "", isAdmin: false }, + ]); }; // NOTE: registered before the parametric route below — otherwise diff --git a/packages/gateway/src/http/me-api.ts b/packages/gateway/src/http/me-api.ts index 0b3bd66..1bce23f 100644 --- a/packages/gateway/src/http/me-api.ts +++ b/packages/gateway/src/http/me-api.ts @@ -26,6 +26,8 @@ interface MeDeps { onPolicyChanged: () => void; /** Close matching live MCP sessions; returns how many were dropped. */ reloadSessions: (match: (session: { principal: Principal }, sessionId: string) => boolean) => number; + /** Live sessions (all principals) — filtered to the caller here. */ + sessionSummaries: () => Array>; } export function createMeRouter(deps: AppDeps, me: MeDeps): Router { @@ -76,7 +78,9 @@ export function createMeRouter(deps: AppDeps, me: MeDeps): Router { string, Array<{ name: string; exposedName: string; tier: string; group: string; enabled: boolean }> >(); - for (const entry of policy.visibleEntries(principal.roleId, manager.catalogEntries())) { + // Union of every role the caller holds (issue #28) — a person in two + // mapped groups sees both surfaces here, not just the higher one. + for (const entry of policy.envelopeFor(principal, manager.catalogEntries())) { const list = byUpstream.get(entry.upstreamId) ?? []; list.push({ name: entry.upstreamToolName, @@ -90,7 +94,11 @@ export function createMeRouter(deps: AppDeps, me: MeDeps): Router { byUpstream.set(entry.upstreamId, list); } res.json({ - principal: { label: principal.label, role: principal.roleName }, + principal: { + label: principal.label, + role: principal.roleName, + roles: principal.roles.map((r) => r.name), + }, servers: [...byUpstream.entries()].map(([upstreamId, tools]) => { const spec = repo.getUpstream(upstreamId)?.spec; const optIn = spec?.userDefault === "off"; @@ -135,7 +143,7 @@ export function createMeRouter(deps: AppDeps, me: MeDeps): Router { // Prefs only make sense inside the envelope; reject junk targets so the // table can't fill with garbage (and enabling can never widen anyway — // "enable" just deletes the personal deny row). - const envelope = policy.visibleEntries(principal.roleId, manager.catalogEntries()); + const envelope = policy.envelopeFor(principal, manager.catalogEntries()); const upstreamKnown = envelope.some((e) => e.upstreamId === body.upstreamId); const toolKnown = body.toolName === "" || @@ -190,6 +198,24 @@ export function createMeRouter(deps: AppDeps, me: MeDeps): Router { * notification, and a stale list is what makes a pref change look ignored — * enforcement was never stale, only the display. Own sessions only. */ + /** + * My live MCP sessions. `streamOpen: false` means the gateway has no channel + * to push tools/list_changed to that client, which is exactly when "Apply + * now" is the only way to make it re-read — worth showing rather than + * explaining. + */ + router.get( + "/sessions", + h((req, res) => { + const who = prefsIdentity(req.principal!); + const mine = me.sessionSummaries().filter((s) => s.principal === who); + res.json({ + sessions: mine.map(({ sessionId, streamOpen, toolCount }) => ({ sessionId, streamOpen, toolCount })), + notificationStream: mine.some((s) => s.streamOpen === true), + }); + }) + ); + router.post( "/sessions/reload", h((req, res) => { diff --git a/packages/gateway/src/mcp/gateway-server.ts b/packages/gateway/src/mcp/gateway-server.ts index a925412..6e7ecd4 100644 --- a/packages/gateway/src/mcp/gateway-server.ts +++ b/packages/gateway/src/mcp/gateway-server.ts @@ -41,16 +41,20 @@ export function createGatewayServer( // Envelope ∧ personal prefs — the same allowsFor gates list AND call, so // a user's own narrowing is enforced at the boundary, not just hidden in UX. - server.setRequestHandler(ListToolsRequestSchema, async () => ({ - tools: [ + server.setRequestHandler(ListToolsRequestSchema, async () => { + const tools = [ // Self-management is offered to admins only; for everyone else these // names simply don't exist (the call handler re-checks anyway). ...(selfTools && principal.isAdmin ? SELF_TOOLS : []), ...policy .visibleEntriesFor(principal, manager.catalogEntries()) .map((entry) => ({ ...entry.tool, name: entry.exposedName })), - ], - })); + ]; + // Logged because "did the client ever re-read the list?" was otherwise + // unanswerable, and that question cost a whole afternoon once. + console.error(`[mcp] ${principal.label} tools/list -> ${tools.length} tool(s)`); + return { tools }; + }); server.setRequestHandler(CallToolRequestSchema, async (request) => { // The `gw` namespace is reserved (config.ts refuses it for upstreams), so @@ -71,6 +75,10 @@ export function createGatewayServer( // their own switch. Every other case keeps the no-oracle wording: unknown, // globally disabled and role-denied tools stay indistinguishable. const reason = entry ? policy.denialReason(principal, entry) : "envelope"; + // Names and decisions only — never arguments, which carry customer data. + console.error( + `[mcp] ${principal.label} tools/call ${request.params.name} -> denied (${entry ? reason : "unknown"})` + ); const where = meUrl ? ` (${meUrl})` : ""; const text = reason === "personal" @@ -81,6 +89,7 @@ export function createGatewayServer( return { isError: true, content: [{ type: "text" as const, text }] }; } const args = request.params.arguments ?? {}; + console.error(`[mcp] ${principal.label} tools/call ${request.params.name} -> allowed`); // sessionMode:"per-user" — route the call over the caller's own // connection, with their registered credential refs layered onto the diff --git a/packages/gateway/src/mcp/self-tools.test.ts b/packages/gateway/src/mcp/self-tools.test.ts index 9b29240..4262ec3 100644 --- a/packages/gateway/src/mcp/self-tools.test.ts +++ b/packages/gateway/src/mcp/self-tools.test.ts @@ -111,6 +111,10 @@ describe("read-only tools", () => { const status = parsed(await callSelfTool(deps, admin, "gw_status", {})); expect(status.toolCount).toBe(3); expect((status.upstreams as Array<{ id: string }>).map((u) => u.id)).toEqual(["fake"]); + // version is the release, mode is standalone/integrated — the field used to + // carry the mode, so gw_status claimed to be running version "integrated" + expect(status.version).toMatch(/^\d+\.\d+\.\d+/); + expect(status.mode).toBe("standalone"); }); it("gw_list_servers redacts literal credential values but shows refs", async () => { diff --git a/packages/gateway/src/mcp/self-tools.ts b/packages/gateway/src/mcp/self-tools.ts index 4a0d032..f49d167 100644 --- a/packages/gateway/src/mcp/self-tools.ts +++ b/packages/gateway/src/mcp/self-tools.ts @@ -28,6 +28,7 @@ import { isMaxTier } from "../domain/policy.js"; import { renderPreset } from "../domain/presets.js"; import { effectiveGroupOf, effectiveTierOf, resolveToolTargets } from "../domain/tool-targets.js"; import { runBackup } from "../db/backup.js"; +import { SERVER_VERSION } from "../version.js"; import type { BackupUploader } from "../db/backup.js"; /** Reserved namespace: `config.ts` refuses it for upstreams. */ @@ -266,7 +267,10 @@ export async function callSelfTool( switch (name) { case `${SELF_NAMESPACE}_status`: return json({ - version: config.mode === "integrated" ? "integrated" : "standalone", + // The field said "version" and carried the MODE — so gw_status claimed + // the gateway was running version "integrated". Report both. + version: SERVER_VERSION, + mode: config.mode, toolCount: entries().length, upstreams: manager.summaries(), secretStore: config.bao ? "openbao" : config.keyVault ? "keyvault" : null,