From 6e781e0ab20cdaeb6570cebdb5f14dbb8557ba04 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Tue, 27 Oct 2026 12:25:01 -0500 Subject: [PATCH] fix(connections): preserve removed grants when saving installs (#17906) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Connections give agents access to external tools through access profiles. > - Operators can remove a broad app profile and keep a limited profile. > - Saving a connection's agent list restored the broad profile to existing installation targets. > - Those restored grants could permit write tools that the limited profile omitted. > - This pull request grants the app profile only to new installation targets. > - The benefit is stable permission choices when an operator edits an installation list. ## Linked Issues or Issue Description **What happened?** `PUT /api/tool-connections/{connectionId}/installs` attached the app profile to every target in the submitted list. An existing target received the profile again if an operator had removed it. Adding an agent, removing another agent, or saving the same list could therefore restore broad tool permissions. The activity log recorded each restored binding as `tool_connection.install_access_extended`. **Expected behavior** Targets that remain installed keep their saved profile assignments. A removed app-profile assignment stays removed. New installation targets receive the current app-profile defaults. Removing an installation still removes its install-derived binding. **Steps to reproduce** 1. Install a connection with read and write tools on two agents. 2. Give one agent a read-only profile and remove its broad app-profile assignment. 3. Save the installation list, add an agent, or remove the other agent. 4. Check the first agent's permission to use the write tool. Before this change, the write tool becomes allowed again. After this change, it stays denied. Related: #17422 adds an agent-edit permission check to direct profile bind and unbind operations. This PR preserves removed assignments during installation-list updates. The duplicate search found no open PR for this defect. ## What Changed - Attach app-profile bindings only for new installation targets. - Preserve removed grants for existing agent and company installations. - Keep legacy connection-wide include cleanup on every non-empty installation save, including saves with no added targets. - Add six PostgreSQL regressions. All six use the real install HTTP route with a local operator fixture. - Check effective read/write decisions, saved assignments, new-agent defaults, uninstall cleanup, legacy-rule cleanup, and access-extension audit records. - Document the behavior in the connector runbook and both access maps. ## Verification - Red: all four new regressions fail before the fix. The policy returns `allow_profile` where the tests require `deny_default`. - Review regression: the two legacy-rule cases fail before the review correction. The original four cases still pass. - Green after review: all six regressions and five existing profile-precedence tests pass after the correction. - Command: `pnpm exec vitest run server/src/__tests__/tool-connection-installs.test.ts server/src/services/tool-profile-binding-precedence.test.ts`. - The local regression used `PAPERCLIP_TOOL_ACCESS_TEST_DATABASE_URL` with a separate, fully migrated PostgreSQL 17 test database. Embedded PostgreSQL could not start because the host had reached its semaphore limit. Skipped tests were not counted as a regression pass. - `pnpm -r typecheck` and `pnpm build` passed for the initial fix. Server `tsc --noEmit` also passes after the review correction. - `node scripts/check-maps.mjs`, `node scripts/check-module-boundaries.mjs`, and `git diff --check` pass. - `pnpm test:run` was started, then stopped after 16 tests in the native-session suite failed during embedded PostgreSQL startup. A focused rerun of its `collect` case confirmed the database startup error. Direct `initdb` output reported `could not create semaphores: No space left on device`. The full local suite is incomplete. All six new regressions executed against the separate test database and passed. - [Initial-fix CI](https://github.com/paperclipai/paperclip/actions/runs/40541191695) passed at `23253ee332b90f142e48df3f553aeaaa7f6fa298`. Greptile gave that commit 5/5. Ponytail found the legacy cleanup gap; this revision restores that cleanup and adds both regression cases. - [Current-commit CI](https://github.com/paperclipai/paperclip/actions/runs/40547366181) passes for `729c487cf2ffaae261a820de0cf9a033a0365fd4`, including type checks, build, test shards, and both `ci / verify` and `ci / e2e` aggregate checks. - The first CI attempt hit a 30-second module-import timeout in the unchanged `issue-agent-mutation-ownership-routes.test.ts` suite. All 179 tests passed locally. One [job retry](https://github.com/paperclipai/paperclip/actions/runs/40547366181/job/122349226221) passed without a code change. The cause of the import slowdown was not established. The failed attempt remains recorded in CI history. - Greptile and Ponytail both give the current commit 5/5 with no actionable findings. Ponytail confirms the legacy cleanup finding is fixed. The review thread is resolved, and the PR has no merge conflicts. Ponytail's advisory check has a neutral conclusion with a 5/5 assessment. ## Risks - New installations retain the current app-profile defaults. Removing and then reinstalling a target is a new installation. - A repeated installation save no longer restores a missing profile assignment. Operators must use an explicit profile assignment when they want to restore that grant. - Existing unwanted grants still require a separate access review and repair. This PR does not change stored grants, reconnect identity behavior, or the default access model. - No database migration or API schema change is required. ## Model Used - OpenAI GPT-6 in Codex, with reasoning, repository tools, and code execution. The session does not expose the exact routed model ID or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/connections/CONNECTOR-PLAYBOOK.md | 11 + maps/agent-access-map/app-permissions.md | 6 +- maps/feature-map/app-permissions.md | 14 ++ .../tool-connection-installs.test.ts | 226 ++++++++++++++++++ .../tool-access/connections/installs.ts | 2 +- 5 files changed, 257 insertions(+), 2 deletions(-) create mode 100644 server/src/__tests__/tool-connection-installs.test.ts diff --git a/doc/connections/CONNECTOR-PLAYBOOK.md b/doc/connections/CONNECTOR-PLAYBOOK.md index 9343d1441c0..b79df0be8be 100644 --- a/doc/connections/CONNECTOR-PLAYBOOK.md +++ b/doc/connections/CONNECTOR-PLAYBOOK.md @@ -764,6 +764,17 @@ IDs to an auditable profile, and leaves **Ask first** available for any action. The open default changes the initial policy; it does not create a route around a policy the operator has applied. +Saving a connection's installation list grants its app profile only to newly +added targets. Targets that remain installed keep their existing profile +assignments. If an operator removes an app profile from an installed agent or +company, a later installation-list save must preserve that removal. This also +applies when the list is unchanged or another agent is removed. Removing and +then reinstalling a target is a new installation and uses the current setup +defaults. +Every non-empty installation-list save also removes legacy connection-wide +include entries from the managed app profile. Keep this cleanup when no targets +are added, so the selected action entries remain authoritative. + If a destructive provider cannot be safe with those defaults, add a narrowly reviewed provider policy with tests. Do not hide a dangerous tool by misclassifying it as read, and do not silently change global defaults in a diff --git a/maps/agent-access-map/app-permissions.md b/maps/agent-access-map/app-permissions.md index 25258dcac9b..5d91fa39f81 100644 --- a/maps/agent-access-map/app-permissions.md +++ b/maps/agent-access-map/app-permissions.md @@ -94,8 +94,12 @@ Gaps: This individual operation has source review but no recorded HTTP verificat API: `present` — `PUT /api/tool-connections/{connectionId}/installs`; JSON REST. Agent access: `available` — agent REST and default native `search_api` / `call_api`. Policy: `tools:manage_profiles` and `tools:manage_connections`; [shared management policy](../../doc/plans/2026-10-13-connection-api-parity.md#authorization-contract). Resolved resources and every affected target are checked; personal ownership remains the responsible user. +Only new installation targets receive the app-profile binding. Existing targets +retain their saved profile assignments, including deliberately removed grants. +Non-empty saves still remove legacy connection-wide include entries from the +managed app profile when no new targets are added. Evidence: `source-only` — [route](../../server/src/routes/tool-access.ts); reviewed in the working tree based on the source commit above. -Verification: `not-run` — individual operation not executed in this audit; shared authorization and representative family flows are exercised by the authenticated integration suite. Follow the route schema, inspect persistence/audit, and assert owner, scope and revoked-authority denial. +Verification: `not-run` — individual agent-authenticated operation not executed in this audit; shared authorization and representative family flows are exercised by the authenticated integration suite. [Installation permission regressions](../../server/src/__tests__/tool-connection-installs.test.ts) exercise operator HTTP requests, persisted assignments, tool-policy decisions, and access-extension audit records with PostgreSQL. Those fixtures do not qualify agent authentication. Follow the route schema, inspect persistence/audit, and assert owner, scope and revoked-authority denial. Gaps: This individual operation has source review but no recorded HTTP verification. Provider availability and live setup remain unqualified. ## `action-policy` diff --git a/maps/feature-map/app-permissions.md b/maps/feature-map/app-permissions.md index 41da9f9496a..524cc9b4feb 100644 --- a/maps/feature-map/app-permissions.md +++ b/maps/feature-map/app-permissions.md @@ -20,6 +20,12 @@ Agent access: [companion](../agent-access-map/app-permissions.md). - `test-action`: `app-permissions/test-action` — choose an actor and inputs and inspect a real action result. - `account-maintenance`: `app-permissions/account-maintenance` — inspect accounts, reconnect/revoke, and review connection activity. +Changing the connection's agent list preserves profile assignments for agents +that remain installed. Saving the list must not restore a removed app-profile +assignment. New installations retain the current app-profile defaults. +Non-empty saves also remove legacy connection-wide include rules, including +unchanged lists and removals with no new targets. + ## How to get to it (user POV) ### `permissions` @@ -45,6 +51,14 @@ Preconditions: follow the [baseline](./README.md#before-driving-a-journey). Prep Automated: [app detail](../../ui/src/pages/apps/AppDetail.test.tsx) covers agent/action controls, quarantined actions, and navigation using mocked requests. +[Installation permission regressions](../../server/src/__tests__/tool-connection-installs.test.ts) +exercise the real install API with a local operator and PostgreSQL. They check +that adding, removing, or retaining agents preserves removed app-profile grants +and denies write tools for agents with read-only profiles. They also check +company installations, new-agent defaults, legacy connection-wide include +cleanup, and access-extension audit records. +These fixtures do not qualify an agent-authenticated request or a live provider. + [Runtime assignment tests](../../server/src/__tests__/agent-runtime-assignments.test.ts) use PostgreSQL to verify committed assignment notifications, bounded snapshots, rollback, and concurrent AI adoption. The owning lifecycle plugin can read diff --git a/server/src/__tests__/tool-connection-installs.test.ts b/server/src/__tests__/tool-connection-installs.test.ts new file mode 100644 index 00000000000..2791a7a8174 --- /dev/null +++ b/server/src/__tests__/tool-connection-installs.test.ts @@ -0,0 +1,226 @@ +import { randomUUID } from "node:crypto"; +import express from "express"; +import request from "supertest"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { + activityLog, + agents, + companies, + closeRegisteredClients, + connectionGrants, + createDb, + toolApplications, + toolCatalogEntries, + toolConnections, + toolProfileBindings, + toolProfileEntries, + toolProfiles, + type Db, +} from "@paperclipai/db"; +import { and, eq } from "drizzle-orm"; +import { errorHandler } from "../middleware/index.js"; +import { toolAccessRoutes } from "../routes/tool-access.js"; +import { toolAccessPolicyService } from "../services/tool-access-policy.js"; +import { toolAccessService } from "../services/tool-access.js"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./helpers/embedded-postgres.js"; + +const externalDatabaseUrl = process.env.PAPERCLIP_TOOL_ACCESS_TEST_DATABASE_URL?.trim(); +const support = externalDatabaseUrl ? { supported: true } : await getEmbeddedPostgresTestSupport(); +const describeDatabase = support.supported ? describe : describe.skip; + +function createRouteApp(db: Db) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.actor = { + type: "board", + userId: "board-user", + isInstanceAdmin: true, + source: "local_implicit", + }; + next(); + }); + app.use("/api", toolAccessRoutes(db)); + app.use(errorHandler); + return app; +} + +describeDatabase("connection install permission preservation", () => { + let database: Awaited>; + let db: Db; + + beforeAll(async () => { + if (externalDatabaseUrl) { + db = createDb(externalDatabaseUrl); + return; + } + database = await startEmbeddedPostgresTestDatabase("paperclip-install-permissions-"); + db = createDb(database.connectionString); + }, 90_000); + + afterAll(async () => { + if (externalDatabaseUrl) await closeRegisteredClients(externalDatabaseUrl); + await database?.cleanup(); + }); + + async function fixture() { + const [company] = await db.insert(companies).values({ + name: "Install permission fixture", + issuePrefix: `IP${randomUUID().slice(0, 6)}`, + }).returning(); + const companyId = company.id; + const agentIds = [randomUUID(), randomUUID(), randomUUID(), randomUUID()]; + await db.insert(agents).values(agentIds.map(id => ({ + id, companyId, name: id, status: "active", adapterType: "process", + }))); + const [application] = await db.insert(toolApplications).values({ + companyId, name: "Fixture app", type: "mcp_http", + }).returning(); + const [connection] = await db.insert(toolConnections).values({ + companyId, applicationId: application.id, name: "Fixture connection", + uid: randomUUID(), transport: "mcp_remote", status: "active", enabled: true, + }).returning(); + await db.insert(connectionGrants).values({ + companyId, connectionId: connection.id, kind: "organization", status: "active", isDefault: true, + }); + const catalog = await db.insert(toolCatalogEntries).values([ + { name: "read_email", riskLevel: "read" as const, isReadOnly: true, isWrite: false }, + { name: "send_email", riskLevel: "write" as const, isReadOnly: false, isWrite: true }, + ].map(tool => ({ + ...tool, toolName: tool.name, companyId, applicationId: application.id, + connectionId: connection.id, versionHash: "fixture-v1", + }))).returning(); + const [appProfile] = await db.insert(toolProfiles).values({ + companyId, profileKey: `app:${connection.id}`, name: "Fixture app access", + defaultAction: "deny", metadata: { source: "app_gallery_finish", connectionId: connection.id }, + }).returning(); + await db.insert(toolProfileEntries).values(catalog.map(entry => ({ + companyId, profileId: appProfile.id, selectorType: "catalog_entry" as const, + catalogEntryId: entry.id, connectionId: connection.id, + }))); + const [limitedProfile] = await db.insert(toolProfiles).values({ + companyId, profileKey: "read-only", name: "Read email only", defaultAction: "deny", + }).returning(); + await db.insert(toolProfileEntries).values({ + companyId, profileId: limitedProfile.id, selectorType: "catalog_entry", catalogEntryId: catalog[0].id, + }); + const app = createRouteApp(db); + const service = toolAccessService(db); + const policy = toolAccessPolicyService(db); + const putTargets = (installs: Array<{ targetType: "agent" | "company"; targetId: string }>) => request(app) + .put(`/api/tool-connections/${connection.id}/installs`) + .send({ installs }) + .expect(response => expect(response.status, JSON.stringify(response.body)).toBe(200)); + const putInstalls = (ids: string[]) => putTargets(ids.map(targetId => ({ targetType: "agent", targetId }))); + const appBindings = () => db.select().from(toolProfileBindings) + .where(and(eq(toolProfileBindings.companyId, companyId), eq(toolProfileBindings.profileId, appProfile.id))); + const extensions = () => db.select().from(activityLog).where(and( + eq(activityLog.companyId, companyId), eq(activityLog.action, "tool_connection.install_access_extended"), + )); + const decide = (agentId: string, toolName: string) => policy.decide({ + companyId, actor: { actorType: "agent", actorId: agentId, agentId }, + request: { + connectionId: connection.id, catalogEntryId: catalog.find(entry => entry.toolName === toolName)!.id, + toolName, arguments: {}, + }, + }); + return { companyId, agentIds, connection, catalog, appProfile, limitedProfile, service, putTargets, putInstalls, appBindings, extensions, decide }; + } + + it.each(["adding an agent", "removing an agent", "saving the same list"])( + "preserves removed app grants when %s", + async change => { + const f = await fixture(); + const [first, second, fullAccess, added] = f.agentIds; + await f.putInstalls([first, second, fullAccess]); + for (const agentId of [first, second]) { + await expect(f.decide(agentId, "send_email")).resolves.toMatchObject({ decision: "allow" }); + await f.service.bindProfile(f.limitedProfile.id, { targetType: "agent", targetId: agentId }); + await f.service.unbindProfile(f.appProfile.id, { targetType: "agent", targetId: agentId }); + await expect(f.decide(agentId, "read_email")).resolves.toMatchObject({ decision: "allow" }); + await expect(f.decide(agentId, "send_email")).resolves.toMatchObject({ decision: "deny", reasonCode: "deny_default" }); + } + const priorBindings = await f.appBindings(); + const priorEvents = await f.extensions(); + const desired = change === "adding an agent" ? [first, second, fullAccess, added] + : change === "removing an agent" ? [first, second] : [first, second, fullAccess]; + + await f.putInstalls(desired); + + for (const agentId of [first, second]) { + await expect(f.decide(agentId, "read_email")).resolves.toMatchObject({ decision: "allow" }); + await expect(f.decide(agentId, "send_email")).resolves.toMatchObject({ decision: "deny", reasonCode: "deny_default" }); + const effective = await f.service.getEffectiveProfilesForAgent(f.companyId, agentId); + expect(effective.installedConnections.map(connection => connection.id)).toContain(f.connection.id); + } + const bindings = await f.appBindings(); + expect(bindings.map(binding => binding.targetId).sort()).toEqual( + desired.filter(id => id !== first && id !== second).sort(), + ); + expect((await f.service.listConnectionInstalls(f.connection.id)).map(install => install.targetId).sort()) + .toEqual([...desired].sort()); + const newEvents = (await f.extensions()).filter(event => !priorEvents.some(prior => prior.id === event.id)); + expect(newEvents.map(event => event.details?.targetId)).toEqual(change === "adding an agent" ? [added] : []); + if (change !== "removing an agent") { + expect(bindings.find(binding => binding.targetId === fullAccess)).toEqual(priorBindings[0]); + await expect(f.decide(fullAccess, "send_email")).resolves.toMatchObject({ decision: "allow" }); + } + if (change === "adding an agent") { + await expect(f.decide(added, "send_email")).resolves.toMatchObject({ decision: "allow", reasonCode: "allow_profile" }); + } + }, + ); + + it.each(["saving the same list", "removing an agent"])( + "removes legacy connection-wide includes when %s", + async change => { + const f = await fixture(); + const [first, second] = f.agentIds; + await f.putInstalls([first, second]); + const writeEntry = f.catalog.find(entry => entry.toolName === "send_email")!; + await db.delete(toolProfileEntries).where(and( + eq(toolProfileEntries.profileId, f.appProfile.id), eq(toolProfileEntries.catalogEntryId, writeEntry.id), + )); + await db.insert(toolProfileEntries).values({ + companyId: f.companyId, profileId: f.appProfile.id, selectorType: "connection", + effect: "include", connectionId: f.connection.id, + }); + await expect(f.decide(first, "send_email")).resolves.toMatchObject({ decision: "allow", reasonCode: "allow_profile" }); + const priorBindings = await f.appBindings(); + const priorEvents = await f.extensions(); + const desired = change === "removing an agent" ? [first] : [first, second]; + + await f.putInstalls(desired); + + await expect(f.decide(first, "read_email")).resolves.toMatchObject({ decision: "allow" }); + await expect(f.decide(first, "send_email")).resolves.toMatchObject({ decision: "deny", reasonCode: "deny_default" }); + expect(await db.select().from(toolProfileEntries).where(and( + eq(toolProfileEntries.profileId, f.appProfile.id), eq(toolProfileEntries.selectorType, "connection"), + ))).toEqual([]); + expect((await f.appBindings()).sort((a, b) => a.targetId.localeCompare(b.targetId))).toEqual( + priorBindings.filter(binding => desired.includes(binding.targetId)).sort((a, b) => a.targetId.localeCompare(b.targetId)), + ); + expect(await f.extensions()).toEqual(priorEvents); + }, + ); + + it("preserves a removed company app grant when saving a company install", async () => { + const f = await fixture(); + const input = { installs: [{ targetType: "company" as const, targetId: f.companyId }] }; + await f.putTargets(input.installs); + await f.service.unbindProfile(f.appProfile.id, { targetType: "company", targetId: f.companyId }); + const priorEvents = await f.extensions(); + + await f.putTargets(input.installs); + + await expect(f.decide(f.agentIds[0], "send_email")).resolves.toMatchObject({ decision: "deny", reasonCode: "deny_default" }); + expect(await f.appBindings()).toEqual([]); + expect(await f.extensions()).toEqual(priorEvents); + expect(await f.service.listConnectionInstalls(f.connection.id)).toEqual([ + expect.objectContaining({ targetType: "company", targetId: f.companyId }), + ]); + }); +}); diff --git a/server/src/services/tool-access/connections/installs.ts b/server/src/services/tool-access/connections/installs.ts index e16cda72c2c..6e4459a4f37 100644 --- a/server/src/services/tool-access/connections/installs.ts +++ b/server/src/services/tool-access/connections/installs.ts @@ -275,7 +275,7 @@ export function createConnectionInstalls(db: Db, { now, getConnectionRow, assert } if (requested.size > 0 && connection.connectionPurpose !== "ai") { const profile = await appProfileForConnection(tx, connection); - for (const install of requested.values()) { + for (const install of additions) { const [binding] = await tx .insert(toolProfileBindings) .values({