diff --git a/doc/connections/GITHUB.md b/doc/connections/GITHUB.md index 0f93999ab86..96826dfddfb 100644 --- a/doc/connections/GITHUB.md +++ b/doc/connections/GITHUB.md @@ -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: diff --git a/server/src/__tests__/generic-mcp-connection.test.ts b/server/src/__tests__/generic-mcp-connection.test.ts index 2978db9bbe1..78bf314dc4f 100644 --- a/server/src/__tests__/generic-mcp-connection.test.ts +++ b/server/src/__tests__/generic-mcp-connection.test.ts @@ -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); diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index 5839a505ec2..166ec8f212a 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -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); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index 3e3f6292565..5b61834c94c 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -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