Address Greptile review on board CLI auth

Co-Authored-By: Paperclip <noreply@paperclip.ing>

server/src/__tests__/cli-auth-routes.test.ts

@@ -18,6 +18,7 @@ const mockBoardAuthService = vi.hoisted(() => ({
   approveCliAuthChallenge: vi.fn(),
   cancelCliAuthChallenge: vi.fn(),
   resolveBoardAccess: vi.fn(),
+  resolveBoardActivityCompanyIds: vi.fn(),
   assertCurrentBoardKey: vi.fn(),
   revokeBoardApiKey: vi.fn(),
 }));
@@ -132,6 +133,7 @@ describe("cli auth routes", () => {
       companyIds: ["company-1"],
       isInstanceAdmin: false,
     });
+    mockBoardAuthService.resolveBoardActivityCompanyIds.mockResolvedValue(["company-1"]);
 
     const app = await createApp({
       type: "board",
@@ -161,4 +163,68 @@ describe("cli auth routes", () => {
       }),
     );
   });
+
+  it("logs approve activity for instance admins without company memberships", async () => {
+    mockBoardAuthService.approveCliAuthChallenge.mockResolvedValue({
+      status: "approved",
+      challenge: {
+        id: "challenge-2",
+        boardApiKeyId: "board-key-2",
+        requestedAccess: "instance_admin_required",
+        requestedCompanyId: null,
+        expiresAt: new Date("2026-03-23T13:00:00.000Z"),
+      },
+    });
+    mockBoardAuthService.resolveBoardActivityCompanyIds.mockResolvedValue(["company-a", "company-b"]);
+
+    const app = await createApp({
+      type: "board",
+      userId: "admin-1",
+      source: "session",
+      isInstanceAdmin: true,
+      companyIds: [],
+    });
+    const res = await request(app)
+      .post("/api/cli-auth/challenges/challenge-2/approve")
+      .send({ token: "pcp_cli_auth_secret" });
+
+    expect(res.status).toBe(200);
+    expect(mockBoardAuthService.resolveBoardActivityCompanyIds).toHaveBeenCalledWith({
+      userId: "admin-1",
+      requestedCompanyId: null,
+      boardApiKeyId: "board-key-2",
+    });
+    expect(mockLogActivity).toHaveBeenCalledTimes(2);
+  });
+
+  it("logs revoke activity with resolved audit company ids", async () => {
+    mockBoardAuthService.assertCurrentBoardKey.mockResolvedValue({
+      id: "board-key-3",
+      userId: "admin-2",
+    });
+    mockBoardAuthService.resolveBoardActivityCompanyIds.mockResolvedValue(["company-z"]);
+
+    const app = await createApp({
+      type: "board",
+      userId: "admin-2",
+      keyId: "board-key-3",
+      source: "board_key",
+      isInstanceAdmin: true,
+      companyIds: [],
+    });
+    const res = await request(app).post("/api/cli-auth/revoke-current").send({});
+
+    expect(res.status).toBe(200);
+    expect(mockBoardAuthService.resolveBoardActivityCompanyIds).toHaveBeenCalledWith({
+      userId: "admin-2",
+      boardApiKeyId: "board-key-3",
+    });
+    expect(mockLogActivity).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({
+        companyId: "company-z",
+        action: "board_api_key.revoked",
+      }),
+    );
+  });
 });

server/src/routes/access.ts

