Record a removal before retiring what the person owned (#574)

Co-authored-by: David McKay <david@copilotkit.ai>

Author: zopeVaibhav

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.
+
 ### Removing a connector takes its grants with it
 
 A grant naming a connector's tool outlived the connector. Removing an app revoked every credential

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) });
   });
 

server/src/people/store.ts

@@ -72,6 +72,7 @@ export type PeopleStore = {
   list: (query?: PeopleQuery) => Promise<PeoplePage>;
   setRole: (userId: string, role: OpenBotRole) => Promise<void>;
   revoke: (userId: string, revokedBy: string) => Promise<void>;
+  retireOwned: (userId: string, revokedBy: string) => Promise<void>;
   restore: (userId: string) => Promise<void>;
   find: (userId: string) => Promise<Person | undefined>;
   isRevoked: (email: string) => Promise<boolean>;
@@ -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);
     },
 

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"]);
+});

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 () => {