diff --git a/CHANGELOG.md b/CHANGELOG.md index f1236b161..1777176a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,16 @@ and is safe to rerun. It kills a port holder only once that process has identifi OpenBot, so an unrelated process on 3010 is named and left alone rather than killed. Nothing is deleted: the database, the Bots' files and their browser profiles are volumes. `--keep-computers` leaves the browsers signed in. +### The trail says when an identity provider was added, not only when one was taken away + +Whoever holds an identity provider decides who can sign in at all, and the audit trail recorded only +half of that. Removing one through the administration screen was written down; registering one was +not, because registration is the sign-in library's own endpoint and nothing this deployment owns ran +on the way through. The event type for it had been declared and never written. Removing a provider +through the library's endpoint rather than the screen was unrecorded for the same reason. Both are +now written where the deployment already stands in front of those routes to check that the person +asking is an administrator, so a provider appearing or disappearing names itself and whoever did it. + ### Two workers on one machine can no longer fire the same routine twice Every process that claims work from the shared queue named itself after its hostname, and the queue diff --git a/server/src/app.ts b/server/src/app.ts index 09f2764bc..8f07911a4 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -6,6 +6,7 @@ import type { BotAccessCheck } from "./agents/profile-policy"; import type { AgentProfileStore } from "./agents/profile-store"; import { createAgentRoutes } from "./agents/routes"; import { + type AuditEventType, type AuditReader, type AuditStore, AuditQueryError, @@ -276,6 +277,11 @@ export function createApp( "/api/auth/sso/delete-provider", ]); + const AUTH_ROUTE_EVENTS: Record = { + "/api/auth/sso/register": "identity_provider.registered", + "/api/auth/sso/delete-provider": "identity_provider.removed", + }; + app.on(["GET", "POST"], "/api/auth/*", async (context) => { if (!auth) { return context.json( @@ -301,7 +307,43 @@ export function createApp( } } - return auth.handler(context.req.raw); + const eventType = + AUTH_ROUTE_EVENTS[new URL(context.req.url).pathname] ?? undefined; + + // Read before the handler runs, because it consumes the stream: a clone taken afterwards is of + // a request whose body is already gone, and the row would name no provider. + const named = + auditStore && eventType + ? ((await context.req.raw + .clone() + .json() + .catch(() => null)) as { providerId?: unknown } | null) + : null; + + const answer = await auth.handler(context.req.raw); + + if (auditStore && eventType && answer.ok) { + const session = await auth.api.getSession({ + headers: context.req.raw.headers, + query: { disableCookieCache: true }, + }); + await recordAuditEvent(auditStore, { + eventType, + targetType: "identity_provider", + ...(typeof named?.providerId === "string" + ? { targetId: named.providerId } + : {}), + ...(session?.user ? { actorUserId: session.user.id } : {}), + payload: { + ...(typeof named?.providerId === "string" + ? { providerId: named.providerId } + : {}), + ...(session?.user?.email ? { by: session.user.email } : {}), + }, + }); + } + + return answer; }); const authenticationUnavailable: MiddlewareHandler<{ diff --git a/server/tests/identity-provider-audit.test.ts b/server/tests/identity-provider-audit.test.ts new file mode 100644 index 000000000..52ba14d61 --- /dev/null +++ b/server/tests/identity-provider-audit.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, test } from "bun:test"; +import { createApp } from "../src/app"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import { loadConfig } from "../src/config"; +import { testEnvironment } from "./support/environment"; + +const ADMIN = { id: "admin", email: "admin@openbot.test" }; + +function app(role = "admin") { + const rows: AuditEventInput[] = []; + const auditStore: AuditStore = { + insert: async (event) => void rows.push(event), + }; + const hono = createApp( + loadConfig(testEnvironment()), + { + /* + * Reads the body, as the real sign-in library does. A handler that leaves the stream + * untouched hides the ordering this depends on: a clone taken after the handler has run is of + * a consumed request, and the row would name no provider. + */ + handler: async (request: Request) => { + await request.json().catch(() => null); + return new Response(JSON.stringify({ ok: true }), { status: 200 }); + }, + api: { getSession: async () => ({ user: ADMIN }) }, + } as never, + { rolesForUser: async () => [role] }, + // Positions 4-12 are the other stores; auditStore is 13. + ...(Array.from({ length: 9 }) as never[]), + auditStore as never, + ); + return { rows, hono }; +} + +const REGISTER_BODY = { + providerId: "acme", + issuer: "https://login.acme.test", + domain: "acme.test", +}; + +describe("the trail says how an identity provider came to exist", () => { + test("records the registration of an identity provider", async () => { + const { rows, hono } = app(); + const response = await hono.request("/api/auth/sso/register", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(REGISTER_BODY), + }); + expect(response.status).toBe(200); + expect(rows.map((row) => row.eventType)).toContain( + "identity_provider.registered", + ); + }); + + test("names who registered it and which provider", async () => { + const { rows, hono } = app(); + await hono.request("/api/auth/sso/register", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(REGISTER_BODY), + }); + const row = rows.find( + (candidate) => candidate.eventType === "identity_provider.registered", + ); + expect(row?.targetId).toBe("acme"); + expect(row?.actorUserId).toBe("admin"); + }); + + test("records a removal made through the library's own endpoint", async () => { + const { rows, hono } = app(); + await hono.request("/api/auth/sso/delete-provider", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ providerId: "acme" }), + }); + expect(rows.map((row) => row.eventType)).toContain( + "identity_provider.removed", + ); + }); + + test("writes nothing when the library refuses the change", async () => { + const rows: AuditEventInput[] = []; + const hono = createApp( + loadConfig(testEnvironment()), + { + handler: async (request: Request) => { + await request.json().catch(() => null); + return new Response("no", { status: 400 }); + }, + api: { getSession: async () => ({ user: ADMIN }) }, + } as never, + { rolesForUser: async () => ["admin"] }, + ...(Array.from({ length: 9 }) as never[]), + { insert: async (event) => void rows.push(event) } as never, + ); + await hono.request("/api/auth/sso/register", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(REGISTER_BODY), + }); + expect(rows).toEqual([]); + }); + + test("writes nothing when a non-administrator is refused", async () => { + const { rows, hono } = app("user"); + const response = await hono.request("/api/auth/sso/register", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(REGISTER_BODY), + }); + expect(response.status).toBe(403); + expect(rows).toEqual([]); + }); +});