Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 16 additions & 13 deletions server/src/plugins/plugin-store/connections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -405,12 +405,13 @@ export function connectionMethods(context: StoreContext) {
* and the filter keeps that a fact this query states rather than one it happens to rely on.
*/
/*
* NO PROVIDER PREDICATE HERE, DELIBERATELY, AND THAT IS NOT AN OVERSIGHT. Each row is resolved
* below at the ACTIVE provider's canonical url, so another provider's row would be silently
* dropped from the page. What keeps that unreachable is the startup activation refusal
* (`assertActivatable`, called from `server/src/index.ts`): it will not boot when the
* configured provider differs from the one on the existing rows. No flow relaxes that refusal
* today; whoever builds one must add the `brokerProvider` predicate here.
* SCOPED TO THE ACTIVE PROVIDER. The settings page draws rows the ACTIVE broker's
* canonical scheme resolves, so a row filed under another provider would be silently
* dropped from the page anyway; filtering at the read keeps that true out loud. It is
* unreachable today because the startup activation refusal (`assertActivatable`, called
* from `server/src/index.ts`) will not boot a deployment whose configured provider differs
* from the one on the existing rows — so this predicate costs nothing now, and is already
* right when any flow relaxes that refusal (#783).
*/
const connections = await database
.select({
Expand All @@ -423,6 +424,7 @@ export function connectionMethods(context: StoreContext) {
.from(brokeredConnections)
.where(
and(
eq(brokeredConnections.provider, brokerProvider),
eq(brokeredConnections.holder, "person"),
eq(brokeredConnections.userId, userId),
),
Expand Down Expand Up @@ -3261,19 +3263,20 @@ export function connectionMethods(context: StoreContext) {
* order.
*/
/*
* NO PROVIDER PREDICATE HERE, DELIBERATELY, AND THAT IS NOT AN OVERSIGHT. The sweep below
* revokes through the ACTIVE broker, so a row filed under another provider would be a
* revocation requested at the wrong vendor, which would not recognise the connection, and an
* audit row claiming `vendorRevocationRequested` about access that was never removed. What
* keeps that unreachable is the startup activation refusal (`assertActivatable`, called from
* `server/src/index.ts`): a deployment holding both providers' rows cannot boot. No flow relaxes
* that refusal today; whoever builds one must add the `brokerProvider` predicate here.
* THE ACTIVE BROKER'S ROWS ONLY. The sweep below revokes through the ACTIVE broker, so a
* row filed under another provider would be a revocation requested at a vendor that would
* not recognise the connection, and an audit row claiming `vendorRevocationRequested` about
* access that was never removed — the one reading of that field that must never be written.
* Unreachable today, because the startup activation refusal (`assertActivatable`, called
* from `server/src/index.ts`) will not boot a deployment holding both providers' rows; the
* predicate keeps the sweep correct the moment any flow relaxes that refusal (#783).
*/
const brokered = await database
.select({ toolkit: brokeredConnections.app })
.from(brokeredConnections)
.where(
and(
eq(brokeredConnections.provider, brokerProvider),
eq(brokeredConnections.holder, "person"),
eq(brokeredConnections.userId, userId),
),
Expand Down
65 changes: 65 additions & 0 deletions server/tests/plugin-store.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8008,6 +8008,71 @@ test("offboarding a brokered account nobody held any more withdraws nothing, and
});
});

/**
* The offboarding sweep must not touch another provider's rows.
*
* CRITERION. `retireConnectionsFor` on somebody holding a foreign-provider row revokes only the
* active broker's rows: the foreign row is not revoked, not deleted, and never audited as
* disconnected.
*
* REASON. The sweep revokes through the ACTIVE broker, so a foreign row would be a revocation
* requested at a vendor that would not recognise the connection, while the audit row claimed
* `vendorRevocationRequested` about access that was never removed — the one reading of that field
* that must never be written. Unreachable while `assertActivatable` refuses to boot a mixed
* deployment; this is what keeps the sweep correct when any flow relaxes that refusal (#783).
*
* THE FOREIGN ROW IS READ BACK, not inferred from the revoke count. An implementation that revoked
* it and deleted it also answers a smaller count once the sweep is otherwise correct, so only the
* standing row distinguishes "left alone" from "silently taken" — and the audit read beside it
* says the trail never claimed a withdrawal this deployment could not have performed.
*/
test("offboarding does not revoke or audit another provider's rows", async () => {
const { broker, order } = brokerSpy({ revoke: async () => true });
const { store, auditStore } = await freshStore({ broker });

// The active provider's own row, so the sweep still retires something.
await database.insert(brokeredConnections).values({
provider: "composio",
app: "gmail",
holder: "person",
userId: "user_leaver",
vendorUserId: "user_leaver",
});
// A row filed under the provider this deployment does not use.
await database.insert(brokeredConnections).values({
provider: "arcade",
app: "slack",
holder: "person",
userId: "user_leaver",
vendorUserId: "user_leaver",
});

expect(await store.retireConnectionsFor("user_leaver", "admin")).toEqual({
// One, because the arcade row is not this broker's to revoke.
retired: 1,
});
expect(order).toEqual(["revoke:user_leaver"]);

const standing = await database
.select({
provider: brokeredConnections.provider,
app: brokeredConnections.app,
})
.from(brokeredConnections)
.where(eq(brokeredConnections.userId, "user_leaver"));
expect(standing).toEqual([{ provider: "arcade", app: "slack" }]);

const disconnected = auditStore
.recorded()
.filter((event) => event.eventType === "mcp.account_disconnected");
expect(disconnected.map((event) => event.targetId)).toEqual(["gmail"]);
expect(
disconnected.every(
(event) => event.payload.vendorRevocationRequested === true,
),
).toBe(true);
});

/**
* The same false, on the other act that ends a brokered connection.
*
Expand Down