mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-06 05:56:19 +01:00
fix: separate fresh setup from archived personal connections (#18902)
## Thinking Path
> - Paperclip helps people manage AI agents and their service access.
> - Each personal connection belongs to one user.
> - Setup can recover an archived connection by its name and provider.
> - A fresh setup can therefore select another user's old identity and
fail its owner check.
> - This change creates a separate connection for fresh setup after
personal archival.
> - Explicit reconnect still requires the existing owner and preserves
its history.
## Linked Issues or Issue Description
**What happened?**
A fresh GitHub setup can return 403 after a personal connection with the
same name was archived. The request does not include a reconnect target.
Setup still selects the archived identity by name and provider.
**Expected behavior**
Fresh setup creates a separate personal identity. It leaves the archived
connection, credentials, grants, and activity unchanged. Explicit
reconnect rejects a different user.
**Steps to reproduce**
1. Create a personal GitHub connection in a test company.
2. Archive that connection and its application.
3. As another authenticated member, start fresh personal setup with the
same name and provider.
4. Observe the owner-check failure before this change.
**Paperclip version or commit**
Base: db9be4c5a. Tests use isolated database and HTTP fixtures.
**Deployment mode**
Authenticated. The legacy owner can be a local board identity.
## What Changed
- Exclude implicitly recovered archived personal connections from fresh
setup.
- Use the existing company-scoped name allocator to create a separate
application and connection.
- Test three owners: a legacy board identity, another user, and the
current user.
- Check credential isolation, unchanged archived records and activity,
missing-key rejection, and explicit reconnect denial.
- Keep managed revival and generic MCP rollback/reconnect fixtures on
explicit reconnect targets.
- Verify generic personal URL setup creates an independent identity and
preserves the archived records.
- Document fresh setup and reconnect behavior.
## Verification
- Final head: `96184e3179aab2eeb88df87d442691741a4875a2`. Full CI
recovered and completed successfully: [run 41752164958, attempt
2](https://github.com/paperclipai/paperclip/actions/runs/41752164958/attempts/2).
PR rollup: 53 successful checks, 3 skipped, none failing or pending.
Typecheck, build, all server shards, Runner lanes, and E2E gates pass.
- Exact-head Greptile: 5/5, zero inline comments or review threads.
Independent engineering/security reviews approved the production
implementation; engineering approved the test-only follow-up. Browser
acceptance review approved with the timing qualification below.
- Final-head local generic MCP suite: 84/84 passed (23.26 s); server
typecheck passed. Reviewer independently reran the three changed
scenarios: 3/3 passed (15.74 s).
- Production head `587631230`: local tool-access suite 504/504,
recursive typecheck, and full build passed. Broad local checks were not
repeated for the test-only delta. Full local `pnpm test:run` remains
incomplete after interruption during the first chat shard; no
full-local-suite pass is claimed.
- Initial full CI at `587631230` failed three generic MCP fixtures that
still relied on implicit archived personal recovery. All three
reproduced locally (15.01 s). The test-only follow-up gives owner
reconnect/rollback scenarios explicit targets and verifies independent
fresh URL setup without relaxing their assertions. Corrected server
shard 12/12 passes on final head. The unfinished earlier run was
cancelled after the follow-up push.
- Initial current-head E2E shard 3/8 failed a 5000 ms task-heading wait
at `tests/e2e/cloud-auth.spec.ts:46` (40 passed, one failed). Its
screenshot/accessibility snapshot contains the expected heading. The
single authorized no-code rerun passed 41/41 with zero test retries,
including all URL, handoff, and login-form checks. The first-heading
assertion took 3774 ms on rerun. Both attempts tested the same merge
commit for this PR head. These observations support a timing concern;
the original cause remains unknown. The unchanged Cloud-auth path does
not invoke archived-connection setup. Recovery is established; an
infrastructure cause is not. If it recurs, capture an instrumented
first-navigation trace and compare an unchanged baseline under
comparable conditions.
- PR remains draft. Merge and deployment are outside this request.
Synthetic provider/browser fixtures do not prove live GitHub
authorization or a deployed fix.
## Risks
- Fresh setup after personal archival now creates a new ID and a
suffixed name, including for the same owner. Clients must supply an
explicit target to reconnect the old identity.
- The guard applies to all personal gallery connections, not only
GitHub.
- No schema migration is required. Explicit owner checks remain in
place.
- Security review approved the unchanged production implementation.
Merge and deployment are outside this change request.
## Model Used
OpenAI GPT-6 through Codex. The runtime does not expose a more specific
model snapshot or context window. Used reasoning, code editing, and
shell tools.
## 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:
1 parent
e2dc2cc0be
commit
58d1b21098
4 files changed
+171
-16
No files matched your search
@@ -77,6 +77,12 @@ repeat an existing instance approval.
|
||||
|
||||
## Identity resolution
|
||||
|
||||
Fresh setup does not reuse an archived personal connection found by name and
|
||||
provider. It creates a separate application and connection with an available
|
||||
name, such as `GitHub (2)`. The archived identity, credentials, grants, and
|
||||
history stay unchanged. An explicit reconnect or application-targeted setup
|
||||
continues the existing identity and requires its personal owner.
|
||||
|
||||
Every MCP call, `gh` invocation, native Git operation, checkout, health check,
|
||||
and webhook binding uses the same order:
|
||||
|
||||
|
||||
@@ -1445,7 +1445,7 @@ describeEmbeddedPostgres("generic remote MCP connections", () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not let another user take over an archived personal URL connection", async () => {
|
||||
it("creates a separate fresh personal URL connection and keeps explicit reconnect owner-bound", async () => {
|
||||
const fixture = installMcpOAuthFixture({ auth: "oauth" });
|
||||
const company = await createCompany(db);
|
||||
const service = toolAccessService(db);
|
||||
@@ -1456,18 +1456,36 @@ describeEmbeddedPostgres("generic remote MCP connections", () => {
|
||||
await db.insert(companyMemberships).values({
|
||||
companyId: company.id, principalType: "user", principalId: "other-user", status: "active", membershipRole: "admin",
|
||||
});
|
||||
const archivedConnection = await db.select().from(toolConnections).where(eq(toolConnections.id, first.connectionId));
|
||||
const archivedApplication = await db.select().from(toolApplications).where(eq(toolApplications.id, first.application.id));
|
||||
fixture.fetchMock.mockClear();
|
||||
|
||||
await expect(service.connectGalleryApp(company.id, input, {
|
||||
actorType: "user", actorId: "other-user",
|
||||
})).rejects.toMatchObject({ status: 403, message: "Only the existing personal identity can reconnect this connection" });
|
||||
for (const target of [{ reconnectConnectionId: first.connectionId }, { applicationId: first.application.id }]) {
|
||||
await expect(service.connectGalleryApp(company.id, { ...input, ...target }, {
|
||||
actorType: "user", actorId: "other-user",
|
||||
})).rejects.toMatchObject({ status: 403, message: "Only the existing personal identity can reconnect this connection" });
|
||||
}
|
||||
|
||||
expect(fixture.fetchMock).not.toHaveBeenCalled();
|
||||
const fresh = await service.connectGalleryApp(company.id, input, { actorType: "user", actorId: "other-user" });
|
||||
expect(fresh.connectionId).not.toBe(first.connectionId);
|
||||
expect(fresh.application.id).not.toBe(first.application.id);
|
||||
expect(fresh.auth).toMatchObject({ kind: "oauth" });
|
||||
await expect(service.getConnection(fresh.connectionId)).resolves.toMatchObject({
|
||||
createdByUserId: "other-user", credentialPolicy: "per_user", credentialSecretRefs: [],
|
||||
});
|
||||
await expect(db.select().from(toolConnections).where(eq(toolConnections.id, first.connectionId)))
|
||||
.resolves.toEqual(archivedConnection);
|
||||
await expect(db.select().from(toolApplications).where(eq(toolApplications.id, first.application.id)))
|
||||
.resolves.toEqual(archivedApplication);
|
||||
// OAuth grants are created only after authorization; discovery is not a grant.
|
||||
await expect(db.select().from(connectionGrants)).resolves.toHaveLength(0);
|
||||
await expect(service.getConnection(first.connectionId)).resolves.toMatchObject({
|
||||
status: "archived", createdByUserId: "board-user",
|
||||
});
|
||||
const resumed = await service.connectGalleryApp(company.id, input, { actorType: "user", actorId: "board-user" });
|
||||
const resumed = await service.connectGalleryApp(company.id, {
|
||||
...input, reconnectConnectionId: first.connectionId,
|
||||
}, { actorType: "user", actorId: "board-user" });
|
||||
expect(resumed.connectionId).toBe(first.connectionId);
|
||||
expect(resumed.auth).toMatchObject({ kind: "oauth" });
|
||||
});
|
||||
@@ -1544,7 +1562,7 @@ describeEmbeddedPostgres("generic remote MCP connections", () => {
|
||||
return jsonResponse({ jsonrpc: "2.0", id: "paperclip-catalog-refresh", result: { tools: FIXTURE_TOOLS } });
|
||||
});
|
||||
const failure = service.connectGalleryApp(company.id, {
|
||||
link: MCP_URL, name: "Personal retry rollback", grantKind: "user",
|
||||
link: MCP_URL, name: "Personal retry rollback", grantKind: "user", reconnectConnectionId: first.connectionId,
|
||||
}, actor).then(() => null, (error: unknown) => error);
|
||||
await Promise.race([catalogPending, failure.then((error) => { throw error ?? new Error("Retry finished before the catalog probe"); })]);
|
||||
try {
|
||||
@@ -1587,7 +1605,9 @@ describeEmbeddedPostgres("generic remote MCP connections", () => {
|
||||
FOR EACH ROW EXECUTE FUNCTION test_personal_profile_failure()
|
||||
`);
|
||||
try {
|
||||
await expect(service.connectGalleryApp(company.id, input, actor)).rejects.toThrow();
|
||||
await expect(service.connectGalleryApp(company.id, {
|
||||
...input, reconnectConnectionId: first.connectionId,
|
||||
}, actor)).rejects.toThrow();
|
||||
await expect(db.select().from(toolCatalogEntries).where(eq(toolCatalogEntries.connectionId, first.connectionId)))
|
||||
.resolves.toEqual(catalogBefore);
|
||||
await expect(db.select().from(toolProfiles).where(eq(toolProfiles.companyId, company.id))).resolves.toHaveLength(0);
|
||||
|
||||
@@ -9795,6 +9795,7 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
const revived = await service.connectGalleryApp(
|
||||
company.id,
|
||||
{
|
||||
reconnectConnectionId: first.connectionId,
|
||||
galleryKey: "google-drive",
|
||||
connectionMethodKey: "paperclip-write",
|
||||
grantKind: "user",
|
||||
@@ -15997,7 +15998,7 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
).rejects.toMatchObject({ status: 404 });
|
||||
});
|
||||
|
||||
it("reuses a removed gallery app while applying the omitted organization identity default", async () => {
|
||||
it("creates a separate gallery app for the omitted organization default after personal removal", async () => {
|
||||
const company = await createCompany(db);
|
||||
const service = createTestToolAccessService(db);
|
||||
const actor = { actorType: "user" as const, actorId: "local-board" };
|
||||
@@ -16022,8 +16023,10 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
actor,
|
||||
);
|
||||
|
||||
expect(second.application.id).toBe(first.application.id);
|
||||
expect(second.connectionId).toBe(first.connectionId);
|
||||
expect(second.application.id).not.toBe(first.application.id);
|
||||
expect(second.connectionId).not.toBe(first.connectionId);
|
||||
await expect(service.getConnection(first.connectionId, company.id))
|
||||
.resolves.toMatchObject({ status: "archived", credentialPolicy: "per_user" });
|
||||
expect(second.application.status).toBe("draft");
|
||||
expect(second.connection.status).toBe("draft");
|
||||
expect(second.connection.credentialPolicy).toBe("shared");
|
||||
@@ -16038,8 +16041,8 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
isDefault: true,
|
||||
}),
|
||||
]);
|
||||
await expect(db.select().from(toolApplications)).resolves.toHaveLength(1);
|
||||
await expect(db.select().from(toolConnections)).resolves.toHaveLength(1);
|
||||
await expect(db.select().from(toolApplications)).resolves.toHaveLength(2);
|
||||
await expect(db.select().from(toolConnections)).resolves.toHaveLength(2);
|
||||
});
|
||||
|
||||
it("restores grants and credential policy when an identity-changing revival fails", async () => {
|
||||
@@ -18414,6 +18417,119 @@ describeEmbeddedPostgres("tool access service", () => {
|
||||
});
|
||||
}, 15000);
|
||||
|
||||
it.each(["local-board", "another-user", "current-user"])(
|
||||
"creates an independent fresh GitHub identity after archiving a personal connection owned by %s",
|
||||
async (ownerUserId) => {
|
||||
const company = await createCompany(db);
|
||||
const service = createTestToolAccessService(db);
|
||||
const currentUserId = "current-user";
|
||||
await grantBoardUser(db, company.id, currentUserId, [], "member");
|
||||
mockToolsList([readTool("get_me", "Read current user")]);
|
||||
await withGalleryServerUrl("github", PUBLIC_MCP_FIXTURE_URL, async () => {
|
||||
const first = await service.connectGalleryApp(company.id, {
|
||||
galleryKey: "github",
|
||||
connectionMethodKey: "mcp-key",
|
||||
name: "GitHub",
|
||||
grantKind: "user",
|
||||
credentialValues: { "credentials.authorization": "archived-fixture-key" },
|
||||
}, { actorType: "user", actorId: ownerUserId });
|
||||
// Model a legacy archived row that still has personal credential refs.
|
||||
// Fresh setup must leave even this retained material untouched.
|
||||
await db.update(toolConnections).set({ status: "archived", enabled: false })
|
||||
.where(eq(toolConnections.id, first.connectionId));
|
||||
await db.update(toolApplications).set({ status: "archived", archivedAt: new Date() })
|
||||
.where(eq(toolApplications.id, first.application.id));
|
||||
await db.insert(activityLog).values({
|
||||
companyId: company.id, actorType: "user", actorId: ownerUserId,
|
||||
action: "tool_connection.archived", entityType: "tool_connection",
|
||||
entityId: first.connectionId,
|
||||
});
|
||||
const retained = async () => ({
|
||||
applications: await db.select().from(toolApplications)
|
||||
.where(eq(toolApplications.id, first.application.id)),
|
||||
connections: await db.select().from(toolConnections)
|
||||
.where(eq(toolConnections.id, first.connectionId)),
|
||||
grants: await db.select().from(connectionGrants)
|
||||
.where(eq(connectionGrants.connectionId, first.connectionId)),
|
||||
secrets: await db.select().from(companySecrets),
|
||||
versions: await db.select().from(companySecretVersions),
|
||||
activity: await db.select().from(activityLog)
|
||||
.where(eq(activityLog.entityId, first.connectionId)),
|
||||
});
|
||||
const before = await retained();
|
||||
const app = createRouteApp(db,
|
||||
boardSessionActor(company.id, "member", currentUserId),
|
||||
undefined, { deploymentMode: "authenticated" });
|
||||
const freshInput = {
|
||||
galleryKey: "github",
|
||||
connectionMethodKey: "mcp-key",
|
||||
name: "GitHub",
|
||||
grantKind: "user",
|
||||
credentialValues: { "credentials.authorization": "fresh-fixture-key" },
|
||||
};
|
||||
|
||||
if (ownerUserId !== currentUserId) {
|
||||
for (const target of [
|
||||
{ reconnectConnectionId: first.connectionId },
|
||||
{ applicationId: first.application.id },
|
||||
]) {
|
||||
await request(app)
|
||||
.post(`/api/companies/${company.id}/tools/apps/connect`)
|
||||
.send({ ...freshInput, ...target }).expect(403);
|
||||
await expect(service.connectGalleryApp(company.id, {
|
||||
...freshInput, grantKind: "user", ...target,
|
||||
}, { actorType: "user", actorId: currentUserId, actorSource: "session" }))
|
||||
.rejects.toMatchObject({
|
||||
status: 403,
|
||||
message: "Only the existing personal identity can reconnect this connection",
|
||||
});
|
||||
}
|
||||
expect(await retained()).toEqual(before);
|
||||
}
|
||||
|
||||
// An omitted key must not borrow the archived personal credential.
|
||||
await request(app)
|
||||
.post(`/api/companies/${company.id}/tools/apps/connect`)
|
||||
.send({ ...freshInput, credentialValues: {} }).expect(400);
|
||||
expect(await retained()).toEqual(before);
|
||||
|
||||
const fresh = await request(app)
|
||||
.post(`/api/companies/${company.id}/tools/apps/connect`)
|
||||
.send(freshInput).expect(201);
|
||||
expect(fresh.body.connectionId).not.toBe(first.connectionId);
|
||||
expect(fresh.body.application.id).not.toBe(first.application.id);
|
||||
expect(fresh.body.application.name).toBe("GitHub (2)");
|
||||
expect(fresh.body.connection).toMatchObject({
|
||||
credentialPolicy: "per_user", credentialSecretRefs: [],
|
||||
});
|
||||
const after = await retained();
|
||||
expect(after.applications).toEqual(before.applications);
|
||||
expect(after.connections).toEqual(before.connections);
|
||||
expect(after.grants).toEqual(before.grants);
|
||||
expect(after.activity).toEqual(before.activity);
|
||||
for (const secret of before.secrets) expect(after.secrets).toContainEqual(secret);
|
||||
for (const version of before.versions) expect(after.versions).toContainEqual(version);
|
||||
const freshGrants = await db.select().from(connectionGrants)
|
||||
.where(eq(connectionGrants.connectionId, fresh.body.connectionId));
|
||||
expect(freshGrants).toEqual([expect.objectContaining({
|
||||
kind: "user", subjectUserId: currentUserId, status: "active",
|
||||
})]);
|
||||
const oldSecretIds = before.grants.flatMap((grant) =>
|
||||
grant.credentialSecretRefs.map((ref) => ref.secretId));
|
||||
expect(freshGrants[0]!.credentialSecretRefs).toHaveLength(1);
|
||||
expect(oldSecretIds).toHaveLength(1);
|
||||
const freshSecretId = freshGrants[0]!.credentialSecretRefs[0]!.secretId;
|
||||
expect(oldSecretIds).not.toContain(freshSecretId);
|
||||
expect(after.secrets.find((secret) => secret.id === freshSecretId))
|
||||
.toMatchObject({ scope: "user", ownerUserId: currentUserId });
|
||||
await expect(db.select().from(activityLog)
|
||||
.where(and(eq(activityLog.entityId, fresh.body.connectionId),
|
||||
eq(activityLog.action, "tool_app.connected"))))
|
||||
.resolves.toEqual([expect.objectContaining({ actorId: currentUserId })]);
|
||||
}, "mcp-key");
|
||||
},
|
||||
);
|
||||
|
||||
it("reconnects a personal key on the existing user grant without creating an organization credential", async () => {
|
||||
const company = await createCompany(db);
|
||||
const service = createTestToolAccessService(db);
|
||||
|
||||
@@ -1692,8 +1692,8 @@ export function toolAccessService(
|
||||
// application/connection rows. A fresh gallery click has no
|
||||
// applicationId, so recover that retained identity by its company-unique
|
||||
// name and source instead of inserting a duplicate application that the
|
||||
// unique index rejects. Active applications are excluded: connecting a
|
||||
// second account still requires its own name/application identity.
|
||||
// unique index rejects. Archived personal identities are excluded below.
|
||||
// Active applications also require a separate application identity.
|
||||
const requestedName =
|
||||
input.name ?? galleryEntry?.name ?? defaultLinkName(input.link ?? "");
|
||||
const recoverableApplicationStatuses = galleryEntry
|
||||
@@ -1841,6 +1841,19 @@ export function toolAccessService(
|
||||
retainedConnection = undefined;
|
||||
existingApplication = null;
|
||||
}
|
||||
// A fresh setup must not revive a personal identity found only by name.
|
||||
// Keep its application, grants, credentials, and history archived, and let
|
||||
// normal name allocation create a separate application/connection below.
|
||||
// Explicit reconnects still use the fixed personal-owner check.
|
||||
if (
|
||||
!requestedConnectionId &&
|
||||
!input.applicationId &&
|
||||
retainedConnection?.status === "archived" &&
|
||||
retainedConnection.credentialPolicy === "per_user"
|
||||
) {
|
||||
existingApplication = null;
|
||||
retainedConnection = undefined;
|
||||
}
|
||||
let applicationName = existingApplication?.name ?? requestedName;
|
||||
let name = retainedConnection?.name ?? requestedName;
|
||||
if (!existingApplication) {
|
||||
@@ -1876,8 +1889,8 @@ export function toolAccessService(
|
||||
: "organization"
|
||||
: null;
|
||||
// An explicit resume/application reconnect continues the retained identity.
|
||||
// A fresh gallery connect may still reuse an archived row for stable history
|
||||
// and company-unique naming, but an explicit Access choice is a new identity
|
||||
// A fresh gallery connect may still reuse an archived non-personal row for
|
||||
// stable history, but an explicit Access choice is a new identity
|
||||
// decision and must replace the archived policy. Without this distinction,
|
||||
// removing a dedicated-agent connection and reconnecting the default personal
|
||||
// account leaves `per_agent` behind and the OAuth callback cannot persist its
|
||||
|
||||
Reference in new issue
Block a user