@@ -1671,8 +1671,12 @@ export function accessRoutes(
       );
 
       if (approved.status === "approved") {
-        const accessSnapshot = await boardAuth.resolveBoardAccess(userId);
-        for (const companyId of accessSnapshot.companyIds) {
+        const companyIds = await boardAuth.resolveBoardActivityCompanyIds({
+          userId,
+          requestedCompanyId: approved.challenge.requestedCompanyId,
+          boardApiKeyId: approved.challenge.boardApiKeyId,
+        });
+        for (const companyId of companyIds) {
           await logActivity(db, {
             companyId,
             actorType: "user",
@@ -1737,8 +1741,11 @@ export function accessRoutes(
       req.actor.userId,
     );
     await boardAuth.revokeBoardApiKey(key.id);
-    const accessSnapshot = await boardAuth.resolveBoardAccess(key.userId);
-    for (const companyId of accessSnapshot.companyIds) {
+    const companyIds = await boardAuth.resolveBoardActivityCompanyIds({
+      userId: key.userId,
+      boardApiKeyId: key.id,
+    });
+    for (const companyId of companyIds) {
       await logActivity(db, {
         companyId,
         actorType: "user",

server/src/services/board-auth.ts

@@ -1,5 +1,5 @@
 import { createHash, randomBytes, timingSafeEqual } from "node:crypto";
-import { and, eq, isNull } from "drizzle-orm";
+import { and, eq, isNull, sql } from "drizzle-orm";
 import type { Db } from "@paperclipai/db";
 import {
   authUsers,
@@ -86,6 +86,46 @@ export function boardAuthService(db: Db) {
     };
   }
 
+  async function resolveBoardActivityCompanyIds(input: {
+    userId: string;
+    requestedCompanyId?: string | null;
+    boardApiKeyId?: string | null;
+  }) {
+    const access = await resolveBoardAccess(input.userId);
+    const companyIds = new Set(access.companyIds);
+
+    if (companyIds.size === 0 && input.requestedCompanyId?.trim()) {
+      companyIds.add(input.requestedCompanyId.trim());
+    }
+
+    if (companyIds.size === 0 && input.boardApiKeyId?.trim()) {
+      const challengeCompanyIds = await db
+        .select({ requestedCompanyId: cliAuthChallenges.requestedCompanyId })
+        .from(cliAuthChallenges)
+        .where(eq(cliAuthChallenges.boardApiKeyId, input.boardApiKeyId.trim()))
+        .then((rows) =>
+          rows
+            .map((row) => row.requestedCompanyId?.trim() ?? null)
+            .filter((value): value is string => Boolean(value)),
+        );
+      for (const companyId of challengeCompanyIds) {
+        companyIds.add(companyId);
+      }
+    }
+
+    if (companyIds.size === 0 && access.isInstanceAdmin) {
+      const allCompanyIds = await db
+        .select({ id: companies.id })
+        .from(companies)
+        .then((rows) => rows.map((row) => row.id));
+      for (const companyId of allCompanyIds) {
+        companyIds.add(companyId);
+      }
+    }
+
+    return Array.from(companyIds);
+  }
+
   async function findBoardApiKeyByToken(token: string) {
     const tokenHash = hashBearerToken(token);
     const now = new Date();
@@ -210,47 +250,59 @@ export function boardAuthService(db: Db) {
   }
 
   async function approveCliAuthChallenge(id: string, token: string, userId: string) {
-    const challenge = await getCliAuthChallengeBySecret(id, token);
-    if (!challenge) throw notFound("CLI auth challenge not found");
-
-    const status = challengeStatusForRow(challenge);
-    if (status === "expired") return { status, challenge };
-    if (status === "cancelled") return { status, challenge };
-
     const access = await resolveBoardAccess(userId);
-    if (challenge.requestedAccess === "instance_admin_required" && !access.isInstanceAdmin) {
-      throw forbidden("Instance admin required");
-    }
+    return db.transaction(async (tx) => {
+      await tx.execute(
+        sql`select ${cliAuthChallenges.id} from ${cliAuthChallenges} where ${cliAuthChallenges.id} = ${id} for update`,
+      );
 
-    let boardKeyId = challenge.boardApiKeyId;
-    if (!boardKeyId) {
-      const createdKey = await db
-        .insert(boardApiKeys)
-        .values({
-          userId,
-          name: challenge.pendingKeyName,
-          keyHash: challenge.pendingKeyHash,
-          expiresAt: boardApiKeyExpiresAt(),
+      const challenge = await tx
+        .select()
+        .from(cliAuthChallenges)
+        .where(eq(cliAuthChallenges.id, id))
+        .then((rows) => rows[0] ?? null);
+      if (!challenge || !tokenHashesMatch(challenge.secretHash, hashBearerToken(token))) {
+        throw notFound("CLI auth challenge not found");
+      }
+
+      const status = challengeStatusForRow(challenge);
+      if (status === "expired") return { status, challenge };
+      if (status === "cancelled") return { status, challenge };
+
+      if (challenge.requestedAccess === "instance_admin_required" && !access.isInstanceAdmin) {
+        throw forbidden("Instance admin required");
+      }
+
+      let boardKeyId = challenge.boardApiKeyId;
+      if (!boardKeyId) {
+        const createdKey = await tx
+          .insert(boardApiKeys)
+          .values({
+            userId,
+            name: challenge.pendingKeyName,
+            keyHash: challenge.pendingKeyHash,
+            expiresAt: boardApiKeyExpiresAt(),
+          })
+          .returning()
+          .then((rows) => rows[0]);
+        boardKeyId = createdKey.id;
+      }
+
+      const approvedAt = challenge.approvedAt ?? new Date();
+      const updated = await tx
+        .update(cliAuthChallenges)
+        .set({
+          approvedByUserId: userId,
+          boardApiKeyId: boardKeyId,
+          approvedAt,
+          updatedAt: new Date(),
         })
+        .where(eq(cliAuthChallenges.id, challenge.id))
         .returning()
-        .then((rows) => rows[0]);
-      boardKeyId = createdKey.id;
-    }
+        .then((rows) => rows[0] ?? challenge);
 
-    const approvedAt = challenge.approvedAt ?? new Date();
-    const updated = await db
-      .update(cliAuthChallenges)
-      .set({
-        approvedByUserId: userId,
-        boardApiKeyId: boardKeyId,
-        approvedAt,
-        updatedAt: new Date(),
-      })
-      .where(eq(cliAuthChallenges.id, challenge.id))
-      .returning()
-      .then((rows) => rows[0] ?? challenge);
-
-    return { status: "approved" as const, challenge: updated };
+      return { status: "approved" as const, challenge: updated };
+    });
   }
 
   async function cancelCliAuthChallenge(id: string, token: string) {
@@ -297,5 +349,6 @@ export function boardAuthService(db: Db) {
     approveCliAuthChallenge,
     cancelCliAuthChallenge,
     assertCurrentBoardKey,
+    resolveBoardActivityCompanyIds,
   };
 }