diff --git a/packages/db/src/connection-grants-phase4-migration.test.ts b/packages/db/src/connection-grants-phase4-migration.test.ts index 539ba181f5d..139de8581b0 100644 --- a/packages/db/src/connection-grants-phase4-migration.test.ts +++ b/packages/db/src/connection-grants-phase4-migration.test.ts @@ -1,31 +1,154 @@ +import { createHash, randomUUID } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; -import { describe, expect, it } from "vitest"; +import postgres from "postgres"; +import { afterEach, describe, expect, it } from "vitest"; +import { applyPendingMigrations, inspectMigrations } from "./client.js"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./test-embedded-postgres.js"; -const sql = fs.readFileSync( - path.join(import.meta.dirname, "migrations/0229_sticky_nightcrawler.sql"), +const MIGRATION_FILE = "0229_sticky_nightcrawler.sql"; +const migrationSql = fs.readFileSync( + path.join(import.meta.dirname, "migrations", MIGRATION_FILE), "utf8", ); +const cleanups: Array<() => Promise> = []; +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); +const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +function migrationHash() { + return createHash("sha256").update(migrationSql).digest("hex"); +} + +afterEach(async () => { + while (cleanups.length > 0) await cleanups.pop()?.(); +}); describe("connection grants phase 4 migration", () => { - it("adds named-agent standing delegations", () => { - expect(sql).toContain(`CREATE TABLE IF NOT EXISTS "connection_grant_delegations"`); - expect(sql).toContain(`"connection_grant_delegations_grant_agent_uq"`); - expect(sql).toContain(`FOREIGN KEY ("company_id","grant_id")`); + it("adds replay-safe named-agent standing delegations", () => { + expect(migrationSql).toContain(`CREATE TABLE IF NOT EXISTS "connection_grant_delegations"`); + expect(migrationSql).toContain(`"connection_grant_delegations_grant_agent_uq"`); + expect(migrationSql).toContain(`FOREIGN KEY ("company_id","grant_id")`); + expect(migrationSql).toContain(`DROP CONSTRAINT IF EXISTS "connection_grant_delegations_company_grant_fk"`); + expect(migrationSql).toContain(`CREATE INDEX IF NOT EXISTS "connection_grant_delegations_company_agent_idx"`); + expect(migrationSql).toContain(`ON CONFLICT DO NOTHING`); }); - it("can replay after schema state moves ahead of the migration journal", () => { - expect(sql).toContain(`DROP CONSTRAINT IF EXISTS "connection_grant_delegations_company_grant_fk"`); - expect(sql).toContain(`CREATE INDEX IF NOT EXISTS "connection_grant_delegations_company_agent_idx"`); - expect(sql).toContain(`CREATE UNIQUE INDEX IF NOT EXISTS "connection_grant_delegations_grant_agent_uq"`); - expect(sql).toContain(`ON CONFLICT DO NOTHING`); - }); - - it("moves personal grant secrets to their owner's user scope", () => { - expect(sql).toContain(`g."kind" = 'user'`); - expect(sql).toContain(`"scope" = 'user'`); - expect(sql).toContain(`"owner_user_id" = owner_map."owner_user_id"`); - expect(sql).toContain(`"user_secret_definition_id" = d."id"`); - expect(sql).toContain(`HAVING count(DISTINCT g."subject_user_id") = 1`); + it("fails ambiguous personal-secret ownership closed with reauthorization guidance", () => { + expect(migrationSql).toContain(`Cannot migrate personal connection credential`); + expect(migrationSql).toContain(`Reauthorize the affected personal connection grants`); + expect(migrationSql).toContain(`count(DISTINCT g."subject_user_id") <> 1`); + expect(migrationSql).toContain(`other_g."kind" <> 'user'`); + expect(migrationSql).toContain(`jsonb_array_elements(c."credential_secret_refs")`); }); }); + +describeEmbeddedPostgres("connection grants phase 4 executable migration", () => { + it("converts one owner and rejects two-owner or mixed legacy references", async () => { + const database = await startEmbeddedPostgresTestDatabase("paperclip-connection-phase4-migration-"); + cleanups.push(database.cleanup); + const sql = postgres(database.connectionString, { max: 1, onnotice: () => {} }); + + async function rewindMigration() { + await sql`DELETE FROM "drizzle"."__drizzle_migrations" WHERE "hash" = ${migrationHash()}`; + expect(await inspectMigrations(database.connectionString)).toMatchObject({ + status: "needsMigrations", + pendingMigrations: [MIGRATION_FILE], + }); + } + + async function seedLegacyCredential(input: { + ownerUserIds: string[]; + includeOrganizationGrant?: boolean; + includeConnectionReference?: boolean; + }) { + const companyId = randomUUID(); + const applicationId = randomUUID(); + const connectionId = randomUUID(); + const secretId = randomUUID(); + await sql` + INSERT INTO "companies" ("id", "name", "issue_prefix") + VALUES (${companyId}, ${`Phase 4 ${companyId}`}, ${`P${companyId.slice(0, 5)}`}) + `; + await sql` + INSERT INTO "tool_applications" ("id", "company_id", "name", "type") + VALUES (${applicationId}, ${companyId}, ${`App ${applicationId}`}, 'mcp_http') + `; + await sql` + INSERT INTO "tool_connections" ( + "id", "company_id", "application_id", "name", "uid", "transport", "credential_secret_refs" + ) VALUES ( + ${connectionId}, ${companyId}, ${applicationId}, ${`Connection ${connectionId}`}, + ${`connection-${connectionId}`}, 'mcp_remote', + ${sql.json(input.includeConnectionReference ? [{ secretId, version: "latest" }] : [])} + ) + `; + await sql` + INSERT INTO "company_secrets" ("id", "company_id", "key", "name", "scope") + VALUES (${secretId}, ${companyId}, ${`secret-${secretId}`}, ${`Secret ${secretId}`}, 'company') + `; + for (const ownerUserId of input.ownerUserIds) { + await sql` + INSERT INTO "connection_grants" ( + "company_id", "connection_id", "kind", "subject_user_id", "credential_secret_refs" + ) VALUES ( + ${companyId}, ${connectionId}, 'user', ${ownerUserId}, + ${sql.json([{ secretId, version: "latest" }])} + ) + `; + } + if (input.includeOrganizationGrant) { + await sql` + INSERT INTO "connection_grants" ( + "company_id", "connection_id", "kind", "credential_secret_refs" + ) VALUES ( + ${companyId}, ${connectionId}, 'organization', + ${sql.json([{ secretId, version: "latest" }])} + ) + `; + } + return { companyId, secretId, ownerUserId: input.ownerUserIds[0]! }; + } + + try { + const valid = await seedLegacyCredential({ ownerUserIds: ["alice"] }); + await rewindMigration(); + await applyPendingMigrations(database.connectionString); + const converted = await sql<{ + scope: string; + owner_user_id: string | null; + user_secret_definition_id: string | null; + }[]>` + SELECT "scope", "owner_user_id", "user_secret_definition_id" + FROM "company_secrets" + WHERE "id" = ${valid.secretId} + `; + expect(converted).toEqual([{ + scope: "user", + owner_user_id: valid.ownerUserId, + user_secret_definition_id: expect.any(String), + }]); + + const ambiguous = await seedLegacyCredential({ ownerUserIds: ["alice", "bob"] }); + await rewindMigration(); + await expect(applyPendingMigrations(database.connectionString)).rejects.toThrow( + /legacy ownership is ambiguous/, + ); + + await sql`DELETE FROM "company_secrets" WHERE "company_id" = ${ambiguous.companyId}`; + await sql`DELETE FROM "companies" WHERE "id" = ${ambiguous.companyId}`; + await seedLegacyCredential({ + ownerUserIds: ["carol"], + includeOrganizationGrant: true, + includeConnectionReference: true, + }); + await expect(applyPendingMigrations(database.connectionString)).rejects.toThrow( + /legacy ownership is ambiguous/, + ); + } finally { + await sql.end(); + } + }, 45_000); +}); diff --git a/packages/db/src/migrations/0229_sticky_nightcrawler.sql b/packages/db/src/migrations/0229_sticky_nightcrawler.sql index 305bc30de79..cebad244231 100644 --- a/packages/db/src/migrations/0229_sticky_nightcrawler.sql +++ b/packages/db/src/migrations/0229_sticky_nightcrawler.sql @@ -19,6 +19,57 @@ ALTER TABLE "connection_grant_delegations" ADD CONSTRAINT "connection_grant_dele CREATE INDEX IF NOT EXISTS "connection_grant_delegations_company_agent_idx" ON "connection_grant_delegations" USING btree ("company_id","agent_id");--> statement-breakpoint CREATE UNIQUE INDEX IF NOT EXISTS "connection_grant_delegations_grant_agent_uq" ON "connection_grant_delegations" USING btree ("grant_id","agent_id"); --> statement-breakpoint +DO $$ +DECLARE + ambiguous_secret record; +BEGIN + SELECT + s."id" AS "secret_id", + count(DISTINCT g."subject_user_id") AS "personal_owner_count", + count(*) FILTER (WHERE other_g."id" IS NOT NULL) AS "organization_reference_count", + count(*) FILTER (WHERE c."id" IS NOT NULL) AS "connection_reference_count" + INTO ambiguous_secret + FROM "company_secrets" s + JOIN "connection_grants" g + ON g."company_id" = s."company_id" + AND g."kind" = 'user' + CROSS JOIN LATERAL jsonb_array_elements(g."credential_secret_refs") personal_ref + LEFT JOIN "connection_grants" other_g + ON other_g."company_id" = s."company_id" + AND other_g."kind" <> 'user' + AND EXISTS ( + SELECT 1 + FROM jsonb_array_elements(other_g."credential_secret_refs") other_ref + WHERE other_ref ->> 'secretId' = s."id"::text + ) + LEFT JOIN "tool_connections" c + ON c."company_id" = s."company_id" + AND EXISTS ( + SELECT 1 + FROM jsonb_array_elements(c."credential_secret_refs") connection_ref + WHERE connection_ref ->> 'secretId' = s."id"::text + ) + WHERE s."id"::text = personal_ref ->> 'secretId' + AND s."scope" = 'company' + GROUP BY s."id" + HAVING count(DISTINCT g."subject_user_id") <> 1 + OR count(*) FILTER (WHERE other_g."id" IS NOT NULL) > 0 + OR count(*) FILTER (WHERE c."id" IS NOT NULL) > 0 + LIMIT 1; + + IF FOUND THEN + RAISE EXCEPTION 'Cannot migrate personal connection credential %: legacy ownership is ambiguous', ambiguous_secret."secret_id" + USING ERRCODE = '23514', + DETAIL = format( + 'personal owners=%s, organization grant references=%s, connection references=%s', + ambiguous_secret."personal_owner_count", + ambiguous_secret."organization_reference_count", + ambiguous_secret."connection_reference_count" + ), + HINT = 'Reauthorize the affected personal connection grants with one credential per user before retrying this migration.'; + END IF; +END $$; +--> statement-breakpoint INSERT INTO "user_secret_definitions" ( "company_id", "key", "name", "description", "provider", "managed_mode", "provider_config_id", "provider_metadata", "created_by_agent_id", "created_by_user_id" diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index fc447a57712..1c0f2ae1549 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -38,7 +38,7 @@ import { toolRuntimeSlots, toolStdioCommandTemplates, } from "@paperclipai/db"; -import { and, eq, inArray } from "drizzle-orm"; +import { and, eq, inArray, sql } from "drizzle-orm"; import { getConnectableAppDefinition } from "@paperclipai/shared"; import { getEmbeddedPostgresTestSupport, @@ -879,6 +879,8 @@ describeEmbeddedPostgres("tool access service", () => { it("allows only the personal grant owner to create named-agent delegations and audits revocation", async () => { const company = await createCompany(db); const agent = await createAgent(db, company.id); + await grantBoardUser(db, company.id, "alice", [], "member"); + await grantBoardUser(db, company.id, "mallory", [], "member"); const { connection } = await createBrokerConnection(db, company.id); const grant = await db.insert(connectionGrants).values({ companyId: company.id, @@ -910,6 +912,88 @@ describeEmbeddedPostgres("tool access service", () => { ])); }); + it("serializes delegation creation behind membership removal so reauthorization cannot revive stale consent", async () => { + const company = await createCompany(db); + const agent = await createAgent(db, company.id); + await grantBoardUser(db, company.id, "alice", [], "member"); + const { connection } = await createBrokerConnection(db, company.id); + const grant = await db.insert(connectionGrants).values({ + companyId: company.id, + connectionId: connection.id, + kind: "user", + subjectUserId: "alice", + status: "active", + isDefault: false, + }).returning().then((rows) => rows[0]!); + const serviceDb = createDb(tempDb!.connectionString, { maxConnections: 1 }); + const service = createTestToolAccessService(serviceDb); + const removalDb = createDb(tempDb!.connectionString, { maxConnections: 1 }); + let releaseRemoval!: () => void; + const removalMayCommit = new Promise((resolve) => { + releaseRemoval = resolve; + }); + let membershipLocked!: () => void; + const membershipIsLocked = new Promise((resolve) => { + membershipLocked = resolve; + }); + + const removal = removalDb.transaction(async (tx) => { + await tx.execute(sql` + SELECT "id" + FROM "company_memberships" + WHERE "company_id" = ${company.id} + AND "principal_type" = 'user' + AND "principal_id" = 'alice' + FOR UPDATE + `); + await tx.execute(sql` + UPDATE "connection_grants" + SET "status" = 'revoked', "revoked_at" = now(), "updated_at" = now() + WHERE "id" = ${grant.id} + `); + await tx.execute(sql` + DELETE FROM "connection_grant_delegations" + WHERE "company_id" = ${company.id} AND "grant_id" = ${grant.id} + `); + membershipLocked(); + await removalMayCommit; + await tx.execute(sql` + UPDATE "company_memberships" + SET "status" = 'suspended', "updated_at" = now() + WHERE "company_id" = ${company.id} + AND "principal_type" = 'user' + AND "principal_id" = 'alice' + `); + }); + + await membershipIsLocked; + let creationSettled = false; + const creation = service.createConnectionGrantDelegation(connection.id, grant.id, agent.id, "alice") + .then( + (value) => ({ value, error: null }), + (error: unknown) => ({ value: null, error }), + ) + .finally(() => { + creationSettled = true; + }); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(creationSettled).toBe(false); + releaseRemoval(); + await removal; + const creationResult = await creation; + expect(creationResult.error).toEqual(expect.objectContaining({ + message: "Only an active company member can delegate their personal grant", + })); + + await db.update(companyMemberships).set({ status: "active" }).where(and( + eq(companyMemberships.companyId, company.id), + eq(companyMemberships.principalId, "alice"), + )); + await db.update(connectionGrants).set({ status: "active", revokedAt: null }).where(eq(connectionGrants.id, grant.id)); + expect(await db.select().from(connectionGrantDelegations).where(eq(connectionGrantDelegations.grantId, grant.id))) + .toHaveLength(0); + }); + it("fails autonomous token minting closed until the named agent has a standing delegation", async () => { const company = await createCompany(db); const agent = await createAgent(db, company.id); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index aba0c3881e7..9cd9b2f7812 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -8219,42 +8219,62 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} ownerUserId: string, ) => { const connection = await getConnectionRow(idOrUid); - const [grant] = await db.select().from(connectionGrants).where(and( - eq(connectionGrants.id, grantId), - eq(connectionGrants.companyId, connection.companyId), - eq(connectionGrants.connectionId, connection.id), - eq(connectionGrants.kind, "user"), - eq(connectionGrants.subjectUserId, ownerUserId), - eq(connectionGrants.status, "active"), - )).limit(1); - if (!grant) throw forbidden("Only the active personal grant owner can create a delegation"); - const [targetAgent] = await db.select({ id: agents.id }).from(agents).where(and( - eq(agents.id, agentId), - eq(agents.companyId, connection.companyId), - )).limit(1); - if (!targetAgent) throw notFound("Agent not found"); - const [existing] = await db.select().from(connectionGrantDelegations).where(and( - eq(connectionGrantDelegations.grantId, grant.id), - eq(connectionGrantDelegations.agentId, agentId), - )).limit(1); - if (existing) return existing; - const [delegation] = await db.insert(connectionGrantDelegations).values({ - companyId: connection.companyId, - grantId: grant.id, - agentId, - createdByUserId: ownerUserId, - }).returning(); - await db.insert(toolAccessAuditEvents).values({ - companyId: connection.companyId, - connectionId: connection.id, - actorType: "user", - actorId: ownerUserId, - action: "connection_grant.delegated", - outcome: "success", - reasonCode: "delegation_created", - details: { grantId: grant.id, delegationId: delegation!.id, agentId }, + return db.transaction(async (tx) => { + // Membership removal/suspension takes this same row lock before sweeping + // personal grants. Whichever operation wins is therefore authoritative: + // removal deletes a delegation committed first, while creation that runs + // second observes the inactive membership and fails closed. + const [membership] = await tx.select({ id: companyMemberships.id }) + .from(companyMemberships) + .where(and( + eq(companyMemberships.companyId, connection.companyId), + eq(companyMemberships.principalType, "user"), + eq(companyMemberships.principalId, ownerUserId), + eq(companyMemberships.status, "active"), + )) + .for("update") + .limit(1); + if (!membership) { + throw forbidden("Only an active company member can delegate their personal grant"); + } + + const [grant] = await tx.select().from(connectionGrants).where(and( + eq(connectionGrants.id, grantId), + eq(connectionGrants.companyId, connection.companyId), + eq(connectionGrants.connectionId, connection.id), + eq(connectionGrants.kind, "user"), + eq(connectionGrants.subjectUserId, ownerUserId), + eq(connectionGrants.status, "active"), + )).limit(1); + if (!grant) throw forbidden("Only the active personal grant owner can create a delegation"); + const [targetAgent] = await tx.select({ id: agents.id }).from(agents).where(and( + eq(agents.id, agentId), + eq(agents.companyId, connection.companyId), + )).limit(1); + if (!targetAgent) throw notFound("Agent not found"); + const [existing] = await tx.select().from(connectionGrantDelegations).where(and( + eq(connectionGrantDelegations.grantId, grant.id), + eq(connectionGrantDelegations.agentId, agentId), + )).limit(1); + if (existing) return existing; + const [delegation] = await tx.insert(connectionGrantDelegations).values({ + companyId: connection.companyId, + grantId: grant.id, + agentId, + createdByUserId: ownerUserId, + }).returning(); + await tx.insert(toolAccessAuditEvents).values({ + companyId: connection.companyId, + connectionId: connection.id, + actorType: "user", + actorId: ownerUserId, + action: "connection_grant.delegated", + outcome: "success", + reasonCode: "delegation_created", + details: { grantId: grant.id, delegationId: delegation!.id, agentId }, + }); + return delegation!; }); - return delegation!; }, revokeConnectionGrantDelegation: async (idOrUid: string, grantId: string, delegationId: string, actor?: ActorInfo) => {