diff --git a/server/src/agents/profile-policy.ts b/server/src/agents/profile-policy.ts index bdf4a67..76bc132 100644 --- a/server/src/agents/profile-policy.ts +++ b/server/src/agents/profile-policy.ts @@ -23,3 +23,17 @@ export function canManageAgent( } export const canRunAgent = canAccessAgent; + +/** + * Whether this person may act as this Bot. + * + * Injected rather than imported, so a surface that acts as a Bot depends on the question and not on + * the agents table. It also keeps the answer in one place: the store's read path already filters on + * {@link canAccessAgent}, so asking it is the same rule the roster and the runtime already apply, + * rather than a second copy that can drift from them. + */ +export type BotAccessCheck = ( + /** The whole actor, not just the id: an administrator reaches every Bot, and a role tells us. */ + actor: AgentActor, + botId: string, +) => Promise; diff --git a/server/src/app.ts b/server/src/app.ts index 7675d47..3a5a872 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -1,5 +1,6 @@ import type { Hono as HonoApp, MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "./agents/profile-policy"; import type { AgentProfileStore } from "./agents/profile-store"; import { createAgentRoutes } from "./agents/routes"; import { type AuditReader, type AuditStore, auditQueryFromUrl } from "./audit"; @@ -295,6 +296,21 @@ export function createApp( app.route("/", copilotHandler); } + /** + * May this person act as this Bot? + * + * The store's own read path already applies `canAccessAgent`, so asking it for the Bot is the same + * question the roster and the runtime ask, rather than a second copy of the rule. + * + * A deployment with no profile store has no agents table and therefore no private Bot to protect: + * its Bots come from the tenant package and are public to everybody who can sign in. Answering yes + * there keeps that deployment working without weakening one that has owners. + */ + const canUseBot: BotAccessCheck = agentProfileStore + ? async (actor, botId) => + (await agentProfileStore.get(actor, botId)) !== null + : async () => true; + // The Bot computer. Acting on a page needs the gateway and the policy it enforces, so all // three arrive together or the routes are not mounted: a computer whose actions were ungoverned is // not a reduced feature, it is the one shape of this feature that must not exist. @@ -306,6 +322,7 @@ export function createApp( computerGateway, computerPolicy, requireUser, + canUseBot, ), ); } @@ -336,12 +353,15 @@ export function createApp( if (componentStore) { app.route( "/api/components", - createComponentRoutes(componentStore, requireUser, auditStore), + createComponentRoutes(componentStore, requireUser, auditStore, canUseBot), ); } if (pluginStore) { - app.route("/api/plugins", createPluginRoutes(pluginStore, requireUser)); + app.route( + "/api/plugins", + createPluginRoutes(pluginStore, requireUser, canUseBot), + ); } if (sandboxedStore) { diff --git a/server/src/components/routes.ts b/server/src/components/routes.ts index 0039099..b1854d6 100644 --- a/server/src/components/routes.ts +++ b/server/src/components/routes.ts @@ -1,5 +1,6 @@ import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "../agents/profile-policy"; import type { AuditStore } from "../audit"; import { recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; @@ -31,7 +32,13 @@ const DEV_ACTOR_EMAIL = "dev@openbot.local"; export function createComponentRoutes( store: ComponentStore, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, - auditStore?: AuditStore, + auditStore: AuditStore | undefined, + /** + * Whether the caller may act as the Bot they named. What a Bot may draw, and the data a drawing + * reads, are facts about that Bot; an administrator granting one is a separate question and stays + * behind `requireAdmin`. + */ + canUseBot: BotAccessCheck, ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -116,11 +123,13 @@ export function createComponentRoutes( * Deliberately says nothing about the components this Bot does NOT hold. A list of everything it * is missing would be a list the surface could accidentally register. */ - routes.get("/for-agent/:agentId", requireUser, async (context) => - context.json({ - components: await store.listForAgent(context.req.param("agentId")), - }), - ); + routes.get("/for-agent/:agentId", requireUser, async (context) => { + const agentId = context.req.param("agentId"); + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + return context.json({ components: await store.listForAgent(agentId) }); + }); /** * May this Bot use this component, right now? @@ -140,6 +149,11 @@ export function createComponentRoutes( if (!agentId) { return context.json({ error: "The Bot is required." }, 400); } + // Asked before the grant is, because the grant belongs to the Bot and says nothing about who is + // asking on its behalf. + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } const functions = Array.isArray(body?.functions) ? body.functions.filter( (entry): entry is string => typeof entry === "string", @@ -213,6 +227,11 @@ export function createComponentRoutes( 400, ); } + // Before the grant, and before anything runs. This is the route that executes, so borrowing a + // Bot here borrows whatever its components were granted. + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } const refuse = async (reason: string) => { await audit(context, "component.function_refused", name, { @@ -325,7 +344,6 @@ export function createComponentRoutes( if (!agentId) { return context.json({ error: "The Bot is required." }, 400); } - try { await store.grant(name, agentId); } catch (error) { diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index 3ce1c28..9b0af54 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -1,5 +1,6 @@ import type { Context, MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "../agents/profile-policy"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; import { @@ -34,14 +35,37 @@ export function createComputerRoutes( gateway: ComputerGateway, policyStore: PolicyStore, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, + /** + * Whether the caller may act as the Bot in the path. Required rather than optional, so a new + * deployment cannot be wired up without an answer to it. + */ + canUseBot: BotAccessCheck, ) { const routes = new Hono<{ Variables: AppVariables }>(); - routes.get("/:botId/status", requireUser, async (context) => + /** + * Every route under a Bot id, in one place. + * + * The Bot travels in the path, so each route would otherwise have to remember to ask, and the one + * that forgot would be the whole surface. Reads are gated as well as actions: a screenshot of + * somebody's Bot is whatever page it is signed into. + * + * The answer is the same for a Bot that does not exist and one belonging to somebody else, so this + * cannot be used to find out which Bots a deployment has. + */ + routes.use("/:botId/*", requireUser, async (context, next) => { + const botId = context.req.param("botId"); + if (botId && !(await canUseBot(context.var.actor, botId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + await next(); + }); + + routes.get("/:botId/status", async (context) => context.json(await client.status(context.req.param("botId"))), ); - routes.get("/:botId/screenshot", requireUser, async (context) => { + routes.get("/:botId/screenshot", async (context) => { try { return context.json( await client.forBot(context.req.param("botId")).screenshot(), @@ -51,7 +75,7 @@ export function createComputerRoutes( } }); - routes.get("/:botId/read", requireUser, async (context) => { + routes.get("/:botId/read", async (context) => { try { return context.json(await gateway.read(context.req.param("botId"))); } catch (error) { @@ -59,7 +83,7 @@ export function createComputerRoutes( } }); - routes.post("/:botId/navigate", requireUser, async (context) => { + routes.post("/:botId/navigate", async (context) => { const body = (await context.req.json().catch(() => null)) as { url?: unknown; } | null; @@ -95,7 +119,7 @@ export function createComputerRoutes( } }); - routes.post("/:botId/snapshot", requireUser, async (context) => { + routes.post("/:botId/snapshot", async (context) => { try { return context.json(await gateway.snapshot(context.req.param("botId"))); } catch (error) { @@ -109,7 +133,7 @@ export function createComputerRoutes( * Each one hands the gateway the computer id, the Bot, the actor and the input, and does no checking * of its own beyond the shape of the request. Where a decision gets made is a single place. */ - routes.post("/:botId/click", requireUser, (context) => + routes.post("/:botId/click", (context) => act(context, (botId, actor, body, signal) => { const ref = asRef(body); if (!ref) return badRef; @@ -117,7 +141,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/type", requireUser, (context) => + routes.post("/:botId/type", (context) => act(context, (botId, actor, body, signal) => { const ref = asRef(body); if (!ref) return badRef; @@ -138,7 +162,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/key", requireUser, (context) => + routes.post("/:botId/key", (context) => act(context, (botId, actor, body, signal) => { if (typeof body?.key !== "string" || !body.key) { return { error: "A key name is required, such as Enter or Tab." }; @@ -157,7 +181,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/scroll", requireUser, (context) => + routes.post("/:botId/scroll", (context) => act(context, (botId, actor, body) => gateway.scroll(botId, botId, actor, { ...(typeof body?.deltaY === "number" ? { deltaY: body.deltaY } : {}), @@ -169,7 +193,7 @@ export function createComputerRoutes( * Who has the wheel. Polled by the surface next to the screen, so the person sees the Bot ask for * help without reloading anything. */ - routes.get("/:botId/control", requireUser, async (context) => { + routes.get("/:botId/control", async (context) => { try { return context.json(await gateway.control(context.req.param("botId"))); } catch (error) { @@ -177,7 +201,7 @@ export function createComputerRoutes( } }); - routes.post("/:botId/control/request", requireUser, (context) => + routes.post("/:botId/control/request", (context) => act(context, (botId, actor, body) => gateway.requestHelp( botId, @@ -197,7 +221,7 @@ export function createComputerRoutes( * it holds a list. `:botId` is still there because every route under this router has it and the * gateway wants somebody to attribute the call to. */ - routes.get("/:botId/computers", requireUser, async (context) => { + routes.get("/:botId/computers", async (context) => { try { return context.json(await gateway.computers()); } catch (error) { @@ -206,25 +230,25 @@ export function createComputerRoutes( }); /** Stop the browser, keep the logins. */ - routes.post("/:botId/computers/stop", requireUser, (context) => + routes.post("/:botId/computers/stop", (context) => act(context, (botId, actor) => gateway.stopComputer(botId, botId, actor)), ); /** Delete the profile. Every login goes with it, which is the point and also the danger. */ - routes.post("/:botId/computers/reset", requireUser, (context) => + routes.post("/:botId/computers/reset", (context) => act(context, (botId, actor) => gateway.resetComputer(botId, botId, actor)), ); - routes.post("/:botId/control/take", requireUser, (context) => + routes.post("/:botId/control/take", (context) => act(context, (botId, actor) => gateway.takeControl(botId, botId, actor)), ); - routes.post("/:botId/control/release", requireUser, (context) => + routes.post("/:botId/control/release", (context) => act(context, (botId, actor) => gateway.releaseControl(botId, botId, actor)), ); /** The Bot asking for a value it must not be told. */ - routes.post("/:botId/control/secret", requireUser, (context) => + routes.post("/:botId/control/secret", (context) => act(context, (botId, actor, body) => { if (typeof body?.ref !== "string" || !body.ref) { return { @@ -253,7 +277,7 @@ export function createComputerRoutes( * route rather than a `kind` on the input route below, so that grepping for where a secret can enter * this server returns exactly one place. */ - routes.post("/:botId/human/secret", requireUser, (context) => + routes.post("/:botId/human/secret", (context) => act(context, (botId, actor, body) => { if (typeof body?.text !== "string" || !body.text) { return { error: "A value is required." }; @@ -270,7 +294,7 @@ export function createComputerRoutes( * unrecorded, because the reason a takeover exists is to let them enter the thing nothing else * should keep. */ - routes.post("/:botId/human/:kind", requireUser, async (context) => { + routes.post("/:botId/human/:kind", async (context) => { const kind = context.req.param("kind"); if ( kind !== "click" && @@ -297,7 +321,7 @@ export function createComputerRoutes( }); /** The Bot's files. Through the gateway, like every other acting call. */ - routes.post("/:botId/files/list", requireUser, (context) => + routes.post("/:botId/files/list", (context) => act(context, (botId, actor, body) => gateway.listFiles(botId, botId, actor, { ...(typeof body?.path === "string" && body.path.trim() @@ -307,7 +331,7 @@ export function createComputerRoutes( ), ); - routes.post("/:botId/files/read", requireUser, (context) => + routes.post("/:botId/files/read", (context) => act(context, (botId, actor, body) => { if (typeof body?.path !== "string" || !body.path.trim()) { return { error: "A file path is required." }; @@ -316,7 +340,7 @@ export function createComputerRoutes( }), ); - routes.post("/:botId/files/write", requireUser, (context) => + routes.post("/:botId/files/write", (context) => act(context, (botId, actor, body) => { if (typeof body?.path !== "string" || !body.path.trim()) { return { error: "A file path is required." }; diff --git a/server/src/index.ts b/server/src/index.ts index 3fc067a..4adfe6a 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -12,9 +12,9 @@ import { startChannelActivityListener, } from "./channels/events"; import { createChannelStore } from "./channels/routes"; +import { websocket as channelSocket } from "./channels/socket"; import { createStallGuard } from "./channels/stall-guard"; import { createThreadIdentity } from "./channels/thread-identity"; -import { websocket as channelSocket } from "./channels/socket"; import { createSandboxedStore } from "./components/sandboxed"; import { createComponentStore } from "./components/store"; import { createComputerClient } from "./computer/client"; @@ -425,10 +425,19 @@ serve({ } // The session guard, applied by hand because middleware does not run on an upgrade. An // unauthenticated socket here would be the whole point of the proxy defeated. - const actor = await identifyUser(request).catch(() => null); + const actor = await resolveRequestActor(request).catch(() => null); if (!actor) { return new Response("Sign in first.", { status: 401 }); } + // And which Bot, which the guard above does not answer. This socket carries that Bot's screen, + // so signing in is not enough: without this, anybody signed in watches anybody's Bot work. + if ( + !(await agentProfileStore + .get({ id: actor.id, role: actor.role }, streamBotId) + .catch(() => null)) + ) { + return new Response("There is no such Bot.", { status: 404 }); + } // Located per Bot when there is a supervisor, and the one shared computer when there is not. let upstream: string; try { diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index 59c6c2a..e37cbfb 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -1,5 +1,6 @@ import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; +import type { BotAccessCheck } from "../agents/profile-policy"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; import { CATALOGUE } from "./catalogue"; @@ -30,6 +31,11 @@ import { export function createPluginRoutes( store: PluginStore, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, + /** + * Whether the caller may act as the Bot they named. Required rather than optional, so a deployment + * cannot end up calling somebody else's tools by leaving an argument off. + */ + canUseBot: BotAccessCheck, ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -332,9 +338,15 @@ export function createPluginRoutes( }); /** What one Bot holds. The runtime reads this to decide what to offer a model. */ - routes.get("/for/:agentId", requireUser, async (context) => - context.json(await store.listForAgent(context.req.param("agentId"))), - ); + routes.get("/for/:agentId", requireUser, async (context) => { + const agentId = context.req.param("agentId"); + // A grant list is a fact about the Bot it belongs to. Left open it says which tools somebody + // else's private coworker has been given. + if (!(await canUseBot(context.var.actor, agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + return context.json(await store.listForAgent(agentId)); + }); /** * Call a tool, as a Bot. @@ -353,6 +365,13 @@ export function createPluginRoutes( return context.json({ error: "A tool and a Bot are required." }, 400); } + // Asked before the grant is looked up, and before anything reaches a vendor. The grant says this + // Bot may use the tool; it says nothing about whether this person may act as this Bot, and the + // call goes out on the deployment's own credential either way. + if (!(await canUseBot(context.var.actor, body.agentId))) { + return context.json({ error: "There is no such Bot." }, 404); + } + try { const result = await store.callTool({ ref: body.ref, diff --git a/server/tests/bot-access.test.ts b/server/tests/bot-access.test.ts new file mode 100644 index 0000000..88f4b8a --- /dev/null +++ b/server/tests/bot-access.test.ts @@ -0,0 +1,329 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AppVariables } from "../src/auth/guards"; +import { createComponentRoutes } from "../src/components/routes"; +import { createComputerRoutes } from "../src/computer/routes"; +import { createPluginRoutes } from "../src/plugins/routes"; + +/** + * Whether the person asking may act as the Bot they named. + * + * `requireUser` answers "is this a signed-in person", which is a different question and the only one + * these surfaces used to ask. A Bot id travels in the URL for the computer and in the body for a tool + * call, so without this a signed-in person acts as any Bot in the deployment, including a private one + * belonging to somebody else: they reset its browser, drive its pages, and fire its granted MCP tools + * against the deployment's own credential. + * + * The rule itself is not new. `canAccessAgent` has always said public, or owner, or administrator, + * and the store's read path has always filtered on it. These are the callers that never asked. + */ + +/** A signed-in person with the base role, which is the lowest privilege that gets past the guard. */ +function signedIn( + id: string, + role: "user" | "admin" = "user", +): MiddlewareHandler<{ Variables: AppVariables }> { + return async (context, next) => { + context.set("actor", { id, email: `${id}@openbot.test`, role }); + await next(); + }; +} + +/** + * Owner sees their own Bot, an administrator sees every Bot, nobody else sees it. Stands in for the + * store's access filter, which decides the same three ways. + */ +const ownedBy = + (owner: string) => + async (actor: { id: string; role: string }, botId: string) => + botId === "sales" && (actor.id === owner || actor.role === "admin"); + +describe("the computer surface", () => { + function app(actorId: string, role: "user" | "admin" = "user") { + const reached: string[] = []; + const gateway = { + resetComputer: async (_c: string, botId: string) => { + reached.push(`reset:${botId}`); + return { reset: true, botId }; + }, + read: async (botId: string) => { + reached.push(`read:${botId}`); + return { text: "a page" }; + }, + } as never; + const client = { + forBot: () => ({ + screenshot: async () => { + reached.push("screenshot"); + return { image: "" }; + }, + }), + status: async (botId: string) => { + reached.push(`status:${botId}`); + return { botId, state: "ready" }; + }, + } as never; + + const routes = createComputerRoutes( + client, + gateway, + { get: () => ({ mode: "enforce", deny: [], allow: [] }) } as never, + signedIn(actorId, role), + ownedBy("owner"), + ); + return { + reached, + hono: new Hono().route("/api/computers", routes), + }; + } + + test("lets the owner act on their own Bot", async () => { + const { hono, reached } = app("owner"); + const response = await hono.request( + "http://t/api/computers/sales/computers/reset", + { method: "POST" }, + ); + + expect(response.status).toBe(200); + expect(reached).toEqual(["reset:sales"]); + }); + + test("refuses somebody else's Bot, and does not act first", async () => { + const { hono, reached } = app("stranger"); + const response = await hono.request( + "http://t/api/computers/sales/computers/reset", + { method: "POST" }, + ); + + expect(response.status).toBe(404); + // The refusal has to happen before the gateway is called. A check that runs after the browser + // has already been wiped is not a check. + expect(reached).toEqual([]); + }); + + // Reading is not a lesser question here. A screenshot of somebody's Bot mid-task is the contents + // of whatever page it is signed into. + test.each([ + ["/api/computers/sales/read", "GET"], + ["/api/computers/sales/screenshot", "GET"], + ["/api/computers/sales/status", "GET"], + ])("refuses %s for somebody else's Bot", async (path, method) => { + const { hono, reached } = app("stranger"); + const response = await hono.request(`http://t${path}`, { method }); + + expect(response.status).toBe(404); + expect(reached).toEqual([]); + }); + + // An administrator already reaches every Bot everywhere else in the product. This must not become + // the one surface where they cannot. + test("still lets an administrator act on any Bot", async () => { + const { hono, reached } = app("someone-else", "admin"); + const response = await hono.request( + "http://t/api/computers/sales/computers/reset", + { method: "POST" }, + ); + + expect(response.status).toBe(200); + expect(reached).toEqual(["reset:sales"]); + }); + + test("says nothing about whether that Bot exists", async () => { + const { hono } = app("stranger"); + const missing = await hono.request( + "http://t/api/computers/no-such-bot/read", + ); + const private_ = await hono.request("http://t/api/computers/sales/read"); + + // Same answer either way, so the surface is not a way to enumerate other people's Bots. + expect(private_.status).toBe(missing.status); + expect(await private_.text()).toBe(await missing.text()); + }); +}); + +describe("the computer surface, unauthenticated", () => { + // The access middleware carries the session guard for everything under a Bot id, so the guard has + // to still refuse a caller with no session at all, and refuse it before anything is asked about a + // Bot. + test("refuses before it asks whose Bot it is", async () => { + const asked: string[] = []; + const reached: string[] = []; + const routes = createComputerRoutes( + {} as never, + { + read: async (botId: string) => { + reached.push(botId); + return { text: "" }; + }, + } as never, + { get: () => ({ mode: "enforce", deny: [], allow: [] }) } as never, + async (context) => + context.json({ error: "Authentication required." }, 401), + async (_actor, botId) => { + asked.push(botId); + return true; + }, + ); + const hono = new Hono().route("/api/computers", routes); + + const response = await hono.request("http://t/api/computers/sales/read"); + + expect(response.status).toBe(401); + expect(asked).toEqual([]); + expect(reached).toEqual([]); + }); +}); + +describe("calling a tool as a Bot", () => { + function app(actorId: string) { + const called: string[] = []; + const store = { + callTool: async (input: { ref: string; botId: string }) => { + called.push(`${input.ref}@${input.botId}`); + return { ok: true }; + }, + listForAgent: async (agentId: string) => { + called.push(`list:${agentId}`); + return { mcp: [], skills: [] }; + }, + listServers: async () => [], + listSkills: async () => [], + } as never; + + return { + called, + hono: new Hono().route( + "/api/plugins", + createPluginRoutes(store, signedIn(actorId), ownedBy("owner")), + ), + }; + } + + test("lets the owner call a tool as their own Bot", async () => { + const { hono, called } = app("owner"); + const response = await hono.request("http://t/api/plugins/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ref: "mcp__slack__post", agentId: "sales" }), + }); + + expect(response.status).toBe(200); + expect(called).toEqual(["mcp__slack__post@sales"]); + }); + + // What a Bot holds is a fact about that Bot, the same as its components. Left open, this says which + // tools somebody else's private coworker has been granted. + test("refuses to list what somebody else's Bot holds", async () => { + const { hono, called } = app("stranger"); + const response = await hono.request("http://t/api/plugins/for/sales"); + + expect(response.status).toBe(404); + expect(called).toEqual([]); + }); + + test("lets the owner list what their own Bot holds", async () => { + const { hono } = app("owner"); + const response = await hono.request("http://t/api/plugins/for/sales"); + + expect(response.status).toBe(200); + }); + + test("refuses a tool call as somebody else's Bot, and does not call it", async () => { + const { hono, called } = app("stranger"); + const response = await hono.request("http://t/api/plugins/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ ref: "mcp__slack__post", agentId: "sales" }), + }); + + expect(response.status).toBe(404); + // The grant belongs to the Bot, so the vendor call would have gone out on the deployment's + // credential. Nothing may reach the vendor before the caller is checked. + expect(called).toEqual([]); + }); +}); + +describe("components, which a Bot answers with", () => { + function app(actorId: string) { + const touched: string[] = []; + const store = { + listForAgent: async (agentId: string) => { + touched.push(`list:${agentId}`); + return [{ name: "chart" }]; + }, + decide: async (name: string, agentId: string) => { + touched.push(`decide:${name}:${agentId}`); + return { allowed: true }; + }, + mayCall: async () => true, + callFunction: async () => { + touched.push("callFunction"); + return { rows: [] }; + }, + } as never; + + return { + touched, + hono: new Hono().route( + "/api/components", + createComponentRoutes( + store, + signedIn(actorId), + undefined, + ownedBy("owner"), + ), + ), + }; + } + + test("lets the owner ask about their own Bot", async () => { + const { hono, touched } = app("owner"); + const response = await hono.request( + "http://t/api/components/for-agent/sales", + ); + + expect(response.status).toBe(200); + expect(touched).toEqual(["list:sales"]); + }); + + // What a Bot may draw is a fact about that Bot. Listing it for a coworker somebody else owns says + // which components they have been granted, which is the same leak the roster refuses. + test("refuses to list somebody else's Bot components", async () => { + const { hono, touched } = app("stranger"); + const response = await hono.request( + "http://t/api/components/for-agent/sales", + ); + + expect(response.status).toBe(404); + expect(touched).toEqual([]); + }); + + test("refuses a decision asked as somebody else's Bot", async () => { + const { hono, touched } = app("stranger"); + const response = await hono.request( + "http://t/api/components/chart/decision", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ agentId: "sales" }), + }, + ); + + expect(response.status).toBe(404); + expect(touched).toEqual([]); + }); + + // The one that runs something. A grant belongs to the Bot, so without this the caller borrows it. + test("refuses a data function called as somebody else's Bot", async () => { + const { hono, touched } = app("stranger"); + const response = await hono.request("http://t/api/components/chart/call", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ agentId: "sales", function: "rows", args: {} }), + }); + + expect(response.status).toBe(404); + expect(touched).toEqual([]); + }); +}); diff --git a/server/tests/component-decision.test.ts b/server/tests/component-decision.test.ts index ad2b639..a16b8aa 100644 --- a/server/tests/component-decision.test.ts +++ b/server/tests/component-decision.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; import type { AppVariables } from "../src/auth/guards"; import { createComponentRoutes } from "../src/components/routes"; import type { ComponentStore } from "../src/components/store"; @@ -34,7 +34,9 @@ const asSignedIn: MiddlewareHandler<{ Variables: AppVariables }> = async ( function app() { return new Hono().route( "/components", - createComponentRoutes(store, asSignedIn), + // These cover the decision itself, so every Bot here is one the caller may use. Whether they may + // is `bot-access.test.ts`. + createComponentRoutes(store, asSignedIn, undefined, async () => true), ); } diff --git a/server/tests/skill-ownership.integration.test.ts b/server/tests/skill-ownership.integration.test.ts index b62c503..cdb44a4 100644 --- a/server/tests/skill-ownership.integration.test.ts +++ b/server/tests/skill-ownership.integration.test.ts @@ -180,10 +180,16 @@ function routesAs(actor: { email: string; role: "admin" | "user"; }) { - return createPluginRoutes(store as never, async (context, next) => { - context.set("actor", actor as never); - await next(); - }); + return createPluginRoutes( + store as never, + async (context, next) => { + context.set("actor", actor as never); + await next(); + }, + // These cover who may GRANT a skill, which is a separate question from who may act as the Bot it + // is granted to. That one is `bot-access.test.ts`. + async () => true, + ); } const asAlice = () =>