diff --git a/CHANGELOG.md b/CHANGELOG.md index a575cc2651..b80fcf2790 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -759,6 +759,15 @@ opening the app's page already did. A Shared app's team account is still recorde If Composio then reports no account after all, Connect says nothing was connected and asks you to press it again. It no longer tells you to disconnect anything. +### A Bot's Slack link code works only in a direct message + +The `link ` message a Bot's Reaching you shows now links on Slack only when it is sent to the +app in a direct message. Sent in a channel, with or without a mention, it is cancelled and the sender +is told to make a new one, as an account code from Connections already was. Whoever sends the code +first is linked to that Bot: their messages run it as its owner, and its questions and approval +requests go to them, so a code others have seen must not still work. Teams codes still link wherever +they are sent, until OpenTag is confirmed to mark a Teams one-to-one chat as private. + ### A Bot with an app or web research can use its browser in a conversation again A Bot that held any app or research tool, such as Parallel Search, could not open a page, click or diff --git a/app/src/components/delivery/reaching-you.tsx b/app/src/components/delivery/reaching-you.tsx index 3cc9c6d0a4..b6813f3b28 100644 --- a/app/src/components/delivery/reaching-you.tsx +++ b/app/src/components/delivery/reaching-you.tsx @@ -514,7 +514,7 @@ function ConnectDialog({ - Send this message to the OpenBot app in{" "} + Send this message to the OpenBot app in a direct message in{" "} {linked.platform === "teams" ? "Microsoft Teams" : "Slack"}{" "} within {Math.ceil((linked.expiresAt - Date.now()) / 60_000)}{" "} minutes. It links that account to this conversation and Bot. diff --git a/docs/configuration.md b/docs/configuration.md index 51c109405b..b5c9e714c1 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -605,7 +605,8 @@ Slack app of its own. Point OpenTag's `AGENT_URL` at `/api/delivery/webhooks/opentag/agent`. A person links their Slack or Teams identity from a Bot's page, under **Reaching you**: OpenBot shows a one-time `link ` message, -valid for ten minutes, which they send to the OpenTag app. After that, only that person's messages +valid for ten minutes, which they send to the OpenTag app in a direct message. On Slack, a code posted +in a shared channel is cancelled, because whoever sends it first is linked to that Bot. After that, only that person's messages reach their chosen Bot, in their own OpenBot conversation, with learning, governance, approvals and audit on every turn. Anyone unlinked is told how to link and nothing runs. Approval requests appear as OpenTag approval cards (allow once, always allow, cancel) and are decided through the approvals diff --git a/server/src/delivery/opentag-agent.ts b/server/src/delivery/opentag-agent.ts index 901df3415d..0a90c9860c 100644 --- a/server/src/delivery/opentag-agent.ts +++ b/server/src/delivery/opentag-agent.ts @@ -124,8 +124,9 @@ const LINK = const LINKS = new RegExp(LINK.source, "gi"); /** - * A channel post the app was not mentioned in: everyone there has seen any account code in it, so - * each one is cancelled, quietly (an observe run never replies). The codes are redacted rather than + * A channel post the app was not mentioned in: everyone there has seen any link code in it, + * Reachability or account, so on Slack each one is cancelled, quietly (an observe run never + * replies; Teams waits on the same rule as `link`). The codes are redacted rather than * the message dropped, so the post still fires the owners' message triggers without carrying a * code. A failed cancel is logged and never stops the triggers. */ @@ -136,22 +137,43 @@ async function withoutLinkCodes( ) { const codes = [...text.matchAll(LINKS)].map((m) => m[1]?.toLowerCase()); if (!codes.length) return text; - if (sender.transport === "slack" && deps.identity) - for (const code of codes) { - if (!code) continue; - try { - await deps.identity.discard(code); - } catch (error) { - console.error( - JSON.stringify({ - type: "opentag-link-code-discard-error", - errorType: error instanceof Error ? error.name : "UnknownError", - }), - ); - } - } + for (const code of codes) { + if (!code) continue; + // Each cancel is tried on its own: a store that fails to spend a Reachability code must not + // leave an account code with the same value live, and the other way round. + if (sender.transport !== "slack") continue; + await logFailure(() => cancelReachabilityCode(deps, code)); + const identity = deps.identity; + if (identity) await logFailure(() => identity.discard(code)); + } return text.replace(LINKS, "link [code removed]"); } + +async function logFailure(cancel: () => Promise) { + try { + await cancel(); + } catch (error) { + console.error( + JSON.stringify({ + type: "opentag-link-code-discard-error", + errorType: error instanceof Error ? error.name : "UnknownError", + }), + ); + } +} + +/** Spends a live Reachability code without binding anyone; true when one was cancelled. */ +async function cancelReachabilityCode(deps: OpenTagAgentDeps, code: string) { + const challenge = await deps.store.readChallenge(code); + return Boolean( + challenge && + (await deps.store.consumeChallenge(code, challenge.ownerUserId)), + ); +} + +const CANCELLED_IN_SHARED = + "That code was posted where others could see it, so I cancelled it. Create a new one in OpenBot and send it to me in a direct message."; + const platformName = (transport: ChatTransport) => transport === "teams" ? "Microsoft Teams" : "Slack"; @@ -417,6 +439,16 @@ async function link( code: string, ) { const challenge = await deps.store.readChallenge(code); + // A Reachability code binds whoever sends it to its issuer's Bot: their messages run as the + // issuer, and the Bot's questions and approval requests go to them. Anyone else in a shared + // conversation has seen it and could send it first, so it is only redeemed in a direct message + // and one posted here is spent, the same rule as for an account code below. Slack only for now: + // a Teams chat is private only by its conversation kind, and until OpenTag is confirmed to send + // `personal` for a one-to-one chat, this rule could spend every code sent from Teams. + if (challenge && sender.transport === "slack" && !sender.private) { + await deps.store.consumeChallenge(code, challenge.ownerUserId); + return { text: CANCELLED_IN_SHARED }; + } if (!challenge && sender.transport === "slack" && deps.identity) { // Anyone else in a shared conversation has now seen the code and could send it first, binding // their own Slack account to its issuer, so an account code is only redeemed in a direct @@ -424,9 +456,7 @@ async function link( // `withoutLinkCodes`). if (!sender.private) { if (await deps.identity.discard(code)) - return { - text: "That code was posted where others could see it, so I cancelled it. Create a new one in OpenBot and send it to me in a direct message.", - }; + return { text: CANCELLED_IN_SHARED }; return { text: "That link code expired, was already used, or is for another app. Create a new one in OpenBot.", }; diff --git a/server/tests/delivery-routes.test.ts b/server/tests/delivery-routes.test.ts index 63e41842be..b92b44b162 100644 --- a/server/tests/delivery-routes.test.ts +++ b/server/tests/delivery-routes.test.ts @@ -32,12 +32,17 @@ function fixture( converse?: ConverseResult; bound?: boolean; identity?: IdentityAdapter; + /** Makes every Reachability lookup throw, as a store that is down would. */ + challengeError?: Error; + /** The transport the Reachability code was issued for. */ + challengeTransport?: "slack" | "teams"; /** Why the gate asked, as the stored request carries it; "task" for a check-in task's. */ policySource?: string; } = {}, ) { const calls: unknown[] = []; const decisions: unknown[] = []; + const consumed: unknown[] = []; const twilio = createTwilioTransport({ accountSid: "AC1", authToken: "token", @@ -73,19 +78,24 @@ function fixture( }, }, store: { - readChallenge: async (id: string) => - id === "11111111-2222-4333-8444-555555555555" + readChallenge: async (id: string) => { + if (options.challengeError) throw options.challengeError; + return id === "11111111-2222-4333-8444-555555555555" ? { id, ownerUserId: "owner", channelId: "ch", agentId: "bot", threadId: "canonical", - transport: "slack", + transport: options.challengeTransport ?? "slack", address: null, } - : null, - consumeChallenge: async () => true, + : null; + }, + consumeChallenge: async (...input) => { + consumed.push(input); + return true; + }, bind: async (input) => { calls.push({ bind: input }); return { ...bound, ...input }; @@ -151,7 +161,7 @@ function fixture( inbox: async () => ({ questions: [] }), }, }); - return { routes, calls, decisions }; + return { routes, calls, decisions, consumed }; } const sender = ( user = "slack:T1:U1", @@ -334,15 +344,82 @@ test("an identity code in a direct message is never discarded", async () => { await run(f, { context: [sender()], ...linkMessage(IDENTITY_CODE) }); expect(discarded).toEqual([]); }); -test("a Reachability code still links from a shared conversation", async () => { - const { f, redeemed, discarded } = identityFixture("linked"); +const REACHABILITY_CODE = "11111111-2222-4333-8444-555555555555"; +test("a Reachability code sent where others can read it is cancelled, never bound", async () => { + // Whoever sends a Reachability code is bound to its issuer's Bot and decides its approvals as + // the issuer, so a code others have seen is spent, exactly like an account code. + for (const conversation of [{ id: "C0123456789", kind: "channel" }, null]) { + const { f, redeemed, discarded } = identityFixture("linked"); + const result = await run(f, { + context: [sender("slack:T1:U1", conversation)], + ...linkMessage(REACHABILITY_CODE), + }); + expect(said(result.events)).toBe(CANCELLED); + expect(f.consumed).toEqual([[REACHABILITY_CODE, "owner"]]); + expect(f.calls).toEqual([]); + expect(redeemed).toEqual([]); + expect(discarded).toEqual([]); + } +}); +test("a Reachability code is cancelled from a shared conversation without an account adapter", async () => { + const f = fixture({ bound: false }); const result = await run(f, { context: [sender("slack:T1:U1", { id: "C0123456789", kind: "channel" })], - ...linkMessage("11111111-2222-4333-8444-555555555555"), + ...linkMessage(REACHABILITY_CODE), + }); + expect(said(result.events)).toBe(CANCELLED); + expect(f.consumed).toEqual([[REACHABILITY_CODE, "owner"]]); + expect(f.calls).toEqual([]); +}); +test("a Reachability code in a direct message still links", async () => { + const { f } = identityFixture("linked"); + const result = await run(f, { + context: [sender()], + ...linkMessage(REACHABILITY_CODE), }); expect(said(result.events)).toContain("Linked."); - expect(redeemed).toEqual([]); - expect(discarded).toEqual([]); + expect(f.calls).toHaveLength(1); +}); +test("a Reachability code in a Teams personal chat still links", async () => { + // A Teams conversation id is never a Slack `D…` id, so the only thing that makes this chat + // private is its kind. Were `personal` not read as private, every Teams code would be spent. + const f = fixture({ bound: false, challengeTransport: "teams" }); + const result = await run(f, { + context: [ + sender("teams:tenant1:user1", { + id: "a:1aBcDeFgHiJkLmNoPqRsTuVwXyZ", + kind: "personal", + }), + ], + ...linkMessage(REACHABILITY_CODE), + }); + expect(said(result.events)).toContain("Linked."); + expect(f.calls).toEqual([ + { + bind: expect.objectContaining({ transport: "teams", identity: "user1" }), + }, + ]); +}); +test("a Reachability code in a shared Teams chat still links until Teams' private chats are confirmed", async () => { + // The direct-message rule is Slack's only for now: a Teams chat is private only by its kind, and + // nothing yet confirms OpenTag sends `personal` for one, so applying the rule could spend every + // code sent from Teams. + const f = fixture({ bound: false, challengeTransport: "teams" }); + const result = await run(f, { + context: [ + sender("teams:tenant1:user1", { + id: "19:meeting_abc@thread.v2", + kind: "groupChat", + }), + ], + ...linkMessage(REACHABILITY_CODE), + }); + expect(said(result.events)).toContain("Linked."); + expect(f.calls).toEqual([ + { + bind: expect.objectContaining({ transport: "teams", identity: "user1" }), + }, + ]); }); const observedMessage = (content: string) => ({ context: [ @@ -380,6 +457,47 @@ test("an identity code pasted in a channel without a mention is cancelled quietl ); } }); +test("a Reachability code pasted in a channel without a mention is cancelled quietly and kept out of triggers", async () => { + const { f, redeemed, discarded } = identityFixture("linked"); + const result = await run( + f, + observedMessage(`here: link ${REACHABILITY_CODE.toUpperCase()}`), + ); + expect(said(result.events)).toBe(""); + expect(f.consumed).toEqual([[REACHABILITY_CODE, "owner"]]); + expect(redeemed).toEqual([]); + expect(discarded).toEqual([REACHABILITY_CODE]); + const triggers = triggersOf(f); + expect(triggers).toHaveLength(1); + expect(JSON.stringify(triggers[0]).toLowerCase()).not.toContain( + REACHABILITY_CODE, + ); +}); +test("a Reachability store that fails still lets the account code with that value be cancelled", async () => { + const discarded: string[] = []; + const f = fixture({ + bound: false, + challengeError: new Error("store is down"), + identity: { + redeem: async () => "linked", + discard: async (code) => { + discarded.push(code); + return true; + }, + }, + }); + const result = await run( + f, + observedMessage(`here: link ${IDENTITY_CODE.toUpperCase()}`), + ); + expect(said(result.events)).toBe(""); + expect(discarded).toEqual([IDENTITY_CODE]); + const triggers = triggersOf(f); + expect(triggers).toHaveLength(1); + expect(JSON.stringify(triggers[0]).toLowerCase()).not.toContain( + IDENTITY_CODE, + ); +}); test("an ordinary observed channel message reaches triggers unchanged and discards nothing", async () => { const { f, discarded } = identityFixture("linked"); const result = await run(f, observedMessage("deploy is broken"));