diff --git a/CHANGELOG.md b/CHANGELOG.md index bed2c0e35e..bdc01c7800 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,14 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. request for an administrator to switch on a work app, grant it to a Bot, or connect a Shared app's team account is kept, with who answered it and when. It starts empty and nothing is backfilled. +### A Bot's Slack or Teams link code works only in a direct message + +The `link ` message a Bot's Reaching you shows now links 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. + ### 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 4a6a7f00c7..c15c980d64 100644 --- a/app/src/components/delivery/reaching-you.tsx +++ b/app/src/components/delivery/reaching-you.tsx @@ -513,7 +513,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 2f0af4edf3..bf84acc5f2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -476,7 +476,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. 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 50ee2e7bc3..7e8c90fee2 100644 --- a/server/src/delivery/opentag-agent.ts +++ b/server/src/delivery/opentag-agent.ts @@ -116,8 +116,8 @@ 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 each one is cancelled, quietly (an observe run never replies). 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. */ @@ -128,22 +128,36 @@ 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 { + for (const code of codes) { + if (!code) continue; + try { + await cancelReachabilityCode(deps, code); + if (sender.transport === "slack" && deps.identity) await deps.identity.discard(code); - } catch (error) { - console.error( - JSON.stringify({ - type: "opentag-link-code-discard-error", - errorType: error instanceof Error ? error.name : "UnknownError", - }), - ); - } + } catch (error) { + console.error( + JSON.stringify({ + type: "opentag-link-code-discard-error", + errorType: error instanceof Error ? error.name : "UnknownError", + }), + ); } + } return text.replace(LINKS, "link [code removed]"); } + +/** 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"; @@ -408,6 +422,14 @@ 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. + if (challenge && !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 @@ -415,9 +437,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 3602a981fe..0e20b30d82 100644 --- a/server/tests/delivery-routes.test.ts +++ b/server/tests/delivery-routes.test.ts @@ -36,6 +36,7 @@ function fixture( ) { const calls: unknown[] = []; const decisions: unknown[] = []; + const consumed: unknown[] = []; const twilio = createTwilioTransport({ accountSid: "AC1", authToken: "token", @@ -83,7 +84,10 @@ function fixture( address: null, } : null, - consumeChallenge: async () => true, + consumeChallenge: async (...input) => { + consumed.push(input); + return true; + }, bind: async (input) => { calls.push({ bind: input }); return { ...bound, ...input }; @@ -139,7 +143,7 @@ function fixture( inbox: async () => ({ questions: [] }), }, }); - return { routes, calls, decisions }; + return { routes, calls, decisions, consumed }; } const sender = ( user = "slack:T1:U1", @@ -322,15 +326,41 @@ 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); }); const observedMessage = (content: string) => ({ context: [ @@ -368,6 +398,22 @@ 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("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"));