diff --git a/CHANGELOG.md b/CHANGELOG.md index 094b013c7..61d04f507 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### An offboarding one app refuses still records the apps that answered + +Removing somebody withdraws each brokered account they connected. When the broker refused one of +those apps, the whole act stopped before anything was recorded: apps already withdrawn at Composio +kept their `composio_connections` row and left nothing on the trail saying the account had ended, +and the retry that #574 made the recovery then asked again, was told there was nothing to withdraw, +and wrote `vendorRevocationRequested: false` about a withdrawal this deployment had asked for and +got. Each app is now asked, recorded with the answer it actually gave and its row removed, and only +the apps that were refused are left standing for the retry. The act still fails and still answers +500, so a refusal is as loud as it was. ### Paging the audit trail no longer skips rows written in the same millisecond `GET /api/admin/audit-events` hands out a `nextCursor` built from the last row's timestamp, which the diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index b96d4d60e..ecdc6d161 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -6691,22 +6691,65 @@ export function createPluginStore(options: PluginStoreOptions) { * rather than the call. False where there is no broker at all: a deployment whose key has * since been unset can still offboard somebody, and it could not have been calling Composio * either way — but nothing was asked there and the row must not claim otherwise. + * + * ONE APP'S REFUSAL IS ONE APP'S REFUSAL, and until now it was everybody's. A throw out of + * `revoke` left this loop before the delete and before the trail, so three accounts already + * withdrawn at Composio kept their rows and got no row on the trail. That was survivable while + * repeating the act did nothing — and #574 made repeating it the documented recovery, so the + * second pass asks again for those three, Composio answers `false` because the accounts are + * gone, and each writes `vendorRevocationRequested: false` about a withdrawal this deployment + * asked for and got. That field exists to tell an account we acted on from one that outlives + * us somewhere else; those three rows say the wrong one. + * + * So the answer is kept per app and the refusal is held rather than thrown. Every app is still + * asked — a later one is not punished for an earlier one — and the first refusal is rethrown + * below, so the act still fails loudly and the administrator still gets a 500. */ - const vendorRevocationRequested = new Map(); + const withdrawn: { toolkit: string; requested: boolean }[] = []; + const refusals: unknown[] = []; for (const connection of brokered) { - vendorRevocationRequested.set( - connection.toolkit, - broker - ? await broker.revoke({ userId, toolkit: connection.toolkit }) - : false, - ); + try { + withdrawn.push({ + toolkit: connection.toolkit, + requested: broker + ? await broker.revoke({ userId, toolkit: connection.toolkit }) + : false, + }); + } catch (error) { + /* + * Held, and the row deliberately left standing. + * + * "An offboarding the vendor refuses leaves the connection standing" is the existing + * criterion and it is unchanged: the row is the only thing naming which app this person + * connected, repeating the act is the recovery, and repeating it is only possible while + * the row is there. What changes is that the rule now applies to the app it is about + * rather than to every app in the same act. + */ + refusals.push(error); + } } - await database - .delete(composioConnections) - .where(eq(composioConnections.userId, userId)); + /* + * Only the apps that answered, which is the other half of the same correction. + * + * Deleting by user id would take the rows of apps that were refused or never reached, and + * those are exactly the rows the recovery needs. Deleting none — what a throw used to do — + * leaves a row and an open `(toolkit, user_id)` gate for an account that is already gone at + * Composio, so the table claims a connection this person does not have. + */ + if (withdrawn.length > 0) { + await database.delete(composioConnections).where( + and( + eq(composioConnections.userId, userId), + inArray( + composioConnections.toolkit, + withdrawn.map((entry) => entry.toolkit), + ), + ), + ); + } - for (const connection of brokered) { + for (const connection of withdrawn) { retired += 1; await recordAuditEvent(auditStore, { eventType: "mcp.account_disconnected", @@ -6729,12 +6772,21 @@ export function createPluginStore(options: PluginStoreOptions) { * ask. The value of the field is exactly that a reader can tell those apart, so a * constant here would be worse than none. */ - vendorRevocationRequested: - vendorRevocationRequested.get(connection.toolkit) ?? false, + vendorRevocationRequested: connection.requested, }, }); } + /* + * Loud, after every app has been asked and every answer recorded. + * + * The first, because the route turns this into a 500 and one sentence is what reaches the + * administrator; the rest are the same act failing more than once, and the trail above already + * says which apps did not end. Thrown last rather than first so a refusal on one app cannot + * cost the record of another — which is the whole of this change. + */ + if (refusals.length > 0) throw refusals[0]; + return { retired }; }, diff --git a/server/tests/composio-connections.test.ts b/server/tests/composio-connections.test.ts index 61e31bca9..f2f541558 100644 --- a/server/tests/composio-connections.test.ts +++ b/server/tests/composio-connections.test.ts @@ -1304,6 +1304,95 @@ test("an offboarding carries the vendor's answer per app, in a fixed order", asy ]); }); +/** + * ONE APP'S REFUSAL IS ONE APP'S REFUSAL, AND THE APPS THAT ANSWERED ARE RECORDED. + * + * CRITERION. Two apps, the vendor refusing the second: the act still fails, the refused app keeps + * its row and gets no trail row, and the app that WAS withdrawn loses its row and is recorded with + * `vendorRevocationRequested: true`. + * + * REASON. A throw out of `revoke` used to leave the loop before the delete and before the trail, so + * an account already gone at Composio kept its row and its `(toolkit, user_id)` gate and left + * nothing on the trail saying it had ended. That was survivable while repeating the act did + * nothing — and #574 made repeating it the documented recovery, so the second pass asks again for + * that app, the vendor answers `false` because the account is already gone, and the row that + * finally lands says `vendorRevocationRequested: false` about a withdrawal this deployment asked + * for and got. `vendorRevocationRequested` exists to tell an account we acted on from one that + * outlives us somewhere else; that row says the wrong one, permanently, and no later act corrects it. + * + * THE REFUSED APP IS UNCHANGED, which is the criterion the single-app test above already states: + * the row is the only thing naming which app this person connected, and repeating the act is only + * possible while it is there. What is new is that the rule now applies to the app it is about + * rather than to every app in the same act. + */ +test("an offboarding one app refuses still records the app that answered", async () => { + await seedApp({ connect: false }); + await database + .insert(composioConnections) + .values({ toolkit, userId: askerId }); + await database + .insert(composioConnections) + .values({ toolkit: secondToolkit, userId: askerId }); + vendorRefuses = ({ toolkit: asked }) => asked === secondToolkit; + + await expect(store.retireConnectionsFor(askerId, admin)).rejects.toThrow( + /would not withdraw/i, + ); + + // Both were asked. A later app is not punished for an earlier one, and on the previous shape the + // throw left the loop, so an app after the refused one was never reached at all. + expect(asksMade()).toEqual([ + `revoke:${toolkit}/${askerId}`, + `revoke:${secondToolkit}/${askerId}`, + ]); + + // The withdrawn app's row is gone; the refused app's stands, so the recovery still has it. + expect(await connectedToolkitsFor(askerId)).toEqual([secondToolkit]); + + const disconnected = recordedOfType("mcp.account_disconnected"); + expect(disconnected).toHaveLength(1); + expect(disconnected[0]?.payload).toMatchObject({ + server: toolkit, + owner: askerId, + reason: "person_removed", + // The answer this app actually got, written while it was still known. + vendorRevocationRequested: true, + }); +}); + +/** + * AND THE REFUSAL IS STILL THROWN, WHATEVER ORDER IT CAME IN. + * + * CRITERION. The vendor refusing the FIRST app: the second is still asked and still recorded, and + * the act still fails with the refusal rather than reporting a success. + * + * REASON. Holding a refusal instead of throwing it is only correct if it is still thrown. The risk + * this pins is the opposite of the one above — that collecting refusals turns a failed offboarding + * into a reported one, which is the state `an offboarding the vendor refuses leaves the connection + * standing` exists to forbid. + */ +test("a refusal on the first app still fails the act and still asks the second", async () => { + await seedApp({ connect: false }); + await database + .insert(composioConnections) + .values({ toolkit, userId: askerId }); + await database + .insert(composioConnections) + .values({ toolkit: secondToolkit, userId: askerId }); + vendorRefuses = ({ toolkit: asked }) => asked === toolkit; + + await expect(store.retireConnectionsFor(askerId, admin)).rejects.toThrow( + /would not withdraw/i, + ); + + expect(asksMade()).toEqual([ + `revoke:${toolkit}/${askerId}`, + `revoke:${secondToolkit}/${askerId}`, + ]); + expect(await connectedToolkitsFor(askerId)).toEqual([toolkit]); + expect(recordedOfType("mcp.account_disconnected")).toHaveLength(1); +}); + /** * THE ANONYMOUS ACTOR OWNS NOTHING, and `notNull` does not exclude the empty string, so a row at * `(toolkit, "")` is legal. Retiring "nobody" must not be what deletes it — that would be an