diff --git a/.changeset/connection-remove-credential-gc.md b/.changeset/connection-remove-credential-gc.md new file mode 100644 index 0000000000..533c6bd1da --- /dev/null +++ b/.changeset/connection-remove-credential-gc.md @@ -0,0 +1,5 @@ +--- +"@executor-js/sdk": patch +--- + +Removing a connection, or removing the integration it belongs to, deleted the rows and left the credentials those connections had minted sitting in the provider, referenced by nothing and visible nowhere in the product. Both removals now delete the items they minted, including the long-lived OAuth refresh token, and an integration removal covers every member's connections under the slug rather than only the remover's own. An item a connection merely referenced is left alone, as is a minted item another connection still points at, and the deletion runs only once the removal has committed. diff --git a/packages/core/sdk/src/connection-remove-credential-gc.test.ts b/packages/core/sdk/src/connection-remove-credential-gc.test.ts new file mode 100644 index 0000000000..f9f6de192c --- /dev/null +++ b/packages/core/sdk/src/connection-remove-credential-gc.test.ts @@ -0,0 +1,435 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Effect, Exit } from "effect"; + +import { StorageError } from "./fuma-runtime"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, + ProviderItemId, + ProviderKey, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import type { CredentialProvider } from "./provider"; +import { makeTestExecutor, makeTestWorkspaceHarness } from "./test-config"; +import { serveOAuthTestServer } from "./testing/oauth-test-server"; + +// Removing a connection has to remove the SECRET, not just the row that points +// at it — an item left behind in the store is still decryptable, which is the +// one thing a user deleting a credential is asking us to stop being true. +// +// The hard half is the opposite case. A connection can REFERENCE an item the +// user already had rather than minting one, and destroying that is +// unrecoverable. So these tests are written in pairs: every "it is gone" has a +// matching "it is still there", because a change that deleted everything would +// pass the first alone. + +const INTEG = IntegrationSlug.make("vercel"); +const TEMPLATE = AuthTemplateSlug.make("apiKey"); + +/** A provider whose store the test can inspect directly, so an assertion reads + * the actual item rather than a resolution a removed connection can no longer + * perform. */ +const inspectableProvider = ( + store: Map, + options: { + /** An ACCESSOR, not a literal, because one case needs a provider that stops + * being writable after it has already minted — the only state in which the + * writability gate is reachable. The executor wraps a registered provider + * with `Object.create`, so the accessor survives the wrap and stays live. */ + readonly isWritable?: () => boolean; + readonly deleteFails?: boolean; + } = {}, +): CredentialProvider => ({ + key: ProviderKey.make("memory"), + get writable() { + return options.isWritable?.() ?? true; + }, + get: (id) => Effect.sync(() => store.get(String(id)) ?? null), + set: (id, value) => + Effect.sync(() => { + store.set(String(id), value); + }), + has: (id) => Effect.sync(() => store.has(String(id))), + delete: (id) => + options.deleteFails === true + ? Effect.fail(new StorageError({ message: "credential store is offline", cause: undefined })) + : Effect.sync(() => { + store.delete(String(id)); + }), +}); + +const demoPlugin = (provider: CredentialProvider) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [provider], + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("deploy"), description: "deploy" }] }), + invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: INTEG, description: "Vercel", config: {} }), + /** The plugin-owned OUTER transaction the removal can find itself inside. */ + inTransaction: (effect: Effect.Effect) => ctx.transaction(effect), + }), + }))(); + +const setup = (provider: CredentialProvider) => + makeTestExecutor({ plugins: [demoPlugin(provider)] as const }).pipe( + Effect.tap((executor) => executor.demo.seed()), + ); + +/** The one id in the store, asserted to be the only one. Minted ids carry a + * per-attempt uuid, so a test cannot spell one out; it reads back what the + * mint actually wrote. */ +const onlyItemId = (store: ReadonlyMap): string => { + const keys = [...store.keys()]; + expect(keys).toHaveLength(1); + const [only = ""] = keys; + return only; +}; + +const OAUTH_INTEG = IntegrationSlug.make("oauthdemo"); +const OAUTH_TEMPLATE = AuthTemplateSlug.make("oauth"); + +const oauthIntegrationPlugin = definePlugin(() => ({ + id: "oauthdemo" as const, + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(OAUTH_TEMPLATE), + oauth: { scopes: [] }, + }, + ], + invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: OAUTH_INTEG, description: "OAuth demo", config: {} }), + }), +}))(); + +describe("removing a connection removes the credential it minted", () => { + // The premise the whole design rests on. A minted item id is NOT derivable + // from the row: it carries a uuid unique to the write attempt, so the + // connection's own columns cannot reproduce it. That is why ownership is read + // from the `credential_write` marker instead of by rebuilding a deterministic + // id and comparing — a rebuild would match nothing and delete nothing. + it.effect("mints an item id the row cannot reproduce", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + + const mintedId = onlyItemId(store); + expect(mintedId).not.toBe("connection:org:vercel:main:token"); + expect(mintedId.startsWith("connection:org:vercel:main:")).toBe(true); + expect(mintedId.endsWith(":token")).toBe(true); + }), + ); + + it.effect("deletes the item a pasted connection minted", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + const mintedId = onlyItemId(store); + expect(store.get(mintedId)).toBe("secret-token"); + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + + expect(store.has(mintedId)).toBe(false); + // Nothing else was swept up on the way past. + expect([...store.keys()]).toEqual([]); + }), + ); + + it.effect("LEAVES an item the connection only referenced", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + // The user already had this, in their own store, under their own id. We + // never wrote it, and deleting it would destroy a credential that has + // nothing to do with this connection. The row records that by leaving + // `credential_write` null. + store.set("ext-item", "user-owned-secret"); + + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("byo"), + integration: INTEG, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make("ext-item") }, + }); + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("byo"), + }); + + expect(store.get("ext-item")).toBe("user-owned-secret"); + }), + ); + + // The writability gate, exercised the only way it is reachable: the item was + // minted while the provider was writable, and the provider stopped being + // writable before the removal. `writable: false` means we never write there, + // and by the same contract we never delete there either. + it.effect("LEAVES a minted item once its provider stops being writable", () => + Effect.gen(function* () { + const store = new Map(); + let writable = true; + const executor = yield* setup(inspectableProvider(store, { isWritable: () => writable })); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + const mintedId = onlyItemId(store); + + writable = false; + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + + expect(store.get(mintedId)).toBe("secret-token"); + }), + ); + + it.effect("LEAVES a minted item that another connection has aliased", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + // `first` mints its own item. + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("first"), + integration: INTEG, + template: TEMPLATE, + value: "shared-token", + }); + const mintedId = onlyItemId(store); + expect(store.get(mintedId)).toBe("shared-token"); + + // `second` points AT that same item instead of minting its own. Nothing + // stops this: the reference path stores whatever id it is handed. + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("second"), + integration: INTEG, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make(mintedId) }, + }); + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("first"), + }); + + // Removing the minting connection must not pull the credential out from + // under the one still using it — that would break a live connection. + expect(store.get(mintedId)).toBe("shared-token"); + }), + ); + + it.effect("removing one connection does not touch another's credential", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + for (const name of ["first", "second"]) { + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make(name), + integration: INTEG, + template: TEMPLATE, + value: `${name}-token`, + }); + } + const idOf = (value: string): string => { + const entry = [...store.entries()].find(([, stored]) => stored === value); + expect(entry).toBeDefined(); + return entry?.[0] ?? ""; + }; + const firstId = idOf("first-token"); + const secondId = idOf("second-token"); + + yield* executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("first"), + }); + + expect(store.has(firstId)).toBe(false); + expect(store.get(secondId)).toBe("second-token"); + }), + ); + + // Best effort, and deliberately so. A provider that cannot delete must not + // resurrect a connection the user has already removed: a failure here leaves + // the orphan that existed before, which is recoverable, rather than failing a + // removal whose rows are already gone. + it.effect("a provider whose delete fails does not fail the removal", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store, { deleteFails: true })); + yield* executor.connections.create({ + owner: "org", + name: ConnectionName.make("main"), + integration: INTEG, + template: TEMPLATE, + value: "secret-token", + }); + const mintedId = onlyItemId(store); + + const outcome = yield* Effect.exit( + executor.connections.remove({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }), + ); + + expect(Exit.isSuccess(outcome)).toBe(true); + // The row is gone even though the item could not be deleted. + const gone = yield* executor.connections.get({ + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + }); + expect(gone).toBeNull(); + expect(store.get(mintedId)).toBe("secret-token"); + }), + ); + + it.effect("deletes BOTH the access and the refresh token of an OAuth connection", () => + Effect.scoped( + Effect.gen(function* () { + // The OAuth mint is the security-relevant half: it parks a long-lived + // REFRESH token, and leaving that behind is far worse than leaving an + // access token. Nothing else in the suite exercises an `oauth:` item id + // or a `refresh_item_id`, so without this the refresh half is unpinned. + const store = new Map(); + const server = yield* serveOAuthTestServer({}); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [demoPlugin(inspectableProvider(store)), oauthIntegrationPlugin] as const, + }); + yield* executor.oauthdemo.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: OAuthClientSlug.make("demo-app"), + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + + const started = yield* executor.oauth.start({ + owner: "org", + client: OAuthClientSlug.make("demo-app"), + clientOwner: "org", + name: ConnectionName.make("main"), + integration: OAUTH_INTEG, + template: OAUTH_TEMPLATE, + }); + if (started.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* executor.oauth.complete({ state: started.state, code: callback.code }); + + // Both halves are versioned per attempt, so they are read back by shape + // rather than spelled out. + const oauthIds = [...store.keys()].filter((id) => id.startsWith("oauth:")); + const refreshIds = oauthIds.filter((id) => id.endsWith(":refresh")); + const accessIds = oauthIds.filter((id) => !id.endsWith(":refresh")); + expect(refreshIds).toHaveLength(1); + expect(accessIds).toHaveLength(1); + const [accessId = ""] = accessIds; + const [refreshId = ""] = refreshIds; + + yield* executor.connections.remove({ + owner: "org", + integration: OAUTH_INTEG, + name: ConnectionName.make("main"), + }); + + expect(store.has(accessId)).toBe(false); + // The long-lived half. Leaving this behind is the worst outcome here. + expect(store.has(refreshId)).toBe(false); + }), + ), + ); +}); + +// The deletion reaches OUTSIDE the database, so it must not run inside the +// transaction that removes the rows. Nothing in a provider — a sealed store, a +// keychain, someone else's API — enlists in that transaction or rolls back with +// it. If an abort restores the connection row after its secret has already been +// destroyed, the result is a live connection pointing at a credential that no +// longer exists: worse than the orphan this whole feature removes, and unlike +// the orphan, unrepairable. +describe("the credential deletion runs after the transaction commits", () => { + it.effect("a rolled-back removal leaves the credential intact", () => + Effect.gen(function* () { + const store = new Map(); + const executor = yield* setup(inspectableProvider(store)); + const ref = { + owner: "org", + integration: INTEG, + name: ConnectionName.make("main"), + } as const; + yield* executor.connections.create({ ...ref, template: TEMPLATE, value: "secret-token" }); + const mintedId = onlyItemId(store); + + // A caller wraps the removal in its own transaction and then fails, so + // the row deletions roll back. + const outcome = yield* Effect.exit( + executor.demo.inTransaction( + Effect.gen(function* () { + yield* executor.connections.remove(ref); + return yield* Effect.fail("rollback" as const); + }), + ), + ); + expect(Exit.isFailure(outcome)).toBe(true); + + // The connection came back... + const stillThere = yield* executor.connections.get(ref); + expect(String(stillThere?.name)).toBe("main"); + // ...so its credential MUST still be there. A restored row pointing at a + // destroyed secret is the one outcome that cannot be repaired. + expect(store.get(mintedId)).toBe("secret-token"); + }), + ); +}); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index fdbc9b7671..1ba9058bb8 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -3410,9 +3410,40 @@ export const createExecutor = b("integration", "=", String(slug)); + // Read the doomed connections BEFORE the cascade destroys them. A + // minted item id carries a per-attempt uuid recorded nowhere but the + // row (`credentialAttemptItemId`), so the moment the row is gone the + // id of the secret it minted is unreconstructible and that secret is + // stranded in the provider for good. + // + // Through `cascadeCore`, the handle doing the DELETE below, and this + // adds NO reach. `ownedExecutorTable` feeds one `ownerVisibility` + // condition to both `onRead` and `onDelete` (`core-schema.ts`), so + // the same context and the same predicate return exactly the rows the + // next statement destroys and never one more; reading rows in order + // to destroy them cannot expose anything the destruction does not + // already reach. The bound `core` handle is the unsafe option here, + // not the safe one: it sees only the remover's own rows, so every + // other member's credential would be silently left behind — which is + // the bulk half of the leak this closes. + const doomed = yield* cascadeCore.findMany("connection", { where }); yield* cascadeCore.deleteMany("tool", { where }); yield* cascadeCore.deleteMany("definition", { where }); yield* cascadeCore.deleteMany("connection", { where }); + // Then delete what those connections minted, after the OUTERMOST + // commit. Same reasoning as `connections.remove`, whose + // `deleteMintedCredentials` this reuses unchanged: a provider does + // not enlist in this transaction, so deleting inside it would let a + // rollback restore every row with its secret already destroyed. + // + // One residual limit, inherited rather than introduced: the alias + // hold-back inside `deleteMintedCredentials` reads through the bound + // handle, so an item that a SURVIVING connection of ANOTHER subject + // references is invisible to it and is deleted with the rest. Closing + // that would mean reading other subjects' credential ids OUTSIDE the + // set being destroyed — a read wider than the delete, and a worse + // defect than the narrow one it would fix. + yield* afterCommit(Effect.forEach(doomed, deleteMintedCredentials, { discard: true })); return existing.plugin_id; }), ).pipe( @@ -4823,6 +4854,108 @@ export const createExecutor = => + Effect.gen(function* () { + // Removing the rows only dropped the POINTER: the secret stayed in the + // provider and stayed decryptable, which is precisely what a user + // removing a connection is asking us to stop being true. + // + // Only ids this connection MINTED. A connection can instead REFERENCE + // an item the user already had (the `from` origin on the create path), + // and the provider contract is explicit that such a removal "only drops + // our routing, leaving the item intact" — deleting one would destroy a + // credential we never wrote and cannot restore. + // + // `credential_write` is what tells the two apart, and it is the only + // thing that can. Provider item ids are opaque after creation by + // design — ownership is "determined exclusively from persisted + // metadata" (`credential-item-reference.ts`) — and rebuilding a + // deterministic id to compare would not work anyway: a minted id embeds + // a per-attempt uuid (`credentialAttemptItemId`) that no other column + // records. + // + // The marker is per ROW rather than per item, which is sufficient + // because a row cannot hold both kinds: `connectionsCreate` rejects a + // connection mixing pasted and external inputs, its external branch + // leaves `credential_write` null, and the OAuth mint sets it for every + // item it writes. Null therefore means every id on this row is external + // or v1-legacy, opaque to core, and left alone. A v1-migrated + // `secret_` item is executor-owned but unmarked, so its value is + // still left behind; closing that needs a schema change, not a looser + // rule here. + if (parseCredentialWriteAttempt(row.credential_write) === null) return; + // `writable` is checked as well as the marker, never instead of it. + // This is only reachable when a provider stops being writable after the + // item was minted, where honouring the contract's "we never write here" + // is the safer reading. + const provider = credentialProviders.get(String(row.provider)); + if (provider?.writable !== true || !provider.delete) return; + const deleteItem = provider.delete; + const minted = [ + ...new Set([ + ...Object.values(connectionItemIds(row)), + ...(row.refresh_item_id === null ? [] : [String(row.refresh_item_id)]), + ]), + ]; + if (minted.length === 0) return; + // Nothing stops a SECOND connection pointing at this one's minted item + // through the `from` origin — the reference path stores whatever id it + // is handed. Deleting the item would then pull the credential out from + // under a connection that is still live and still using it. This row is + // already gone by the time this runs, so anything still naming the id + // is by definition somebody else, and the item stays. + // + // An item id only means anything inside ONE provider's namespace, so a + // connection on a different provider holding the same string is not an + // alias. Counting it as one would leave this connection's secret + // behind, which is the orphan this delete exists to remove. + // + // This read is owner-scoped by the table's own visibility policy: it + // sees the org partition plus this caller's own rows and NOT another + // subject's. An alias held by a different subject is therefore + // invisible here and its credential is still deleted. That limit is + // accepted deliberately — reading around a tenant-isolation boundary to + // widen a DELETE would be a worse defect than the narrow one it closes. + const others = yield* core.findMany("connection", { + where: (b: AnyCb) => b("provider", "=", String(row.provider)), + }); + const stillReferenced = new Set( + others.flatMap((other) => [ + ...Object.values(connectionItemIds(other)), + ...(other.refresh_item_id === null ? [] : [String(other.refresh_item_id)]), + ]), + ); + yield* Effect.forEach( + minted.filter((id) => !stillReferenced.has(id)), + (id) => deleteItem(ProviderItemId.make(id)).pipe(Effect.ignore), + { discard: true }, + ); + }).pipe(Effect.ignoreCause({ log: false })); + const connectionsRemove = ( ref: ConnectionRef, ): Effect.Effect => @@ -4868,8 +5001,10 @@ export const createExecutor = , + options: { + readonly deleteFails?: boolean; + /** Every id the removal ASKED to delete, recorded before the outcome, so a + * best-effort test can tell a swallowed failure from a delete that was + * never attempted at all. */ + readonly deleteAttempts?: string[]; + } = {}, +): CredentialProvider => ({ + key: ProviderKey.make("memory"), + writable: true, + get: (id) => Effect.sync(() => store.get(String(id)) ?? null), + set: (id, value) => + Effect.sync(() => { + store.set(String(id), value); + }), + has: (id) => Effect.sync(() => store.has(String(id))), + delete: (id) => + Effect.suspend(() => { + options.deleteAttempts?.push(String(id)); + return options.deleteFails === true + ? Effect.fail( + new StorageError({ message: "credential store is offline", cause: undefined }), + ) + : Effect.sync(() => { + store.delete(String(id)); + }); + }), +}); + +const demoPlugin = (provider: CredentialProvider) => + definePlugin(() => ({ + id: "demo" as const, + credentialProviders: [provider], + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("query"), description: "query" }] }), + invokeTool: ({ toolRow }) => Effect.succeed({ ran: toolRow.name }), + extension: (ctx) => ({ + seed: (slug: IntegrationSlug) => + ctx.core.integrations.register({ slug, description: String(slug), config: {} }), + /** The plugin-owned OUTER transaction the removal can find itself inside. */ + inTransaction: (effect: Effect.Effect) => ctx.transaction(effect), + }), + }))(); + +/** Two executors over ONE database: `alice` is the admin who seeds and removes, + * `bob` is a member whose rows the removal reaches but whose rows the remover's + * own bound handle cannot see. */ +const setup = (provider: CredentialProvider) => + Effect.gen(function* () { + const config = makeTestConfig({ plugins: [demoPlugin(provider)] as const, subject: ALICE }); + const alice = yield* createExecutor(config); + const bob = yield* createExecutor({ + ...config, + subject: Subject.make(BOB), + db: withQueryContext(config.testDb.db, { tenant: String(config.tenant), subject: BOB }), + }); + yield* Effect.addFinalizer(() => + alice.close().pipe(Effect.andThen(bob.close()), Effect.ignore), + ); + yield* alice.demo.seed(INTEG); + yield* alice.demo.seed(KEPT); + return { alice, bob }; + }); + +const connectPersonal = ( + executor: Executor, + integration: IntegrationSlug, + name: string, + value: string, +) => + executor.connections.create({ + owner: "user", + name: ConnectionName.make(name), + integration, + template: TEMPLATE, + value, + }); + +const referencePersonal = ( + executor: Executor, + integration: IntegrationSlug, + name: string, + itemId: string, +) => + executor.connections.create({ + owner: "user", + name: ConnectionName.make(name), + integration, + template: TEMPLATE, + from: { provider: ProviderKey.make("memory"), id: ProviderItemId.make(itemId) }, + }); + +/** Minted ids carry a per-attempt uuid, so a test cannot spell one out; it finds + * the id by the value the mint actually wrote under it. */ +const idOf = (store: ReadonlyMap, value: string): string => { + const entry = [...store.entries()].find(([, stored]) => stored === value); + expect(entry).toBeDefined(); + return entry?.[0] ?? ""; +}; + +const OAUTH_INTEG = IntegrationSlug.make("oauthdemo"); +const OAUTH_TEMPLATE = AuthTemplateSlug.make("oauth"); + +const oauthIntegrationPlugin = definePlugin(() => ({ + id: "oauthdemo" as const, + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(OAUTH_TEMPLATE), + oauth: { scopes: [] }, + }, + ], + invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), + extension: (ctx) => ({ + seed: () => + ctx.core.integrations.register({ slug: OAUTH_INTEG, description: "OAuth demo", config: {} }), + }), +}))(); + +describe("removing an integration removes the credentials its connections minted", () => { + it.effect("deletes the minted item of EVERY subject's connection under the slug", () => + Effect.gen(function* () { + const store = new Map(); + const { alice, bob } = yield* setup(inspectableProvider(store)); + yield* connectPersonal(alice, INTEG, "aliceDd", "alice-token"); + yield* connectPersonal(bob, INTEG, "bobDd", "bob-token"); + yield* connectPersonal(bob, KEPT, "bobLinear", "bob-kept-token"); + const aliceId = idOf(store, "alice-token"); + const bobId = idOf(store, "bob-token"); + const keptId = idOf(store, "bob-kept-token"); + + yield* alice.integrations.remove(INTEG); + + expect(store.has(aliceId)).toBe(false); + // The half that only this change closes. The removal destroys Bob's row + // through the tenant-reach cascade, but the remover's own bound handle + // cannot even see that row — so the doomed set has to be read through the + // same widened handle that deletes it, or Bob's secret is left behind. + expect(store.has(bobId)).toBe(false); + // And the sweep stops at the slug: an integration that was not removed + // keeps its credential. + expect(store.get(keptId)).toBe("bob-kept-token"); + expect([...store.keys()]).toEqual([keptId]); + }).pipe(Effect.scoped), + ); + + it.effect("LEAVES an item a doomed connection only referenced", () => + Effect.gen(function* () { + const store = new Map(); + const { alice, bob } = yield* setup(inspectableProvider(store)); + // Bob already had this, in his own store, under his own id. We never + // wrote it, so deleting it would destroy a credential that has nothing to + // do with the integration being removed. The row records that by leaving + // `credential_write` null. + store.set("bob-owned-item", "bob-owned-secret"); + yield* referencePersonal(bob, INTEG, "byo", "bob-owned-item"); + // A minted item under the SAME slug, so the sweep is known to have run + // and to have skipped the referenced one rather than skipped everything. + yield* connectPersonal(alice, INTEG, "aliceDd", "alice-token"); + const aliceId = idOf(store, "alice-token"); + + yield* alice.integrations.remove(INTEG); + + expect(store.has(aliceId)).toBe(false); + expect(store.get("bob-owned-item")).toBe("bob-owned-secret"); + }).pipe(Effect.scoped), + ); + + it.effect("LEAVES a minted item a SURVIVING connection still points at", () => + Effect.gen(function* () { + const store = new Map(); + const { alice } = yield* setup(inspectableProvider(store)); + yield* connectPersonal(alice, INTEG, "doomed", "shared-token"); + const sharedId = idOf(store, "shared-token"); + // A connection under an integration that is NOT being removed points at + // that same item instead of minting its own. Nothing stops this: the + // reference path stores whatever id it is handed. + yield* referencePersonal(alice, KEPT, "survivor", sharedId); + // A second doomed connection with an item nobody else points at, so the + // survival below is a hold-back and not an inert sweep. + yield* connectPersonal(alice, INTEG, "alsoDoomed", "lonely-token"); + const lonelyId = idOf(store, "lonely-token"); + + yield* alice.integrations.remove(INTEG); + + expect(store.has(lonelyId)).toBe(false); + // Sweeping the aliased item away would pull the credential out from under + // a connection that is still live and still using it. + expect(store.get(sharedId)).toBe("shared-token"); + }).pipe(Effect.scoped), + ); + + // Best effort, and deliberately so. A provider that cannot delete must not + // resurrect an integration the user has already removed: the failure leaves + // the orphan that existed before, which is recoverable, where failing the + // removal would leave rows the catalog no longer has an entry for. + it.effect("a provider whose delete fails does not fail the removal", () => + Effect.gen(function* () { + const store = new Map(); + const deleteAttempts: string[] = []; + const { alice, bob } = yield* setup( + inspectableProvider(store, { deleteFails: true, deleteAttempts }), + ); + yield* connectPersonal(bob, INTEG, "bobDd", "bob-token"); + const bobId = idOf(store, "bob-token"); + + const outcome = yield* Effect.exit(alice.integrations.remove(INTEG)); + + expect(Exit.isSuccess(outcome)).toBe(true); + // The delete was ATTEMPTED and its failure swallowed. Without this the + // test would pass just as happily against a removal that never tried. + expect(deleteAttempts).toEqual([bobId]); + // The rows went even though the item could not be deleted. + const bobConnections = yield* bob.connections.list(); + expect(bobConnections.map((connection) => String(connection.integration))).toEqual([]); + expect(store.get(bobId)).toBe("bob-token"); + }).pipe(Effect.scoped), + ); + + it.effect("deletes BOTH the access and the refresh token of an OAuth connection", () => + Effect.scoped( + Effect.gen(function* () { + // The OAuth mint is the security-relevant half: it parks a long-lived + // REFRESH token, and an integration removal sweeping up every member at + // once is exactly where leaving those behind adds up. + const store = new Map(); + const server = yield* serveOAuthTestServer({}); + const { executor } = yield* makeTestWorkspaceHarness({ + plugins: [demoPlugin(inspectableProvider(store)), oauthIntegrationPlugin] as const, + }); + yield* executor.oauthdemo.seed(); + yield* executor.oauth.createClient({ + owner: "org", + slug: OAuthClientSlug.make("demo-app"), + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + // Everything in the store at this point belongs to the OAuth APP, not + // to any connection. Captured before the flow so the assertion below + // cannot accidentally be about nothing. + const clientItemIds = [...store.keys()]; + expect(clientItemIds.length).toBeGreaterThan(0); + + const started = yield* executor.oauth.start({ + owner: "org", + client: OAuthClientSlug.make("demo-app"), + clientOwner: "org", + name: ConnectionName.make("main"), + integration: OAUTH_INTEG, + template: OAUTH_TEMPLATE, + }); + if (started.status !== "redirect") { + return yield* Effect.die("expected a redirect-status OAuth start"); + } + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* executor.oauth.complete({ state: started.state, code: callback.code }); + + // Both halves are versioned per attempt, so they are read back by shape + // rather than spelled out. + const oauthIds = [...store.keys()].filter((id) => id.startsWith("oauth:")); + const refreshIds = oauthIds.filter((id) => id.endsWith(":refresh")); + const accessIds = oauthIds.filter((id) => !id.endsWith(":refresh")); + expect(refreshIds).toHaveLength(1); + expect(accessIds).toHaveLength(1); + const [accessId = ""] = accessIds; + const [refreshId = ""] = refreshIds; + + yield* executor.integrations.remove(OAUTH_INTEG); + + expect(store.has(accessId)).toBe(false); + // The long-lived half. Leaving this behind is the worst outcome here. + expect(store.has(refreshId)).toBe(false); + // The app's own client secret is the OAuth client's, not the + // connection's, and it outlives the integration it was registered + // against. Nothing here is entitled to delete it. + expect(clientItemIds.filter((id) => store.has(id))).toEqual(clientItemIds); + }), + ), + ); +}); + +// The deletion reaches OUTSIDE the database, so it must not run inside the +// transaction that removes the rows. Nothing in a provider — a sealed store, a +// keychain, someone else's API — enlists in that transaction or rolls back with +// it. If an abort restores the rows after their secrets have already been +// destroyed, the result is a whole integration's worth of live connections +// pointing at credentials that no longer exist. +describe("the credential deletion runs after the transaction commits", () => { + it.effect("a rolled-back removal leaves every subject's credential intact", () => + Effect.gen(function* () { + const store = new Map(); + const { alice, bob } = yield* setup(inspectableProvider(store)); + yield* connectPersonal(alice, INTEG, "aliceDd", "alice-token"); + yield* connectPersonal(bob, INTEG, "bobDd", "bob-token"); + const aliceId = idOf(store, "alice-token"); + const bobId = idOf(store, "bob-token"); + + // A caller wraps the removal in its own transaction and then fails, so + // the catalog row and the whole cascade roll back. + const outcome = yield* Effect.exit( + alice.demo.inTransaction( + Effect.gen(function* () { + yield* alice.integrations.remove(INTEG); + return yield* Effect.fail("rollback" as const); + }), + ), + ); + expect(Exit.isFailure(outcome)).toBe(true); + + // The connections came back... + const bobConnections = yield* bob.connections.list(); + expect(bobConnections.map((connection) => String(connection.integration))).toEqual([ + String(INTEG), + ]); + // ...so their credentials MUST still be there. A restored row pointing at + // a destroyed secret is the one outcome that cannot be repaired. + expect(store.get(aliceId)).toBe("alice-token"); + expect(store.get(bobId)).toBe("bob-token"); + + // And the rollback is what spared them, not an inert fixture: the same + // removal, committed this time, takes both. + yield* alice.integrations.remove(INTEG); + expect(store.has(aliceId)).toBe(false); + expect(store.has(bobId)).toBe(false); + }).pipe(Effect.scoped), + ); +});