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 <noreply@paperclip.ing>
This commit is contained in:
DottaandPaperclip authored and GitHub committed 2026-10-27 12:25:01 -05:00
1 parent f6ba9afab0
commit 6e781e0ab2
5 files changed
+257 -2

No files matched your search

+11
View File
@@ -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
+5 -1
View File
@@ -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`
+14
View File
@@ -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
@@ -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<ReturnType<typeof startEmbeddedPostgresTestDatabase>>;
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 }),
]);
});
});
@@ -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({