From ab1aff48784f3f60c25e077402a07a2e2768a474 Mon Sep 17 00:00:00 2001 From: Vaibhav Zope Date: Wed, 16 Sep 2026 13:11:41 +0530 Subject: [PATCH] Record a removal before retiring what the person owned --- CHANGELOG.md | 9 ++ server/src/app.ts | 4 + server/src/people/store.ts | 29 +++-- ...offboarding-retirement.integration.test.ts | 119 ++++++++++++++++++ server/tests/people-routes.test.ts | 8 +- 5 files changed, 155 insertions(+), 14 deletions(-) create mode 100644 server/tests/offboarding-retirement.integration.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 831690d21..c3299195b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### Removing somebody is recorded even when retiring what they owned fails + +Removing somebody denies their access and ends their sessions, then retires the credentials and +brokered connections they had granted this deployment. When that second half failed — a vault or +Composio not answering — the removal was already committed but nothing was written to the audit trail, +and removing them a second time reported success without retrying it, leaving those connections +standing. The removal is now recorded as soon as it takes effect, and removing somebody already +removed finishes the retirement that failed. + ### A vendor that broke no longer reads as a refusal to a Bot running its own loop When a Bot that calls tools back from its own process, such as the LangGraph Bots, called a tool diff --git a/server/src/app.ts b/server/src/app.ts index 44a255fba..bc9a6cc2f 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -782,6 +782,10 @@ export function createApp( ); } + if (revoked) { + await peopleStore.retireOwned(userId, context.var.actor.id); + } + return context.json({ person: await peopleStore.find(userId) }); }); diff --git a/server/src/people/store.ts b/server/src/people/store.ts index 7d598e043..ada40807f 100644 --- a/server/src/people/store.ts +++ b/server/src/people/store.ts @@ -72,6 +72,7 @@ export type PeopleStore = { list: (query?: PeopleQuery) => Promise; setRole: (userId: string, role: OpenBotRole) => Promise; revoke: (userId: string, revokedBy: string) => Promise; + retireOwned: (userId: string, revokedBy: string) => Promise; restore: (userId: string) => Promise; find: (userId: string) => Promise; isRevoked: (email: string) => Promise; @@ -309,20 +310,22 @@ export function createPeopleStore( .onConflictDoNothing(); await tx.delete(sessions).where(eq(sessions.userId, userId)); }); + }, - /* - * After the transaction, and deliberately not inside it. - * - * Retiring a credential is a write to the vault plus an audit row, and the vault is reached - * through its own interface rather than this transaction's handle. Holding the person's removal - * open until that finishes would make an unrelated failure able to undo the deny-list row and - * the session deletion, which are the two things that must not fail to stick. - * - * So the order is: stop them getting in, then stop us holding their secret. If the second half - * throws, the first is already done and the audit trail shows a removal with no retirement - * beside it — which is the honest record of what happened, and is recoverable by removing them - * again. - */ + /* + * After `revoke`, and deliberately not inside it. + * + * Retiring a credential is a write to the vault plus an audit row, and the vault is reached + * through its own interface rather than that transaction's handle. Holding the person's removal + * open until that finishes would make an unrelated failure able to undo the deny-list row and + * the session deletion, which are the two things that must not fail to stick. + * + * So the order is: stop them getting in, then stop us holding their secret. If the second half + * throws, the first is already done and the audit trail shows a removal with no retirement + * beside it — which is the honest record of what happened, and is recoverable by removing them + * again. + */ + async retireOwned(userId, revokedBy) { await retireOwnedCredentials?.(userId, revokedBy); }, diff --git a/server/tests/offboarding-retirement.integration.test.ts b/server/tests/offboarding-retirement.integration.test.ts new file mode 100644 index 000000000..d0539a786 --- /dev/null +++ b/server/tests/offboarding-retirement.integration.test.ts @@ -0,0 +1,119 @@ +import { afterAll, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq, inArray } from "drizzle-orm"; +import { createApp } from "../src/app"; +import { loadConfig } from "../src/config"; +import { createDatabase } from "../src/db/client"; +import { revokedAccess, sessions, users } from "../src/db/schema"; +import { createPeopleStore } from "../src/people/store"; +import { TEST_POOL, testDatabaseUrl } from "./support/database"; +import { testEnvironment } from "./support/environment"; + +const database = createDatabase(testDatabaseUrl(), TEST_POOL); + +const suite = randomUUID().slice(0, 8); +const adminId = `offboarding-admin-${suite}`; +const memberId = `offboarding-member-${suite}`; +const memberEmail = `${memberId}@openbot.test`; + +const ADMIN = { + id: adminId, + email: `${adminId}@openbot.test`, + name: "An Administrator", + image: null, +}; + +afterAll(async () => { + await database + .delete(revokedAccess) + .where(eq(revokedAccess.email, memberEmail)); + await database.delete(sessions).where(eq(sessions.userId, memberId)); + await database.delete(users).where(inArray(users.id, [adminId, memberId])); +}); + +function appFor(retire: () => Promise<{ retired: number }>) { + const events: string[] = []; + const store = createPeopleStore(database, [], retire); + const auditStore = { + insert: async (event: { eventType: string }) => { + events.push(event.eventType); + }, + }; + + const app = createApp( + loadConfig(testEnvironment()), + { + handler: () => new Response(null, { status: 204 }), + api: { getSession: async () => ({ user: ADMIN }) }, + } as never, + { rolesForUser: async () => ["admin"] }, + ...(Array.from({ length: 9 }) as never[]), + auditStore as never, + ...(Array.from({ length: 4 }) as never[]), + store as never, + ); + + return { + events, + remove: () => + app.request(`http://openbot.test/api/admin/people/${memberId}/access`, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ revoked: true }), + }), + }; +} + +test("a broker that will not answer still leaves the removal on the trail, and asking again finishes it", async () => { + await database + .insert(users) + .values([ + { + id: adminId, + email: ADMIN.email, + name: "An Administrator", + emailVerified: true, + }, + { + id: memberId, + email: memberEmail, + name: "A Member", + emailVerified: true, + }, + ]) + .onConflictDoNothing(); + await database.insert(sessions).values({ + id: `${memberId}-session`, + userId: memberId, + token: `${memberId}-token`, + expiresAt: new Date(Date.now() + 86_400_000), + }); + + let attempts = 0; + const { events, remove } = appFor(async () => { + attempts += 1; + if (attempts === 1) { + throw new Error("composio: 503 Service Unavailable"); + } + return { retired: 0 }; + }); + + expect((await remove()).status).toBe(500); + + const denied = await database + .select({ email: revokedAccess.email }) + .from(revokedAccess) + .where(eq(revokedAccess.email, memberEmail)); + expect(denied).toHaveLength(1); + expect( + await database + .select({ id: sessions.id }) + .from(sessions) + .where(eq(sessions.userId, memberId)), + ).toEqual([]); + expect(events).toEqual(["person.access_revoked"]); + + expect((await remove()).status).toBe(200); + expect(attempts).toBe(2); + expect(events).toEqual(["person.access_revoked"]); +}); diff --git a/server/tests/people-routes.test.ts b/server/tests/people-routes.test.ts index 92bdca3a3..da6783d4e 100644 --- a/server/tests/people-routes.test.ts +++ b/server/tests/people-routes.test.ts @@ -51,6 +51,9 @@ function appWith( revoke: async (userId, by) => { calls.push(`revoke:${userId}:${by}`); }, + retireOwned: async (userId, by) => { + calls.push(`retireOwned:${userId}:${by}`); + }, restore: async (userId) => { calls.push(`restore:${userId}`); }, @@ -188,7 +191,10 @@ describe("people routes", () => { await request("/api/admin/people/u1/access", json({ revoked: true })); - expect(calls).toEqual([`revoke:u1:${ADMIN.id}`]); + expect(calls).toEqual([ + `revoke:u1:${ADMIN.id}`, + `retireOwned:u1:${ADMIN.id}`, + ]); }); test("restores access for somebody already removed", async () => {