diff --git a/.env.example b/.env.example index ed7a9c2f4..545f48d63 100644 --- a/.env.example +++ b/.env.example @@ -121,6 +121,20 @@ FRONTEND_BASE_URL=http://localhost:5173 # deeplink. # GATEWAY_PUBLIC_URL=http://localhost:8000 +# Public origin a messaging platform reaches Switch on: the base of the OAuth +# redirect and the event URLs under /messaging for the distributed Slack app. +# Must be https and must resolve from the internet, so it is usually a +# different host from GATEWAY_PUBLIC_URL. Required to offer workspace installs. +# MESSAGING_PUBLIC_URL=https://switch.example.com + +# Credentials of the distributed Slack app — the one a customer installs by +# clicking a button, not the one an operator registers for themselves (that is +# SLACK_BOT_TOKEN et al). Set all three or none; setting some is refused at +# startup. See docs/old/bridges/SLACK_DISTRIBUTED_APP.md. +# SLACK_APP_CLIENT_ID= +# SLACK_APP_CLIENT_SECRET= +# SLACK_APP_SIGNING_SECRET= + # ── Mattermost (local dev) ─────────────────────────────────────────────────── MATTERMOST_ADMIN_USER=admin MATTERMOST_ADMIN_PASSWORD= diff --git a/console/apps/switch-console-desktop/src/main/core/managed-switch-server/env-file.test.ts b/console/apps/switch-console-desktop/src/main/core/managed-switch-server/env-file.test.ts index c0d3bdc82..63ca8994a 100644 --- a/console/apps/switch-console-desktop/src/main/core/managed-switch-server/env-file.test.ts +++ b/console/apps/switch-console-desktop/src/main/core/managed-switch-server/env-file.test.ts @@ -90,9 +90,24 @@ describe('buildEnvFile', () => { const interpolated = new Set( [...composeBody.matchAll(/\$\{([A-Z_][A-Z0-9_]*)/g)].map((m) => m[1]) ); - // Nothing is exempt today. An entry here must say why the stack is correct - // without it — leaving a var unset is a decision, not a default. - const intentionallyUnset = new Set(); + // An entry here must say why the stack is correct without it — leaving a + // var unset is a decision, not a default. + const intentionallyUnset = new Set([ + // The four below configure switch-core as a distributed messaging app — + // one app we own, installed by a customer into their own workspace, with + // the platform posting events to URLs declared once in the app manifest. + // A managed stack cannot be one of those and is not meant to be: it binds + // to loopback, so no platform can reach its callback or event URLs, and + // the credentials are the app owner's rather than anything this machine + // could hold. switch-core registers no installer without them and the + // operator UI says so rather than offering a button that would fail at + // Slack. Connecting a workspace from here is the other path — an operator + // registering a bridge with their own app's token. + 'MESSAGING_PUBLIC_URL', + 'SLACK_APP_CLIENT_ID', + 'SLACK_APP_CLIENT_SECRET', + 'SLACK_APP_SIGNING_SECRET', + ]); const missing = [...interpolated] .filter((key) => !intentionallyUnset.has(key)) diff --git a/console/apps/switch-console-desktop/src/main/core/managed-switch-server/resources/standalone-docker-compose.pinned.yml b/console/apps/switch-console-desktop/src/main/core/managed-switch-server/resources/standalone-docker-compose.pinned.yml index 8830b9439..1df73b93a 100644 --- a/console/apps/switch-console-desktop/src/main/core/managed-switch-server/resources/standalone-docker-compose.pinned.yml +++ b/console/apps/switch-console-desktop/src/main/core/managed-switch-server/resources/standalone-docker-compose.pinned.yml @@ -214,6 +214,10 @@ services: GATEWAY_ADMIN_PASSWORD: ${GATEWAY_ADMIN_PASSWORD} FRONTEND_BASE_URL: ${FRONTEND_BASE_URL} GATEWAY_PUBLIC_URL: ${GATEWAY_PUBLIC_URL:-} + MESSAGING_PUBLIC_URL: ${MESSAGING_PUBLIC_URL:-} + SLACK_APP_CLIENT_ID: ${SLACK_APP_CLIENT_ID:-} + SLACK_APP_CLIENT_SECRET: ${SLACK_APP_CLIENT_SECRET:-} + SLACK_APP_SIGNING_SECRET: ${SLACK_APP_SIGNING_SECRET:-} depends_on: postgres: condition: service_healthy diff --git a/core/switch_core/bridges/agent/auth.py b/core/switch_core/bridges/agent/auth.py index fac0bb763..5fa9a4f11 100644 --- a/core/switch_core/bridges/agent/auth.py +++ b/core/switch_core/bridges/agent/auth.py @@ -12,6 +12,9 @@ from switch_core.bridges.agent.api_key_cache import ApiKeyCache from switch_core.bridges.agent.registration_bootstrap import REGISTRATION_KEY_TYPES +from switch_core.bridges.collaboration.install import ( + PUBLIC_PATH_PREFIX as MESSAGING_INSTALL_PREFIX, +) from switch_core.db.models import Agent, ApiKey from switch_core.db.session_scope import tenant_session from switch_core.db.stores.agent_store import AgentStore @@ -37,6 +40,11 @@ # Public switchdash:// deeplink HTTP redirect — followed by whoever clicks # the "Open in Switch Console" link in an external channel, so no bearer token. "/deeplink", + # Workspace installs of the distributed messaging apps: the OAuth callback + # and the platforms' event webhooks. Unauthenticated by nature — an inbound + # Slack event carries no credential of ours — so each route proves its own + # origin from the platform's signature before it does anything else. + MESSAGING_INSTALL_PREFIX, ) diff --git a/core/switch_core/bridges/collaboration/adapter.py b/core/switch_core/bridges/collaboration/adapter.py index e4df7bea6..36e16f7d4 100644 --- a/core/switch_core/bridges/collaboration/adapter.py +++ b/core/switch_core/bridges/collaboration/adapter.py @@ -5,7 +5,7 @@ from abc import ABC, abstractmethod from collections.abc import Awaitable, Callable from dataclasses import dataclass, replace -from typing import ClassVar +from typing import Any, ClassVar from switch_core.agent_display_name import defuse_label_markup from switch_core.agent_icon import default_icon_url @@ -20,6 +20,7 @@ InboundMessage, InboundUserJoin, OutboundAttachment, + WebhookDeliveryUnsupported, ) logger = logging.getLogger(__name__) @@ -248,6 +249,29 @@ async def start( @abstractmethod async def stop(self) -> None: ... + async def dispatch_event( + self, *, envelope_type: str, payload: dict[str, Any] + ) -> None: + """Handle one event that arrived over the public webhook. + + Concrete on the base and raising, rather than abstract, because + receiving events over HTTP is a property of a platform and of which app + a bridge's token came from — most adapters dial out and are handed + their events on a connection they opened, and have nothing to override + here. + + Raising rather than returning quietly matters: the caller is a route + that has already proved the request genuine and resolved which bridge + it belongs to, so reaching an adapter that cannot take it means a + workspace's traffic is being delivered nowhere. Silence there is the + failure that reads as "the platform has gone quiet". + """ + raise WebhookDeliveryUnsupported( + f"{type(self).__name__} does not receive events over HTTP, so the " + "event posted for this bridge cannot be delivered. A bridge reached " + "this way was installed as a distributed app; this one was not." + ) + @abstractmethod async def send_message( self, diff --git a/core/switch_core/bridges/collaboration/install.py b/core/switch_core/bridges/collaboration/install.py new file mode 100644 index 000000000..ceb5c16a8 --- /dev/null +++ b/core/switch_core/bridges/collaboration/install.py @@ -0,0 +1,351 @@ +"""Installing *our* app into someone else's workspace. + +A `CollaborationAdapter` is what a bridge runs; this is what happens before +there is one. The distinction is not organisational — the two have genuinely +different lifetimes and genuinely different secrets: + +- An adapter is per bridge, built from a `connection_config` that already + holds a working credential, and it exists only while that bridge runs. +- An installer is per deployment, built once from the credentials of the app + *we* registered with the platform, and it exists whether or not any bridge + does. It is what turns a click on "Add to Slack" into a credential, and what + proves an inbound webhook came from the platform rather than from anyone who + found the URL. + +So an installer is not a classmethod on the adapter. Making it one would mean +threading a client secret through every call, and would put "which app did we +register with Slack" on an object whose whole scope is one customer's bridge. + +**The seam between the two is `connection_config`.** An installer's last act +is to render the grant into exactly the dict the platform's adapter already +takes, so nothing downstream of the install knows an install happened: +`CollaborationBridgeLifecycleService.register` validates and starts it the way +it does a bridge an operator typed in by hand. That is deliberate — an +installed bridge and a self-registered one differ in where the token came +from, and in nothing else. + +**Registration is the feature flag.** There is no `installs_enabled` setting. +An installer exists for a platform when that platform's app credentials are +configured, and the endpoints refuse when it does not — a deployment that has +not registered an app cannot half-offer installs. +""" + +from __future__ import annotations + +from abc import ABC, abstractmethod +from collections.abc import Mapping +from dataclasses import dataclass +from typing import Any, ClassVar, Literal + +#: The public prefix every install endpoint hangs off. +#: +#: Its own prefix rather than a corner of an existing one, because everything +#: under it is unauthenticated by nature — a Slack event carries no credential +#: of ours, and a callback arrives before there is anything to authenticate +#: against. Grouping them makes that one property of the prefix rather than of +#: each route. +#: +#: Deliberately not under `/gateway`, which is cookie-authenticated and is not +#: routed to this application from outside; and deliberately not under +#: `/oauth`, which already belongs to agents authenticating *to* Switch and +#: would share only the word. +PUBLIC_PATH_PREFIX = "/messaging" + + +def oauth_callback_path(platform: str) -> str: + return f"{PUBLIC_PATH_PREFIX}/{platform}/oauth/callback" + + +def events_path(platform: str) -> str: + return f"{PUBLIC_PATH_PREFIX}/{platform}/events" + + +def interactive_path(platform: str) -> str: + return f"{PUBLIC_PATH_PREFIX}/{platform}/interactive" + + +def commands_path(platform: str) -> str: + return f"{PUBLIC_PATH_PREFIX}/{platform}/commands" + + +def public_url(public_origin: str, path: str) -> str: + """Absolute URL for one install path, given the deployment's public origin. + + The origin is `MESSAGING_PUBLIC_URL`, which is validated at startup as + scheme and host with no path, so this is a join and not a merge. It exists as a + function so the redirect URI sent to the platform and the one registered + with the app are built the same way — the platform compares them exactly, + and a trailing slash on one side is a refused install with a message that + does not say so. + """ + return f"{public_origin.rstrip('/')}{path}" + + +class MessagingInstallError(RuntimeError): + """An install could not be completed, with a reason fit to show an operator. + + Raised rather than returned for the usual reason: every caller of these + methods is a request handler that must not continue on a failure, and a + falsy return is the kind of thing a caller forgets to check. + """ + + +class WebhookAuthenticityError(RuntimeError): + """An inbound webhook did not prove it came from the platform. + + Separate from `MessagingInstallError` because the two are answered + differently: an install failure is shown to the operator who caused it, + while this one is a request from an unknown party and gets a bare 401 with + nothing in it. Never log the body alongside this — it is unauthenticated + input. + """ + + +class WebhookPayloadError(RuntimeError): + """A webhook proved genuine and then could not be read. + + Distinct from :class:`WebhookAuthenticityError` because it says something + entirely different: the signature checked out, so this really is the + platform, and what it sent is a shape this build does not know. That is a + fault worth seeing in a log — an unrecognised body is how a platform's + change first shows up — where a bad signature is just the internet. + """ + + +#: Which of a platform's inbound endpoints a request arrived on. +#: +#: Three, because that is what the platforms ask for and what the app manifests +#: declare: events, interactivity, and slash commands. They are separate URLs +#: rather than one, because a platform decides that, not us — and they carry +#: genuinely different bodies (Slack posts JSON to the first and a form to the +#: other two), which is why the endpoint is an argument to parsing rather than +#: something a handler could infer. +WebhookEndpoint = Literal["events", "interactive", "commands"] + + +@dataclass(frozen=True) +class InboundWebhook: + """One authenticated inbound event, in the shape a running adapter takes. + + `envelope_type` and `payload` are deliberately the two arguments Socket + Mode's own listener is handed, so an event that arrived over HTTP and the + same event over a socket reach `dispatch_event` indistinguishable from one + another. Anything that made them differ would be two code paths for one + behaviour, drifting apart at the speed of whichever gets used more. + + `handshake` is the exception, and it is not an event at all: a platform + proving the URL it was given is really ours (Slack's `url_verification`) + expects a specific string echoed straight back and nothing dispatched. It + is `None` for every real event, and it arrives before any workspace has + installed anything — so it must be answerable with no tenant, no install + row, and nothing running. + + `external_event_id` is the platform's own id for this delivery, and the + only thing two copies of one event have in common — the payload is + identical, so nothing else could tell a retry from a second message saying + the same words. `None` means the platform does not number this kind of + envelope, which on Slack means the kind it also does not retry; it never + means "this one was not checked". + + `delivery_attempt` is how many times the platform has given up on us and + sent this again, zero on a first delivery. It changes nothing about how the + event is handled — the receipt decides that — and is carried because it is + the only place the deployment is told its own acknowledgements are arriving + too late. + """ + + envelope_type: str + payload: dict[str, Any] + handshake: str | None + external_event_id: str | None + delivery_attempt: int + + +@dataclass(frozen=True) +class InstallGrant: + """What the platform handed back when a workspace installed us. + + Deliberately four fields and not the platform's whole response. What a + grant *is*, across platforms, is a workspace, a credential, and the + permissions that credential was actually given — everything else in the + response is Slack's shape and belongs behind `connection_config`. + + `workspace_name` is the exception, and it earns its place by being the only + thing here a person recognises. It names the bridge in the operator's list, + where the alternative is a row of opaque platform ids. It is the customer's + own text and is never matched on. + + `scopes` is the platform's own spelling, kept verbatim. A scope string that + means nothing to us is still the thing to show an operator asking why a + call was refused, and parsing it into a list here would be a parser to keep + in step with someone else's vocabulary for no gain. + """ + + external_workspace_id: str + workspace_name: str + bot_token: str + scopes: str + + +class MessagingAppInstaller(ABC): + """The install half of one platform, holding that platform's app credentials. + + One instance per platform per deployment, built at boot from config and + registered by platform name. Every method is deliberately synchronous + except the code exchange, which is the only one that talks to the network. + """ + + #: The platform this installs, matching the adapter registry's key and the + #: `platform` column on `messaging_installs`. + platform: ClassVar[str] + + @abstractmethod + def authorize_url(self, *, state: str, redirect_uri: str) -> str: + """Where to send the browser to begin an install. + + `state` is opaque here and is not the installer's to interpret: it is + minted, signed and redeemed by the install service, and this method's + only obligation is to hand it back to the platform unchanged. + """ + + @abstractmethod + async def redeem(self, *, code: str, redirect_uri: str) -> InstallGrant: + """Exchange the authorization code the callback carried for a credential. + + `redirect_uri` is passed again because the platform checks it matches + the one the flow started with — it is part of the proof, not a + convenience. + + Raise :class:`MessagingInstallError` on anything short of a usable + grant, including a well-formed response the platform marked as failed. + """ + + @abstractmethod + async def revoke(self, *, bot_token: str) -> None: + """Tell the platform the credential it granted us is finished with. + + Called when an operator disconnects an install. Deleting our copy is + not the same act: the token stays valid at the platform, so a dump + taken before the disconnect would still hold a working key into a + customer's workspace. This is the half only the platform can do. + + **A token the platform already considers dead is a success, not a + failure.** The common reason to be disconnecting at all is that the + customer removed the app on their side, and an implementation that + raised on "this token is already invalid" would make exactly that + install impossible to disconnect, forever. + + Raise :class:`MessagingInstallError` for anything else — a refusal we + do not understand leaves a live credential behind and the operator + should hear about it rather than see a disconnect that reports + success. + + Not the same thing as the platform's own uninstall. Revoking a token + does not remove the app from the workspace; it ends this deployment's + access with it. + """ + + @abstractmethod + def verify_webhook(self, *, headers: Mapping[str, str], body: bytes) -> None: + """Prove an inbound event came from the platform, or raise. + + Takes the **raw body**, not a parsed payload, because every platform's + signature covers the bytes as sent: re-serialising a parsed dict + produces different bytes and a signature that never verifies. + + This is the first thing any webhook handler does — before parsing, + before resolving a tenant, before logging the body. Raise + :class:`WebhookAuthenticityError`. + """ + + @abstractmethod + def parse_webhook( + self, *, endpoint: WebhookEndpoint, headers: Mapping[str, str], body: bytes + ) -> InboundWebhook: + """Read a verified request into an event a running adapter takes. + + Called only after :meth:`verify_webhook` has passed, and separate from + it for exactly that reason: parsing before verifying is how an + unauthenticated body gets to choose which code runs. + + Takes the raw bytes rather than a parsed payload because only this + method knows the encoding, which is per platform and per endpoint — + Slack posts JSON to one of its three and form data to the other two. + + Takes the headers as well as the body because a delivery is described + in both: the event is in the body, and how many times it has been sent + is in a header. Which header, and whether there is one, is the + platform's business rather than the route's. + + Raise :class:`WebhookPayloadError` for a body that cannot be read. + """ + + @abstractmethod + def workspace_of_event(self, payload: Mapping[str, object]) -> str: + """Which workspace an authenticated event came from. + + The answer is what resolves a tenant, so this runs on a request with + nothing bound and must not touch the database. Raise + :class:`WebhookPayloadError` for a payload that names no workspace: an + event we cannot route is not an event we may guess at. + """ + + @abstractmethod + def revocation_of_event(self, payload: Mapping[str, object]) -> str | None: + """Why this event says the install is over, or `None` if it does not. + + Platforms report the end of an install as an ordinary event on the + ordinary endpoint, which makes it easy to treat as one: the app is + removed from a workspace, an event arrives saying so, nothing reads + it, and the deployment goes on holding a dead token, a running bridge + and a claim on a workspace whose owner believes they have left. + + A reason rather than a flag because it is the thing worth logging — an + operator asking why their bridge stopped needs the platform's own + answer, and there is more than one way an install can end. + + Runs on a payload that has been authenticated and not yet routed, so + like :meth:`workspace_of_event` it must not touch the database. + """ + + @abstractmethod + def connection_config(self, grant: InstallGrant) -> dict[str, object]: + """Render a grant as the connection config this platform's adapter takes. + + The whole point of the abstraction: after this, an installed bridge is + indistinguishable from one an operator registered by hand, and every + line of lifecycle, validation and start-up code is shared. + """ + + +class MessagingInstallerRegistry: + """The installers this deployment has app credentials for. + + A dict with a loud `__getitem__`, which is the only behaviour worth a class + here: asking for a platform nobody registered an app for is the ordinary + case (most deployments will register none), and it has to produce a + sentence an operator can act on rather than a `KeyError` in a traceback. + """ + + def __init__(self) -> None: + self._installers: dict[str, MessagingAppInstaller] = {} + + def register(self, installer: MessagingAppInstaller) -> None: + if installer.platform in self._installers: + raise MessagingInstallError( + f"an installer for {installer.platform!r} is already registered" + ) + self._installers[installer.platform] = installer + + def get(self, platform: str) -> MessagingAppInstaller: + try: + return self._installers[platform] + except KeyError: + raise MessagingInstallError( + f"no {platform} app is registered with this deployment, so it " + "cannot be installed into a workspace. Configure the app's " + "credentials and restart." + ) from None + + def platforms(self) -> list[str]: + return sorted(self._installers) diff --git a/core/switch_core/bridges/collaboration/install_routes.py b/core/switch_core/bridges/collaboration/install_routes.py new file mode 100644 index 000000000..fb8818228 --- /dev/null +++ b/core/switch_core/bridges/collaboration/install_routes.py @@ -0,0 +1,321 @@ +"""The public half of an install: the OAuth callback, and the events after it. + +Everything here is unauthenticated in the sense that matters — no caller holds +a credential of ours. A browser mid-redirect proves itself with a state we +signed; a platform posting an event proves itself with a signature over the +raw body. Neither is a session, and nothing here may assume a tenant before it +has established one. + +Mounted on the agent-bridge app rather than inside `/gateway`, because this is +the leg the outside world reaches. `/gateway` is not routed here from the +public load balancer at all, and it would not help if it were — it is +cookie-authenticated and this caller has no cookie of ours. + +**The reply is a page, not a redirect.** Sending the browser on to the gateway +would work today, when the person installing is an operator who can reach it, +and would break the moment the same flow is offered to a customer who cannot: +the gateway is on a private hostname and the callback is not. So the outcome is +rendered here, in one self-contained page, on the origin the browser already +reached. It is deliberately plain; when there is a place to send people, this +becomes a redirect and the page becomes its fallback. + +The event routes answer nobody who reads English, so they answer in status +codes and say the rest in the log. +""" + +from __future__ import annotations + +import html +import logging + +from fastapi import APIRouter, BackgroundTasks, Query, Request, Response +from starlette.responses import HTMLResponse, PlainTextResponse + +from switch_core.bridges.collaboration.install import ( + PUBLIC_PATH_PREFIX, + InboundWebhook, + MessagingInstallError, + WebhookAuthenticityError, + WebhookEndpoint, + WebhookPayloadError, +) +from switch_core.bridges.collaboration.install_service import ( + InstallPlatformMismatch, + MessagingInstallService, + Revocation, + WebhookBridgeUnavailable, + WebhookTarget, + WebhookWorkspaceUnknown, +) +from switch_core.bridges.collaboration.install_state import InstallStateError +from switch_core.db.stores.messaging_install_store import ( + MessagingInstallClaimedError, + MessagingInstallStateError, +) + +logger = logging.getLogger(__name__) + +_PAGE = """ + +{title} + +

{title}

{detail}

+""" + + +def _page(*, title: str, detail: str, status: int) -> HTMLResponse: + return HTMLResponse( + _PAGE.format(title=html.escape(title), detail=html.escape(detail)), + status_code=status, + ) + + +def create_messaging_install_router( + service: MessagingInstallService, +) -> APIRouter: + """Build the public install routes over one already-configured service. + + A factory closing over the service rather than a module-level router with + dependencies, because this app has no dependency-injection module of its + own and adding one for a single object would be the larger change. + """ + router = APIRouter(prefix=PUBLIC_PATH_PREFIX) + + @router.get("/{platform}/oauth/callback") + async def oauth_callback( + platform: str, + code: str | None = Query(default=None), + state: str | None = Query(default=None), + error: str | None = Query(default=None), + ) -> HTMLResponse: + # The platform's own refusal, which is usually the customer deciding + # not to install after all. Nothing went wrong here and nothing was + # written; saying so is the whole handling. + if error: + return _page( + title="Install cancelled", + detail=f"{platform} reported: {error}. Nothing was connected.", + status=200, + ) + + if not code or not state: + return _page( + title="Install could not be completed", + detail=( + f"{platform} did not send back everything this needs. Start " + "the install again from Switch." + ), + status=400, + ) + + try: + install = await service.complete( + platform=platform, code=code, state_token=state + ) + except (InstallStateError, InstallPlatformMismatch) as failure: + # Unauthenticated input, so the reply says nothing the caller did + # not already know. The reason goes to the log, at warning: a + # forged state is worth noticing and is not worth an alert. + logger.warning("Refused a %s install callback: %s", platform, failure) + return _page( + title="Install could not be completed", + detail=( + "This install link is not one Switch recognises. Start the " + "install again from Switch." + ), + status=400, + ) + except MessagingInstallStateError as failure: + return _page( + title="Install link already used", + detail=str(failure), + status=400, + ) + except MessagingInstallClaimedError as failure: + return _page( + title="Workspace already connected", + detail=str(failure), + status=409, + ) + except MessagingInstallError as failure: + return _page( + title="Install could not be completed", + detail=str(failure), + status=400, + ) + + return _page( + title="Switch is connected", + detail=( + f"The {platform} workspace {install.external_workspace_id} is " + "connected to Switch. You can close this page and finish setting " + "it up there." + ), + status=200, + ) + + async def _deliver(target: WebhookTarget, event: InboundWebhook) -> None: + """Handle one event after the platform has been answered. + + Its own wrapper so a failure is logged here rather than raised into the + server's background-task machinery, where it would surface — if at all + — as an unattributed traceback with nothing in it naming the bridge. + """ + try: + await service.deliver(target, event) + except Exception: + logger.exception( + "Failed to handle a %s event for bridge %s (tenant %s)", + event.envelope_type, + target.bridge_id, + target.tenant_id, + ) + + async def _end_install(platform: str, revocation: Revocation) -> None: + """Act on the platform's news after it has been acknowledged. + + Logged and not raised for the same reason as `_deliver`, and with more + at stake: the platform redelivers what it gets no answer to, and a + traceback out of a background task would leave a dead install claiming + a workspace with nothing in the log naming it. + """ + try: + await service.revoked( + platform=platform, + workspace_id=revocation.workspace_id, + reason=revocation.reason, + ) + except Exception: + logger.exception( + "Failed to end the install of %s workspace %s after the platform " + "reported it was over (%s)", + platform, + revocation.workspace_id, + revocation.reason, + ) + + async def _inbound( + platform: str, + endpoint: WebhookEndpoint, + request: Request, + background: BackgroundTasks, + ) -> Response: + """One verified event, from any of a platform's three inbound URLs. + + The status codes are read by the platform, not by a person, and they + are chosen for what it does with them. Slack retries a 5xx and gives up + on a 4xx, and counts failures against the app as a whole — so a + permanent condition must not look transient, and a transient one must + not look permanent. + """ + body = await request.body() + try: + event = service.authenticate( + platform=platform, + endpoint=endpoint, + headers=dict(request.headers), + body=body, + ) + except MessagingInstallError: + # No app registered for this platform, so nothing here could have + # signed anything. Not found rather than an explanation: the caller + # is unauthenticated and learns only that there is nothing here. + return Response(status_code=404) + except WebhookAuthenticityError as failure: + logger.warning("Refused an unverified %s webhook: %s", platform, failure) + return Response(status_code=401) + except WebhookPayloadError as failure: + # Verified, so this really is the platform sending something this + # build cannot read. Worth an error rather than a shrug — it is how + # a platform's change to its own payloads first becomes visible. + logger.error("Could not read a verified %s webhook: %s", platform, failure) + return Response(status_code=400) + + if event.handshake is not None: + logger.info("Answered a %s URL verification", platform) + return PlainTextResponse(event.handshake) + + if event.delivery_attempt > 0: + # The only signal this deployment gets that its own acknowledgements + # are arriving too late. The event itself is handled normally — the + # receipt decides whether it is a duplicate — but a run of these is + # the platform saying the three-second answer is being missed, and + # nothing else in the system would say so. + logger.warning( + "%s is re-sending a %s event (attempt %s), which means an earlier " + "delivery was not acknowledged in time", + platform, + event.envelope_type, + event.delivery_attempt, + ) + + try: + # Before resolving, because this is the one event that arrives as + # the bridge it would be resolved to is going away. + revocation = service.revocation(platform=platform, event=event) + if revocation is not None: + background.add_task(_end_install, platform, revocation) + return Response(status_code=200) + + target = await service.resolve(platform=platform, event=event) + except WebhookPayloadError as failure: + logger.error( + "A verified %s event named no workspace: %s", platform, failure + ) + return Response(status_code=400) + except WebhookWorkspaceUnknown as failure: + # A 200 for an event that reached nobody, which is the one place + # this file answers something other than what happened. The app + # left behind in a workspace whose install ended goes on posting, + # the platform cannot act on a 404, and it counts the refusals + # against the app as a whole — so the honest answer costs every + # other customer's delivery. The log is where it is visible. + logger.warning("Dropped a %s event: %s", platform, failure) + return Response(status_code=200) + except WebhookBridgeUnavailable as failure: + # Deliberately a 503: the platform retrying is the right behaviour + # while a bridge restarts, and a 200 here would drop a real message + # on the floor and report that it had been handled. + logger.error("Could not deliver a %s event: %s", platform, failure) + return Response(status_code=503) + + # Answered first, handled after. The platform's deadline is short and + # what happens next is not bounded by it — a turn can take minutes — + # so acknowledging on the way out is what keeps a slow room from + # becoming a retried, duplicated one. + background.add_task(_deliver, target, event) + return Response(status_code=200) + + @router.post("/{platform}/events") + async def events( + platform: str, request: Request, background: BackgroundTasks + ) -> Response: + return await _inbound(platform, "events", request, background) + + @router.post("/{platform}/interactive") + async def interactive( + platform: str, request: Request, background: BackgroundTasks + ) -> Response: + """Interactions with a message this app posted. + + Routed like any other event and, on Slack today, handled by nothing: + the app declares an interactivity URL because features it does use + require one, and its stop button arrives as an ordinary event instead. + The route exists because the URL is declared — a declared URL that 404s + counts against the app — and because the alternative is deciding here, + rather than in the adapter, what a platform's interactions mean. + """ + return await _inbound(platform, "interactive", request, background) + + @router.post("/{platform}/commands") + async def commands( + platform: str, request: Request, background: BackgroundTasks + ) -> Response: + return await _inbound(platform, "commands", request, background) + + return router diff --git a/core/switch_core/bridges/collaboration/install_service.py b/core/switch_core/bridges/collaboration/install_service.py new file mode 100644 index 000000000..062d3f678 --- /dev/null +++ b/core/switch_core/bridges/collaboration/install_service.py @@ -0,0 +1,576 @@ +"""The two legs of an install, and the order they have to happen in. + +An install is one operation split across two requests that share no session, +no origin and no credential — only the state token. The first leg is +authenticated and decides the tenant; the second is a stranger arriving from +the platform. Everything here is about that asymmetry. + +The finishing order is load-bearing and not the obvious one: + +1. **Verify the signature, then bind the tenant.** Nothing before this touches + the database, because until the signature verifies there is no tenant to + bind and RLS would refuse every statement anyway. +2. **Burn the state, and commit.** Before talking to the platform, not after. + Redeeming first would leave a window in which a replayed callback is still + redeemable, and burning inside the same transaction as the rest would hold + a row lock across a call to someone else's API. The cost is that a platform + outage burns the link — the customer starts a ten-second flow again, and + the property we keep in exchange is that a captured state is worth nothing. +3. **Exchange the code**, which is the only network call. +4. **Claim the workspace**, which is where a workspace already held by another + tenant fails, in the database rather than in a check above it. +5. **Register the bridge** from the rendered connection config, exactly as if + an operator had typed the token in, and point the install row at it. + +Step 5 last is deliberate too: a bridge that exists with no install row behind +it is an orphan nothing can revoke, whereas an install row with no bridge is a +recorded credential waiting to be used, which is a state the schema already +allows for. + +**Ending one runs the same steps backwards, and there are two ways in.** An +operator disconnects here, or the platform tells us the app is gone. They +differ in exactly one step — whether there is a live token to revoke — and in +nothing else, because both have to leave the same state behind: no bridge, no +stored credential, a released workspace and a row saying what happened. A +deployment that handled only the first would go on holding a dead token and a +claim on a workspace whose owner believes they have left. + +The revoking order is: tell the platform first, then destroy things. A +revocation we do not understand then leaves the install exactly as it was and +the operator can try again, where the other order would have removed the +bridge and left a working key into a customer's workspace in whatever dump was +taken next. + +Then the row, and the bridge last — the reverse of the building order and for +the same reason read backwards. The install's pointer at the bridge is a real +foreign key, so the row has to let go before the bridge can be deleted at all; +and if the deletion then fails, what is left is a bridge with no credential +that an operator can see and remove, rather than an install still claiming a +workspace it has already been thrown out of. +""" + +from __future__ import annotations + +import logging +from collections.abc import Mapping +from dataclasses import dataclass + +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from switch_core.bridges.collaboration.adapter import CollaborationAdapter +from switch_core.bridges.collaboration.install import ( + InboundWebhook, + MessagingAppInstaller, + MessagingInstallerRegistry, + WebhookEndpoint, + oauth_callback_path, + public_url, +) +from switch_core.bridges.collaboration.install_state import ( + InstallState, + mint, + verify, +) +from switch_core.bridges.collaboration.lifecycle_service import ( + CollaborationBridgeLifecycleService, +) +from switch_core.crypto import decrypt_token, encrypt_token +from switch_core.db.models import MessagingInstall +from switch_core.db.session_scope import tenant_session +from switch_core.db.stores.messaging_event_store import MessagingEventReceiptStore +from switch_core.db.stores.messaging_install_store import ( + INSTALL_ACTIVE, + INSTALL_DISCONNECTED, + INSTALL_REVOKED, + MessagingInstallStore, +) +from switch_core.db.tenant_lookup import tenant_of_messaging_install +from switch_core.tenant_context import no_tenant, tenant_scope + +logger = logging.getLogger(__name__) + + +class WebhookWorkspaceUnknown(RuntimeError): + """An authentic event named a workspace no tenant here has installed. + + Ordinary rather than alarming: an app left in a workspace whose install was + removed goes on posting for as long as someone leaves it there. It is still + an error here, because a resolver that returned nothing would make "nobody + holds this" and "here is where it goes" the same shape — but it is one the + route absorbs rather than reports, since the platform cannot fix it and + telling it repeatedly that its posts fail is held against the app itself. + """ + + +class WebhookBridgeUnavailable(RuntimeError): + """The workspace resolves to a tenant, and nothing is running to take it. + + Transient by nature — a bridge mid-restart, or one that has not been built + for a recorded install yet — so it is worth telling the platform to try + again rather than swallowing the event. + """ + + +@dataclass(frozen=True) +class Revocation: + """A platform saying, in an ordinary event, that an install is over.""" + + workspace_id: str + reason: str + + +@dataclass(frozen=True) +class WebhookTarget: + """Where one verified event goes: a tenant, a bridge, and its live adapter. + + `platform` is carried rather than passed alongside because it is part of + the answer: the same workspace id could in principle be issued by two + platforms, and every row written about this delivery is keyed by the pair. + """ + + tenant_id: str + platform: str + bridge_id: str + adapter: CollaborationAdapter + + +class InstallPlatformMismatch(RuntimeError): + """A state minted for one platform arrived at another's callback. + + Only reachable with a valid signature, so this is not an attack so much as + a deployment that has crossed its own wires — but it would otherwise redeem + a real state against the wrong installer, and the store's own platform + predicate would then refuse it with a message about expiry that is not + true. + """ + + +class MessagingInstallService: + def __init__( + self, + *, + session_factory: async_sessionmaker[AsyncSession], + store: MessagingInstallStore, + receipts: MessagingEventReceiptStore, + installers: MessagingInstallerRegistry, + lifecycle: CollaborationBridgeLifecycleService, + public_origin: str, + secret: str, + ) -> None: + self._session_factory = session_factory + self._store = store + self._receipts = receipts + self._installers = installers + self._lifecycle = lifecycle + self._public_origin = public_origin + self._secret = secret + + def _redirect_uri(self, platform: str) -> str: + """Where the platform sends the browser back to. + + Built from the public origin rather than from the incoming request, + because the two legs arrive on different hostnames and the platform + compares this string byte for byte against the one registered with the + app. Deriving it from `request.url` would produce the gateway's + hostname on the first leg and a redirect the platform refuses. + """ + return public_url(self._public_origin, oauth_callback_path(platform)) + + def installer(self, platform: str) -> MessagingAppInstaller: + return self._installers.get(platform) + + def platforms(self) -> list[str]: + return self._installers.platforms() + + async def begin(self, session: AsyncSession, *, platform: str, user_id: str) -> str: + """Start an install and return where to send the browser. + + Runs on the caller's own scoped session, so the tenant recorded is the + tenant they are authenticated for and there is no parameter through + which they could name another. + """ + installer = self._installers.get(platform) + state = await self._store.start_install( + session, platform=platform, user_id=user_id + ) + token = mint( + InstallState( + tenant_id=state.tenant_id, state_id=state.id, platform=platform + ), + secret=self._secret, + ) + return installer.authorize_url( + state=token, redirect_uri=self._redirect_uri(platform) + ) + + async def complete( + self, *, platform: str, code: str, state_token: str + ) -> MessagingInstall: + """Finish an install begun elsewhere, on behalf of nobody in particular. + + The caller is unauthenticated: everything trusted here comes out of the + signature on `state_token` or out of the platform's own response. + """ + installer = self._installers.get(platform) + state = verify(state_token, secret=self._secret) + if state.platform != platform: + raise InstallPlatformMismatch( + f"an install state for {state.platform} was presented to the " + f"{platform} callback" + ) + + with tenant_scope(state.tenant_id): + async with tenant_session( + self._session_factory, state.tenant_id + ) as session: + burnt = await self._store.redeem_state( + session, state_id=state.state_id, platform=platform + ) + await session.commit() + + grant = await installer.redeem( + code=code, redirect_uri=self._redirect_uri(platform) + ) + + async with tenant_session( + self._session_factory, state.tenant_id + ) as session: + install = await self._store.record_install( + session, + platform=platform, + external_workspace_id=grant.external_workspace_id, + encrypted_bot_token=encrypt_token(grant.bot_token, self._secret), + scopes=grant.scopes, + user_id=burnt.created_by_user_id, + ) + install_id = install.id + await session.commit() + + bridge = await self._lifecycle.register( + bridge_type=platform, + display_name=grant.workspace_name, + connection_config=installer.connection_config(grant), + # Off, though the granted scopes would allow it. Nobody was + # asked: an install has no registration form, and letting an + # app create channels in a customer's workspace is a decision + # someone should make rather than inherit. + channel_creation_enabled=False, + ) + + async with tenant_session( + self._session_factory, state.tenant_id + ) as session: + attached = await self._store.attach_bridge( + session, install_id=install_id, bridge_id=bridge.id + ) + await session.commit() + + logger.info( + "Installed %s workspace %s for tenant %s as bridge %s", + platform, + grant.external_workspace_id, + state.tenant_id, + bridge.id, + ) + return attached + + async def list_installs(self, session: AsyncSession) -> list[MessagingInstall]: + """The bound tenant's installs, for the operator's own list. + + On the caller's session like `begin`, and scoped by it: there is no + tenant argument because there is no tenant to choose. + """ + return await self._store.list_for_tenant(session) + + # ── Ending an install ──────────────────────────────────────────────────── + + async def disconnect(self, *, tenant_id: str, install_id: str) -> MessagingInstall: + """End an install because somebody here said to. + + Idempotent: disconnecting an install that has already ended returns it + unchanged. An operator can reach this for a row the platform's own + `app_uninstalled` ended a second earlier, and that is not an error to + show them. + + **Removing the bridge detaches every room that used it**, which become + internal-only. That is the honest consequence of disconnecting a + messaging app and it is not softened here — but it is the reason this + is an explicit action with a confirmation in front of it rather than + something inferred. + """ + async with tenant_session(self._session_factory, tenant_id) as session: + install = await self._store.get(session, install_id=install_id) + platform = install.platform + workspace_id = install.external_workspace_id + bridge_id = install.bridge_id + token = install.encrypted_bot_token + already_ended = install.status != INSTALL_ACTIVE + + if already_ended: + logger.info( + "Install %s of %s workspace %s had already ended; nothing to do", + install_id, + platform, + workspace_id, + ) + async with tenant_session(self._session_factory, tenant_id) as session: + return await self._store.get(session, install_id=install_id) + + with tenant_scope(tenant_id): + if token is not None: + await self._installers.get(platform).revoke( + bot_token=decrypt_token(token, self._secret) + ) + + async with tenant_session(self._session_factory, tenant_id) as session: + ended = await self._store.end( + session, install_id=install_id, status=INSTALL_DISCONNECTED + ) + await session.commit() + + if bridge_id is not None: + await self._lifecycle.remove(bridge_id) + + logger.info( + "Disconnected %s workspace %s for tenant %s", + platform, + workspace_id, + tenant_id, + ) + return ended + + async def revoked(self, *, platform: str, workspace_id: str, reason: str) -> None: + """End an install because the platform said it is over. + + Resolves the workspace itself rather than going through `resolve`, + which is built for delivering an event and insists on a running bridge. + Here a missing bridge is beside the point: the news is that the install + is finished, and a deployment that could only record that while the + bridge happened to be up would keep the dead ones it most needs to + clear. + + A workspace that resolves to nobody is the ordinary case, not a fault. + The platform retries these events, so the second delivery arrives after + the first has already ended the install. + + No revocation call: the token this would revoke is the one the platform + has just told us it killed. + """ + tenant_id = await tenant_of_messaging_install( + self._session_factory, platform, workspace_id + ) + if tenant_id is None: + logger.info( + "Ignoring end-of-install for %s workspace %s (%s): no tenant " + "holds it, so it has already ended", + platform, + workspace_id, + reason, + ) + return + + with tenant_scope(tenant_id): + async with tenant_session(self._session_factory, tenant_id) as session: + install = await self._store.get_for_workspace( + session, platform=platform, external_workspace_id=workspace_id + ) + if install is None: + logger.warning( + "End-of-install for %s workspace %s resolved to tenant " + "%s and could not then be read as that tenant", + platform, + workspace_id, + tenant_id, + ) + return + install_id = install.id + bridge_id = install.bridge_id + + async with tenant_session(self._session_factory, tenant_id) as session: + await self._store.end( + session, install_id=install_id, status=INSTALL_REVOKED + ) + await session.commit() + + if bridge_id is not None: + await self._lifecycle.remove(bridge_id) + + logger.warning( + "Ended the install of %s workspace %s for tenant %s: %s", + platform, + workspace_id, + tenant_id, + reason, + ) + + # ── Inbound events ─────────────────────────────────────────────────────── + # + # The other direction, and the one that runs constantly. Three steps, kept + # separate because each answers to something different: + # + # `authenticate` is pure and does no I/O, so a request that cannot prove + # itself is refused without this deployment doing any work on its behalf. + # `resolve` is two indexed reads and decides *whose* event this is. + # `deliver` is the handling, which can take as long as the work takes. + # + # The split is what lets the route acknowledge in time. Slack gives three + # seconds and retries what it does not get an answer to, so a handler that + # posts to Matrix before replying turns one slow room into duplicate + # messages. Everything up to and including `resolve` is fast enough to + # answer inside, and `deliver` runs after the response has gone. + + def authenticate( + self, + *, + platform: str, + endpoint: WebhookEndpoint, + headers: Mapping[str, str], + body: bytes, + ) -> InboundWebhook: + """Prove an inbound request came from the platform, and read it. + + Verification is first and unconditional. Nothing above it inspects the + body, so an unsigned request cannot pick which parser runs, and nothing + is logged from it either — it is a stranger's bytes until this passes. + """ + installer = self._installers.get(platform) + installer.verify_webhook(headers=headers, body=body) + return installer.parse_webhook(endpoint=endpoint, headers=headers, body=body) + + def revocation(self, *, platform: str, event: InboundWebhook) -> Revocation | None: + """Whether this event is the platform ending the install, and for whom. + + Asked before `resolve` and not after, because the two disagree about + what a missing bridge means. `resolve` insists on one, and the event + most worth reading here is precisely the one that arrives when the + bridge is on its way out — so an uninstall routed through the ordinary + path would be dropped exactly when it mattered. + + Pure, like `authenticate`: it reads the payload and nothing else, so + the route can answer the platform before any of the work begins. + """ + installer = self._installers.get(platform) + reason = installer.revocation_of_event(event.payload) + if reason is None: + return None + return Revocation( + workspace_id=installer.workspace_of_event(event.payload), reason=reason + ) + + async def resolve(self, *, platform: str, event: InboundWebhook) -> WebhookTarget: + """Turn a workspace id into the one bridge entitled to the event. + + The tenant comes from the exempt lookup (`db/tenant_lookup.py`), which + is the only way to answer it: the caller authenticated to nothing, and + every table that could say is scoped. The install row is then read + *again* under that tenant rather than returned by the lookup — a + deliberate second check, so a wrong answer above is a miss here instead + of a cross-tenant read. + """ + installer = self._installers.get(platform) + workspace_id = installer.workspace_of_event(event.payload) + + tenant_id = await tenant_of_messaging_install( + self._session_factory, platform, workspace_id + ) + if tenant_id is None: + raise WebhookWorkspaceUnknown( + f"no tenant has installed Switch into {platform} workspace " + f"{workspace_id}" + ) + + async with tenant_session(self._session_factory, tenant_id) as session: + install = await self._store.get_for_workspace( + session, platform=platform, external_workspace_id=workspace_id + ) + if install is None: + raise WebhookWorkspaceUnknown( + f"the install of {platform} workspace {workspace_id} resolved to " + f"tenant {tenant_id} and could not then be read as that tenant" + ) + if install.bridge_id is None: + raise WebhookBridgeUnavailable( + f"the install of {platform} workspace {workspace_id} has no " + "bridge yet, so there is nothing to deliver its events to" + ) + + adapter = self._lifecycle.get_adapter(install.bridge_id) + if adapter is None: + raise WebhookBridgeUnavailable( + f"bridge {install.bridge_id}, which serves {platform} workspace " + f"{workspace_id}, is not running" + ) + return WebhookTarget( + tenant_id=tenant_id, + platform=platform, + bridge_id=install.bridge_id, + adapter=adapter, + ) + + async def deliver(self, target: WebhookTarget, event: InboundWebhook) -> None: + """Hand a resolved event to the bridge, at most once. + + Claiming comes first and the dispatch second, so a retry that arrives + while the first delivery is still working finds the event taken and + stops. That ordering is the whole of the deduplication: the platform + sends the same event again whenever it is not answered in time, and + the payload of a retry is identical to the original, so nothing later + in the stack could tell it from someone saying the same thing twice. + + An event the platform does not number is dispatched without a claim. + That is not a weaker guarantee quietly accepted — Slack numbers what it + retries, so an envelope with no id is one that arrives exactly once. + + The dispatch itself runs with **nothing bound**, which is not an + oversight. A bridge that receives over a socket dispatches from a task + that binds no tenant, and every handler below it binds the tenant of + the room it is acting on. Binding here would make the two delivery + paths differ in the one respect that decides who a message reaches, and + would hide a handler that had forgotten to bind for itself — for + exactly as long as it took someone to receive the same event over a + socket instead. + """ + if event.external_event_id is None: + await self._dispatch(target, event) + return + + async with tenant_session(self._session_factory, target.tenant_id) as session: + receipt = await self._receipts.claim( + session, + platform=target.platform, + external_event_id=event.external_event_id, + ) + if receipt is None: + logger.info( + "Dropped a repeat delivery of %s event %s to bridge %s " + "(attempt %s); it has already been taken", + target.platform, + event.external_event_id, + target.bridge_id, + event.delivery_attempt, + ) + return + receipt_id = receipt.id + # Before the dispatch, not with it. The index entry this writes is + # what a concurrent retry collides with, and an uncommitted one + # makes that retry wait for the turn instead of losing to it. + await session.commit() + + await self._dispatch(target, event) + + async with tenant_session(self._session_factory, target.tenant_id) as session: + await self._receipts.mark_handled(session, receipt_id=receipt_id) + pruned = await self._receipts.prune(session) + await session.commit() + if pruned: + logger.info( + "Pruned %s expired messaging event receipts for tenant %s", + pruned, + target.tenant_id, + ) + + async def _dispatch(self, target: WebhookTarget, event: InboundWebhook) -> None: + with no_tenant(): + await target.adapter.dispatch_event( + envelope_type=event.envelope_type, payload=event.payload + ) diff --git a/core/switch_core/bridges/collaboration/install_state.py b/core/switch_core/bridges/collaboration/install_state.py new file mode 100644 index 000000000..fda5928be --- /dev/null +++ b/core/switch_core/bridges/collaboration/install_state.py @@ -0,0 +1,133 @@ +"""The `state` parameter that carries a tenant across the install round trip. + +An install starts on the gateway, where the caller is authenticated and their +tenant is known, and finishes on a public callback the platform redirects the +browser to. Something has to get the tenant from one to the other, and the two +obvious candidates do not work here: + +- **A cookie** is what the gateway's own OIDC flow uses, and it cannot be used + for this. The gateway is reached on one hostname and the public callback on + another — they are different origins by deployment, not by accident — so a + cookie set at the start leg is simply not sent to the callback. +- **A database lookup** keyed by the state would work, but the callback runs + with no tenant bound and RLS refuses every read until one is. Making it + possible would mean a ninth `SECURITY DEFINER` function, and the shortness of + that list is the property that makes it reviewable. + +So the state is a token *we* sign, naming the tenant. The callback verifies the +signature, binds the tenant it names, and from that point runs as an ordinary +scoped request — every subsequent read and write is checked by RLS in the +normal way. A forged or edited state fails the signature and never reaches the +database at all. + +**A signature is not enough on its own**, which is why the token names a row as +well as a tenant. A signed token stays valid as long as the key does, so a +captured state could be replayed to install an attacker's workspace against the +victim's tenant — message injection into someone else's rooms. Single use is +the missing half and only the database can provide it: the row named here is +burnt with a conditional update, and a second attempt finds nothing to burn. + +The signing key is derived from `JWT_SECRET_KEY` rather than being another +value to deploy, but it is *derived* rather than reused: a token minted here +must never be mistakable for an agent's JWT, or for whatever the next thing to +want a signature turns out to be. +""" + +from __future__ import annotations + +import base64 +import hashlib +import hmac +import json +from dataclasses import dataclass + +#: Distinguishes this key from every other use of `JWT_SECRET_KEY`, and this +#: token format from whatever replaces it. Changing either string invalidates +#: every state in flight, which for a flow measured in seconds is free. +_KEY_INFO = b"switch/messaging-install-state/v1" + +_PREFIX = "v1." + + +class InstallStateError(RuntimeError): + """A state parameter was absent, malformed, or not signed by us. + + All three are the same answer to the caller — the install is refused — + and deliberately the same answer to an observer: nothing distinguishes a + truncated token from a forged one, because the difference is only ever + interesting to someone probing. + """ + + +@dataclass(frozen=True) +class InstallState: + """What a verified state names. + + `tenant_id` is trusted *because* the signature verified, and is bound to + the session before anything else happens. `state_id` names the row to burn. + `platform` is carried so the callback route's own path cannot be used to + redeem a state minted for a different platform. + """ + + tenant_id: str + state_id: str + platform: str + + +def _signing_key(secret: str) -> bytes: + return hmac.new(secret.encode(), _KEY_INFO, hashlib.sha256).digest() + + +def _b64(raw: bytes) -> str: + return base64.urlsafe_b64encode(raw).decode().rstrip("=") + + +def _unb64(encoded: str) -> bytes: + return base64.urlsafe_b64decode(encoded + "=" * (-len(encoded) % 4)) + + +def mint(state: InstallState, *, secret: str) -> str: + """Sign a state for the platform to hand back to us unchanged.""" + payload = _b64( + json.dumps( + {"tid": state.tenant_id, "sid": state.state_id, "plat": state.platform}, + separators=(",", ":"), + sort_keys=True, + ).encode() + ) + signature = hmac.new(_signing_key(secret), payload.encode(), hashlib.sha256) + return f"{_PREFIX}{payload}.{_b64(signature.digest())}" + + +def verify(token: str, *, secret: str) -> InstallState: + """Recover the state from a token, or raise. + + Nothing here touches the database, and nothing here is trusted before the + comparison: the payload is not decoded until its signature has matched, so + a hostile token is bytes we compared and discarded. + """ + if not token.startswith(_PREFIX): + raise InstallStateError("install state is not a state this deployment minted") + + try: + payload, signature = token[len(_PREFIX) :].split(".") + except ValueError: + raise InstallStateError("install state is malformed") from None + + expected = hmac.new(_signing_key(secret), payload.encode(), hashlib.sha256).digest() + try: + matches = hmac.compare_digest(_unb64(signature), expected) + except ValueError: + raise InstallStateError("install state is malformed") from None + if not matches: + raise InstallStateError("install state was not signed by this deployment") + + try: + decoded = json.loads(_unb64(payload)) + return InstallState( + tenant_id=decoded["tid"], + state_id=decoded["sid"], + platform=decoded["plat"], + ) + except (ValueError, KeyError, TypeError): + raise InstallStateError("install state is malformed") from None diff --git a/core/switch_core/bridges/collaboration/models.py b/core/switch_core/bridges/collaboration/models.py index 054369cde..94eb44f6a 100644 --- a/core/switch_core/bridges/collaboration/models.py +++ b/core/switch_core/bridges/collaboration/models.py @@ -17,6 +17,17 @@ class ChannelCreationUnsupported(ValueError): """ +class WebhookDeliveryUnsupported(RuntimeError): + """A verified inbound event was routed to a bridge that cannot take one. + + Only reachable when a bridge is recorded as serving an installed workspace + and its adapter receives events some other way — so it is a deployment + inconsistency rather than anything the sender did, and it is raised rather + than logged-and-dropped so the route answers with a failure the platform's + own delivery log records. + """ + + class Attachment(BaseModel): """An inbound file attachment of any type, with its raw bytes. diff --git a/core/switch_core/bridges/collaboration/slack/adapter.py b/core/switch_core/bridges/collaboration/slack/adapter.py index fb6beebfc..009b69da6 100644 --- a/core/switch_core/bridges/collaboration/slack/adapter.py +++ b/core/switch_core/bridges/collaboration/slack/adapter.py @@ -8,10 +8,11 @@ from collections import OrderedDict from collections.abc import Awaitable, Callable from dataclasses import replace -from typing import Any, ClassVar +from typing import Any, ClassVar, Literal import httpx -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, model_validator +from pydantic.json_schema import SkipJsonSchema from slack_sdk.errors import SlackApiError from slack_sdk.socket_mode.aiohttp import SocketModeClient from slack_sdk.socket_mode.request import SocketModeRequest @@ -196,7 +197,29 @@ class SlackUser(BaseModel): class SlackConnectionConfig(BridgeConnectionConfig): bot_token: str - app_token: str + #: How inbound events reach this bridge, which is decided by which Slack + #: app the token came from and is not an operator's preference. + #: + #: An app the operator registered themselves uses Socket Mode: Switch dials + #: out and needs no inbound route. The distributed app cannot — Slack does + #: not permit Socket Mode for it — so its events arrive as signed HTTP + #: posts to a public endpoint, and there is no app-level token at all. + #: + #: Hidden from the registration form because it is not a question an + #: operator filling that form can be asked: reaching the form at all means + #: Socket Mode, and the webhook value is written by the install flow. + event_delivery: SkipJsonSchema[Literal["socket_mode", "webhook"]] = "socket_mode" + #: Required under Socket Mode and meaningless without it, which the + #: validator below enforces rather than leaving to be discovered. + app_token: str | None = Field( + default=None, + title="App-level token", + description=( + "The xapp-… token with the connections:write scope, from the app's " + "Basic Information page. Required: it is what opens the connection " + "Slack delivers events down." + ), + ) workspace_id: str # The descriptions are not decoration: both registration forms build # themselves from this schema, so what is written here is the only @@ -221,6 +244,28 @@ class SlackConnectionConfig(BridgeConnectionConfig): ), ) + @model_validator(mode="after") + def _app_token_matches_delivery(self) -> SlackConnectionConfig: + """Refuse the two states that would look configured and receive nothing. + + A Socket Mode bridge with no app token opens no connection, so it + sends fine and never hears a word back — the exact shape of failure + that reads as "Slack is quiet today" for a week. An app token on a + webhook bridge is the opposite mistake: a credential for a mechanism + this bridge does not use, which will be read as evidence that it does. + """ + if self.event_delivery == "socket_mode" and not self.app_token: + raise ValueError( + "app_token is required: without it Switch opens no Socket Mode " + "connection and this bridge would receive no Slack events at all." + ) + if self.event_delivery == "webhook" and self.app_token: + raise ValueError( + "app_token must be empty for a bridge whose events arrive over " + "HTTP; the distributed Slack app has no app-level token." + ) + return self + class SlackAdapter(CollaborationAdapter): # Pin a turn's status to the message being worked on, opening a thread on @@ -362,15 +407,28 @@ async def start( auth.get("team", ""), ) - self._socket_client = SocketModeClient( - app_token=self._config.app_token, - web_client=self._web_client, - ) - self._socket_client.socket_mode_request_listeners.append( - self._handle_socket_event # type: ignore[arg-type] - ) - await self._socket_client.connect() - logger.info("Slack Socket Mode connected") + if self._config.event_delivery == "webhook": + # Nothing to connect: events are posted to the public endpoint, + # which verifies them and calls dispatch_event below. Said out loud + # because a bridge that opens no connection and logs nothing is + # indistinguishable from one that failed to. + logger.info( + "Slack adapter listening over HTTP; events arrive at the " + "messaging install endpoint rather than over Socket Mode" + ) + else: + # Narrowing for the type checker; the config validator is what + # actually guarantees it. + assert self._config.app_token is not None + self._socket_client = SocketModeClient( + app_token=self._config.app_token, + web_client=self._web_client, + ) + self._socket_client.socket_mode_request_listeners.append( + self._handle_socket_event # type: ignore[arg-type] + ) + await self._socket_client.connect() + logger.info("Slack Socket Mode connected") logger.debug( _TRACE + "build carries agent sessions; config agent_sessions=%s", self._config.agent_sessions, @@ -1920,7 +1978,7 @@ def _replace(match: re.Match[str]) -> str: return re.sub(r"@([A-Za-z0-9][A-Za-z0-9._-]*)", _replace, content) - # ── Socket Mode event handling ─────────────────────────────────────────── + # ── Event handling ─────────────────────────────────────────────────────── async def _handle_socket_event( self, client: SocketModeClient, req: SocketModeRequest @@ -1928,15 +1986,32 @@ async def _handle_socket_event( await client.send_socket_mode_response( SocketModeResponse(envelope_id=req.envelope_id) ) + await self.dispatch_event(envelope_type=req.type, payload=req.payload) - if req.type == "slash_commands": - await self._handle_slash_command(req.payload) + async def dispatch_event( + self, *, envelope_type: str, payload: dict[str, Any] + ) -> None: + """Route one Slack event, whichever transport carried it. + + Socket Mode hands over an envelope type and a payload; the public + webhook parses the same two out of the request it has already proved + genuine. Everything after that point is identical, so it is written + once — a dispatch that differed by transport is how the two delivery + modes would drift into behaving differently for the same event. + + Acknowledgement is *not* here, because the two transports acknowledge + incompatibly: Socket Mode replies on the socket before dispatching, + while HTTP acknowledges by returning 200 to the post. Both must do it + promptly and neither can do it for the other. + """ + if envelope_type == "slash_commands": + await self._handle_slash_command(payload) return - if req.type != "events_api": + if envelope_type != "events_api": return - event = req.payload.get("event", {}) + event = payload.get("event", {}) event_type = event.get("type") event_subtype = event.get("subtype") diff --git a/core/switch_core/bridges/collaboration/slack/install.py b/core/switch_core/bridges/collaboration/slack/install.py new file mode 100644 index 000000000..09beb5119 --- /dev/null +++ b/core/switch_core/bridges/collaboration/slack/install.py @@ -0,0 +1,352 @@ +"""Installing the distributed Slack app into a customer's workspace. + +The counterpart to `SLACK_SETUP.md`'s self-registered app, and a different +Slack app from it. See `docs/old/bridges/SLACK_DISTRIBUTED_APP.md` for the +registration walkthrough and the manifest; `BOT_SCOPES` below is the same list +as that manifest's, and a test compares them so the two cannot drift. + +The one structural difference from the self-registered app runs through +everything here: it has no app-level token, because Slack does not permit +Socket Mode for a distributed app and a single socket could not be shared by +replicas anyway. Events arrive over HTTPS and are proved genuine by the +signing secret instead — which is why `verify_webhook` exists on this side of +the boundary and has no equivalent on the adapter. +""" + +from __future__ import annotations + +import json +import logging +from collections.abc import Mapping +from typing import Any, ClassVar +from urllib.parse import parse_qsl, urlencode + +from slack_sdk.errors import SlackApiError +from slack_sdk.signature import SignatureVerifier +from slack_sdk.web.async_client import AsyncWebClient + +from switch_core.bridges.collaboration.install import ( + InboundWebhook, + InstallGrant, + MessagingAppInstaller, + MessagingInstallError, + WebhookAuthenticityError, + WebhookEndpoint, + WebhookPayloadError, +) + +logger = logging.getLogger(__name__) + +AUTHORIZE_URL = "https://slack.com/oauth/v2/authorize" + +#: Slack's ways of saying the token is finished already. Revoking one of these +#: is the outcome the caller wanted, so it is a success — and it is the usual +#: case, because the reason to be disconnecting is often that the customer +#: removed the app first. +_ALREADY_DEAD = frozenset({"invalid_auth", "token_revoked", "account_inactive"}) + +#: The two events by which Slack says an install is over, and what each means +#: in words an operator can read. `app_uninstalled` is the customer removing +#: the app; `tokens_revoked` is the narrower case of the tokens being killed +#: while the app stays. Both leave us unable to act in the workspace. +_REVOCATION_EVENTS: dict[str, str] = { + "app_uninstalled": "the app was removed from the Slack workspace", + "tokens_revoked": "Slack revoked this workspace's tokens", +} + + +def _json_object(raw: bytes) -> dict[str, Any]: + try: + parsed = json.loads(raw) + except ValueError as error: + raise WebhookPayloadError("Slack posted a body that is not JSON") from error + if not isinstance(parsed, dict): + raise WebhookPayloadError("Slack posted JSON that is not an object") + return parsed + + +#: Slack's count of how many times it has re-sent a delivery, absent on the +#: first. Header names arrive from Starlette lower-cased and are compared that +#: way; Slack's own spelling is `X-Slack-Retry-Num`. +_RETRY_NUM_HEADER = "x-slack-retry-num" + + +def _retry_number(headers: Mapping[str, str]) -> int: + """How many times Slack has sent this already, defaulting to none. + + Unauthenticated in the sense that it is not covered by the signature — the + signature is over the body and the timestamp — so it is read as a hint and + never as a decision. A forged value cannot make an event be handled twice + or dropped, because the receipt decides that; the worst it can do is put a + wrong number in a log line. + """ + raw = headers.get(_RETRY_NUM_HEADER) + if raw is None: + return 0 + try: + return int(raw) + except ValueError: + return 0 + + +def _form_fields(raw: bytes) -> dict[str, Any]: + """Slack's form encoding as a flat dict, matching Socket Mode's payload. + + `parse_qsl` and not `parse_qs`: the latter makes every value a list, and + the adapter reads these fields as the strings Socket Mode hands it. A + repeated field would be a Slack change worth noticing rather than a list to + accommodate, and the last value wins the way a form is usually read. + """ + try: + return dict( + parse_qsl(raw.decode(), keep_blank_values=True, strict_parsing=True) + ) + except (UnicodeDecodeError, ValueError) as error: + raise WebhookPayloadError( + "Slack posted a body that is not form data" + ) from error + + +#: The bot scopes the distributed app requests, in the manifest's order. +#: +#: Identical to the self-registered app's: the two apps differ in how they are +#: installed and how events reach them, never in what the bot may do once it is +#: in a channel. +BOT_SCOPES: tuple[str, ...] = ( + "files:read", + "files:write", + "assistant:write", + "channels:history", + "channels:manage", + "channels:read", + "chat:write", + "chat:write.customize", + "commands", + "groups:history", + "groups:read", + "groups:write", + "im:history", + "im:read", + "im:write", + "mpim:history", + "reactions:read", + "reactions:write", + "users:read", + "usergroups:read", + "usergroups:write", +) + + +class SlackAppInstaller(MessagingAppInstaller): + platform: ClassVar[str] = "slack" + + def __init__( + self, *, client_id: str, client_secret: str, signing_secret: str + ) -> None: + self._client_id = client_id + self._client_secret = client_secret + self._verifier = SignatureVerifier(signing_secret) + + def authorize_url(self, *, state: str, redirect_uri: str) -> str: + return f"{AUTHORIZE_URL}?" + urlencode( + { + "client_id": self._client_id, + "scope": ",".join(BOT_SCOPES), + "redirect_uri": redirect_uri, + "state": state, + } + ) + + async def redeem(self, *, code: str, redirect_uri: str) -> InstallGrant: + try: + response = await AsyncWebClient().oauth_v2_access( + client_id=self._client_id, + client_secret=self._client_secret, + code=code, + redirect_uri=redirect_uri, + ) + except SlackApiError as error: + raise MessagingInstallError( + f"Slack refused the install: {error.response.get('error', error)}" + ) from error + + # Slack answers 200 with `ok: false` for most refusals, so the absence + # of an exception above proves nothing on its own. + if not response.get("ok"): + raise MessagingInstallError( + f"Slack refused the install: {response.get('error', 'unknown error')}" + ) + + if response.get("is_enterprise_install"): + raise MessagingInstallError( + "this app was installed org-wide on Slack Enterprise Grid, which " + "Switch cannot yet record: an org-wide install is identified by " + "an enterprise rather than by a single workspace, and an " + "installation is stored against one workspace. Install it into " + "a single workspace instead." + ) + + team = response.get("team") or {} + workspace_id = team.get("id") + access_token = response.get("access_token") + if not workspace_id or not access_token: + raise MessagingInstallError( + "Slack accepted the install but returned no workspace id or no " + "bot token, so there is nothing to record. Nothing was saved." + ) + + return InstallGrant( + external_workspace_id=workspace_id, + workspace_name=team.get("name") or workspace_id, + bot_token=access_token, + scopes=response.get("scope") or "", + ) + + async def revoke(self, *, bot_token: str) -> None: + try: + response = await AsyncWebClient(token=bot_token).auth_revoke() + except SlackApiError as error: + reason = error.response.get("error", "") + if reason in _ALREADY_DEAD: + logger.info( + "Slack reports the bot token was already invalid (%s); " + "treating the revocation as done", + reason, + ) + return + raise MessagingInstallError( + f"Slack refused to revoke the bot token: {reason or error}" + ) from error + + # `revoked: false` with `ok: true` is Slack accepting the call and + # telling you it did nothing — which for a disconnect is the whole of + # what was asked for, so it cannot be read off `ok` alone. + if not response.get("revoked"): + raise MessagingInstallError( + "Slack accepted the revocation request and reported the token " + "was not revoked, so it is still valid." + ) + + def verify_webhook(self, *, headers: Mapping[str, str], body: bytes) -> None: + try: + valid = self._verifier.is_valid_request(body, dict(headers)) + except ValueError as error: + # A non-numeric timestamp header reaches int() inside the verifier. + # It is unauthenticated input, so it must answer like any other bad + # signature rather than as a server fault. + raise WebhookAuthenticityError("malformed Slack timestamp") from error + if not valid: + raise WebhookAuthenticityError("bad Slack signature") + + def parse_webhook( + self, *, endpoint: WebhookEndpoint, headers: Mapping[str, str], body: bytes + ) -> InboundWebhook: + attempt = _retry_number(headers) + + if endpoint == "commands": + # A slash command posts its fields as a form, and Socket Mode + # delivers that same flat dict — so the parsed form *is* the + # payload, with no unwrapping. + return InboundWebhook( + envelope_type="slash_commands", + payload=_form_fields(body), + handshake=None, + # Slack retries neither of the two form endpoints. A command + # and an interaction are a person waiting on a dialog, and a + # reply that arrives a minute late is worse than none — so + # Slack sends each exactly once and there is no id on it to + # deduplicate by. + external_event_id=None, + delivery_attempt=attempt, + ) + + if endpoint == "interactive": + # Nested once: the form carries a single `payload` field whose + # value is JSON. + raw = _form_fields(body).get("payload") + if not raw: + raise WebhookPayloadError( + "Slack posted an interaction with no payload field" + ) + return InboundWebhook( + envelope_type="interactive", + payload=_json_object(raw.encode()), + handshake=None, + external_event_id=None, + delivery_attempt=attempt, + ) + + envelope = _json_object(body) + if envelope.get("type") == "url_verification": + # Slack proving the URL is ours, at the moment the Request URL is + # saved. It is signed like any other post, so it reaches here + # having been verified — and it names no workspace, because none + # has installed anything yet. + challenge = envelope.get("challenge") + if not isinstance(challenge, str) or not challenge: + raise WebhookPayloadError( + "Slack sent a URL verification with no challenge to echo" + ) + return InboundWebhook( + envelope_type="url_verification", + payload=envelope, + handshake=challenge, + external_event_id=None, + delivery_attempt=attempt, + ) + + event_id = envelope.get("event_id") + return InboundWebhook( + envelope_type="events_api", + payload=envelope, + handshake=None, + # The one envelope Slack numbers, and the one it retries. A + # retried delivery carries the same `event_id` as the original, + # which is the whole basis of handling it once. + external_event_id=event_id + if isinstance(event_id, str) and event_id + else None, + delivery_attempt=attempt, + ) + + def workspace_of_event(self, payload: Mapping[str, object]) -> str: + """Which workspace an event came from, over Slack's two spellings. + + An event callback and a slash command name it flat, as `team_id`; an + interaction nests it under `team`. Both are read here rather than + normalised at parse time, because the payload handed to the adapter has + to stay byte-identical to the one Socket Mode delivers. + """ + workspace_id = payload.get("team_id") + if not isinstance(workspace_id, str) or not workspace_id: + team = payload.get("team") + workspace_id = team.get("id") if isinstance(team, dict) else None + if not isinstance(workspace_id, str) or not workspace_id: + raise WebhookPayloadError("Slack event names no workspace") + return workspace_id + + def revocation_of_event(self, payload: Mapping[str, object]) -> str | None: + """Read the two end-of-install events out of an Events API envelope. + + Only `event_callback` envelopes carry one. A slash command or an + interaction cannot say the app was uninstalled — there would be nobody + left to press the button — so the inner `event.type` is the only place + worth looking, and reading a `type` from anywhere else would let an + interaction payload with the right-looking field disconnect a + customer's workspace. + """ + event = payload.get("event") + if not isinstance(event, dict): + return None + return _REVOCATION_EVENTS.get(event.get("type", "")) + + def connection_config(self, grant: InstallGrant) -> dict[str, object]: + return { + "bot_token": grant.bot_token, + "workspace_id": grant.external_workspace_id, + # Not a detail of this rendering: it is the difference between the + # two apps. Left out, the config validates as a Socket Mode bridge + # missing its app token, and the install fails at registration + # rather than at anything a reader would look at. + "event_delivery": "webhook", + } diff --git a/core/switch_core/config.py b/core/switch_core/config.py index ab01ca1f4..1881292a4 100644 --- a/core/switch_core/config.py +++ b/core/switch_core/config.py @@ -178,6 +178,36 @@ class SwitchConfig(BaseSettings): # unset, the raw `switchdash://` deeplink is posted as-is. gateway_public_url: str | None = None + # Credentials of the distributed Slack app *we* registered — the one a + # customer installs by clicking a button, as opposed to the app an operator + # registers themselves and pastes tokens for. See + # `docs/old/bridges/SLACK_DISTRIBUTED_APP.md`. + # + # Setting all three is what enables workspace installs at all: there is no + # separate on/off switch, because an app with no credentials is not an app. + # Setting some is a mistake and is refused at startup. + # + # The signing secret is the one that must never be treated as optional in + # spirit: it is the whole of what distinguishes a Slack event from a post by + # anyone who learned the URL. + slack_app_client_id: str | None = None + slack_app_client_secret: str | None = None + slack_app_signing_secret: str | None = None + + # Public origin (scheme + host, no path) that a messaging platform reaches + # Switch on: the base of the OAuth redirect and of the three event URLs + # under `/messaging`, and the one registered with the app. + # + # Separate from `gateway_public_url` because the two answer to different + # audiences and need not be the same host. The gateway URL is opened by a + # person following a deeplink and may live on a private network; this one + # is dialled by Slack from the internet and must resolve and present a + # browser-trusted certificate there. A deployment whose gateway is + # reachable only over a VPN can still offer installs, and pointing the + # gateway URL at the internet-facing host to achieve that would silently + # move every deeplink along with it. + messaging_public_url: str | None = None + # Upper bound on a single attachment an agent may post to a room (and that # a collaboration bridge will relay out). Uploads over this raise instead # of being truncated or silently dropped. @@ -394,6 +424,28 @@ def _validate_gateway_public_url(self) -> "SwitchConfig": ) return self + @model_validator(mode="after") + def _validate_messaging_public_url(self) -> "SwitchConfig": + # Slack compares the redirect URI it is sent against the one registered + # with the app, byte for byte, and reports a mismatch as a generic + # refusal. A path here would make every install URL wrong in a way the + # error message does not name, so it is a startup error instead. + if self.messaging_public_url: + parts = urlsplit(self.messaging_public_url) + if not parts.scheme or not parts.netloc or parts.path not in ("", "/"): + raise ValueError( + "MESSAGING_PUBLIC_URL must be a scheme + host only " + "(e.g. https://switch.example), with no path, " + f"got {self.messaging_public_url!r}." + ) + if parts.scheme != "https": + raise ValueError( + "MESSAGING_PUBLIC_URL must be https. Slack refuses to " + "register an http redirect or event URL, so an http origin " + f"cannot work, got {self.messaging_public_url!r}." + ) + return self + @model_validator(mode="after") def _validate_gateway_oidc(self) -> "SwitchConfig": required = ( @@ -421,6 +473,34 @@ def _validate_gateway_oidc(self) -> "SwitchConfig": ) return self + @model_validator(mode="after") + def _validate_slack_app(self) -> "SwitchConfig": + required = ( + self.slack_app_client_id, + self.slack_app_client_secret, + self.slack_app_signing_secret, + ) + set_count = sum(1 for value in required if value) + if 0 < set_count < len(required): + raise ValueError( + "Partial distributed Slack app config: set all of " + "SLACK_APP_CLIENT_ID / SLACK_APP_CLIENT_SECRET / " + "SLACK_APP_SIGNING_SECRET, or none of them." + ) + # The redirect URI and the events URL are both built from the public + # origin, and Slack checks the redirect matches the one registered with + # the app. Without the origin they would be built against nothing, so a + # deployment configured to offer installs and unable to name itself is + # a startup error rather than a broken button. + if set_count and not self.messaging_public_url: + raise ValueError( + "A distributed Slack app is configured but MESSAGING_PUBLIC_URL " + "is not. The install redirect and the events endpoint are built " + "from it, and Slack rejects a redirect that does not match the " + "one registered with the app." + ) + return self + @property def gateway_oidc_enabled(self) -> bool: return bool( diff --git a/core/switch_core/db/models.py b/core/switch_core/db/models.py index f61bfb12f..caa5dc649 100644 --- a/core/switch_core/db/models.py +++ b/core/switch_core/db/models.py @@ -1131,6 +1131,221 @@ class CollaborationBridge(TenantScoped, Base): ) +# ── Messaging Installs ───────────────────────────────────────────────────────── + + +class MessagingInstall(TenantScoped, Base): + """A tenant's installation of the Switch app into one external workspace. + + The difference from `collaboration_bridges` is who supplied the + credential. A bridge holds a token an operator pasted in, from an app that + operator registered; an install holds a token *we* were granted, for our + app, by whoever clicked Add to Slack. Both end up driving the same adapter, + so this table records only what the install added: which workspace, whose + token, and what it may do. + + **`(platform, external_workspace_id)` is unique across the whole + deployment among rows that are still `active`**, and that is the single + most important line here. Inbound events arrive over one public endpoint + carrying a workspace id and no tenant, so a workspace claimed by two + tenants is a message with two possible destinations and no way to choose — + which is the failure this whole phase exists to make unrepresentable. The + database decides it rather than a read-then-insert in application code, + because the check and the write cannot be made atomic from outside. + + It is a *partial* index rather than a plain constraint because an install + has to be able to end. A customer who removes the app in Slack, or an + operator who disconnects it here, leaves a row behind — and a row that + still occupied the workspace would mean nobody could ever install that + workspace again, including the customer who just removed it. Ending an + install therefore frees the workspace, and keeps the record of the one + that ended. + + That index is also the one place a tenant learns something about another: + claiming a workspace somebody else already holds fails, and the failure + says so. It is the right answer — the alternative is a silent second claim + — and what it discloses is that *some* tenant holds a workspace the caller + was already able to name. + + `status` is `active`, `disconnected` (an operator here ended it) or + `revoked` (the platform told us it was over). The two endings are recorded + apart because they call for different things: one is somebody's decision + and the other is news, and an operator looking at a bridge that stopped + working needs to know which. + + `bridge_id` is nullable because the install row is written before anything + is built on it, and because removing a bridge should not force the + credential to be thrown away and re-granted. A null there means the + install is recorded and not yet serving. + + `encrypted_bot_token` uses the same key as every other credential this + schema stores (`crypto.encrypt_token` over the configured secret), so it + is protected against a stolen dump and not against a compromised process. + A per-tenant key is a stronger boundary and a later decision. It is + nullable so that an install which has ended can keep its record without + keeping its secret: the token is worthless by then, and a worthless + credential still reads like a credential to whoever finds the dump. + + `scopes` is the platform's own spelling of what was granted, stored + verbatim rather than parsed into a list — a scope string that means + nothing to us is still the thing to show an operator asking why a call was + refused. + """ + + __tablename__ = "messaging_installs" + __table_args__ = ( + Index( + "uq_messaging_installs_workspace", + "platform", + "external_workspace_id", + unique=True, + postgresql_where=text("status = 'active'"), + ), + UniqueConstraint("id", "tenant_id", name="uq_messaging_installs_id_tenant"), + ForeignKeyConstraint( + ["tenant_id", "bridge_id"], + ["collaboration_bridges.tenant_id", "collaboration_bridges.id"], + name="fk_messaging_installs_bridge", + ), + ) + + id: Mapped[str] = mapped_column(Text, primary_key=True, default=_uuid) + platform: Mapped[str] = mapped_column(Text, nullable=False) + external_workspace_id: Mapped[str] = mapped_column(Text, nullable=False) + encrypted_bot_token: Mapped[str | None] = mapped_column(Text, nullable=True) + scopes: Mapped[str] = mapped_column(Text, nullable=False) + status: Mapped[str] = mapped_column(Text, nullable=False) + installed_by_user_id: Mapped[str] = mapped_column( + Text, ForeignKey("users.id"), nullable=False + ) + bridge_id: Mapped[str | None] = mapped_column(Text, nullable=True) + installed_at: Mapped[str] = mapped_column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + ended_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) + + +class MessagingInstallState(TenantScoped, Base): + """One in-flight install: minted when the flow starts, burnt when it lands. + + An install is two requests with a trip through the platform in between. The + first is an authenticated operator asking to install; the second is a + browser arriving back at a public endpoint from the platform, carrying an + authorization code and a `state` we chose. Nothing else ties the two + together, so `state` has to carry the whole of what the second request may + not be trusted to assert: which tenant, and on whose behalf. + + **The row is not what carries the tenant across.** The `state` parameter is + a signed token naming the tenant, so the callback binds a tenant it can + verify without reading anything first. That is the point of the design: + every other unauthenticated entry point resolves its tenant through a + `SECURITY DEFINER` lookup, and this one does not have to, so it does not — + the closed list in `db/tenant_lookup.py` stays as short as it is. What this + row adds is the one property a signature cannot have: **single use.** A + signed token is valid until it expires and a captured one can be replayed; + the redemption below happens once because `consumed_at` is set in the same + statement that checks it is null. + + Which makes the failure this prevents worth naming. Replaying a captured + state completes an install of the attacker's own workspace against the + victim's tenant — that workspace's messages then arrive in the victim's + rooms, which is message injection, not a leak. Single use and a short + expiry are what close it. + + Redemption is a scoped write like any other, run after the signature has + bound the tenant, so row-level security is a second check on the first: a + token whose signed tenant disagrees with the row's finds no row at all. + + The two timestamps are both needed and mean different things. `expires_at` + is a bound on how long the platform's round trip may take; `consumed_at` + is the fact of redemption, kept rather than deleted so an operator asking + why a link stopped working can see it was used rather than lost. + """ + + __tablename__ = "messaging_install_states" + + id: Mapped[str] = mapped_column(Text, primary_key=True, default=_uuid) + platform: Mapped[str] = mapped_column(Text, nullable=False) + created_by_user_id: Mapped[str] = mapped_column( + Text, ForeignKey("users.id"), nullable=False + ) + created_at: Mapped[str] = mapped_column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + expires_at: Mapped[str] = mapped_column(DateTime(timezone=True), nullable=False) + consumed_at: Mapped[str | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) + + +class MessagingEventReceipt(TenantScoped, Base): + """One inbound platform event, claimed once so it is handled once. + + Platforms deliver at least once. Slack gives three seconds to acknowledge + an event and retries what it does not get an answer to — so a deployment + under load, or one restarted mid-request, is told the same thing again. + Without a record of what has been taken, the second telling produces a + second answer in the customer's channel, which is the visible failure: an + agent replying twice to one question. + + **The row is written before the work, not after**, and the unique index is + what arbitrates. Two retries in flight at once both reach the insert and + exactly one survives it; the loser stops there. Recording afterwards would + order the two the wrong way round — both would dispatch, and the duplicate + would be detected once it no longer mattered. + + That ordering chooses at-most-once over at-least-once, which is worth + stating plainly: an event claimed by a process that then dies is not + retried, because the platform has already been told 200 and this table says + the event is taken. It is not a new loss. The route has acknowledged before + handling since it was written — it has to, the deadline is shorter than a + turn — so the event was already unrecoverable at that point. What this adds + is `handled_at`, which makes the loss visible: a claimed row that never + completed is a real event that reached nobody, and it can be found. + + `external_event_id` is the platform's own id for the delivery, and only + some envelopes have one. Slack numbers Events API envelopes and retries + only those; a slash command and an interaction get one shot and no id, so + there is nothing to deduplicate and no row here. A missing id means "the + platform does not retry this", not "this was not checked". + + Uniqueness is `(tenant_id, platform, external_event_id)` and not the + deployment-wide pair, unlike the workspace claim on `messaging_installs`. + The two would be equivalent — an event id is unique in the platform's own + namespace and a workspace belongs to one tenant — so the tenant-local index + is the one to prefer: it keeps one customer's event ids out of another's + namespace entirely, and it means a conflict is always with a row the + inserting tenant can actually see rather than an opaque refusal naming + somebody else's. + """ + + __tablename__ = "messaging_event_receipts" + __table_args__ = ( + UniqueConstraint( + "tenant_id", + "platform", + "external_event_id", + name="uq_messaging_event_receipts_event", + ), + # Pruning reads this and nothing else. Receipts are only useful for as + # long as the platform might still retry, and the table would otherwise + # grow with every message the busiest workspace ever sends. + Index("ix_messaging_event_receipts_received_at", "received_at"), + ) + + id: Mapped[str] = mapped_column(Text, primary_key=True, default=_uuid) + platform: Mapped[str] = mapped_column(Text, nullable=False) + external_event_id: Mapped[str] = mapped_column(Text, nullable=False) + received_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), server_default=func.now(), nullable=False + ) + handled_at: Mapped[datetime | None] = mapped_column( + DateTime(timezone=True), nullable=True + ) + + # ── Server-Side Connectors ──────────────────────────────────────────────────── diff --git a/core/switch_core/db/stores/__init__.py b/core/switch_core/db/stores/__init__.py index dbee2199f..1ba7e80ba 100644 --- a/core/switch_core/db/stores/__init__.py +++ b/core/switch_core/db/stores/__init__.py @@ -7,6 +7,8 @@ from switch_core.db.stores.external_user_store import ExternalUserStore from switch_core.db.stores.invitation_store import InvitationStore from switch_core.db.stores.message_store import MessageStore +from switch_core.db.stores.messaging_event_store import MessagingEventReceiptStore +from switch_core.db.stores.messaging_install_store import MessagingInstallStore from switch_core.db.stores.reference_store import ReferenceStore from switch_core.db.stores.reference_type_store import ReferenceTypeStore from switch_core.db.stores.room_group_store import RoomGroupStore @@ -28,6 +30,8 @@ "ExternalUserStore", "InvitationStore", "MessageStore", + "MessagingEventReceiptStore", + "MessagingInstallStore", "ReferenceStore", "ReferenceTypeStore", "RoomGroupStore", diff --git a/core/switch_core/db/stores/messaging_event_store.py b/core/switch_core/db/stores/messaging_event_store.py new file mode 100644 index 000000000..deea6a095 --- /dev/null +++ b/core/switch_core/db/stores/messaging_event_store.py @@ -0,0 +1,94 @@ +from __future__ import annotations + +from datetime import UTC, datetime, timedelta + +from sqlalchemy import delete, update +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession + +from switch_core.db.models import MessagingEventReceipt + +#: How long a handled event is remembered. +#: +#: Two different needs, and the longer one sets it. Deduplication needs only to +#: outlast the platform's retry schedule, which is tens of minutes — Slack has +#: given up long before an hour. `handled_at` is the reason for days: a claimed +#: receipt that never completed is an event that reached nobody, and an hour is +#: not long enough for anyone to notice and come looking. A week of one row per +#: inbound event is a table Postgres does not notice. +RECEIPT_RETENTION = timedelta(days=7) + + +class MessagingEventReceiptStore: + """Who has already taken an inbound event, and whether they finished it.""" + + async def claim( + self, session: AsyncSession, *, platform: str, external_event_id: str + ) -> MessagingEventReceipt | None: + """Take an event to handle, or return `None` because someone else has. + + The insert *is* the claim. Reading for an existing receipt and then + writing one would leave the window this exists to close: two retries + arriving together both read nothing, both write, and the customer gets + two answers. Here the unique index decides, and the loser learns it did + by failing. + + `None` rather than an exception because a duplicate is the ordinary + case this is built for, not a fault — platforms deliver at least once + by design, and a caller that treats the second one as an error would + fill the log with the system working correctly. + + **Commit before doing the work.** An uncommitted claim still holds the + index entry, so a concurrent retry's insert blocks on it rather than + failing — and would stay blocked for as long as the first delivery + takes, which is the length of an agent's turn. Committing straight away + turns that wait into the immediate refusal it should be. + """ + receipt = MessagingEventReceipt( + platform=platform, external_event_id=external_event_id + ) + session.add(receipt) + try: + await session.flush() + except IntegrityError as exc: + if "uq_messaging_event_receipts_event" not in str(exc.orig): + raise + await session.rollback() + return None + return receipt + + async def mark_handled(self, session: AsyncSession, *, receipt_id: str) -> None: + """Record that the claimed event was seen all the way through. + + Its absence is the useful half. A receipt still unhandled long after it + was claimed is an event the platform was told we had and nobody ever + answered — a process that died mid-turn — and without this column that + is indistinguishable from an event handled perfectly. + """ + await session.execute( + update(MessagingEventReceipt) + .where(MessagingEventReceipt.id == receipt_id) + .values(handled_at=datetime.now(UTC)) + ) + + async def prune(self, session: AsyncSession) -> int: + """Drop the bound tenant's receipts older than the retention window. + + Opportunistic rather than scheduled, the way stale role leases are + cleared when the next agent takes a role. This backend has no janitor + to hang a sweep on, and inventing one for a single table would be a + larger change than the table deserves; running it off the traffic that + creates the rows keeps the work proportional to that traffic and needs + nothing started at boot. + + Called after the event has been dispatched, not before it. A prune in + the claiming transaction would sit between a retry and the refusal it + is waiting for. + """ + result = await session.execute( + delete(MessagingEventReceipt).where( + MessagingEventReceipt.received_at + < datetime.now(UTC) - RECEIPT_RETENTION + ) + ) + return int(result.rowcount or 0) # type: ignore[attr-defined] diff --git a/core/switch_core/db/stores/messaging_install_store.py b/core/switch_core/db/stores/messaging_install_store.py new file mode 100644 index 000000000..5e6ae18e8 --- /dev/null +++ b/core/switch_core/db/stores/messaging_install_store.py @@ -0,0 +1,279 @@ +from __future__ import annotations + +from datetime import UTC, datetime, timedelta + +from sqlalchemy import select, update +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession + +from switch_core.db.models import ( + MessagingInstall, + MessagingInstallState, + require_tenant_id, +) + +#: How long a customer has to finish the platform's half of the flow. Long +#: enough for a real person to read a consent screen and pick a workspace, +#: short enough that a state captured from a browser's history or a proxy log +#: is worthless by the time anyone reads it. +STATE_TTL = timedelta(minutes=10) + +#: An install that is serving. The only status the workspace uniqueness index +#: covers, so it is also the answer to "who holds this workspace". +INSTALL_ACTIVE = "active" + +#: Ended here, by someone who decided to. The bridge is gone with it. +INSTALL_DISCONNECTED = "disconnected" + +#: Ended there: the platform told us the app was removed or its token killed. +#: Distinct from `disconnected` because an operator whose bridge stopped +#: working needs to know whether it was news or a decision. +INSTALL_REVOKED = "revoked" + + +class MessagingInstallClaimedError(RuntimeError): + """Another tenant has already installed the app into this workspace. + + Worth its own type because it is the one install failure that is not a + mistake by the caller and cannot be retried into working. What it discloses + — that *somebody* holds this workspace — is deliberate: the alternative is + accepting a second claim and delivering that workspace's messages to + whichever tenant the router happened to pick. + """ + + +class MessagingInstallStateError(RuntimeError): + """A state could not be redeemed: already used, expired, or not ours.""" + + +class MessagingInstallNotFound(RuntimeError): + """No install of that id is visible to the tenant bound on this session. + + "Not visible" rather than "does not exist", and the two are deliberately + not distinguished: the reads here are scoped, so an id belonging to + another tenant misses exactly as an invented one does. Telling them apart + would be a way to ask whether an id exists elsewhere. + """ + + +class MessagingInstallStore: + async def start_install( + self, session: AsyncSession, *, platform: str, user_id: str + ) -> MessagingInstallState: + """Record an install about to be attempted, and return the row to sign. + + `tenant_id` is left to the model's default, which reads the tenant + bound to this session — the caller does not get to name one. That is + the whole reason the start leg is authenticated and the callback is + not: this is where the tenant is decided. + """ + state = MessagingInstallState( + platform=platform, + created_by_user_id=user_id, + expires_at=datetime.now(UTC) + STATE_TTL, + ) + session.add(state) + await session.flush() + return state + + async def redeem_state( + self, session: AsyncSession, *, state_id: str, platform: str + ) -> MessagingInstallState: + """Burn a state, or raise. Exactly one caller can succeed. + + The check and the write are one statement on purpose. Reading the row, + deciding it is unused and then updating it leaves a window in which two + callbacks both read `consumed_at IS NULL` — and the whole reason this + row exists is to make a replayed state fail. `expires_at` is in the + same predicate for the same reason. + + Scoped to the bound tenant as well, which the signature has already + established: a state signed for one tenant and naming a row belonging + to another matches nothing, so RLS is a second, independent check on + the first. + """ + result = await session.execute( + update(MessagingInstallState) + .where( + MessagingInstallState.id == state_id, + MessagingInstallState.tenant_id == require_tenant_id(), + MessagingInstallState.platform == platform, + MessagingInstallState.consumed_at.is_(None), + MessagingInstallState.expires_at > datetime.now(UTC), + ) + .values(consumed_at=datetime.now(UTC)) + .returning(MessagingInstallState) + ) + state = result.scalars().one_or_none() + if state is None: + raise MessagingInstallStateError( + "this install link has already been used or has expired. Start " + "the install again from Switch." + ) + return state + + async def record_install( + self, + session: AsyncSession, + *, + platform: str, + external_workspace_id: str, + encrypted_bot_token: str, + scopes: str, + user_id: str, + ) -> MessagingInstall: + """Claim a workspace for the bound tenant. + + The claim is the insert: `(platform, external_workspace_id)` is unique + across the deployment among active rows, so the database decides who + holds a workspace rather than a read followed by a write that cannot be + made atomic with it. A second tenant's install therefore fails here, + loudly, instead of producing an event with two possible destinations. + + Installs that have ended are not in the index, so re-installing a + workspace somebody released is an ordinary insert and needs no check of + its own. + """ + install = MessagingInstall( + platform=platform, + external_workspace_id=external_workspace_id, + encrypted_bot_token=encrypted_bot_token, + scopes=scopes, + status=INSTALL_ACTIVE, + installed_by_user_id=user_id, + ) + session.add(install) + try: + await session.flush() + except IntegrityError as exc: + if "uq_messaging_installs_workspace" not in str(exc.orig): + raise + raise MessagingInstallClaimedError( + f"the {platform} workspace {external_workspace_id} is already " + "connected to Switch. Disconnect the existing install — from " + "the organisation that holds it, which may not be yours — " + "before connecting it again." + ) from exc + return install + + async def get_for_workspace( + self, session: AsyncSession, *, platform: str, external_workspace_id: str + ) -> MessagingInstall | None: + """The bound tenant's live install of one workspace, if it is theirs. + + Deliberately still scoped, even though the unique index means at most + one active row exists deployment-wide: the caller is an inbound webhook + that resolved a tenant from the workspace a moment ago, and re-reading + it under RLS is what makes a mistake there a miss rather than a + cross-tenant read. + + The status predicate matches the index's and `tenant_of_messaging_ + install`'s. Without it a workspace installed, removed and installed + again returns two rows and this raises — an outage for the live + install, caused by the ended one. + """ + result = await session.execute( + select(MessagingInstall).where( + MessagingInstall.platform == platform, + MessagingInstall.external_workspace_id == external_workspace_id, + MessagingInstall.status == INSTALL_ACTIVE, + ) + ) + return result.scalars().one_or_none() + + async def get_for_bridge( + self, session: AsyncSession, *, bridge_id: str + ) -> MessagingInstall | None: + """The live install a bridge was built for, if it was built for one. + + Asked from the other direction than the rest of this store, and by + something that does not otherwise know installs exist: the bridge + delete endpoint, which has to refuse rather than tear down a bridge + whose credential is a token nobody here has revoked. + + Live installs only. An ended one has already released its pointer, so a + row matching here is always a bridge that is still somebody's install. + """ + result = await session.execute( + select(MessagingInstall).where( + MessagingInstall.bridge_id == bridge_id, + MessagingInstall.status == INSTALL_ACTIVE, + ) + ) + return result.scalars().one_or_none() + + async def get(self, session: AsyncSession, *, install_id: str) -> MessagingInstall: + """One of the bound tenant's installs, by id, or raise.""" + install = await session.get(MessagingInstall, install_id) + if install is None: + raise MessagingInstallNotFound( + f"no install {install_id} belongs to this organisation" + ) + return install + + async def list_for_tenant(self, session: AsyncSession) -> list[MessagingInstall]: + """Every install this tenant has ever made, newest first. + + Ended ones included, and that is the point of the method rather than a + side effect. An operator looking at this list is usually looking at it + because something stopped working, and a list of only the live installs + answers "there is nothing here" to the question "what happened to the + one that was here yesterday". + """ + result = await session.execute( + select(MessagingInstall).order_by( + MessagingInstall.installed_at.desc(), MessagingInstall.id + ) + ) + return list(result.scalars()) + + async def end( + self, session: AsyncSession, *, install_id: str, status: str + ) -> MessagingInstall: + """Mark an install finished, and discard the credential and bridge with it. + + `status` says which of the two ways it ended — see + `INSTALL_DISCONNECTED` and `INSTALL_REVOKED`. Anything else is a + programming error rather than a state the column may hold: the + uniqueness index reads `status = 'active'`, so a typo here would leave + a row that is neither serving nor releasing its workspace. + + **Ending an install twice is success.** The platform retries the event + that says an app was uninstalled, and an operator can click disconnect + on a row a retry has already ended; both must reach the same place. + That is also why this is an ordinary read-then-write rather than the + single conditional statement `redeem_state` uses — there is no race to + lose here, because two writers racing to end the same install both want + what the other is doing. + """ + if status not in (INSTALL_DISCONNECTED, INSTALL_REVOKED): + raise ValueError( + f"{status!r} is not a way an install can end; expected " + f"{INSTALL_DISCONNECTED!r} or {INSTALL_REVOKED!r}" + ) + install = await self.get(session, install_id=install_id) + if install.status != INSTALL_ACTIVE: + return install + + install.status = status + install.ended_at = datetime.now(UTC) + # The token is worthless the moment the platform is told so, and a + # worthless credential still reads like a live one to whoever finds + # the dump. Keeping the row is the record; keeping the secret is not + # part of it. + install.encrypted_bot_token = None + # And the pointer goes with it, because the bridge is about to. The + # foreign key has no `ON DELETE`, so a row still naming the bridge is + # what would refuse its deletion. + install.bridge_id = None + await session.flush() + return install + + async def attach_bridge( + self, session: AsyncSession, *, install_id: str, bridge_id: str + ) -> MessagingInstall: + """Point an install at the bridge now serving it.""" + install = await self.get(session, install_id=install_id) + install.bridge_id = bridge_id + await session.flush() + return install diff --git a/core/switch_core/db/tenant_lookup.py b/core/switch_core/db/tenant_lookup.py index dcb3fbcfe..371a457da 100644 --- a/core/switch_core/db/tenant_lookup.py +++ b/core/switch_core/db/tenant_lookup.py @@ -1,4 +1,4 @@ -"""The whole exemption from row-level security, written out as eight functions. +"""The whole exemption from row-level security, written out as nine functions. Row-level security is enforced by `require_tenant_id()` (`db/rls_ddl.py`), which raises when no tenant is bound. That is the property everything else @@ -64,8 +64,9 @@ So the property this design actually holds is: **the exemption discloses the shape of the deployment — which tenants exist, and which tenant a given user, -credential, room, bridge, connector or invitation belongs to — and no row of -any tenant-scoped table.** It is a boundary on data, not on metadata. Narrowing +credential, room, bridge, connector, invitation or installed workspace belongs +to — and no row of any tenant-scoped table.** It is a boundary on data, not on +metadata. Narrowing the second is a question about who may hold the runtime role's credentials at all, since everything above is reachable by anyone who has them. @@ -108,6 +109,26 @@ only the tenant id does, which is the property every lookup in this module rests on. +**`tenant_of_messaging_install` is the ninth, and the only one whose caller is +not a person.** An event from the Switch app installed in a customer's Slack +arrives over a public endpoint that no one has authenticated to: what it +carries is a workspace id, and the tenant is precisely what has to be worked out before +anything else may happen. There is no row in hand to read the answer off, so +this is shape 1 with the credential replaced by a workspace — resolve the +tenant, bind it, and read everything after that through the ordinary scoped +store. It takes both the platform and the workspace id because that pair, +not the workspace id alone, is what `messaging_installs` makes unique; a +lookup on the id alone would answer twice the first time two platforms +happened to mint the same string, and refuse a customer's traffic for a +reason in someone else's account. + +It also filters on `status = 'active'`, which is not a refinement but the +other half of the same uniqueness. Only active rows are unique by index, so a +workspace that was installed, removed and installed again has two rows and +this would otherwise answer twice — refusing a customer's live traffic on the +strength of an install they themselves ended. The predicate here and the one +on the index are the same predicate, and have to stay that way. + Why not the obvious alternatives is argued in `docs/old/multi-tenancy-phase1-db.md`, "The bootstrap problem"; the short version is that returning rows instead of tenant ids would put a second copy @@ -160,9 +181,9 @@ class TenantLookupError(RuntimeError): @dataclass(frozen=True) class TenantLookup: - """One exempt function: its name, its single argument, and what it reads. + """One exempt function: its name, its arguments, and what it reads. - `argument` is the parameter name *without* the `p_` prefix the SQL carries. + `arguments` are the parameter names *without* the `p_` prefix the SQL carries. The prefix is not decoration: a `LANGUAGE sql` function whose parameter is spelled like a column of a table in its own query resolves the name to the column, silently, rather than to the parameter. Verified on Postgres 16, @@ -183,25 +204,29 @@ class TenantLookup: """ name: str - argument: str | None + arguments: tuple[str, ...] query: str purpose: str @property - def parameter(self) -> str | None: - return None if self.argument is None else f"p_{self.argument}" + def parameters(self) -> tuple[str, ...]: + return tuple(f"p_{argument}" for argument in self.arguments) + + @property + def parameter_declaration(self) -> str: + return ", ".join(f"{parameter} text" for parameter in self.parameters) @property def signature(self) -> str: - return f"{self.name}({'' if self.argument is None else 'text'})" + return f"{self.name}({', '.join('text' for _ in self.arguments)})" -# The seven of them. Ordered as the three shapes above: enumeration, then +# The nine of them. Ordered as the three shapes above: enumeration, then # credential resolution, then deriving a tenant from an identifier in hand. TENANT_LOOKUPS: tuple[TenantLookup, ...] = ( TenantLookup( name="all_tenant_ids", - argument=None, + arguments=(), query="SELECT id FROM tenants ORDER BY created_at, id", purpose=( "Every tenant in the deployment, oldest first. The one question a " @@ -211,7 +236,7 @@ def signature(self) -> str: ), TenantLookup( name="tenants_of_user", - argument="user_id", + arguments=("user_id",), query=( "SELECT tenant_id FROM tenant_members " "WHERE user_id = p_user_id ORDER BY tenant_id" @@ -224,7 +249,7 @@ def signature(self) -> str: ), TenantLookup( name="tenant_of_api_key", - argument="key_hash", + arguments=("key_hash",), query="SELECT tenant_id FROM api_keys WHERE key_hash = p_key_hash", purpose=( "Which tenant a bearer credential belongs to. `api_keys.key_hash` " @@ -234,7 +259,7 @@ def signature(self) -> str: ), TenantLookup( name="tenant_of_agent_oauth_client", - argument="oauth_client_id", + arguments=("oauth_client_id",), query=( "SELECT tenant_id FROM agents WHERE oauth_client_id = p_oauth_client_id" ), @@ -247,7 +272,7 @@ def signature(self) -> str: ), TenantLookup( name="tenant_of_room", - argument="room_id", + arguments=("room_id",), query="SELECT tenant_id FROM rooms WHERE id = p_room_id", purpose=( "Which tenant a room belongs to, for work reaching a room by its " @@ -256,7 +281,7 @@ def signature(self) -> str: ), TenantLookup( name="tenant_of_collaboration_bridge", - argument="bridge_id", + arguments=("bridge_id",), query="SELECT tenant_id FROM collaboration_bridges WHERE id = p_bridge_id", purpose=( "Which tenant a bridge belongs to. Asked from boot, with nothing " @@ -266,13 +291,13 @@ def signature(self) -> str: ), TenantLookup( name="tenant_of_server_connector", - argument="connector_id", + arguments=("connector_id",), query="SELECT tenant_id FROM server_connectors WHERE id = p_connector_id", purpose="Same as the bridge, for a server-side connector.", ), TenantLookup( name="tenant_of_invitation", - argument="token_hash", + arguments=("token_hash",), query="SELECT tenant_id FROM invitations WHERE token_hash = p_token_hash", purpose=( "Which tenant an invitation belongs to, resolved from the hash of " @@ -282,6 +307,25 @@ def signature(self) -> str: "refuses to a session with nothing bound yet." ), ), + TenantLookup( + name="tenant_of_messaging_install", + arguments=("platform", "external_workspace_id"), + query=( + "SELECT tenant_id FROM messaging_installs " + "WHERE platform = p_platform " + "AND external_workspace_id = p_external_workspace_id " + "AND status = 'active'" + ), + purpose=( + "Which tenant an inbound event from an installed workspace belongs " + "to. The public webhook is unauthenticated by nature and knows only " + "the platform it was posted to and the workspace the payload names, " + "so this runs before anything else the request does. The status " + "predicate is the index's own: only active rows are unique, and an " + "install that ended must stop answering for its workspace rather " + "than compete with the one that replaced it." + ), + ), ) TENANT_LOOKUPS_BY_NAME: dict[str, TenantLookup] = { @@ -297,9 +341,8 @@ def signature(self) -> str: def create_lookup_ddl(lookup: TenantLookup) -> str: - parameters = "" if lookup.parameter is None else f"{lookup.parameter} text" return ( - f"CREATE OR REPLACE FUNCTION {lookup.name}({parameters})\n" + f"CREATE OR REPLACE FUNCTION {lookup.name}({lookup.parameter_declaration})\n" f" RETURNS SETOF text\n" f" LANGUAGE sql STABLE SECURITY DEFINER\n" f" SET search_path = {SECURE_SEARCH_PATH}\n" @@ -337,33 +380,40 @@ def attach_tenant_lookups(metadata: MetaData) -> None: # ── Calling them ────────────────────────────────────────────────────────────── # One `text()` per lookup, written out rather than assembled from `lookup.name` -# at call time: the eight names are fixed and known here, so there is nothing -# for a call site to build. The assertion below is what keeps this dict from -# quietly falling behind `TENANT_LOOKUPS` — a ninth lookup with no entry -# here fails at import, not with a `KeyError` on whatever request reaches it -# first. +# at call time: the nine names are fixed and known here, so there is nothing +# for a call site to build. Each bind is named after the lookup's own argument, +# which is what lets `_call` zip them positionally against the dataclass and +# fail loudly on a mismatch rather than binding the workspace to the platform. +# The assertion below is what keeps this dict from quietly falling behind +# `TENANT_LOOKUPS` — a tenth lookup with no entry here fails at import, not +# with a `KeyError` on whatever request reaches it first. _LOOKUP_STATEMENTS: dict[str, TextClause] = { "all_tenant_ids": text("SELECT tenant_id FROM all_tenant_ids() AS tenant_id"), "tenants_of_user": text( - "SELECT tenant_id FROM tenants_of_user(:argument) AS tenant_id" + "SELECT tenant_id FROM tenants_of_user(:user_id) AS tenant_id" ), "tenant_of_api_key": text( - "SELECT tenant_id FROM tenant_of_api_key(:argument) AS tenant_id" + "SELECT tenant_id FROM tenant_of_api_key(:key_hash) AS tenant_id" ), "tenant_of_agent_oauth_client": text( - "SELECT tenant_id FROM tenant_of_agent_oauth_client(:argument) AS tenant_id" + "SELECT tenant_id FROM tenant_of_agent_oauth_client(:oauth_client_id) " + "AS tenant_id" ), "tenant_of_room": text( - "SELECT tenant_id FROM tenant_of_room(:argument) AS tenant_id" + "SELECT tenant_id FROM tenant_of_room(:room_id) AS tenant_id" ), "tenant_of_collaboration_bridge": text( - "SELECT tenant_id FROM tenant_of_collaboration_bridge(:argument) AS tenant_id" + "SELECT tenant_id FROM tenant_of_collaboration_bridge(:bridge_id) AS tenant_id" ), "tenant_of_server_connector": text( - "SELECT tenant_id FROM tenant_of_server_connector(:argument) AS tenant_id" + "SELECT tenant_id FROM tenant_of_server_connector(:connector_id) AS tenant_id" ), "tenant_of_invitation": text( - "SELECT tenant_id FROM tenant_of_invitation(:argument) AS tenant_id" + "SELECT tenant_id FROM tenant_of_invitation(:token_hash) AS tenant_id" + ), + "tenant_of_messaging_install": text( + "SELECT tenant_id FROM " + "tenant_of_messaging_install(:platform, :external_workspace_id) AS tenant_id" ), } @@ -375,7 +425,7 @@ def attach_tenant_lookups(metadata: MetaData) -> None: async def _call( session_factory: async_sessionmaker[AsyncSession], lookup: TenantLookup, - argument: str | None = None, + *arguments: str, ) -> list[str]: """Run one lookup on a session of its own, with nothing bound. @@ -384,10 +434,14 @@ async def _call( — and the answer must not be narrowed to it. Nothing bound is also the honest state: this session may not touch a scoped table, and the database is what enforces that now rather than an allowlist. + + `strict=True` on the zip is the arity check. A lookup called with one + argument too few would otherwise leave a bind unfilled, and a lookup whose + two arguments were passed the wrong way round is a webhook resolving the + wrong tenant — both are worth a `ValueError` here rather than a surprise + further down. """ - parameters: dict[str, object] = {} - if lookup.parameter is not None: - parameters["argument"] = argument + parameters: dict[str, object] = dict(zip(lookup.arguments, arguments, strict=True)) with no_tenant(): async with session_factory() as session: result = await session.execute(_LOOKUP_STATEMENTS[lookup.name], parameters) @@ -460,3 +514,14 @@ async def tenant_of_invitation( ) -> str | None: lookup = TENANT_LOOKUPS_BY_NAME["tenant_of_invitation"] return _at_most_one(lookup, await _call(session_factory, lookup, token_hash)) + + +async def tenant_of_messaging_install( + session_factory: async_sessionmaker[AsyncSession], + platform: str, + external_workspace_id: str, +) -> str | None: + lookup = TENANT_LOOKUPS_BY_NAME["tenant_of_messaging_install"] + return _at_most_one( + lookup, await _call(session_factory, lookup, platform, external_workspace_id) + ) diff --git a/core/switch_core/gateway/app.py b/core/switch_core/gateway/app.py index 6f1b46d2f..a97deaf73 100644 --- a/core/switch_core/gateway/app.py +++ b/core/switch_core/gateway/app.py @@ -8,6 +8,9 @@ from switch_core.bridges.agent.server_connectors.lifecycle import ( ServerSideConnectorLifecycleService, ) +from switch_core.bridges.collaboration.install_service import ( + MessagingInstallService, +) from switch_core.bridges.collaboration.lifecycle_service import ( CollaborationBridgeLifecycleService, ) @@ -32,6 +35,9 @@ from switch_core.gateway.dependencies import init_dependencies from switch_core.gateway.documents import router as documents_router from switch_core.gateway.ecosystem import router as ecosystem_router +from switch_core.gateway.messaging_installs import ( + router as messaging_installs_router, +) from switch_core.gateway.oidc_routes import register_oidc_client from switch_core.gateway.oidc_routes import router as oidc_router from switch_core.gateway.packages import router as packages_router @@ -64,6 +70,7 @@ def create_gateway_app( template_store: TemplateStore, resource_service: ResourceService, protocol: ProtocolService, + install_service: MessagingInstallService | None, config: SwitchConfig, ) -> FastAPI: init_dependencies( @@ -85,6 +92,7 @@ def create_gateway_app( template_store=template_store, resource_service=resource_service, protocol=protocol, + install_service=install_service, config=config, ) @@ -120,5 +128,10 @@ def create_gateway_app( app.include_router(packages_router, tags=["packages"]) app.include_router(templates_router, tags=["templates"]) app.include_router(ecosystem_router, prefix="/ecosystem", tags=["ecosystem"]) + app.include_router( + messaging_installs_router, + prefix="/messaging-apps", + tags=["messaging-apps"], + ) return app diff --git a/core/switch_core/gateway/collaborations.py b/core/switch_core/gateway/collaborations.py index 902a5c974..3490b6216 100644 --- a/core/switch_core/gateway/collaborations.py +++ b/core/switch_core/gateway/collaborations.py @@ -19,6 +19,7 @@ from switch_core.db.models import CollaborationBridge, ExternalUser, User from switch_core.db.stores.collaboration_bridge_store import CollaborationBridgeStore from switch_core.db.stores.external_user_store import ExternalUserStore +from switch_core.db.stores.messaging_install_store import MessagingInstallStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore from switch_core.gateway.auth import ( @@ -30,6 +31,7 @@ get_bridge_store, get_collab_lifecycle, get_external_user_store, + get_install_store, get_room_service, get_room_store, get_session, @@ -677,6 +679,7 @@ async def delete_bridge( bridge_store: Annotated[CollaborationBridgeStore, Depends(get_bridge_store)], room_store: Annotated[RoomStore, Depends(get_room_store)], room_service: Annotated[RoomService, Depends(get_room_service)], + install_store: Annotated[MessagingInstallStore, Depends(get_install_store)], collab_lifecycle: Annotated[ CollaborationBridgeLifecycleService, Depends(get_collab_lifecycle) ], @@ -688,6 +691,25 @@ async def delete_bridge( if bridge is None: raise HTTPException(status_code=404, detail="Bridge not found") + # Before a single room is deleted, because the rooms do not come back. A + # bridge built by an install holds a token this deployment did not issue + # and the platform still honours, and the install row's pointer at it is a + # real foreign key — so this delete would destroy every room on the bridge + # and *then* be refused by Postgres, leaving the bridge running, the rooms + # gone and a live credential nobody has revoked. + install = await install_store.get_for_bridge(session, bridge_id=bridge_id) + if install is not None: + raise HTTPException( + status_code=409, + detail=( + f"This connection was created by installing the Switch app into " + f"{install.platform} workspace {install.external_workspace_id}, so " + "it cannot be deleted here — the app would stay installed and its " + "token would stay valid. Disconnect the app instead, which revokes " + "the token at the platform and then removes this connection." + ), + ) + rooms = await room_store.get_by_bridge(session, bridge_id) for room in rooms: await room_service.delete_room(room.id) diff --git a/core/switch_core/gateway/dependencies.py b/core/switch_core/gateway/dependencies.py index 962561960..5358de849 100644 --- a/core/switch_core/gateway/dependencies.py +++ b/core/switch_core/gateway/dependencies.py @@ -10,6 +10,9 @@ from switch_core.bridges.agent.server_connectors.lifecycle import ( ServerSideConnectorLifecycleService, ) +from switch_core.bridges.collaboration.install_service import ( + MessagingInstallService, +) from switch_core.bridges.collaboration.lifecycle_service import ( CollaborationBridgeLifecycleService, ) @@ -21,6 +24,7 @@ from switch_core.db.stores.collaboration_bridge_store import CollaborationBridgeStore from switch_core.db.stores.external_user_store import ExternalUserStore from switch_core.db.stores.invitation_store import InvitationStore +from switch_core.db.stores.messaging_install_store import MessagingInstallStore from switch_core.db.stores.room_group_store import RoomGroupStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.server_connector_store import ServerConnectorStore @@ -52,6 +56,7 @@ def init_dependencies( template_store: TemplateStore, resource_service: ResourceService, protocol: ProtocolService, + install_service: MessagingInstallService | None, config: SwitchConfig, ) -> None: _state["agent_store"] = agent_store @@ -72,6 +77,7 @@ def init_dependencies( _state["template_store"] = template_store _state["resource_service"] = resource_service _state["protocol"] = protocol + _state["install_service"] = install_service _state["config"] = config @@ -215,5 +221,30 @@ def get_protocol() -> ProtocolService: return _state["protocol"] # type: ignore[no-any-return] +def get_install_store() -> MessagingInstallStore: + """Built here rather than threaded through `init_dependencies`. + + It is stateless — no connection, no configuration, nothing for a shared + instance to own — and `get_room_yaml_service` above already constructs + rather than reads. + + Deliberately not reached through `get_install_service`, which is `None` on + a deployment that registered no app of its own. Install rows outlive those + credentials: a bridge built by an install has to stay protected after the + credentials are taken away, which is exactly when the service is gone. + """ + return MessagingInstallStore() + + +def get_install_service() -> MessagingInstallService | None: + """None when this deployment registered no messaging app of its own. + + Nullable rather than absent because that is the ordinary case, and the + endpoints have to answer it with a refusal that says so rather than with a + KeyError. + """ + return _state["install_service"] # type: ignore[no-any-return] + + def get_resource_service() -> ResourceService: return _state["resource_service"] # type: ignore[no-any-return] diff --git a/core/switch_core/gateway/messaging_installs.py b/core/switch_core/gateway/messaging_installs.py new file mode 100644 index 000000000..ac730188d --- /dev/null +++ b/core/switch_core/gateway/messaging_installs.py @@ -0,0 +1,168 @@ +"""Starting an install: the authenticated leg, and the only one that picks a tenant. + +The endpoint answers with a URL rather than a redirect. The install has to +begin in a top-level browser window on the platform's own domain, and a +redirect from an XHR the operator's dashboard made would be followed by the +XHR, not by the window. Handing the URL back and letting the page navigate is +the shape that works. +""" + +from __future__ import annotations + +import logging +from datetime import datetime +from typing import Annotated + +from fastapi import APIRouter, Depends, HTTPException +from pydantic import BaseModel +from sqlalchemy.ext.asyncio import AsyncSession + +from switch_core.bridges.collaboration.install import MessagingInstallError +from switch_core.bridges.collaboration.install_service import MessagingInstallService +from switch_core.db.models import User, require_tenant_id +from switch_core.db.stores.messaging_install_store import MessagingInstallNotFound +from switch_core.gateway.auth import require_admin +from switch_core.gateway.dependencies import get_install_service, get_session + +logger = logging.getLogger(__name__) + +router = APIRouter() + + +class InstallStart(BaseModel): + authorize_url: str + + +class InstallablePlatforms(BaseModel): + platforms: list[str] + + +class InstalledApp(BaseModel): + """One install as an operator needs to see it. + + No token and nothing derived from one. `status` and `ended_at` are the + point of the shape rather than decoration: an operator looks at this list + because a bridge stopped working, and "disconnected" and "revoked" are the + difference between a decision someone here made and news from the + platform. + """ + + id: str + platform: str + external_workspace_id: str + status: str + scopes: str + bridge_id: str | None + installed_at: datetime + ended_at: datetime | None + + +class InstalledApps(BaseModel): + installs: list[InstalledApp] + + +def _require_installs( + service: MessagingInstallService | None, +) -> MessagingInstallService: + """Refuse rather than pretend when this deployment registered no app. + + Most deployments will register none — the self-hosted path is an operator + registering their own app and pasting the token in — so this is an ordinary + answer and not an error condition. It is still a refusal: a deployment that + offered the button and then failed at the platform would be worse. + """ + if service is None: + raise HTTPException( + status_code=501, + detail=( + "This Switch deployment has no messaging app of its own to " + "install. Register a bridge with your own app's credentials " + "instead." + ), + ) + return service + + +@router.get("") +async def installable_platforms( + service: Annotated[MessagingInstallService | None, Depends(get_install_service)], + _user: Annotated[User, Depends(require_admin)], +) -> InstallablePlatforms: + return InstallablePlatforms( + platforms=[] if service is None else service.platforms() + ) + + +@router.post("/{platform}/install") +async def begin_install( + platform: str, + session: Annotated[AsyncSession, Depends(get_session)], + service: Annotated[MessagingInstallService | None, Depends(get_install_service)], + user: Annotated[User, Depends(require_admin)], +) -> InstallStart: + try: + authorize_url = await _require_installs(service).begin( + session, platform=platform, user_id=user.id + ) + except MessagingInstallError as failure: + raise HTTPException(status_code=404, detail=str(failure)) from failure + + await session.commit() + logger.info("Started a %s install for user %s", platform, user.id) + return InstallStart(authorize_url=authorize_url) + + +@router.get("/installs") +async def list_installs( + session: Annotated[AsyncSession, Depends(get_session)], + service: Annotated[MessagingInstallService | None, Depends(get_install_service)], + _user: Annotated[User, Depends(require_admin)], +) -> InstalledApps: + """This organisation's installs, ended ones included. + + An empty list when the deployment registered no app of its own, rather + than the 501 the install endpoints answer with: a deployment can have + installs recorded from before its credentials were removed, and a page + that cannot even show them is worse than one with nothing on it. + """ + if service is None: + return InstalledApps(installs=[]) + return InstalledApps( + installs=[ + InstalledApp.model_validate(install, from_attributes=True) + for install in await service.list_installs(session) + ] + ) + + +@router.delete("/installs/{install_id}") +async def disconnect_install( + install_id: str, + service: Annotated[MessagingInstallService | None, Depends(get_install_service)], + user: Annotated[User, Depends(require_admin)], +) -> InstalledApp: + """End an install: revoke the credential, remove the bridge, free the workspace. + + Not on the request's session, deliberately. Disconnecting revokes a token + at the platform and tears a bridge down, and holding this request's + transaction open across both would keep a row locked while somebody else's + API is slow. The service opens what it needs, in the order failure can be + recovered from. + + **Rooms that used the bridge become internal-only**, which is why this is a + delete an operator has to ask for rather than anything inferred. + """ + try: + ended = await _require_installs(service).disconnect( + tenant_id=require_tenant_id(), install_id=install_id + ) + except MessagingInstallNotFound as missing: + raise HTTPException(status_code=404, detail=str(missing)) from missing + except MessagingInstallError as failure: + # The platform refused to revoke and nothing was destroyed, so this is + # upstream's answer rather than a fault here — and it is retryable, + # which the operator needs to be told rather than left to guess. + raise HTTPException(status_code=502, detail=str(failure)) from failure + + logger.info("User %s disconnected messaging install %s", user.id, install_id) + return InstalledApp.model_validate(ended, from_attributes=True) diff --git a/core/switch_core/main.py b/core/switch_core/main.py index 1c0423e2f..12ce05586 100644 --- a/core/switch_core/main.py +++ b/core/switch_core/main.py @@ -49,6 +49,11 @@ DiscordAdapter, DiscordConnectionConfig, ) +from switch_core.bridges.collaboration.install import MessagingInstallerRegistry +from switch_core.bridges.collaboration.install_routes import ( + create_messaging_install_router, +) +from switch_core.bridges.collaboration.install_service import MessagingInstallService from switch_core.bridges.collaboration.lifecycle_service import ( CollaborationBridgeLifecycleService, ) @@ -60,6 +65,7 @@ SlackAdapter, SlackConnectionConfig, ) +from switch_core.bridges.collaboration.slack.install import SlackAppInstaller from switch_core.bridges.collaboration.teams.adapter import ( TeamsAdapter, TeamsConnectionConfig, @@ -100,6 +106,8 @@ from switch_core.db.stores.invitation_store import InvitationStore from switch_core.db.stores.media_store import MediaStore from switch_core.db.stores.message_store import MessageStore +from switch_core.db.stores.messaging_event_store import MessagingEventReceiptStore +from switch_core.db.stores.messaging_install_store import MessagingInstallStore from switch_core.db.stores.package_store import PackageStore from switch_core.db.stores.reference_store import ReferenceStore from switch_core.db.stores.reference_type_store import ReferenceTypeStore @@ -475,6 +483,37 @@ async def run(config: SwitchConfig) -> None: "opencode", OpenCodeConnector, OpenCodeConnectionConfig ) + # ── Messaging app installs ────────────────────────────────────────────── + # Registration is the feature flag. An installer exists for a platform when + # this deployment holds that platform's app credentials, and the whole + # install surface refuses when none does — a deployment that registered no + # app cannot half-offer the button. Config validation has already required + # the three Slack values all together and a public origin with them. + installers = MessagingInstallerRegistry() + if config.slack_app_client_id: + assert config.slack_app_client_secret is not None + assert config.slack_app_signing_secret is not None + installers.register( + SlackAppInstaller( + client_id=config.slack_app_client_id, + client_secret=config.slack_app_client_secret, + signing_secret=config.slack_app_signing_secret, + ) + ) + + install_service: MessagingInstallService | None = None + if installers.platforms(): + assert config.messaging_public_url is not None + install_service = MessagingInstallService( + session_factory=session_factory, + store=MessagingInstallStore(), + receipts=MessagingEventReceiptStore(), + installers=installers, + lifecycle=collab_lifecycle, + public_origin=config.messaging_public_url, + secret=config.jwt_secret_key, + ) + # ── Gateway app ─────────────────────────────────────────────────────────── gateway_app = create_gateway_app( agent_store=agent_store, @@ -495,6 +534,7 @@ async def run(config: SwitchConfig) -> None: template_store=template_store, resource_service=resource_service, protocol=protocol, + install_service=install_service, config=config, ) @@ -516,6 +556,15 @@ async def run(config: SwitchConfig) -> None: async def health_check() -> JSONResponse: return JSONResponse({"status": "ok"}) + # Mounted on the agent-bridge app, not inside /gateway: this is the leg a + # platform and a customer's browser reach, and /gateway is neither routed + # here from outside nor reachable without a cookie they do not have. + if install_service is not None: + agent_bridge_app.include_router( + create_messaging_install_router(install_service), + tags=["messaging-installs"], + ) + agent_bridge_app.mount("/gateway", gateway_app) # ── Ensure system clients exist ───────────────────────────────────────── diff --git a/core/switch_core/migrations/versions/a7f2c3e9b481_messaging_installs_can_end.py b/core/switch_core/migrations/versions/a7f2c3e9b481_messaging_installs_can_end.py new file mode 100644 index 000000000..98a1c950d --- /dev/null +++ b/core/switch_core/migrations/versions/a7f2c3e9b481_messaging_installs_can_end.py @@ -0,0 +1,108 @@ +"""an install that can end, and a workspace that can be installed again + +`c8a4e21f6d30` gave `messaging_installs` a plain unique constraint on +`(platform, external_workspace_id)`, on the grounds that a workspace claimed +by two tenants is an event with two destinations. That grounds is still right; +the constraint was too strong for it. It made a claim permanent — the row it +protects is never deleted, so a customer who removed the Switch app from their +own Slack could not put it back, and the error they got told them to "remove +the existing install" through a path that did not exist. + +What actually has to be unique is the set of installs that are *serving*. So +the constraint becomes a unique index over `status = 'active'` under the same +name, and ending an install frees the workspace while keeping the record of +what happened to it. + +`tenant_of_messaging_install` is redefined against the same predicate, and +that is not a tidy-up. The function resolves a workspace to a tenant for +traffic nobody has authenticated, and it was written to rely on the old +constraint answering at most once. Left alone, the first workspace to be +installed twice would make it answer twice and the caller refuses an ambiguous +answer — so the customer's live install would stop receiving events because of +one they had themselves ended. + +Two column changes come with it. `ended_at` records when, alongside the +`status` that says which of the two ways. And `encrypted_bot_token` becomes +nullable so an ended install can keep its record without keeping its secret: +the token is dead by then, and a dead credential still reads like a credential +to whoever finds the dump. + +The lookup DDL below is a verbatim copy of `switch_core/db/tenant_lookup.py` +as it stood when this migration was written, copied rather than imported for +the reason every revision in this chain copies: a migration records a change +that already happened, and importing the live module would let a later edit +silently change what this one means. +`tests/switch_core/db/test_frozen_ddl_matches_create_all.py` is what keeps the +copy from drifting. + +Revision ID: a7f2c3e9b481 +Revises: d3f6b0c95a17 +Create Date: 2026-09-15 00:00:00.000000 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "a7f2c3e9b481" +down_revision: str | None = "d3f6b0c95a17" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +TABLE = "messaging_installs" +INDEX_NAME = "uq_messaging_installs_workspace" + +SECURE_SEARCH_PATH = "pg_catalog, public, pg_temp" + +CREATE_TENANT_OF_MESSAGING_INSTALL = f"""CREATE OR REPLACE FUNCTION tenant_of_messaging_install(p_platform text, p_external_workspace_id text) + RETURNS SETOF text + LANGUAGE sql STABLE SECURITY DEFINER + SET search_path = {SECURE_SEARCH_PATH} +AS $$SELECT tenant_id FROM messaging_installs WHERE platform = p_platform AND external_workspace_id = p_external_workspace_id AND status = 'active'$$""" + +RESTORE_TENANT_OF_MESSAGING_INSTALL = f"""CREATE OR REPLACE FUNCTION tenant_of_messaging_install(p_platform text, p_external_workspace_id text) + RETURNS SETOF text + LANGUAGE sql STABLE SECURITY DEFINER + SET search_path = {SECURE_SEARCH_PATH} +AS $$SELECT tenant_id FROM messaging_installs WHERE platform = p_platform AND external_workspace_id = p_external_workspace_id$$""" + + +def upgrade() -> None: + # Same name as the constraint it replaces, deliberately: the store reads + # the name out of the integrity error to tell a claimed workspace from any + # other write that failed, and Postgres reports a unique index by the same + # name a unique constraint would have carried. + op.drop_constraint(INDEX_NAME, TABLE, type_="unique") + op.create_index( + INDEX_NAME, + TABLE, + ["platform", "external_workspace_id"], + unique=True, + postgresql_where=sa.text("status = 'active'"), + ) + op.add_column( + TABLE, sa.Column("ended_at", sa.DateTime(timezone=True), nullable=True) + ) + op.alter_column( + TABLE, "encrypted_bot_token", existing_type=sa.Text(), nullable=True + ) + op.execute(CREATE_TENANT_OF_MESSAGING_INSTALL) + + +def downgrade() -> None: + op.execute(RESTORE_TENANT_OF_MESSAGING_INSTALL) + op.alter_column( + TABLE, "encrypted_bot_token", existing_type=sa.Text(), nullable=False + ) + op.drop_column(TABLE, "ended_at") + op.drop_index(INDEX_NAME, table_name=TABLE) + # Both of the statements above and this one fail on data the old schema + # cannot hold: an ended install whose token was discarded, and a workspace + # holding more than one row. That is the honest outcome — going back means + # choosing which of those installs survives, and a migration is not the + # thing that gets to choose. + op.create_unique_constraint( + INDEX_NAME, TABLE, ["platform", "external_workspace_id"] + ) diff --git a/core/switch_core/migrations/versions/b2e9d41c7f60_messaging_event_receipts.py b/core/switch_core/migrations/versions/b2e9d41c7f60_messaging_event_receipts.py new file mode 100644 index 000000000..89249574e --- /dev/null +++ b/core/switch_core/migrations/versions/b2e9d41c7f60_messaging_event_receipts.py @@ -0,0 +1,86 @@ +"""messaging_event_receipts + +A platform that retries is a platform that will eventually deliver the same +event twice, and the visible failure is an agent answering one question twice +in a customer's channel. This table is what makes the second delivery cheap to +recognise: a row is written before the work starts, the unique index arbitrates +between concurrent retries, and the loser stops. + +`received_at` carries an index of its own because pruning reads that column and +nothing else. Receipts are only interesting for as long as the platform might +still re-send, and without a sweep the table would grow with every message the +busiest workspace ever sends. + +The `tenant_isolation` policy is written out below rather than inherited, +because `265ed188ad6f` installed the policies as they stood then and a table +added afterwards has to bring its own. The DDL is a verbatim copy of +`switch_core/db/rls_ddl.py` as it stood when this migration was written, +copied rather than imported for the reason every revision in this chain +copies: a migration records a change that already happened, and importing the +live module would let a later edit silently change what this one means. +`tests/switch_core/db/test_frozen_ddl_matches_create_all.py` is what keeps the +copy from drifting. + +Revision ID: b2e9d41c7f60 +Revises: a7f2c3e9b481 +Create Date: 2026-09-15 00:00:00.000000 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "b2e9d41c7f60" +down_revision: str | None = "a7f2c3e9b481" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +TABLE = "messaging_event_receipts" +POLICY_NAME = "tenant_isolation" + +ENABLE_RLS = f'ALTER TABLE "{TABLE}" ENABLE ROW LEVEL SECURITY' + +CREATE_POLICY = f"""CREATE POLICY {POLICY_NAME} ON "{TABLE}" + FOR ALL + USING ("tenant_id" = (SELECT require_tenant_id())) + WITH CHECK ("tenant_id" = (SELECT require_tenant_id()))""" + +DROP_POLICY = f'DROP POLICY IF EXISTS {POLICY_NAME} ON "{TABLE}"' + + +def upgrade() -> None: + op.create_table( + TABLE, + sa.Column("id", sa.Text(), nullable=False), + sa.Column("tenant_id", sa.Text(), nullable=False), + sa.Column("platform", sa.Text(), nullable=False), + sa.Column("external_event_id", sa.Text(), nullable=False), + sa.Column( + "received_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.Column("handled_at", sa.DateTime(timezone=True), nullable=True), + sa.PrimaryKeyConstraint("id"), + sa.ForeignKeyConstraint( + ["tenant_id"], ["tenants.id"], name="fk_messaging_event_receipts_tenant" + ), + sa.UniqueConstraint( + "tenant_id", + "platform", + "external_event_id", + name="uq_messaging_event_receipts_event", + ), + ) + op.create_index("ix_messaging_event_receipts_received_at", TABLE, ["received_at"]) + op.execute(ENABLE_RLS) + op.execute(CREATE_POLICY) + + +def downgrade() -> None: + op.execute(DROP_POLICY) + op.drop_index("ix_messaging_event_receipts_received_at", table_name=TABLE) + op.drop_table(TABLE) diff --git a/core/switch_core/migrations/versions/c8a4e21f6d30_messaging_installs.py b/core/switch_core/migrations/versions/c8a4e21f6d30_messaging_installs.py new file mode 100644 index 000000000..160a15d79 --- /dev/null +++ b/core/switch_core/migrations/versions/c8a4e21f6d30_messaging_installs.py @@ -0,0 +1,126 @@ +"""messaging_installs, and the lookup that resolves one to a tenant + +The table behind the official Slack app: one row per external workspace a +tenant has installed us into, holding the token that install granted. It is +tenant-scoped like everything else, so it gets the same `tenant_isolation` +policy as the other tables — written out below rather than inherited, because +`265ed188ad6f` installed the policies as they stood then and a table added +afterwards has to bring its own. + +`(platform, external_workspace_id)` is unique across the deployment rather +than per tenant. Inbound events arrive over one public endpoint carrying a +workspace id and no tenant, so a workspace claimed twice is an event with two +possible destinations; the constraint makes that unrepresentable, and it has +to be the database that decides because a read-then-insert in application code +cannot be made atomic. It is also globally unique for the same reason +`api_keys.key_hash` is: the value is resolved before a tenant is known, so a +per-tenant index could not answer the question being asked. + +`tenant_of_messaging_install(platform, workspace)` is the lookup that asks it, +and it is an addition to the closed set of `SECURITY DEFINER` functions +`9c41a7b0e5d8` installed as the whole exemption from row-level security. That +set is deliberately short and every entry in it is callable by anyone holding +the runtime role's credentials, so adding one is meant to be argued with. The +argument for this one: the webhook is unauthenticated by nature and holds +nothing but a workspace id, the answer it returns is a tenant id and never a +row, and without it there is no way to bind a tenant before touching the +payload — which is the only order in which the payload may be touched at all. + +It takes two arguments where every other lookup takes one, because the pair is +what the table makes unique. A lookup on the workspace id alone would answer +twice the first time two platforms minted the same string, and the caller +refuses an ambiguous answer rather than picking — so one customer's traffic +would start failing for a reason in another platform's namespace. + +The DDL below is a verbatim copy of `switch_core/db/rls_ddl.py` and +`switch_core/db/tenant_lookup.py` as they stood when this migration was +written, copied rather than imported for the same reason every other revision +in this chain copies rather than imports: a migration is a record of a change +that already happened, and importing the live module would let a later edit +silently change what this one means. +`tests/switch_core/db/test_frozen_ddl_matches_create_all.py` is what keeps the +copy from drifting. + +Revision ID: c8a4e21f6d30 +Revises: b1d7c4f0a92e +Create Date: 2026-09-11 00:00:00.000000 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "c8a4e21f6d30" +down_revision: str | None = "5daaea6b674d" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +TABLE = "messaging_installs" +POLICY_NAME = "tenant_isolation" + +ENABLE_RLS = f'ALTER TABLE "{TABLE}" ENABLE ROW LEVEL SECURITY' + +CREATE_POLICY = f"""CREATE POLICY {POLICY_NAME} ON "{TABLE}" + FOR ALL + USING ("tenant_id" = (SELECT require_tenant_id())) + WITH CHECK ("tenant_id" = (SELECT require_tenant_id()))""" + +DROP_POLICY = f'DROP POLICY IF EXISTS {POLICY_NAME} ON "{TABLE}"' + +SECURE_SEARCH_PATH = "pg_catalog, public, pg_temp" + +CREATE_TENANT_OF_MESSAGING_INSTALL = f"""CREATE OR REPLACE FUNCTION tenant_of_messaging_install(p_platform text, p_external_workspace_id text) + RETURNS SETOF text + LANGUAGE sql STABLE SECURITY DEFINER + SET search_path = {SECURE_SEARCH_PATH} +AS $$SELECT tenant_id FROM messaging_installs WHERE platform = p_platform AND external_workspace_id = p_external_workspace_id$$""" + +DROP_TENANT_OF_MESSAGING_INSTALL = ( + "DROP FUNCTION IF EXISTS tenant_of_messaging_install(text, text)" +) + + +def upgrade() -> None: + op.create_table( + TABLE, + sa.Column("id", sa.Text(), nullable=False), + sa.Column("tenant_id", sa.Text(), nullable=False), + sa.Column("platform", sa.Text(), nullable=False), + sa.Column("external_workspace_id", sa.Text(), nullable=False), + sa.Column("encrypted_bot_token", sa.Text(), nullable=False), + sa.Column("scopes", sa.Text(), nullable=False), + sa.Column("status", sa.Text(), nullable=False), + sa.Column("installed_by_user_id", sa.Text(), nullable=False), + sa.Column("bridge_id", sa.Text(), nullable=True), + sa.Column( + "installed_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.PrimaryKeyConstraint("id"), + sa.ForeignKeyConstraint( + ["tenant_id"], ["tenants.id"], name="fk_messaging_installs_tenant" + ), + sa.ForeignKeyConstraint(["installed_by_user_id"], ["users.id"]), + sa.ForeignKeyConstraint( + ["tenant_id", "bridge_id"], + ["collaboration_bridges.tenant_id", "collaboration_bridges.id"], + name="fk_messaging_installs_bridge", + ), + sa.UniqueConstraint( + "platform", "external_workspace_id", name="uq_messaging_installs_workspace" + ), + sa.UniqueConstraint("id", "tenant_id", name="uq_messaging_installs_id_tenant"), + ) + op.execute(ENABLE_RLS) + op.execute(CREATE_POLICY) + op.execute(CREATE_TENANT_OF_MESSAGING_INSTALL) + + +def downgrade() -> None: + op.execute(DROP_TENANT_OF_MESSAGING_INSTALL) + op.execute(DROP_POLICY) + op.drop_table(TABLE) diff --git a/core/switch_core/migrations/versions/d3f6b0c95a17_messaging_install_states.py b/core/switch_core/migrations/versions/d3f6b0c95a17_messaging_install_states.py new file mode 100644 index 000000000..b5b8522cd --- /dev/null +++ b/core/switch_core/migrations/versions/d3f6b0c95a17_messaging_install_states.py @@ -0,0 +1,87 @@ +"""messaging_install_states + +The other half of an install: the in-flight record that ties the operator who +started one to the callback that finishes it. `messaging_installs` is the +result; this is the ticket. + +It installs no lookup, and that is the interesting thing about it. The +callback is as unauthenticated as the webhook is, so the obvious shape would +have been a ninth `SECURITY DEFINER` function resolving a state to its tenant +— exactly `tenant_of_api_key`'s shape, an opaque credential presented by a +stranger. It is not needed: the `state` parameter is a token we signed, so the +tenant travels inside it and the callback can bind before it reads. The row +exists for the one thing a signature cannot do, which is to stop being valid +the second time it is presented. + +So redemption is an ordinary scoped write — set `consumed_at` where it is +still null, in one statement, and take the absence of a returned row as the +refusal. Row-level security then checks the signature's claim a second time +for free: a token naming the wrong tenant matches no row. + +The `tenant_isolation` policy is written out below rather than inherited, +because `265ed188ad6f` installed the policies as they stood then and a table +added afterwards has to bring its own. The DDL is a verbatim copy of +`switch_core/db/rls_ddl.py` as it stood when this migration was written, +copied rather than imported for the reason every revision in this chain +copies: a migration records a change that already happened, and importing the +live module would let a later edit silently change what this one means. +`tests/switch_core/db/test_frozen_ddl_matches_create_all.py` is what keeps the +copy from drifting. + +Revision ID: d3f6b0c95a17 +Revises: c8a4e21f6d30 +Create Date: 2026-09-11 00:00:00.000000 + +""" + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "d3f6b0c95a17" +down_revision: str | None = "c8a4e21f6d30" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +TABLE = "messaging_install_states" +POLICY_NAME = "tenant_isolation" + +ENABLE_RLS = f'ALTER TABLE "{TABLE}" ENABLE ROW LEVEL SECURITY' + +CREATE_POLICY = f"""CREATE POLICY {POLICY_NAME} ON "{TABLE}" + FOR ALL + USING ("tenant_id" = (SELECT require_tenant_id())) + WITH CHECK ("tenant_id" = (SELECT require_tenant_id()))""" + +DROP_POLICY = f'DROP POLICY IF EXISTS {POLICY_NAME} ON "{TABLE}"' + + +def upgrade() -> None: + op.create_table( + TABLE, + sa.Column("id", sa.Text(), nullable=False), + sa.Column("tenant_id", sa.Text(), nullable=False), + sa.Column("platform", sa.Text(), nullable=False), + sa.Column("created_by_user_id", sa.Text(), nullable=False), + sa.Column( + "created_at", + sa.DateTime(timezone=True), + server_default=sa.text("now()"), + nullable=False, + ), + sa.Column("expires_at", sa.DateTime(timezone=True), nullable=False), + sa.Column("consumed_at", sa.DateTime(timezone=True), nullable=True), + sa.PrimaryKeyConstraint("id"), + sa.ForeignKeyConstraint( + ["tenant_id"], ["tenants.id"], name="fk_messaging_install_states_tenant" + ), + sa.ForeignKeyConstraint(["created_by_user_id"], ["users.id"]), + ) + op.execute(ENABLE_RLS) + op.execute(CREATE_POLICY) + + +def downgrade() -> None: + op.execute(DROP_POLICY) + op.drop_table(TABLE) diff --git a/core/tests/switch_core/bridges/agent/test_version_is_not_public.py b/core/tests/switch_core/bridges/agent/test_version_is_not_public.py index 8c946faa2..5d009dab3 100644 --- a/core/tests/switch_core/bridges/agent/test_version_is_not_public.py +++ b/core/tests/switch_core/bridges/agent/test_version_is_not_public.py @@ -39,4 +39,10 @@ def test_the_unauthenticated_allowlist_has_not_grown() -> None: "/oauth", "/gateway", "/deeplink", + # Workspace installs of the distributed messaging apps: the OAuth + # callback and the platforms' event webhooks. Public because the + # callers are Slack and a browser mid-redirect, neither of which holds + # a credential of ours. Nothing under it discloses a version, and each + # route checks the platform's signature before it acts. + "/messaging", } diff --git a/core/tests/switch_core/bridges/collaboration/test_bridge_type_registry.py b/core/tests/switch_core/bridges/collaboration/test_bridge_type_registry.py index 34fafc77f..6d0bb7940 100644 --- a/core/tests/switch_core/bridges/collaboration/test_bridge_type_registry.py +++ b/core/tests/switch_core/bridges/collaboration/test_bridge_type_registry.py @@ -100,7 +100,11 @@ def test_get_config_schema_exposes_required_fields() -> None: "agent_usergroups", "agent_sessions", } - assert set(schema["required"]) == {"bot_token", "app_token", "workspace_id"} + # app_token is offered but not required by the schema: a bridge whose + # events arrive over HTTP has none. What enforces it for a Socket Mode + # bridge — which receives nothing at all without one — is the model + # validator, not this form. + assert set(schema["required"]) == {"bot_token", "workspace_id"} def test_get_config_schema_unknown_type_raises() -> None: diff --git a/core/tests/switch_core/bridges/collaboration/test_install_service.py b/core/tests/switch_core/bridges/collaboration/test_install_service.py new file mode 100644 index 000000000..a6879881d --- /dev/null +++ b/core/tests/switch_core/bridges/collaboration/test_install_service.py @@ -0,0 +1,631 @@ +"""The install as one operation, across the two requests it is actually made of. + +The individual pieces are covered elsewhere — the signature in +`test_install_state.py`, single use in the store's own tests, Slack's responses +in `test_slack_installer.py`. What is only visible here is the join: that the +tenant the first leg was authenticated for is the tenant the second leg writes +to, that a replay gets no further than the burn, and that a bridge appears at +the end configured the way an operator's own would be. + +Postgres is real because row-level security is the second half of the argument +and a mock has no policies. +""" + +from __future__ import annotations + +import uuid +from collections.abc import Mapping +from typing import ClassVar +from urllib.parse import parse_qs, urlparse + +import pytest +from sqlalchemy.ext.asyncio import async_sessionmaker + +from switch_core.bridges.collaboration.install import ( + InboundWebhook, + InstallGrant, + MessagingAppInstaller, + MessagingInstallerRegistry, + MessagingInstallError, + WebhookEndpoint, +) +from switch_core.bridges.collaboration.install_service import ( + InstallPlatformMismatch, + MessagingInstallService, +) +from switch_core.bridges.collaboration.install_state import ( + InstallState, + InstallStateError, + mint, +) +from switch_core.crypto import decrypt_token +from switch_core.db.models import ( + Client, + CollaborationBridge, + MessagingInstall, + Tenant, + User, +) +from switch_core.db.session_scope import tenant_session +from switch_core.db.stores.messaging_event_store import MessagingEventReceiptStore +from switch_core.db.stores.messaging_install_store import ( + INSTALL_ACTIVE, + INSTALL_DISCONNECTED, + INSTALL_REVOKED, + MessagingInstallClaimedError, + MessagingInstallNotFound, + MessagingInstallStateError, + MessagingInstallStore, +) +from tests.conftest import RLSHarness + +pytestmark = pytest.mark.no_ambient_tenant + +_SECRET = "test-secret" +_ORIGIN = "https://switch.example" + + +class _FakeInstaller(MessagingAppInstaller): + """A platform that always says yes, and counts how often it was asked.""" + + platform: ClassVar[str] = "slack" + + def __init__(self, workspace_id: str) -> None: + self.workspace_id = workspace_id + self.redeem_calls: list[str] = [] + self.revoked_tokens: list[str] = [] + self.revoke_error: Exception | None = None + + def authorize_url(self, *, state: str, redirect_uri: str) -> str: + return f"https://platform.example/authorize?state={state}" + + async def redeem(self, *, code: str, redirect_uri: str) -> InstallGrant: + self.redeem_calls.append(redirect_uri) + return InstallGrant( + external_workspace_id=self.workspace_id, + workspace_name="Acme", + bot_token="xoxb-granted", + scopes="chat:write", + ) + + async def revoke(self, *, bot_token: str) -> None: + if self.revoke_error is not None: + raise self.revoke_error + self.revoked_tokens.append(bot_token) + + def revocation_of_event(self, payload: Mapping[str, object]) -> str | None: + event = payload.get("event") + if not isinstance(event, dict): + return None + if event.get("type") == "app_uninstalled": + return "the app was removed" + return None + + def verify_webhook(self, *, headers: Mapping[str, str], body: bytes) -> None: + return None + + def parse_webhook( + self, *, endpoint: WebhookEndpoint, headers: Mapping[str, str], body: bytes + ) -> InboundWebhook: + return InboundWebhook( + envelope_type=endpoint, + payload={}, + handshake=None, + external_event_id=None, + delivery_attempt=0, + ) + + def workspace_of_event(self, payload: Mapping[str, object]) -> str: + return self.workspace_id + + def connection_config(self, grant: InstallGrant) -> dict[str, object]: + return {"bot_token": grant.bot_token, "workspace_id": grant.workspace_name} + + +class _FakeLifecycle: + """Stands in for the bridge lifecycle, which starts real network clients. + + It still writes the rows, because the install points at the bridge through + a composite foreign key and a stub returning an id that is not in the table + would prove the last step works when it does not. + """ + + def __init__( + self, factory: async_sessionmaker, tenant_id: str, suffix: str + ) -> None: + self._factory = factory + self._tenant_id = tenant_id + self._suffix = suffix + self.registered: list[dict[str, object]] = [] + self.removed: list[str] = [] + + async def register(self, **kwargs: object) -> CollaborationBridge: + self.registered.append(kwargs) + async with tenant_session(self._factory, self._tenant_id) as session: + client = Client( + matrix_user_id=f"@bridge-{len(self.registered)}:{self._suffix}", + display_name=str(kwargs["display_name"]), + type="collaboration_bridge", + ) + session.add(client) + await session.flush() + bridge = CollaborationBridge( + type=str(kwargs["bridge_type"]), + display_name=str(kwargs["display_name"]), + connection_config={}, + client_id=client.id, + status="active", + ) + session.add(bridge) + await session.flush() + bridge_id = bridge.id + await session.commit() + return CollaborationBridge(id=bridge_id) + + async def remove(self, bridge_id: str) -> None: + """Delete the row, not just record the call. + + The install points at the bridge through a foreign key with no + `ON DELETE`, so a stub that only counted removals would let a caller + that forgot to release the pointer pass here and fail in production. + """ + self.removed.append(bridge_id) + async with tenant_session(self._factory, self._tenant_id) as session: + bridge = await session.get(CollaborationBridge, bridge_id) + if bridge is not None: + await session.delete(bridge) + await session.commit() + + +class _Fixture: + def __init__(self) -> None: + self.tenant_a: str = "" + self.tenant_b: str = "" + self.user_id: str = "" + self.workspace: str = "" + self.installer: _FakeInstaller + self.lifecycle: _FakeLifecycle + self.service: MessagingInstallService + + +async def _fixture(harness: RLSHarness) -> _Fixture: + fixture = _Fixture() + suffix = uuid.uuid4().hex[:8] + fixture.tenant_a = f"tenant-a-{suffix}" + fixture.tenant_b = f"tenant-b-{suffix}" + fixture.workspace = f"T-{suffix}" + + async with harness.owner() as session: + for tenant_id in (fixture.tenant_a, fixture.tenant_b): + session.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + user = User(name="installer", email=f"{suffix}@example.test", role="user") + session.add(user) + await session.flush() + fixture.user_id = user.id + await session.commit() + + fixture.installer = _FakeInstaller(fixture.workspace) + fixture.lifecycle = _FakeLifecycle(harness.restricted, fixture.tenant_a, suffix) + installers = MessagingInstallerRegistry() + installers.register(fixture.installer) + fixture.service = MessagingInstallService( + session_factory=harness.restricted, + store=MessagingInstallStore(), + receipts=MessagingEventReceiptStore(), + installers=installers, + lifecycle=fixture.lifecycle, # type: ignore[arg-type] + public_origin=_ORIGIN, + secret=_SECRET, + ) + return fixture + + +async def _begin(factory: async_sessionmaker, fixture: _Fixture, tenant_id: str) -> str: + """Start an install as `tenant_id` and return the state it minted.""" + async with tenant_session(factory, tenant_id) as session: + url = await fixture.service.begin( + session, platform="slack", user_id=fixture.user_id + ) + await session.commit() + return parse_qs(urlparse(url).query)["state"][0] + + +async def _installed( + factory: async_sessionmaker, fixture: _Fixture, tenant_id: str +) -> MessagingInstall: + """Run both legs and return the install they produced.""" + state = await _begin(factory, fixture, tenant_id) + return await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + + +async def _reread( + factory: async_sessionmaker, tenant_id: str, install_id: str +) -> MessagingInstall: + async with tenant_session(factory, tenant_id) as session: + return await MessagingInstallStore().get(session, install_id=install_id) + + +class TestTheRoundTrip: + async def test_an_install_lands_in_the_tenant_that_started_it( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + + install = await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + + assert install.tenant_id == fixture.tenant_a + assert install.external_workspace_id == fixture.workspace + assert install.installed_by_user_id == fixture.user_id + + async def test_the_bot_token_is_encrypted_at_rest( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + + install = await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + + assert "xoxb-granted" not in install.encrypted_bot_token + assert decrypt_token(install.encrypted_bot_token, _SECRET) == "xoxb-granted" + + async def test_it_ends_with_a_bridge_the_install_points_at( + self, rls_harness: RLSHarness + ) -> None: + """The seam. An install that records a credential and builds nothing is + a customer who clicked Add to Slack and got nothing.""" + fixture = await _fixture(rls_harness) + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + + install = await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + + assert len(fixture.lifecycle.registered) == 1 + registered = fixture.lifecycle.registered[0] + assert registered["bridge_type"] == "slack" + assert registered["display_name"] == "Acme" + assert install.bridge_id is not None + + async def test_the_redirect_is_the_public_one_on_both_legs( + self, rls_harness: RLSHarness + ) -> None: + """The platform compares them, so a mismatch is a refused install. + + Built from the configured origin rather than from either request, + which is the only way the two legs — on different hostnames — agree. + """ + fixture = await _fixture(rls_harness) + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + + assert fixture.installer.redeem_calls == [ + f"{_ORIGIN}/messaging/slack/oauth/callback" + ] + + +class TestWhatTheCallbackWillNotDo: + async def test_a_replayed_state_never_reaches_the_platform( + self, rls_harness: RLSHarness + ) -> None: + """Burnt before the code is exchanged, not after. + + The count is the assertion: a replay that got as far as redeeming would + have obtained a second credential before anything refused it. + """ + fixture = await _fixture(rls_harness) + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + + with pytest.raises(MessagingInstallStateError): + await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + assert len(fixture.installer.redeem_calls) == 1 + + async def test_a_state_naming_a_tenant_it_does_not_own_is_refused( + self, rls_harness: RLSHarness + ) -> None: + """What is left if the signing key ever leaks. + + Forging a state for tenant B against tenant A's row gets past the + signature by construction, and row-level security still finds no row + to burn. + """ + fixture = await _fixture(rls_harness) + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state_id = ( + await MessagingInstallStore().start_install( + session, platform="slack", user_id=fixture.user_id + ) + ).id + await session.commit() + + forged = mint( + InstallState( + tenant_id=fixture.tenant_b, state_id=state_id, platform="slack" + ), + secret=_SECRET, + ) + with pytest.raises(MessagingInstallStateError): + await fixture.service.complete( + platform="slack", code="the-code", state_token=forged + ) + assert fixture.installer.redeem_calls == [] + assert state # the untouched state is still redeemable; nothing was burnt + + async def test_an_unsigned_state_never_touches_the_database( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + with pytest.raises(InstallStateError): + await fixture.service.complete( + platform="slack", code="the-code", state_token="v1.aaa.bbb" + ) + + async def test_a_state_for_another_platform_is_refused( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + forged = mint( + InstallState( + tenant_id=fixture.tenant_a, state_id="whatever", platform="teams" + ), + secret=_SECRET, + ) + with pytest.raises(InstallPlatformMismatch): + await fixture.service.complete( + platform="slack", code="the-code", state_token=forged + ) + + async def test_a_workspace_another_tenant_holds_is_refused( + self, rls_harness: RLSHarness + ) -> None: + """And no bridge is built for it. + + The whole point of the deployment-wide unique constraint: one + workspace's events have exactly one destination. + """ + fixture = await _fixture(rls_harness) + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + session.add( + MessagingInstall( + tenant_id=fixture.tenant_b, + platform="slack", + external_workspace_id=fixture.workspace, + encrypted_bot_token="ciphertext", + scopes="chat:write", + status="active", + installed_by_user_id=fixture.user_id, + ) + ) + await session.commit() + + state = await _begin(rls_harness.restricted, fixture, fixture.tenant_a) + with pytest.raises(MessagingInstallClaimedError): + await fixture.service.complete( + platform="slack", code="the-code", state_token=state + ) + assert fixture.lifecycle.registered == [] + + +class TestDisconnecting: + """An install an operator ended, and what has to be true afterwards.""" + + async def test_the_platform_is_told_before_anything_is_destroyed( + self, rls_harness: RLSHarness + ) -> None: + """Deleting our copy of a token does not stop it working. + + Revoking is the half only the platform can do, so a disconnect that + skipped it would leave a live key into a customer's workspace in every + backup taken before it. + """ + fixture = await _fixture(rls_harness) + install = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=install.id + ) + + assert fixture.installer.revoked_tokens == ["xoxb-granted"] + + async def test_it_leaves_no_credential_and_no_bridge( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + install = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + bridge_id = install.bridge_id + + ended = await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=install.id + ) + + assert ended.status == INSTALL_DISCONNECTED + assert ended.ended_at is not None + assert ended.encrypted_bot_token is None + assert ended.bridge_id is None + assert fixture.lifecycle.removed == [bridge_id] + + async def test_the_workspace_can_be_installed_again( + self, rls_harness: RLSHarness + ) -> None: + """The reason the uniqueness index is partial. + + A customer who disconnects and changes their mind must be able to + click Add to Slack again, and a row that went on occupying the + workspace would mean nobody ever could. + """ + fixture = await _fixture(rls_harness) + first = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=first.id + ) + + second = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + assert second.id != first.id + assert second.status == INSTALL_ACTIVE + assert second.bridge_id is not None + + async def test_disconnecting_twice_is_success( + self, rls_harness: RLSHarness + ) -> None: + """An operator can click it on a row the platform ended a second ago.""" + fixture = await _fixture(rls_harness) + install = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=install.id + ) + + again = await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=install.id + ) + + assert again.status == INSTALL_DISCONNECTED + assert fixture.installer.revoked_tokens == ["xoxb-granted"] + assert len(fixture.lifecycle.removed) == 1 + + async def test_a_refusal_nobody_understands_leaves_the_install_intact( + self, rls_harness: RLSHarness + ) -> None: + """Which is the whole reason the platform is told first. + + The operator sees the failure and can try again; the alternative is a + bridge already gone and a token still valid. + """ + fixture = await _fixture(rls_harness) + install = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + fixture.installer.revoke_error = MessagingInstallError("Slack said no") + + with pytest.raises(MessagingInstallError): + await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=install.id + ) + + still = await _reread(rls_harness.restricted, fixture.tenant_a, install.id) + assert still.status == INSTALL_ACTIVE + assert still.encrypted_bot_token is not None + assert still.bridge_id is not None + assert fixture.lifecycle.removed == [] + + async def test_another_tenants_install_is_not_disconnectable( + self, rls_harness: RLSHarness + ) -> None: + """And misses the way an invented id would, telling the caller nothing.""" + fixture = await _fixture(rls_harness) + install = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + with pytest.raises(MessagingInstallNotFound): + await fixture.service.disconnect( + tenant_id=fixture.tenant_b, install_id=install.id + ) + + assert fixture.installer.revoked_tokens == [] + + +class TestThePlatformEndingIt: + async def test_an_uninstall_event_ends_the_install( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + install = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + await fixture.service.revoked( + platform="slack", + workspace_id=fixture.workspace, + reason="the app was removed", + ) + + ended = await _reread(rls_harness.restricted, fixture.tenant_a, install.id) + assert ended.status == INSTALL_REVOKED + assert ended.encrypted_bot_token is None + assert ended.bridge_id is None + assert fixture.lifecycle.removed == [install.bridge_id] + + async def test_nothing_is_revoked_at_a_platform_that_already_did_it( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + await fixture.service.revoked( + platform="slack", + workspace_id=fixture.workspace, + reason="the app was removed", + ) + + assert fixture.installer.revoked_tokens == [] + + async def test_a_retried_event_is_not_an_error( + self, rls_harness: RLSHarness + ) -> None: + """Slack redelivers, so the second one arrives after the workspace is free.""" + fixture = await _fixture(rls_harness) + await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + await fixture.service.revoked( + platform="slack", workspace_id=fixture.workspace, reason="removed" + ) + + await fixture.service.revoked( + platform="slack", workspace_id=fixture.workspace, reason="removed" + ) + + assert len(fixture.lifecycle.removed) == 1 + + async def test_a_workspace_nobody_installed_is_ignored( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + + await fixture.service.revoked( + platform="slack", workspace_id="T-nobody", reason="removed" + ) + + assert fixture.lifecycle.removed == [] + + +class TestWhatTheOperatorSees: + async def test_the_list_keeps_installs_that_ended( + self, rls_harness: RLSHarness + ) -> None: + """A list of only the live ones answers "nothing here" to "what happened + to the one that was here yesterday".""" + fixture = await _fixture(rls_harness) + first = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + await fixture.service.disconnect( + tenant_id=fixture.tenant_a, install_id=first.id + ) + second = await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + listed = await MessagingInstallStore().list_for_tenant(session) + + assert {install.id for install in listed} == {first.id, second.id} + assert {install.status for install in listed} == { + INSTALL_ACTIVE, + INSTALL_DISCONNECTED, + } + + async def test_the_list_is_the_bound_tenants_own( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + await _installed(rls_harness.restricted, fixture, fixture.tenant_a) + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + listed = await MessagingInstallStore().list_for_tenant(session) + + assert listed == [] diff --git a/core/tests/switch_core/bridges/collaboration/test_install_state.py b/core/tests/switch_core/bridges/collaboration/test_install_state.py new file mode 100644 index 000000000..6ba915df0 --- /dev/null +++ b/core/tests/switch_core/bridges/collaboration/test_install_state.py @@ -0,0 +1,117 @@ +"""The state token is the only thing binding a tenant across the install. + +It is minted on a page a customer's admin loaded and comes back from the +public internet, so every test here is about what happens when what comes back +is not what went out. The consequence of getting it wrong is not a failed +install: it is an attacker's Slack workspace attached to somebody else's +tenant, delivering messages into their rooms. +""" + +from __future__ import annotations + +import base64 +import hashlib +import hmac +import json + +import pytest + +from switch_core.bridges.collaboration.install_state import ( + InstallState, + InstallStateError, + mint, + verify, +) + +_SECRET = "test-jwt-secret" + +_STATE = InstallState(tenant_id="tenant-a", state_id="state-1", platform="slack") + + +def test_a_minted_state_verifies_back_to_what_went_in() -> None: + assert verify(mint(_STATE, secret=_SECRET), secret=_SECRET) == _STATE + + +def test_the_token_is_safe_in_a_url() -> None: + """It travels as a query parameter through a redirect the platform builds.""" + token = mint(_STATE, secret=_SECRET) + assert all(c.isalnum() or c in "-_." for c in token) + + +class TestForgery: + def test_an_edited_tenant_is_refused(self) -> None: + """The attack the signature exists to stop. + + Swapping the tenant in a captured state is how you would attach your + own workspace to someone else's rooms. + """ + token = mint(_STATE, secret=_SECRET) + payload, signature = token.removeprefix("v1.").split(".") + decoded = json.loads(base64.urlsafe_b64decode(payload + "==")) + decoded["tid"] = "tenant-b" + forged = ( + base64.urlsafe_b64encode(json.dumps(decoded).encode()).decode().rstrip("=") + ) + + with pytest.raises(InstallStateError): + verify(f"v1.{forged}.{signature}", secret=_SECRET) + + def test_a_state_from_another_deployment_is_refused(self) -> None: + token = mint(_STATE, secret="a-different-secret") + with pytest.raises(InstallStateError, match="not signed by this deployment"): + verify(token, secret=_SECRET) + + def test_the_key_is_not_the_jwt_key_itself(self) -> None: + """Domain separation, so one signature can never be read as the other. + + Asserted by construction rather than by outcome: the token is signed + under a derived key, so signing the same payload with the raw secret + produces something this refuses. + """ + token = mint(_STATE, secret=_SECRET) + payload = token.removeprefix("v1.").split(".")[0] + raw = hmac.new(_SECRET.encode(), payload.encode(), hashlib.sha256).digest() + naive = base64.urlsafe_b64encode(raw).decode().rstrip("=") + + with pytest.raises(InstallStateError): + verify(f"v1.{payload}.{naive}", secret=_SECRET) + + +class TestMalformed: + @pytest.mark.parametrize( + "token", + [ + "", + "not-a-token", + "v1.", + "v1.only-one-part", + "v1.a.b.c", + "v1.!!!.!!!", + "v2." + mint(_STATE, secret=_SECRET).removeprefix("v1."), + ], + ) + def test_garbage_is_an_error_and_not_a_crash(self, token: str) -> None: + """Reachable by anyone who finds the callback URL. + + Every one of these has to be a refused install rather than a traceback, + because the caller is unauthenticated by nature. + """ + with pytest.raises(InstallStateError): + verify(token, secret=_SECRET) + + def test_a_well_signed_payload_that_is_not_a_state_is_refused(self) -> None: + """Signed by us, but not this. A key used for two things is a bug.""" + payload = base64.urlsafe_b64encode(b'{"sub":"agent-1"}').decode().rstrip("=") + key = hmac.new( + _SECRET.encode(), b"switch/messaging-install-state/v1", hashlib.sha256 + ).digest() + signature = ( + base64.urlsafe_b64encode( + hmac.new(key, payload.encode(), hashlib.sha256).digest() + ) + .decode() + .rstrip("=") + ) + + with pytest.raises(InstallStateError, match="malformed"): + verify(f"v1.{payload}.{signature}", secret=_SECRET) diff --git a/core/tests/switch_core/bridges/collaboration/test_install_webhook.py b/core/tests/switch_core/bridges/collaboration/test_install_webhook.py new file mode 100644 index 000000000..7b72dc33f --- /dev/null +++ b/core/tests/switch_core/bridges/collaboration/test_install_webhook.py @@ -0,0 +1,882 @@ +"""An event from an installed workspace, from the wire to the right bridge. + +This is the half of multi-tenancy that has no session to lean on. A Slack post +arrives at one public URL shared by every tenant on the deployment, carrying no +credential of ours, and something has to decide whose it is. Everything that +decides it is here: the signature, the workspace id in the payload, and the +lookup from that workspace to a tenant. + +Two tenants exist in every fixture below, each with the app installed into its +own workspace, and the assertion that matters is the same one each time: an +event for one of them reaches that one's bridge and **no part of it reaches the +other's**. A test with a single tenant would pass against a router that ignored +the payload entirely. + +Real Postgres, the restricted role, and the real Slack installer — including +the real signature check. What is faked is only what would otherwise open a +socket: the bridge lifecycle and the adapter at the end of it. +""" + +from __future__ import annotations + +import asyncio +import hashlib +import hmac +import json +import time +import uuid +from dataclasses import dataclass +from datetime import UTC, datetime, timedelta +from typing import Any +from urllib.parse import urlencode + +import httpx +import pytest +from fastapi import FastAPI +from sqlalchemy import select +from sqlalchemy.ext.asyncio import async_sessionmaker + +from switch_core.bridges.collaboration.adapter import CollaborationAdapter +from switch_core.bridges.collaboration.install import ( + MessagingInstallerRegistry, + commands_path, + events_path, + interactive_path, +) +from switch_core.bridges.collaboration.install_routes import ( + create_messaging_install_router, +) +from switch_core.bridges.collaboration.install_service import MessagingInstallService +from switch_core.bridges.collaboration.slack.install import SlackAppInstaller +from switch_core.db.models import ( + Client, + CollaborationBridge, + MessagingEventReceipt, + MessagingInstall, + Tenant, + User, +) +from switch_core.db.session_scope import tenant_session +from switch_core.db.stores.messaging_event_store import ( + RECEIPT_RETENTION, + MessagingEventReceiptStore, +) +from switch_core.db.stores.messaging_install_store import MessagingInstallStore +from switch_core.tenant_context import current_tenant_id +from tests.conftest import RLSHarness + +pytestmark = pytest.mark.no_ambient_tenant + +_SIGNING_SECRET = "test-signing-secret" +_SECRET = "test-secret" +_ORIGIN = "https://switch.example" + + +def _signed(body: bytes) -> dict[str, str]: + timestamp = str(int(time.time())) + digest = hmac.new( + _SIGNING_SECRET.encode(), + b"v0:" + timestamp.encode() + b":" + body, + hashlib.sha256, + ).hexdigest() + return { + "X-Slack-Request-Timestamp": timestamp, + "X-Slack-Signature": f"v0={digest}", + "Content-Type": "application/json", + } + + +class _SocketOnlyAdapter(CollaborationAdapter): + """A bridge that takes its events some other way. + + Concrete only so one can be built; every platform call is a stub, because + nothing on the webhook path may reach one. It inherits `dispatch_event` + from the base — the refusal — which is the behaviour one test below is + about. + """ + + def __init__(self) -> None: ... + + async def start(self, *a: Any, **k: Any) -> Any: ... + async def stop(self, *a: Any, **k: Any) -> Any: ... + async def send_message(self, *a: Any, **k: Any) -> Any: ... + async def send_typing(self, *a: Any, **k: Any) -> Any: ... + async def update_message(self, *a: Any, **k: Any) -> Any: ... + async def delete_message(self, *a: Any, **k: Any) -> Any: ... + async def create_channel(self, *a: Any, **k: Any) -> Any: ... + async def get_channel_type(self, *a: Any, **k: Any) -> Any: ... + async def get_channel_agent_names(self, *a: Any, **k: Any) -> Any: ... + async def add_agents_to_channel(self, *a: Any, **k: Any) -> Any: ... + async def add_users_to_channel(self, *a: Any, **k: Any) -> Any: ... + async def create_agent_identity(self, *a: Any, **k: Any) -> Any: ... + async def remove_agent_identity(self, *a: Any, **k: Any) -> Any: ... + def translate_inbound(self, *a: Any, **k: Any) -> Any: ... + def translate_outbound(self, *a: Any, **k: Any) -> Any: ... + + +class _RecordingAdapter(_SocketOnlyAdapter): + """An adapter that only remembers what it was handed. + + It also records the tenant bound at the moment of dispatch, which is the + subject of one of the tests below: the answer has to be "none", the same as + it is under Socket Mode. + """ + + def __init__(self) -> None: + self.dispatched: list[tuple[str, dict[str, Any]]] = [] + self.tenants_bound: list[str | None] = [] + + async def dispatch_event( + self, *, envelope_type: str, payload: dict[str, Any] + ) -> None: + self.dispatched.append((envelope_type, payload)) + self.tenants_bound.append(current_tenant_id()) + + +class _GatedAdapter(_SocketOnlyAdapter): + """An adapter that holds a dispatch open until it is let go. + + What it buys is determinism. The race worth testing is a retry arriving + while the first delivery is still working — the whole reason the claim is + written before the dispatch rather than after — and an adapter that returns + instantly would let the first delivery finish before the second began, + proving only that a repeat is refused once the first is over. + """ + + def __init__(self) -> None: + self.dispatched: list[tuple[str, dict[str, Any]]] = [] + self.entered = asyncio.Event() + self.release = asyncio.Event() + + async def dispatch_event( + self, *, envelope_type: str, payload: dict[str, Any] + ) -> None: + self.dispatched.append((envelope_type, payload)) + self.entered.set() + await self.release.wait() + + +class _FakeLifecycle: + def __init__(self, factory: async_sessionmaker) -> None: + self._factory = factory + self.adapters: dict[str, CollaborationAdapter] = {} + self.removed: list[str] = [] + + def get_adapter(self, bridge_id: str) -> CollaborationAdapter | None: + return self.adapters.get(bridge_id) + + async def remove(self, bridge_id: str) -> None: + """Delete the row, on an unscoped session like the real one. + + The real lifecycle opens a plain session and lets the tenant bound + around the call decide what it can see, so a fake that took a tenant + argument would not be exercising the same thing. + """ + self.removed.append(bridge_id) + self.adapters.pop(bridge_id, None) + async with self._factory() as session: + bridge = await session.get(CollaborationBridge, bridge_id) + if bridge is not None: + await session.delete(bridge) + await session.commit() + + +@dataclass +class _Workspace: + tenant_id: str + workspace_id: str + bridge_id: str + adapter: _RecordingAdapter + + +class _Fixture: + def __init__(self, harness: RLSHarness) -> None: + self.harness = harness + self.lifecycle = _FakeLifecycle(harness.restricted) + self.a: _Workspace + self.b: _Workspace + self.client: httpx.AsyncClient + self.service: MessagingInstallService + + +async def _make_bridge(factory: async_sessionmaker, tenant_id: str, suffix: str) -> str: + async with tenant_session(factory, tenant_id) as session: + client = Client( + matrix_user_id=f"@bridge-{tenant_id}:{suffix}", + display_name="bridge", + type="collaboration_bridge", + ) + session.add(client) + await session.flush() + bridge = CollaborationBridge( + type="slack", + display_name="Acme", + connection_config={}, + client_id=client.id, + status="active", + ) + session.add(bridge) + await session.flush() + bridge_id = bridge.id + await session.commit() + return bridge_id + + +async def _fixture(harness: RLSHarness) -> _Fixture: + fixture = _Fixture(harness) + suffix = uuid.uuid4().hex[:8] + + async with harness.owner() as session: + for label in ("a", "b"): + tenant_id = f"tenant-{label}-{suffix}" + session.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + user = User(name="installer", email=f"{suffix}@example.test", role="user") + session.add(user) + await session.flush() + user_id = user.id + await session.commit() + + workspaces = [] + for label in ("a", "b"): + tenant_id = f"tenant-{label}-{suffix}" + workspace_id = f"T-{label}-{suffix}" + bridge_id = await _make_bridge(harness.restricted, tenant_id, suffix) + async with tenant_session(harness.restricted, tenant_id) as session: + session.add( + MessagingInstall( + tenant_id=tenant_id, + platform="slack", + external_workspace_id=workspace_id, + encrypted_bot_token="ciphertext", + scopes="chat:write", + status="active", + installed_by_user_id=user_id, + bridge_id=bridge_id, + ) + ) + await session.commit() + adapter = _RecordingAdapter() + fixture.lifecycle.adapters[bridge_id] = adapter + workspaces.append( + _Workspace( + tenant_id=tenant_id, + workspace_id=workspace_id, + bridge_id=bridge_id, + adapter=adapter, + ) + ) + fixture.a, fixture.b = workspaces + + installers = MessagingInstallerRegistry() + installers.register( + SlackAppInstaller( + client_id="1234.5678", + client_secret="test-client-secret", + signing_secret=_SIGNING_SECRET, + ) + ) + fixture.service = MessagingInstallService( + session_factory=harness.restricted, + store=MessagingInstallStore(), + receipts=MessagingEventReceiptStore(), + installers=installers, + lifecycle=fixture.lifecycle, # type: ignore[arg-type] + public_origin=_ORIGIN, + secret=_SECRET, + ) + + app = FastAPI() + app.include_router(create_messaging_install_router(fixture.service)) + fixture.client = httpx.AsyncClient( + transport=httpx.ASGITransport(app=app), base_url=_ORIGIN + ) + return fixture + + +def _event(workspace_id: str, text: str) -> bytes: + return json.dumps( + { + "type": "event_callback", + "team_id": workspace_id, + "event": {"type": "message", "text": text, "channel": "C1"}, + } + ).encode() + + +def _numbered_event(workspace_id: str, text: str, event_id: str) -> bytes: + """An event carrying the id Slack puts on everything it retries. + + Separate from `_event` rather than an argument to it, so the tests above go + on exercising the unnumbered path — which is the one a slash command and an + interaction take, and which must dispatch rather than being refused for + having nothing to deduplicate by. + """ + return json.dumps( + { + "type": "event_callback", + "team_id": workspace_id, + "event_id": event_id, + "event": {"type": "message", "text": text, "channel": "C1"}, + } + ).encode() + + +async def _receipts(factory: async_sessionmaker, tenant_id: str) -> list[Any]: + async with tenant_session(factory, tenant_id) as session: + rows = await session.execute(select(MessagingEventReceipt)) + return list(rows.scalars()) + + +def _uninstalled(workspace_id: str) -> bytes: + return json.dumps( + { + "type": "event_callback", + "team_id": workspace_id, + "event": {"type": "app_uninstalled"}, + } + ).encode() + + +def _texts(adapter: _RecordingAdapter) -> list[str]: + return [payload["event"]["text"] for _, payload in adapter.dispatched] + + +async def _post(fixture: _Fixture, path: str, body: bytes) -> httpx.Response: + return await fixture.client.post(path, content=body, headers=_signed(body)) + + +class TestAnEventReachesOneTenant: + async def test_it_reaches_the_tenant_that_installed_the_workspace( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + body = _event(fixture.a.workspace_id, "hello") + + response = await _post(fixture, events_path("slack"), body) + + assert response.status_code == 200 + assert len(fixture.a.adapter.dispatched) == 1 + envelope_type, payload = fixture.a.adapter.dispatched[0] + assert envelope_type == "events_api" + assert payload["event"]["text"] == "hello" + + async def test_it_reaches_no_other_tenant(self, rls_harness: RLSHarness) -> None: + """The whole point. One URL, two customers, and the payload decides. + + Asserted from both directions in one test, because the failure this + guards against is a router that resolves the tenant once and then + fans out to every bridge it knows: that passes any test that only + looks at the intended recipient. + """ + fixture = await _fixture(rls_harness) + + await _post(fixture, events_path("slack"), _event(fixture.a.workspace_id, "a")) + await _post(fixture, events_path("slack"), _event(fixture.b.workspace_id, "b")) + + assert _texts(fixture.a.adapter) == ["a"] + assert _texts(fixture.b.adapter) == ["b"] + + async def test_nothing_is_bound_when_the_adapter_runs( + self, rls_harness: RLSHarness + ) -> None: + """Same as Socket Mode, deliberately. + + A bridge that dials out dispatches from a task with no tenant bound, + and every handler underneath binds the tenant of the room it is acting + on. Binding one here would make the two delivery paths differ in the + one respect that decides who a message reaches, and would hide a + handler that had forgotten to bind for itself. + """ + fixture = await _fixture(rls_harness) + + await _post(fixture, events_path("slack"), _event(fixture.a.workspace_id, "x")) + + assert fixture.a.adapter.tenants_bound == [None] + + async def test_a_slash_command_arrives_as_its_form_fields( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + body = urlencode( + { + "command": "/agents-status", + "text": "", + "team_id": fixture.a.workspace_id, + "channel_id": "C1", + } + ).encode() + + response = await _post(fixture, commands_path("slack"), body) + + assert response.status_code == 200 + envelope_type, payload = fixture.a.adapter.dispatched[0] + assert envelope_type == "slash_commands" + assert payload["command"] == "/agents-status" + + async def test_an_interaction_is_routed_by_its_nested_team( + self, rls_harness: RLSHarness + ) -> None: + """Slack names the workspace differently here, and it still routes.""" + fixture = await _fixture(rls_harness) + body = urlencode( + { + "payload": json.dumps( + {"type": "block_actions", "team": {"id": fixture.b.workspace_id}} + ) + } + ).encode() + + response = await _post(fixture, interactive_path("slack"), body) + + assert response.status_code == 200 + assert fixture.b.adapter.dispatched[0][0] == "interactive" + assert fixture.a.adapter.dispatched == [] + + +class TestWhatTheEndpointRefuses: + async def test_an_unsigned_post_is_refused_and_delivers_nothing( + self, rls_harness: RLSHarness + ) -> None: + """The first line, and the only one that runs before anything is read. + + A workspace id in an unsigned body is just a string somebody typed; + acting on it would let anyone who knows a customer's Slack team id post + into their rooms. + """ + fixture = await _fixture(rls_harness) + body = _event(fixture.a.workspace_id, "forged") + + response = await fixture.client.post( + events_path("slack"), + content=body, + headers={"Content-Type": "application/json"}, + ) + + assert response.status_code == 401 + assert fixture.a.adapter.dispatched == [] + + async def test_a_signature_over_a_different_body_is_refused( + self, rls_harness: RLSHarness + ) -> None: + """A captured signature re-used over a payload naming another tenant.""" + fixture = await _fixture(rls_harness) + captured = _signed(_event(fixture.b.workspace_id, "theirs")) + + response = await fixture.client.post( + events_path("slack"), + content=_event(fixture.a.workspace_id, "mine"), + headers=captured, + ) + + assert response.status_code == 401 + assert fixture.a.adapter.dispatched == [] + assert fixture.b.adapter.dispatched == [] + + async def test_an_unknown_workspace_is_dropped_without_telling_the_platform( + self, rls_harness: RLSHarness + ) -> None: + """200, and the one place this endpoint answers something other than + what happened. + + The app is still installed in a workspace we no longer serve, so it + posts for as long as someone leaves it there. The platform cannot act + on a refusal — there is nothing for it to fix — and it counts the + refusals against the app as a whole, so the honest 404 would be paid + for by every other customer's delivery. It is dropped, and it is in the + log. + """ + fixture = await _fixture(rls_harness) + + response = await _post(fixture, events_path("slack"), _event("T-nobody", "x")) + + assert response.status_code == 200 + assert fixture.a.adapter.dispatched == [] + assert fixture.b.adapter.dispatched == [] + + async def test_an_event_naming_no_workspace_is_refused( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + body = json.dumps({"type": "event_callback", "event": {}}).encode() + + response = await _post(fixture, events_path("slack"), body) + + assert response.status_code == 400 + + async def test_a_body_that_cannot_be_read_is_refused( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + + response = await _post(fixture, events_path("slack"), b"not json") + + assert response.status_code == 400 + + async def test_a_platform_with_no_app_registered_is_not_found( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + + response = await _post(fixture, events_path("teams"), _event("T1", "x")) + + assert response.status_code == 404 + + +class TestWhenTheBridgeIsNotThere: + async def test_a_stopped_bridge_asks_the_platform_to_retry( + self, rls_harness: RLSHarness + ) -> None: + """503, so Slack retries while a bridge restarts. + + The alternative is a 200 that discards a real message and reports it + handled — the exact shape of failure that reads as "Slack lost a + message" and is never found. + """ + fixture = await _fixture(rls_harness) + del fixture.lifecycle.adapters[fixture.a.bridge_id] + + response = await _post( + fixture, events_path("slack"), _event(fixture.a.workspace_id, "x") + ) + + assert response.status_code == 503 + + async def test_an_install_with_no_bridge_yet_says_the_same( + self, rls_harness: RLSHarness + ) -> None: + """A recorded credential nothing has been built on. The schema allows + it, so the router has to answer for it.""" + fixture = await _fixture(rls_harness) + async with tenant_session( + rls_harness.restricted, fixture.a.tenant_id + ) as session: + install = await MessagingInstallStore().get_for_workspace( + session, + platform="slack", + external_workspace_id=fixture.a.workspace_id, + ) + assert install is not None + install.bridge_id = None + await session.commit() + + response = await _post( + fixture, events_path("slack"), _event(fixture.a.workspace_id, "x") + ) + + assert response.status_code == 503 + + +class TestTheHandshake: + async def test_it_is_answered_before_anyone_has_installed_anything( + self, rls_harness: RLSHarness + ) -> None: + """Slack saves a Request URL only if this is echoed back. + + It carries no workspace, so a handshake that had to resolve a tenant + could never be answered — and the app could never be configured at all. + """ + fixture = await _fixture(rls_harness) + body = json.dumps( + {"type": "url_verification", "challenge": "let-me-in"} + ).encode() + + response = await _post(fixture, events_path("slack"), body) + + assert response.status_code == 200 + assert response.text == "let-me-in" + assert fixture.a.adapter.dispatched == [] + + async def test_an_unsigned_handshake_is_still_refused( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + + response = await fixture.client.post( + events_path("slack"), + content=b'{"type":"url_verification","challenge":"let-me-in"}', + ) + + assert response.status_code == 401 + + +class TestABridgeThatCannotTakeEvents: + async def test_the_refusal_is_not_swallowed(self, rls_harness: RLSHarness) -> None: + """A webhook install pointed at a socket-mode adapter is a fault. + + It cannot happen through the install flow — the rendered config says + `event_delivery: webhook` — but it is exactly the state a hand-edited + connection config could reach, and delivering nowhere while answering + 200 is how it would stay hidden. + """ + fixture = await _fixture(rls_harness) + adapter = _SocketOnlyAdapter() + fixture.lifecycle.adapters[fixture.a.bridge_id] = adapter + + event = fixture.service.authenticate( + platform="slack", + endpoint="events", + headers=_signed(_event(fixture.a.workspace_id, "x")), + body=_event(fixture.a.workspace_id, "x"), + ) + target = await fixture.service.resolve(platform="slack", event=event) + with pytest.raises(Exception, match="does not receive events over HTTP"): + await fixture.service.deliver(target, event) + + +class TestAnEventIsHandledOnce: + """The platform delivers at least once; the customer must hear once. + + Slack allows three seconds to acknowledge and re-sends what it does not get + an answer to, so a deployment under load is told the same thing twice. The + payload of a retry is byte-identical to the original — the id is the only + thing that distinguishes it from someone saying the same words again — and + the visible failure is an agent answering one question twice in a + customer's channel. + """ + + async def test_a_retry_of_the_same_event_dispatches_once( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + body = _numbered_event(fixture.a.workspace_id, "hello", "Ev1") + + first = await _post(fixture, events_path("slack"), body) + second = await _post(fixture, events_path("slack"), body) + + assert (first.status_code, second.status_code) == (200, 200) + assert _texts(fixture.a.adapter) == ["hello"] + + async def test_a_retry_arriving_mid_turn_loses_to_the_delivery_in_flight( + self, rls_harness: RLSHarness + ) -> None: + """The case the ordering exists for, and the only one that is a race. + + Recording the receipt after the work instead of before would order the + two the wrong way round: both would find nothing claimed, both would + dispatch, and the duplicate would be noticed once it no longer + mattered. + """ + fixture = await _fixture(rls_harness) + gated = _GatedAdapter() + fixture.lifecycle.adapters[fixture.a.bridge_id] = gated + body = _numbered_event(fixture.a.workspace_id, "hello", "Ev1") + event = fixture.service.authenticate( + platform="slack", endpoint="events", headers=_signed(body), body=body + ) + target = await fixture.service.resolve(platform="slack", event=event) + + in_flight = asyncio.create_task(fixture.service.deliver(target, event)) + await gated.entered.wait() + await fixture.service.deliver(target, event) + + assert len(gated.dispatched) == 1 + gated.release.set() + await in_flight + + async def test_an_event_with_no_id_is_dispatched_every_time( + self, rls_harness: RLSHarness + ) -> None: + """Not a weaker guarantee quietly accepted. + + Slack numbers only the envelopes it retries, so one arriving without an + id arrives exactly once. Two of them are two real messages, and + refusing the second for having nothing to deduplicate by would drop a + customer's message on the floor. + """ + fixture = await _fixture(rls_harness) + body = _event(fixture.a.workspace_id, "same words") + + await _post(fixture, events_path("slack"), body) + await _post(fixture, events_path("slack"), body) + + assert _texts(fixture.a.adapter) == ["same words", "same words"] + assert await _receipts(rls_harness.restricted, fixture.a.tenant_id) == [] + + async def test_one_tenants_event_ids_do_not_block_anothers( + self, rls_harness: RLSHarness + ) -> None: + """Uniqueness is per tenant, and this is why that is the shape to want. + + Slack's ids are unique in its own namespace, so a deployment-wide index + would protect exactly as well — but it would let one customer's traffic + refuse another's, and a conflict would name a row the inserting tenant + cannot see. + """ + fixture = await _fixture(rls_harness) + + await _post( + fixture, + events_path("slack"), + _numbered_event(fixture.a.workspace_id, "a", "Ev-shared"), + ) + await _post( + fixture, + events_path("slack"), + _numbered_event(fixture.b.workspace_id, "b", "Ev-shared"), + ) + + assert _texts(fixture.a.adapter) == ["a"] + assert _texts(fixture.b.adapter) == ["b"] + + async def test_a_completed_delivery_is_recorded_as_handled( + self, rls_harness: RLSHarness + ) -> None: + """The absence of this is the useful half. + + Claiming before the work chooses at-most-once: an event taken by a + process that then dies is not retried, because the platform has already + been told 200. `handled_at` is what keeps that loss findable — a + claimed receipt that never completed is a real event that reached + nobody. + """ + fixture = await _fixture(rls_harness) + + await _post( + fixture, + events_path("slack"), + _numbered_event(fixture.a.workspace_id, "hello", "Ev1"), + ) + + rows = await _receipts(rls_harness.restricted, fixture.a.tenant_id) + assert [(row.platform, row.external_event_id) for row in rows] == [ + ("slack", "Ev1") + ] + assert rows[0].handled_at is not None + + async def test_a_receipt_past_its_retention_is_pruned_by_the_next_event( + self, rls_harness: RLSHarness + ) -> None: + """Opportunistic, because this backend has no janitor to hang it on. + + The table would otherwise grow with every message the busiest workspace + ever sends. Running the sweep off the traffic that creates the rows + keeps the work proportional to that traffic and needs nothing started + at boot. + """ + fixture = await _fixture(rls_harness) + async with tenant_session( + rls_harness.restricted, fixture.a.tenant_id + ) as session: + session.add( + MessagingEventReceipt( + tenant_id=fixture.a.tenant_id, + platform="slack", + external_event_id="Ev-ancient", + received_at=datetime.now(UTC) + - RECEIPT_RETENTION + - timedelta(days=1), + ) + ) + await session.commit() + + await _post( + fixture, + events_path("slack"), + _numbered_event(fixture.a.workspace_id, "hello", "Ev-fresh"), + ) + + rows = await _receipts(rls_harness.restricted, fixture.a.tenant_id) + assert [row.external_event_id for row in rows] == ["Ev-fresh"] + + async def test_a_retry_of_an_event_nobody_holds_is_still_dropped_at_resolve( + self, rls_harness: RLSHarness + ) -> None: + """Deduplication sits after resolution, so an unknown workspace writes + no receipt at all — there is no tenant to write it as.""" + fixture = await _fixture(rls_harness) + + response = await _post( + fixture, events_path("slack"), _numbered_event("T-nobody", "x", "Ev1") + ) + + assert response.status_code == 200 + assert await _receipts(rls_harness.restricted, fixture.a.tenant_id) == [] + + async def test_a_bridge_that_was_down_still_takes_the_retry( + self, rls_harness: RLSHarness + ) -> None: + """The 503 and the receipt compose, and this is the case that proves it. + + A bridge mid-restart makes the event fail before any claim is written, + so the retry Slack sends in response finds nothing taken and is handled + normally. Claiming any earlier — before `resolve` — would turn a + transient outage into a permanently lost message. + """ + fixture = await _fixture(rls_harness) + body = _numbered_event(fixture.a.workspace_id, "hello", "Ev1") + del fixture.lifecycle.adapters[fixture.a.bridge_id] + + refused = await _post(fixture, events_path("slack"), body) + fixture.lifecycle.adapters[fixture.a.bridge_id] = fixture.a.adapter + retried = await _post(fixture, events_path("slack"), body) + + assert (refused.status_code, retried.status_code) == (503, 200) + assert _texts(fixture.a.adapter) == ["hello"] + + +class TestThePlatformSayingTheInstallIsOver: + """`app_uninstalled` arrives on the same URL as everything else.""" + + async def test_it_ends_the_install_instead_of_dispatching( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _fixture(rls_harness) + + response = await _post( + fixture, events_path("slack"), _uninstalled(fixture.a.workspace_id) + ) + + assert response.status_code == 200 + assert fixture.a.adapter.dispatched == [] + assert fixture.lifecycle.removed == [fixture.a.bridge_id] + + async with tenant_session( + rls_harness.restricted, fixture.a.tenant_id + ) as session: + installs = await MessagingInstallStore().list_for_tenant(session) + assert [install.status for install in installs] == ["revoked"] + + async def test_it_ends_nobody_elses(self, rls_harness: RLSHarness) -> None: + """The payload decides here as well, and the blast radius is larger. + + A revocation routed by anything but the workspace in the event would + disconnect a customer who did nothing. + """ + fixture = await _fixture(rls_harness) + + await _post(fixture, events_path("slack"), _uninstalled(fixture.a.workspace_id)) + await _post(fixture, events_path("slack"), _event(fixture.b.workspace_id, "b")) + + assert _texts(fixture.b.adapter) == ["b"] + assert fixture.lifecycle.removed == [fixture.a.bridge_id] + + async def test_the_workspace_stops_resolving_to_anyone( + self, rls_harness: RLSHarness + ) -> None: + """Which is what frees it to be installed again. + + An app removed from a workspace can still post on its way out, and an + ended install that went on answering for the workspace would route + those to a bridge that no longer exists. + """ + fixture = await _fixture(rls_harness) + await _post(fixture, events_path("slack"), _uninstalled(fixture.a.workspace_id)) + + trailing = await _post( + fixture, events_path("slack"), _event(fixture.a.workspace_id, "late") + ) + + assert trailing.status_code == 200 + assert fixture.a.adapter.dispatched == [] + + async def test_a_redelivery_is_not_an_error(self, rls_harness: RLSHarness) -> None: + """Slack retries what it is slow to hear back from, and it hears 200 + from the first delivery only after the install has already gone.""" + fixture = await _fixture(rls_harness) + body = _uninstalled(fixture.a.workspace_id) + + first = await _post(fixture, events_path("slack"), body) + second = await _post(fixture, events_path("slack"), body) + + assert (first.status_code, second.status_code) == (200, 200) + assert fixture.lifecycle.removed == [fixture.a.bridge_id] diff --git a/core/tests/switch_core/bridges/collaboration/test_slack_distributed_app.py b/core/tests/switch_core/bridges/collaboration/test_slack_distributed_app.py new file mode 100644 index 000000000..1396c38ac --- /dev/null +++ b/core/tests/switch_core/bridges/collaboration/test_slack_distributed_app.py @@ -0,0 +1,122 @@ +"""The registration walkthrough and the code have to agree, so this compares them. + +`docs/old/bridges/SLACK_DISTRIBUTED_APP.md` carries a manifest an operator +pastes into Slack. Everything in it is a promise the running system has to +keep: the scopes it requests are the scopes the authorize URL asks for, and +the URLs it registers are the paths this application serves. Neither is +checked by anything at runtime — Slack simply refuses a redirect that does not +match, or grants a scope we never use, and the failure surfaces as a customer's +install not working for a reason nobody can see from here. + +The manifest is parsed out of the markdown rather than kept in a fixture, +because a fixture would be a third copy and the operator pastes the markdown. +""" + +from __future__ import annotations + +import json +import re +from pathlib import Path + +import pytest + +from switch_core.bridges.collaboration.install import ( + commands_path, + events_path, + interactive_path, + oauth_callback_path, + public_url, +) +from switch_core.bridges.collaboration.slack.install import BOT_SCOPES + +_DOC = ( + Path(__file__).resolve().parents[5] + / "docs" + / "old" + / "bridges" + / "SLACK_DISTRIBUTED_APP.md" +) + +_HOST = "HOST" + + +@pytest.fixture(scope="module") +def manifest() -> dict: + text = _DOC.read_text() + blocks = re.findall(r"```json\n(.*?)\n```", text, re.DOTALL) + assert len(blocks) == 1, ( + f"expected exactly one json block in {_DOC.name}, found {len(blocks)}" + ) + return json.loads(blocks[0]) + + +def test_the_manifest_requests_the_scopes_the_authorize_url_asks_for( + manifest: dict, +) -> None: + """Same scopes, same order. + + Order matters less to Slack than to a reader diffing the two, and keeping + it exact costs nothing. + """ + assert tuple(manifest["oauth_config"]["scopes"]["bot"]) == BOT_SCOPES + + +def test_the_manifest_registers_the_redirect_the_callback_is_served_at( + manifest: dict, +) -> None: + """The one mismatch Slack refuses outright. + + A redirect URI is compared byte for byte against the registered list, so a + path that drifted here is every install failing with `bad_redirect_uri` + and nothing in our logs at all — the refusal happens at Slack. + """ + expected = public_url(f"https://{_HOST}", oauth_callback_path("slack")) + assert manifest["oauth_config"]["redirect_urls"] == [expected] + + +def test_the_manifest_points_events_and_interactivity_at_the_served_paths( + manifest: dict, +) -> None: + settings = manifest["settings"] + assert settings["event_subscriptions"]["request_url"] == public_url( + f"https://{_HOST}", events_path("slack") + ) + assert settings["interactivity"]["request_url"] == public_url( + f"https://{_HOST}", interactive_path("slack") + ) + + +def test_every_slash_command_posts_to_the_commands_path(manifest: dict) -> None: + expected = public_url(f"https://{_HOST}", commands_path("slack")) + urls = {command["url"] for command in manifest["features"]["slash_commands"]} + assert urls == {expected} + + +def test_the_manifest_does_not_enable_socket_mode(manifest: dict) -> None: + """The property the whole distributed app exists to have. + + Slack forbids Socket Mode for a Marketplace-listed app, and a single + socket could not be shared across replicas even if it did not. Turning it + on here would produce an app that works in a one-replica test and silently + delivers to one arbitrary replica in production. + """ + assert manifest["settings"]["socket_mode_enabled"] is False + + +def test_the_manifest_does_not_declare_the_app_an_agent(manifest: dict) -> None: + """`agent_view` is irreversible per app and needs re-review to distribute. + + Left out deliberately rather than forgotten — see the doc. If it is added, + that is a decision, and this test is where it gets recorded as one. + """ + assert "agent_view" not in manifest["features"] + + +def test_the_manifest_does_not_enable_org_wide_deploy(manifest: dict) -> None: + """An org-wide install has an enterprise id and no single workspace id. + + `messaging_installs` is unique on `(platform, external_workspace_id)` and + the installer refuses an enterprise install outright, so enabling this + would offer customers a button that always fails. + """ + assert manifest["settings"]["org_deploy_enabled"] is False diff --git a/core/tests/switch_core/bridges/collaboration/test_slack_installer.py b/core/tests/switch_core/bridges/collaboration/test_slack_installer.py new file mode 100644 index 000000000..ea8695a44 --- /dev/null +++ b/core/tests/switch_core/bridges/collaboration/test_slack_installer.py @@ -0,0 +1,538 @@ +"""What the installer does with what Slack sends back, including when it lies. + +The install endpoints are the only ones in the system a stranger can reach +with no credential at all, so the interesting cases here are the hostile and +the malformed ones rather than the happy path. +""" + +from __future__ import annotations + +import hashlib +import hmac +import json +import time +from urllib.parse import urlencode + +import pytest +from slack_sdk.web.async_client import AsyncWebClient + +from switch_core.bridges.collaboration.install import ( + InstallGrant, + MessagingInstallerRegistry, + MessagingInstallError, + WebhookAuthenticityError, + WebhookEndpoint, + WebhookPayloadError, + events_path, + oauth_callback_path, + public_url, +) +from switch_core.bridges.collaboration.slack.adapter import SlackConnectionConfig +from switch_core.bridges.collaboration.slack.install import ( + BOT_SCOPES, + SlackAppInstaller, +) + +_SIGNING_SECRET = "test-signing-secret" + + +@pytest.fixture +def installer() -> SlackAppInstaller: + return SlackAppInstaller( + client_id="1234.5678", + client_secret="test-client-secret", + signing_secret=_SIGNING_SECRET, + ) + + +def _sign(body: bytes, timestamp: str) -> str: + digest = hmac.new( + _SIGNING_SECRET.encode(), + b"v0:" + timestamp.encode() + b":" + body, + hashlib.sha256, + ).hexdigest() + return f"v0={digest}" + + +def _signed_headers(body: bytes, *, age_seconds: int = 0) -> dict[str, str]: + timestamp = str(int(time.time()) - age_seconds) + return { + "X-Slack-Request-Timestamp": timestamp, + "X-Slack-Signature": _sign(body, timestamp), + } + + +class TestAuthorizeUrl: + def test_it_carries_the_state_and_redirect_unchanged( + self, installer: SlackAppInstaller + ) -> None: + redirect = public_url("https://switch.example", oauth_callback_path("slack")) + url = installer.authorize_url(state="opaque-state", redirect_uri=redirect) + + assert url.startswith("https://slack.com/oauth/v2/authorize?") + assert "state=opaque-state" in url + assert ( + "redirect_uri=https%3A%2F%2Fswitch.example%2Fmessaging%2Fslack%2Foauth%2Fcallback" + in url + ) + + def test_it_asks_for_every_scope_the_bot_needs( + self, installer: SlackAppInstaller + ) -> None: + url = installer.authorize_url(state="s", redirect_uri="https://x.example/cb") + for scope in BOT_SCOPES: + assert scope.replace(":", "%3A") in url + + +class TestWebhookVerification: + def test_a_correctly_signed_body_passes(self, installer: SlackAppInstaller) -> None: + body = b'{"type":"event_callback","team_id":"T1"}' + installer.verify_webhook(headers=_signed_headers(body), body=body) + + def test_a_tampered_body_is_refused(self, installer: SlackAppInstaller) -> None: + body = b'{"type":"event_callback","team_id":"T1"}' + headers = _signed_headers(body) + with pytest.raises(WebhookAuthenticityError): + installer.verify_webhook(headers=headers, body=body + b" ") + + def test_an_old_signature_is_refused(self, installer: SlackAppInstaller) -> None: + """Replay window. A capture stays valid forever without it.""" + body = b'{"team_id":"T1"}' + with pytest.raises(WebhookAuthenticityError): + installer.verify_webhook( + headers=_signed_headers(body, age_seconds=60 * 10), body=body + ) + + def test_missing_headers_are_refused_rather_than_skipped( + self, installer: SlackAppInstaller + ) -> None: + with pytest.raises(WebhookAuthenticityError): + installer.verify_webhook(headers={}, body=b"{}") + + def test_a_nonnumeric_timestamp_is_a_bad_signature_not_a_crash( + self, installer: SlackAppInstaller + ) -> None: + """Reachable by anyone who finds the URL. + + The verifier calls `int()` on the header, so without the guard this + unauthenticated input is a 500 and a traceback per request rather than + a 401. + """ + body = b"{}" + with pytest.raises(WebhookAuthenticityError): + installer.verify_webhook( + headers={ + "X-Slack-Request-Timestamp": "not-a-number", + "X-Slack-Signature": "v0=deadbeef", + }, + body=body, + ) + + def test_header_case_does_not_matter(self, installer: SlackAppInstaller) -> None: + body = b'{"team_id":"T1"}' + headers = {k.lower(): v for k, v in _signed_headers(body).items()} + installer.verify_webhook(headers=headers, body=body) + + +class TestRedeem: + async def _redeem_returning( + self, monkeypatch: pytest.MonkeyPatch, installer: SlackAppInstaller, response + ) -> InstallGrant: + async def fake(self, **kwargs): # noqa: ANN001, ANN202 + return response + + monkeypatch.setattr(AsyncWebClient, "oauth_v2_access", fake) + return await installer.redeem(code="c", redirect_uri="https://x.example/cb") + + async def test_a_good_grant_becomes_the_few_things_we_keep( + self, installer: SlackAppInstaller, monkeypatch: pytest.MonkeyPatch + ) -> None: + grant = await self._redeem_returning( + monkeypatch, + installer, + { + "ok": True, + "access_token": "xoxb-granted", + "scope": "chat:write,commands", + "team": {"id": "T123", "name": "Acme"}, + }, + ) + assert grant == InstallGrant( + external_workspace_id="T123", + workspace_name="Acme", + bot_token="xoxb-granted", + scopes="chat:write,commands", + ) + + async def test_a_workspace_with_no_name_falls_back_to_its_id( + self, installer: SlackAppInstaller, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The name is only ever a label, so its absence is not a failure.""" + grant = await self._redeem_returning( + monkeypatch, + installer, + { + "ok": True, + "access_token": "xoxb-granted", + "scope": "chat:write", + "team": {"id": "T123"}, + }, + ) + assert grant.workspace_name == "T123" + + async def test_a_two_hundred_saying_not_ok_is_still_a_failure( + self, installer: SlackAppInstaller, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Slack answers 200 with `ok: false` for most refusals.""" + with pytest.raises(MessagingInstallError, match="invalid_code"): + await self._redeem_returning( + monkeypatch, installer, {"ok": False, "error": "invalid_code"} + ) + + async def test_an_org_wide_install_is_refused_with_a_reason( + self, installer: SlackAppInstaller, monkeypatch: pytest.MonkeyPatch + ) -> None: + """It has an enterprise id and no single workspace id to be unique on.""" + with pytest.raises(MessagingInstallError, match="org-wide"): + await self._redeem_returning( + monkeypatch, + installer, + { + "ok": True, + "access_token": "xoxb-granted", + "is_enterprise_install": True, + "enterprise": {"id": "E1"}, + "team": None, + }, + ) + + async def test_a_grant_with_no_workspace_is_refused( + self, installer: SlackAppInstaller, monkeypatch: pytest.MonkeyPatch + ) -> None: + with pytest.raises(MessagingInstallError, match="nothing to record"): + await self._redeem_returning( + monkeypatch, installer, {"ok": True, "access_token": "xoxb-granted"} + ) + + +class TestParsingAWebhook: + """Turning three differently-shaped bodies into what Socket Mode delivers. + + The comparison that matters is not with Slack's documentation but with the + socket transport: `dispatch_event` is shared, so anything parsed into a + different shape here is a bridge that behaves differently depending on + which Slack app its token came from. + """ + + def test_an_event_callback_becomes_the_socket_mode_envelope( + self, installer: SlackAppInstaller + ) -> None: + body = json.dumps( + { + "type": "event_callback", + "team_id": "T1", + "event": {"type": "message", "text": "hello"}, + } + ).encode() + + parsed = installer.parse_webhook(endpoint="events", headers={}, body=body) + + assert parsed.envelope_type == "events_api" + assert parsed.handshake is None + # The whole envelope, not the inner event: `dispatch_event` reads + # `payload["event"]` out of it, exactly as Socket Mode hands it over. + assert parsed.payload["event"] == {"type": "message", "text": "hello"} + assert parsed.payload["team_id"] == "T1" + + def test_a_url_verification_is_answered_and_not_dispatched( + self, installer: SlackAppInstaller + ) -> None: + """Slack proving the URL at the moment it is saved. + + It arrives before any workspace has installed anything, so a handshake + that had to resolve a tenant could never be answered — which would mean + the Request URL could not be saved at all. + """ + body = json.dumps({"type": "url_verification", "challenge": "abc123"}).encode() + + parsed = installer.parse_webhook(endpoint="events", headers={}, body=body) + + assert parsed.handshake == "abc123" + + def test_a_url_verification_with_nothing_to_echo_is_refused( + self, installer: SlackAppInstaller + ) -> None: + with pytest.raises(WebhookPayloadError, match="challenge"): + installer.parse_webhook( + endpoint="events", headers={}, body=b'{"type":"url_verification"}' + ) + + def test_a_slash_command_is_its_form_fields( + self, installer: SlackAppInstaller + ) -> None: + body = urlencode( + {"command": "/agents-status", "text": "", "team_id": "T1", "user_id": "U1"} + ).encode() + + parsed = installer.parse_webhook(endpoint="commands", headers={}, body=body) + + assert parsed.envelope_type == "slash_commands" + assert parsed.payload["command"] == "/agents-status" + # A string, not a one-element list: the adapter reads these the way + # Socket Mode delivers them. + assert parsed.payload["text"] == "" + + def test_an_interaction_is_unwrapped_from_its_payload_field( + self, installer: SlackAppInstaller + ) -> None: + inner = {"type": "block_actions", "team": {"id": "T1"}} + body = urlencode({"payload": json.dumps(inner)}).encode() + + parsed = installer.parse_webhook(endpoint="interactive", headers={}, body=body) + + assert parsed.envelope_type == "interactive" + assert parsed.payload == inner + + def test_an_interaction_with_no_payload_field_is_refused( + self, installer: SlackAppInstaller + ) -> None: + with pytest.raises(WebhookPayloadError, match="payload"): + installer.parse_webhook(endpoint="interactive", headers={}, body=b"other=1") + + def test_an_event_callback_carries_slacks_own_event_id( + self, installer: SlackAppInstaller + ) -> None: + """The id is the only thing two copies of one event have in common. + + The payload of a retry is byte-identical to the original, so without + this nothing downstream could tell a re-send from somebody saying the + same words twice. + """ + body = json.dumps( + {"type": "event_callback", "team_id": "T1", "event_id": "Ev123"} + ).encode() + + parsed = installer.parse_webhook(endpoint="events", headers={}, body=body) + + assert parsed.external_event_id == "Ev123" + + @pytest.mark.parametrize( + "envelope", + [ + pytest.param({"type": "event_callback", "team_id": "T1"}, id="absent"), + pytest.param( + {"type": "event_callback", "team_id": "T1", "event_id": ""}, + id="empty", + ), + pytest.param( + {"type": "event_callback", "team_id": "T1", "event_id": 7}, + id="not-a-string", + ), + ], + ) + def test_an_event_callback_with_no_usable_id_deduplicates_by_nothing( + self, installer: SlackAppInstaller, envelope: dict[str, object] + ) -> None: + """No id means dispatch it, never refuse it. + + An empty or missing `event_id` is not a reason to drop a real message + from a customer's channel — it only means there is nothing to key a + receipt on, so the event is handled the way an unnumbered one is. + """ + parsed = installer.parse_webhook( + endpoint="events", headers={}, body=json.dumps(envelope).encode() + ) + + assert parsed.external_event_id is None + + @pytest.mark.parametrize( + ("endpoint", "body"), + [ + pytest.param( + "commands", urlencode({"team_id": "T1"}).encode(), id="command" + ), + pytest.param( + "interactive", + urlencode({"payload": json.dumps({"team": {"id": "T1"}})}).encode(), + id="interaction", + ), + ], + ) + def test_the_form_endpoints_carry_no_id_because_slack_sends_them_once( + self, installer: SlackAppInstaller, endpoint: WebhookEndpoint, body: bytes + ) -> None: + """Slack retries neither, so there is nothing to deduplicate. + + A command and an interaction are a person waiting on a dialog. Slack + sends each exactly once and puts no id on it. + """ + parsed = installer.parse_webhook(endpoint=endpoint, headers={}, body=body) + + assert parsed.external_event_id is None + + @pytest.mark.parametrize( + ("headers", "expected"), + [ + pytest.param({}, 0, id="first-delivery"), + pytest.param({"x-slack-retry-num": "2"}, 2, id="second-retry"), + pytest.param({"x-slack-retry-num": "nonsense"}, 0, id="unparseable"), + ], + ) + def test_the_retry_count_is_read_as_a_hint_and_never_as_a_decision( + self, + installer: SlackAppInstaller, + headers: dict[str, str], + expected: int, + ) -> None: + """Unsigned input, so a bad value must not be able to refuse an event. + + The retry header is outside the signature — which covers the body and + the timestamp — so a garbled or forged one degrades to zero rather than + raising. The worst a forgery achieves is a wrong number in a log line. + """ + body = json.dumps( + {"type": "event_callback", "team_id": "T1", "event_id": "Ev1"} + ).encode() + + parsed = installer.parse_webhook(endpoint="events", headers=headers, body=body) + + assert parsed.delivery_attempt == expected + + @pytest.mark.parametrize( + "body", + [ + pytest.param(b"not json at all", id="not-json"), + pytest.param(b"[1, 2, 3]", id="json-but-not-an-object"), + pytest.param(b"", id="empty"), + ], + ) + def test_a_body_that_cannot_be_read_is_a_payload_error( + self, installer: SlackAppInstaller, body: bytes + ) -> None: + """Verified, so it really is Slack. That makes it worth a loud error. + + A bad signature is the internet; a signed body this build cannot parse + is either a Slack change or a bug of ours, and both want a log line + rather than a shrug. + """ + with pytest.raises(WebhookPayloadError): + installer.parse_webhook(endpoint="events", headers={}, body=body) + + +class TestWhichWorkspaceSentIt: + """The one question that decides who the event belongs to.""" + + def test_an_event_names_it_flat(self, installer: SlackAppInstaller) -> None: + assert installer.workspace_of_event({"team_id": "T1"}) == "T1" + + def test_an_interaction_names_it_nested(self, installer: SlackAppInstaller) -> None: + assert installer.workspace_of_event({"team": {"id": "T1"}}) == "T1" + + @pytest.mark.parametrize( + "payload", + [ + pytest.param({}, id="absent"), + pytest.param({"team_id": ""}, id="empty"), + pytest.param({"team_id": 7}, id="not-a-string"), + pytest.param({"team": "T1"}, id="team-not-an-object"), + ], + ) + def test_an_event_naming_none_is_refused_rather_than_guessed_at( + self, installer: SlackAppInstaller, payload: dict[str, object] + ) -> None: + """There is no safe default. Any guess picks somebody's tenant.""" + with pytest.raises(WebhookPayloadError): + installer.workspace_of_event(payload) + + +class TestConnectionConfig: + def test_a_grant_renders_a_config_the_adapter_accepts( + self, installer: SlackAppInstaller + ) -> None: + """The seam: after this an installed bridge is an ordinary bridge. + + Validated as-is, with nothing added. What the installer renders is what + `register` is handed, so a rendering that only validates once the test + has helped it along is a registration that fails in production. + """ + rendered = installer.connection_config( + InstallGrant( + external_workspace_id="T123", + workspace_name="Acme", + bot_token="xoxb-granted", + scopes="chat:write", + ) + ) + config = SlackConnectionConfig.model_validate(rendered) + assert config.bot_token == "xoxb-granted" + assert config.workspace_id == "T123" + assert config.event_delivery == "webhook" + assert config.app_token is None + + +class TestDeliveryModeValidation: + def test_socket_mode_without_an_app_token_is_refused(self) -> None: + """The failure that otherwise reads as "Slack is quiet today".""" + with pytest.raises(ValueError, match="app_token is required"): + SlackConnectionConfig.model_validate( + {"bot_token": "xoxb-x", "workspace_id": "T1"} + ) + + def test_a_webhook_bridge_with_an_app_token_is_refused(self) -> None: + with pytest.raises(ValueError, match="must be empty"): + SlackConnectionConfig.model_validate( + { + "bot_token": "xoxb-x", + "workspace_id": "T1", + "app_token": "xapp-x", + "event_delivery": "webhook", + } + ) + + def test_the_hand_registered_shape_still_validates(self) -> None: + config = SlackConnectionConfig.model_validate( + {"bot_token": "xoxb-x", "app_token": "xapp-x", "workspace_id": "T1"} + ) + assert config.event_delivery == "socket_mode" + + def test_the_registration_form_never_offers_the_delivery_mode(self) -> None: + """It is not a question an operator filling that form can be asked.""" + assert ( + "event_delivery" + not in SlackConnectionConfig.model_json_schema()["properties"] + ) + + +class TestRegistry: + def test_an_unregistered_platform_says_what_to_do(self) -> None: + registry = MessagingInstallerRegistry() + with pytest.raises(MessagingInstallError, match="no slack app is registered"): + registry.get("slack") + + def test_registering_twice_is_refused(self, installer: SlackAppInstaller) -> None: + registry = MessagingInstallerRegistry() + registry.register(installer) + with pytest.raises(MessagingInstallError, match="already registered"): + registry.register(installer) + + def test_a_registered_installer_comes_back( + self, installer: SlackAppInstaller + ) -> None: + registry = MessagingInstallerRegistry() + registry.register(installer) + assert registry.get("slack") is installer + assert registry.platforms() == ["slack"] + + +class TestPaths: + def test_the_public_prefix_bypasses_bearer_auth(self) -> None: + """Otherwise every Slack event is a 401 nobody sees.""" + from switch_core.bridges.agent.auth import PUBLIC_PATH_PREFIXES + + assert events_path("slack").startswith(PUBLIC_PATH_PREFIXES) + + def test_joining_an_origin_with_a_trailing_slash_does_not_double_it(self) -> None: + """Slack compares the redirect byte for byte.""" + assert public_url( + "https://switch.example/", oauth_callback_path("slack") + ) == public_url("https://switch.example", oauth_callback_path("slack")) diff --git a/core/tests/switch_core/db/stores/test_messaging_install_store.py b/core/tests/switch_core/db/stores/test_messaging_install_store.py new file mode 100644 index 000000000..5b1304417 --- /dev/null +++ b/core/tests/switch_core/db/stores/test_messaging_install_store.py @@ -0,0 +1,271 @@ +"""Redeeming an install state is a race, and only one caller may win it. + +The state is the only thing tying an authenticated "install this" to an +unauthenticated callback from the platform, and a signature alone cannot make +it single-use. This is where that property lives, so these run against a real +Postgres through the restricted role — a test that redeemed a state in Python +would prove nothing about the statement that actually runs. +""" + +from __future__ import annotations + +import asyncio +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +from sqlalchemy.ext.asyncio import async_sessionmaker + +from switch_core.db.models import Tenant, User +from switch_core.db.session_scope import tenant_session +from switch_core.db.stores.messaging_install_store import ( + MessagingInstallClaimedError, + MessagingInstallStateError, + MessagingInstallStore, +) +from tests.conftest import RLSHarness + +pytestmark = pytest.mark.no_ambient_tenant + + +class _Fixture: + def __init__(self) -> None: + self.tenant_a: str = "" + self.tenant_b: str = "" + self.user_id: str = "" + self.workspace: str = "" + + +async def _two_tenants(owner: async_sessionmaker) -> _Fixture: + fixture = _Fixture() + suffix = uuid.uuid4().hex[:8] + fixture.tenant_a = f"tenant-a-{suffix}" + fixture.tenant_b = f"tenant-b-{suffix}" + fixture.workspace = f"T-{suffix}" + + async with owner() as session: + for tenant_id in (fixture.tenant_a, fixture.tenant_b): + session.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + user = User(name="installer", email=f"{suffix}@example.test", role="user") + session.add(user) + await session.flush() + fixture.user_id = user.id + await session.commit() + return fixture + + +class TestStartingAnInstall: + async def test_the_state_takes_its_tenant_from_the_session( + self, rls_harness: RLSHarness + ) -> None: + """The caller never names a tenant; the bound session decides. + + This is the whole reason the start leg is the authenticated one. + """ + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state = await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + await session.commit() + assert state.tenant_id == fixture.tenant_a + + +class TestRedeemingAState: + async def test_a_state_redeems_once(self, rls_harness: RLSHarness) -> None: + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state_id = ( + await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + ).id + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + redeemed = await store.redeem_state( + session, state_id=state_id, platform="slack" + ) + await session.commit() + assert redeemed.consumed_at is not None + + async def test_replaying_it_is_refused(self, rls_harness: RLSHarness) -> None: + """The attack. A captured state must not install a second workspace.""" + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state_id = ( + await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + ).id + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + await store.redeem_state(session, state_id=state_id, platform="slack") + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + with pytest.raises(MessagingInstallStateError, match="already been used"): + await store.redeem_state(session, state_id=state_id, platform="slack") + + async def test_two_simultaneous_redemptions_produce_one_winner( + self, rls_harness: RLSHarness + ) -> None: + """Sequential replay is the easy half; this is the one that needs the + check and the write to be a single statement. + + Two callbacks arriving together would both read `consumed_at IS NULL` + under a read-then-update, and both would proceed. + """ + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state_id = ( + await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + ).id + await session.commit() + + async def attempt() -> bool: + async with tenant_session( + rls_harness.restricted, fixture.tenant_a + ) as session: + try: + await store.redeem_state( + session, state_id=state_id, platform="slack" + ) + except MessagingInstallStateError: + return False + await session.commit() + return True + + outcomes = await asyncio.gather(attempt(), attempt()) + assert sum(outcomes) == 1 + + async def test_an_expired_state_is_refused(self, rls_harness: RLSHarness) -> None: + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state = await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + state.expires_at = datetime.now(UTC) - timedelta(seconds=1) + await session.commit() + state_id = state.id + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + with pytest.raises(MessagingInstallStateError, match="expired"): + await store.redeem_state(session, state_id=state_id, platform="slack") + + async def test_another_tenant_cannot_redeem_it( + self, rls_harness: RLSHarness + ) -> None: + """The second check on the signature. + + A state's tenant is established by the signature before this runs, so + reaching here with the wrong one means the signature was forged or the + key leaked. Row-level security still refuses, independently. + """ + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state_id = ( + await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + ).id + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + with pytest.raises(MessagingInstallStateError): + await store.redeem_state(session, state_id=state_id, platform="slack") + + async def test_a_state_cannot_be_redeemed_on_another_platform( + self, rls_harness: RLSHarness + ) -> None: + """The path a callback is served on does not get to pick the state.""" + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + state_id = ( + await store.start_install( + session, platform="slack", user_id=fixture.user_id + ) + ).id + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + with pytest.raises(MessagingInstallStateError): + await store.redeem_state(session, state_id=state_id, platform="teams") + + +class TestRecordingAnInstall: + async def test_a_claimed_workspace_raises_something_showable( + self, rls_harness: RLSHarness + ) -> None: + """An IntegrityError reaching a request handler is a 500 and a + traceback; this failure is ordinary and has to read as one.""" + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + await store.record_install( + session, + platform="slack", + external_workspace_id=fixture.workspace, + encrypted_bot_token="ciphertext", + scopes="chat:write", + user_id=fixture.user_id, + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + with pytest.raises(MessagingInstallClaimedError, match="already connected"): + await store.record_install( + session, + platform="slack", + external_workspace_id=fixture.workspace, + encrypted_bot_token="ciphertext", + scopes="chat:write", + user_id=fixture.user_id, + ) + + async def test_only_the_owning_tenant_reads_it_back( + self, rls_harness: RLSHarness + ) -> None: + fixture = await _two_tenants(rls_harness.owner) + store = MessagingInstallStore() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + await store.record_install( + session, + platform="slack", + external_workspace_id=fixture.workspace, + encrypted_bot_token="ciphertext", + scopes="chat:write", + user_id=fixture.user_id, + ) + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + mine = await store.get_for_workspace( + session, platform="slack", external_workspace_id=fixture.workspace + ) + assert mine is not None + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + theirs = await store.get_for_workspace( + session, platform="slack", external_workspace_id=fixture.workspace + ) + assert theirs is None diff --git a/core/tests/switch_core/db/test_messaging_install_claim.py b/core/tests/switch_core/db/test_messaging_install_claim.py new file mode 100644 index 000000000..c1af5cd94 --- /dev/null +++ b/core/tests/switch_core/db/test_messaging_install_claim.py @@ -0,0 +1,268 @@ +"""One external workspace belongs to one tenant, and the database is what says so. + +Inbound events from the Switch messaging app arrive over a public endpoint +carrying a workspace id and nothing else — no tenant, no credential of ours. +`tenant_of_messaging_install` turns that workspace into a tenant, and the whole +of why it can is that `messaging_installs` makes `(platform, +external_workspace_id)` unique across the deployment. Two tenants holding the +same workspace would be an event with two possible destinations and a lookup +that refuses rather than picking; every message from that workspace would stop. + +So the constraint is the routing guarantee, and it is asserted here through the +restricted role rather than through the owner, because the interesting part is +what row-level security does *not* do to it. A unique index is enforced against +rows the policy hides, which is the behaviour this needs and is easy to assume +the other way round: the second tenant cannot read the first tenant's row and +is still refused the insert. + +That refusal discloses one bit — some tenant holds this workspace — to a caller +who could already name it. That is deliberate, and the alternative is worse: a +silent second claim, discovered when a customer's messages start arriving in +somebody else's rooms. + +The claim covers **active** installs only, and the second half of this file is +about that. A claim that outlived the install would mean a workspace could be +connected once ever — including by the customer who had just disconnected it — +so ending an install releases the workspace. The lookup has to agree with the +index about which rows count, and the tests below measure that agreement from +both directions: a released workspace can be claimed again, and a workspace +with history resolves to whoever holds it now rather than refusing because +two rows name it. +""" + +from __future__ import annotations + +import uuid +from datetime import UTC, datetime + +import pytest +from sqlalchemy import select, update +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from switch_core.db.models import MessagingInstall, Tenant, User +from switch_core.db.session_scope import tenant_session +from switch_core.db.tenant_lookup import tenant_of_messaging_install +from tests.conftest import RLSHarness + +pytestmark = pytest.mark.no_ambient_tenant + + +class _Fixture: + def __init__(self) -> None: + self.tenant_a: str = "" + self.tenant_b: str = "" + self.user_id: str = "" + self.workspace: str = "" + + +async def _two_tenants_and_a_workspace(owner: async_sessionmaker) -> _Fixture: + fixture = _Fixture() + suffix = uuid.uuid4().hex[:8] + fixture.tenant_a = f"tenant-a-{suffix}" + fixture.tenant_b = f"tenant-b-{suffix}" + fixture.workspace = f"T-{suffix}" + + async with owner() as session: + for tenant_id in (fixture.tenant_a, fixture.tenant_b): + session.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + user = User(name="installer", email=f"{suffix}@example.test", role="user") + session.add(user) + await session.flush() + fixture.user_id = user.id + await session.commit() + return fixture + + +def _install(fixture: _Fixture, tenant_id: str) -> MessagingInstall: + return MessagingInstall( + tenant_id=tenant_id, + platform="slack", + external_workspace_id=fixture.workspace, + encrypted_bot_token="ciphertext", + scopes="chat:write", + status="active", + installed_by_user_id=fixture.user_id, + ) + + +async def _end(session: AsyncSession, install_id: str, status: str) -> None: + """End an install the way the store will, without depending on it yet. + + A direct write, because what is under test here is the index and the + lookup: the properties have to hold for any row in that state, not only + for rows a particular method happened to produce. + """ + await session.execute( + update(MessagingInstall) + .where(MessagingInstall.id == install_id) + .values(status=status, ended_at=datetime.now(UTC), encrypted_bot_token=None) + ) + + +async def test_a_second_tenant_cannot_claim_a_claimed_workspace( + rls_harness: RLSHarness, +) -> None: + fixture = await _two_tenants_and_a_workspace(rls_harness.owner) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + session.add(_install(fixture, fixture.tenant_a)) + await session.commit() + + with pytest.raises(IntegrityError) as raised: + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + session.add(_install(fixture, fixture.tenant_b)) + await session.commit() + assert "uq_messaging_installs_workspace" in str(raised.value) + + +async def test_the_claiming_tenant_still_cannot_see_the_row_it_collided_with( + rls_harness: RLSHarness, +) -> None: + """The measurement that makes the test above mean something. + + If tenant B could read tenant A's install, the constraint would be + redundant with an ordinary application-level check. It cannot, so the + constraint is the only thing standing between two claims on one workspace. + """ + fixture = await _two_tenants_and_a_workspace(rls_harness.owner) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + session.add(_install(fixture, fixture.tenant_a)) + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + visible = ( + await session.execute( + select(MessagingInstall.id).where( + MessagingInstall.external_workspace_id == fixture.workspace + ) + ) + ).scalars() + assert list(visible) == [] + + +async def test_the_same_workspace_on_another_platform_is_a_separate_claim( + rls_harness: RLSHarness, +) -> None: + """Uniqueness is on the pair. Two platforms minting the same string is a + coincidence, not a conflict, and refusing the second install would take a + customer's Teams workspace away because somebody's Slack workspace shares + an id.""" + fixture = await _two_tenants_and_a_workspace(rls_harness.owner) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + session.add(_install(fixture, fixture.tenant_a)) + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + install = _install(fixture, fixture.tenant_b) + install.platform = "teams" + session.add(install) + await session.commit() + + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "slack", fixture.workspace + ) + == fixture.tenant_a + ) + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "teams", fixture.workspace + ) + == fixture.tenant_b + ) + + +async def test_an_install_that_ended_releases_the_workspace( + rls_harness: RLSHarness, +) -> None: + """The claim lasts as long as the install and not a moment longer. + + A permanent claim is not a stricter version of this guarantee, it is a + different and wrong one: a customer who tries Switch, disconnects, and + comes back finds their own workspace held by a row nobody can see and + nobody can release. + """ + fixture = await _two_tenants_and_a_workspace(rls_harness.owner) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + install = _install(fixture, fixture.tenant_a) + session.add(install) + await session.flush() + await _end(session, install.id, "disconnected") + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_b) as session: + session.add(_install(fixture, fixture.tenant_b)) + await session.commit() + + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "slack", fixture.workspace + ) + == fixture.tenant_b + ) + + +async def test_the_lookup_does_not_answer_twice_for_a_workspace_with_history( + rls_harness: RLSHarness, +) -> None: + """The failure the status predicate exists to prevent. + + The lookup refuses an ambiguous answer rather than picking one, which is + right — but it makes a second row for the same workspace an outage for the + live install rather than a stale record. So the predicate on the function + has to be the index's predicate, and this measures it with enough history + to catch a lookup that merely takes the first row: two ended installs and + one live one, inserted in that order. + """ + fixture = await _two_tenants_and_a_workspace(rls_harness.owner) + + for _ in range(2): + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + install = _install(fixture, fixture.tenant_a) + session.add(install) + await session.flush() + await _end(session, install.id, "revoked") + await session.commit() + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + session.add(_install(fixture, fixture.tenant_a)) + await session.commit() + + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "slack", fixture.workspace + ) + == fixture.tenant_a + ) + + +async def test_a_workspace_nobody_holds_any_longer_resolves_to_nobody( + rls_harness: RLSHarness, +) -> None: + """An ended install must not go on routing the workspace's traffic. + + The app can still be sitting in the customer's Slack after a disconnect + here, posting events at us for as long as someone leaves it there. Those + events belong to no tenant now, and the lookup saying so is what turns + them into a refusal instead of a delivery into rooms the customer has + stopped paying for. + """ + fixture = await _two_tenants_and_a_workspace(rls_harness.owner) + + async with tenant_session(rls_harness.restricted, fixture.tenant_a) as session: + install = _install(fixture, fixture.tenant_a) + session.add(install) + await session.flush() + await _end(session, install.id, "disconnected") + await session.commit() + + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "slack", fixture.workspace + ) + is None + ) diff --git a/core/tests/switch_core/db/test_row_level_security.py b/core/tests/switch_core/db/test_row_level_security.py index b1446b51c..0a3945552 100644 --- a/core/tests/switch_core/db/test_row_level_security.py +++ b/core/tests/switch_core/db/test_row_level_security.py @@ -328,8 +328,8 @@ def _expected_predicate(tenant_column: str) -> str: ) -def _migration_module() -> ModuleType: - """The `265ed188ad6f` revision module, loaded through Alembic. +def _revision_module(revision: str) -> ModuleType: + """One revision module, loaded through Alembic. Alembic's own loader, rather than an `importlib` call on a path, so this finds the file the same way a deployment would and fails the same way if @@ -338,8 +338,12 @@ def _migration_module() -> ModuleType: core = Path(switch_core.__file__).resolve().parents[1] config = Config(str(core / "alembic.ini")) config.set_main_option("script_location", str(core / "switch_core" / "migrations")) - revision = ScriptDirectory.from_config(config).get_revision(_RLS_REVISION) - return revision.module + return ScriptDirectory.from_config(config).get_revision(revision).module + + +def _migration_module() -> ModuleType: + """The revision that installed the policies.""" + return _revision_module(_RLS_REVISION) class TestCatalogueCoverage: diff --git a/core/tests/switch_core/db/test_tenant_exemption_allowlist.py b/core/tests/switch_core/db/test_tenant_exemption_allowlist.py index 64c129911..3d57a604d 100644 --- a/core/tests/switch_core/db/test_tenant_exemption_allowlist.py +++ b/core/tests/switch_core/db/test_tenant_exemption_allowlist.py @@ -77,6 +77,13 @@ # `_room_tenant`'s fallback: which tenant is this room in, asked when the # answer is not already cached alongside the channel mapping. "switch_core.bridges.collaboration.bridge_core", + # An inbound webhook from an installed workspace: the platform's signature + # proves the sender and the payload names a workspace, and nothing in + # either names a tenant. It is the one read that must happen before a + # tenant can be bound at all, and the install row is then re-read scoped to + # the tenant it produced, so a wrong answer here is a miss rather than a + # cross-tenant read. + "switch_core.bridges.collaboration.install_service", # `switch_core.transport.postgres` and `switch_core.clients.agent_client` # came off this list with `tenant_of_client`: both were built from a # `clients` row that already named the tenant, so they carry it instead of diff --git a/core/tests/switch_core/db/test_tenant_lookup.py b/core/tests/switch_core/db/test_tenant_lookup.py index 70cb344bc..a68e939ef 100644 --- a/core/tests/switch_core/db/test_tenant_lookup.py +++ b/core/tests/switch_core/db/test_tenant_lookup.py @@ -52,6 +52,7 @@ Client, CollaborationBridge, Invitation, + MessagingInstall, Room, ServerConnector, Tenant, @@ -69,6 +70,7 @@ tenant_of_api_key, tenant_of_collaboration_bridge, tenant_of_invitation, + tenant_of_messaging_install, tenant_of_room, tenant_of_server_connector, tenants_of_user, @@ -92,11 +94,23 @@ # The same bookkeeping in the other direction: a lookup the live module names # that `9c41a7b0e5d8` never created, and the revision that did create it. The # frozen-copy comparison below has to know about both to stay exact — without -# this entry the only way to keep it green would be to loosen it to a subset +# these entries the only way to keep it green would be to loosen it to a subset # check, and a subset check passes for a lookup that exists in the module and -# in no migration at all, which is a deployment whose invitation acceptance -# cannot resolve a tenant with the whole suite green. -_ADDED_SINCE = {"tenant_of_invitation": "5daaea6b674d"} +# in no migration at all, which is a deployment whose invitation acceptance — +# or whose inbound webhook — cannot resolve a tenant with the whole suite green. +_ADDED_SINCE = { + "tenant_of_invitation": "5daaea6b674d", + "tenant_of_messaging_install": "c8a4e21f6d30", +} + +# A third kind of change, and the quietest: a lookup whose body a later +# revision replaced. Creation and removal both show up as a function that is +# there or is not; a redefinition leaves a function of the right name and +# signature answering a different question, which nothing about the shape of +# the schema reveals. So the revision that last wrote the body is named here +# and compared against the module, while the creating revision above goes on +# owning the drop. +_REDEFINED_SINCE = {"tenant_of_messaging_install": "a7f2c3e9b481"} def _revision_module(revision: str) -> ModuleType: @@ -128,6 +142,8 @@ def __init__(self) -> None: self.bridge_a: str = "" self.connector_a: str = "" self.key_hash_a: str = "" + self.workspace_a: str = "" + self.workspace_b: str = "" self.oauth_client_a: str = "" self.user_a: str = "" self.user_in_both: str = "" @@ -151,6 +167,8 @@ async def _two_populated_tenants(harness: RLSHarness) -> _Fixture: fixture.tenant_b = f"tenant-b-{suffix}" fixture.oauth_client_a = f"oauth-{suffix}" fixture.key_hash_a = f"hash-a-{suffix}" + fixture.workspace_a = f"T-a-{suffix}" + fixture.workspace_b = f"T-b-{suffix}" async with harness.owner() as session: for tenant_id in (fixture.tenant_a, fixture.tenant_b): @@ -226,6 +244,17 @@ async def _two_populated_tenants(harness: RLSHarness) -> _Fixture: created_by=user_both.id, ) session.add(invitation) + session.add( + MessagingInstall( + tenant_id=tenant_id, + platform="slack", + external_workspace_id=f"T-{tag}-{suffix}", + encrypted_bot_token="x", + scopes="chat:write", + status="active", + installed_by_user_id=user_both.id, + ) + ) session.add( Agent( tenant_id=tenant_id, @@ -453,6 +482,57 @@ async def test_an_unknown_invitation_token_resolves_to_nothing( await tenant_of_invitation(rls_harness.restricted, "no-such-hash") is None ) + async def test_an_installed_workspace_resolves_to_the_tenant_that_installed_it( + self, rls_harness: RLSHarness + ) -> None: + """The whole of what routes an inbound webhook. + + Both tenants have installed the same platform, so a lookup that + ignored its arguments, or matched on the platform alone, answers twice + and is refused rather than passing. + """ + fixture = await _two_populated_tenants(rls_harness) + restricted = rls_harness.restricted + assert ( + await tenant_of_messaging_install(restricted, "slack", fixture.workspace_a) + == fixture.tenant_a + ) + assert ( + await tenant_of_messaging_install(restricted, "slack", fixture.workspace_b) + == fixture.tenant_b + ) + + async def test_a_workspace_on_another_platform_resolves_to_nothing( + self, rls_harness: RLSHarness + ) -> None: + """The pair is the key, not either half of it. + + A workspace id that exists under `slack` must not answer for `teams`; + the two arguments being applied to the columns they name is the + difference between routing an event and delivering it to whoever + happened to mint the same string first. + """ + fixture = await _two_populated_tenants(rls_harness) + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "teams", fixture.workspace_a + ) + is None + ) + + async def test_an_uninstalled_workspace_resolves_to_nothing_rather_than_raising( + self, rls_harness: RLSHarness + ) -> None: + """A webhook for a workspace we are not installed in is a request to + reject, not a fault. Same shape as an unknown bearer token.""" + await _two_populated_tenants(rls_harness) + assert ( + await tenant_of_messaging_install( + rls_harness.restricted, "slack", "T-never-installed" + ) + is None + ) + async def test_an_ambiguous_answer_is_refused_rather_than_picked( self, rls_harness: RLSHarness ) -> None: @@ -614,7 +694,7 @@ def test_the_frozen_list_matches_the_live_one(self) -> None: live = { ( lookup.name, - "" if lookup.parameter is None else f"{lookup.parameter} text", + lookup.parameter_declaration, lookup.signature, lookup.query, ) @@ -640,24 +720,37 @@ def test_the_added_lookup_is_installed_by_a_revision_and_not_only_here( A lookup added to `db/tenant_lookup.py` is built by `create_all`, so every test in this file exercises it and passes. A deployment's schema is built by Alembic, which knows nothing about it: the function is - absent, and the first call — an invitation acceptance trying to - resolve a tenant — fails at runtime in production and nowhere else. + absent, and the first call — an invitation acceptance or an inbound + webhook trying to resolve a tenant — fails at runtime in production and + nowhere else. Comparing the rendered statement rather than the pieces, for the same reason the test below does: a revision that created the function `SECURITY INVOKER`, or without the `search_path`, would install something that cannot read across tenants at all. + + Driven by `_ADDED_SINCE` rather than naming one lookup, so the next + entry is covered by adding it there and nowhere else. + + The body is compared against `_REDEFINED_SINCE` where there is an + entry, because the revision that creates a function is not always the + one that last says what it does. The drop is still the creating + revision's: a redefinition replaces a body and leaves the function it + replaced nothing to undo. """ - revision = _ADDED_SINCE["tenant_of_invitation"] - lookup = TENANT_LOOKUPS_BY_NAME["tenant_of_invitation"] - module = _revision_module(revision) - assert module.CREATE_TENANT_OF_INVITATION == create_lookup_ddl(lookup), ( - f"revision {revision} would install tenant_of_invitation with " - "different DDL from the one db/tenant_lookup.py builds." - ) - assert module.DROP_TENANT_OF_INVITATION == ( - f"DROP FUNCTION IF EXISTS {lookup.signature}" - ) + for name, revision in _ADDED_SINCE.items(): + lookup = TENANT_LOOKUPS_BY_NAME[name] + defining = _REDEFINED_SINCE.get(name, revision) + module = _revision_module(defining) + assert getattr(module, f"CREATE_{name.upper()}") == create_lookup_ddl( + lookup + ), ( + f"revision {defining} would install {name} with different DDL " + "from the one db/tenant_lookup.py builds." + ) + assert getattr(_revision_module(revision), f"DROP_{name.upper()}") == ( + f"DROP FUNCTION IF EXISTS {lookup.signature}" + ) def test_the_dropped_lookup_is_dropped_by_a_revision_and_not_only_here( self, @@ -713,9 +806,8 @@ def test_the_statement_it_would_run_is_the_statement_the_module_builds( for lookup in TENANT_LOOKUPS: if lookup.name in _ADDED_SINCE: continue - parameters = "" if lookup.parameter is None else f"{lookup.parameter} text" assert module.create_lookup_ddl( - lookup.name, parameters, lookup.query + lookup.name, lookup.parameter_declaration, lookup.query ) == create_lookup_ddl(lookup), ( f"migration 9c41a7b0e5d8 would install {lookup.name} with " "different DDL from the one db/tenant_lookup.py builds." diff --git a/core/tests/switch_core/gateway/test_bridge_delete_guard.py b/core/tests/switch_core/gateway/test_bridge_delete_guard.py new file mode 100644 index 000000000..c0c39ea18 --- /dev/null +++ b/core/tests/switch_core/gateway/test_bridge_delete_guard.py @@ -0,0 +1,252 @@ +"""Deleting a bridge an install built must be refused, not half-done. + +`messaging_installs.bridge_id` is a real foreign key with no `ON DELETE`, and +the delete endpoint tears down every room on the bridge *before* it removes the +bridge itself. Without a guard the ordering plays out as: rooms irreversibly +deleted, Postgres then refuses the bridge deletion, operator gets a 500 — and +the app is still installed with a token nobody revoked. +""" + +from __future__ import annotations + +import uuid + +import pytest +from fastapi import HTTPException +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from switch_core.db.models import ( + Client, + CollaborationBridge, + MessagingInstall, + Room, + User, +) +from switch_core.db.stores.collaboration_bridge_store import CollaborationBridgeStore +from switch_core.db.stores.messaging_install_store import ( + INSTALL_ACTIVE, + INSTALL_DISCONNECTED, + MessagingInstallStore, +) +from switch_core.db.stores.room_store import RoomStore +from switch_core.gateway.collaborations import delete_bridge + +_BRIDGE_STORE = CollaborationBridgeStore() +_ROOM_STORE = RoomStore() +_INSTALL_STORE = MessagingInstallStore() + + +class _RecordingRoomService: + """Remembers the rooms it was told to delete and deletes nothing. + + A spy rather than the real service because the property under test is that + it is never called at all: an assertion on the list is the assertion that + the refusal came first. + """ + + def __init__(self) -> None: + self.deleted: list[str] = [] + + async def delete_room(self, room_id: str) -> None: + self.deleted.append(room_id) + + +class _RecordingLifecycle: + def __init__(self) -> None: + self.removed: list[str] = [] + + async def remove(self, bridge_id: str) -> None: + self.removed.append(bridge_id) + + +async def _make_admin(session: AsyncSession) -> User: + user = User( + id=f"admin-{uuid.uuid4().hex[:8]}", + name=f"admin-{uuid.uuid4().hex[:8]}", + email=f"admin-{uuid.uuid4().hex[:8]}@example.test", + role="admin", + ) + session.add(user) + await session.flush() + return user + + +async def _make_bridge(session: AsyncSession) -> str: + client = Client( + matrix_user_id=f"@bridge-{uuid.uuid4().hex[:12]}:test", + display_name="bridge client", + type="bridge", + ) + session.add(client) + await session.flush() + bridge = CollaborationBridge( + type="slack", + display_name="Slack", + client_id=client.id, + status="active", + ) + session.add(bridge) + await session.flush() + return bridge.id + + +async def _make_room(session: AsyncSession, *, bridge_id: str) -> str: + room = Room( + matrix_room_id=f"!{uuid.uuid4().hex[:8]}:test", + name="bridged room", + description="mirror of an external channel", + bridge_id=bridge_id, + channel_type="channel_public", + external_channel_id="C123", + ) + session.add(room) + await session.flush() + return room.id + + +async def _make_install( + session: AsyncSession, *, bridge_id: str | None, status: str, user_id: str +) -> str: + install = MessagingInstall( + platform="slack", + external_workspace_id=f"T{uuid.uuid4().hex[:8]}", + encrypted_bot_token="encrypted-placeholder", + scopes="app_mentions:read,chat:write", + status=status, + installed_by_user_id=user_id, + bridge_id=bridge_id, + ) + session.add(install) + await session.flush() + return install.id + + +async def test_a_bridge_an_install_built_cannot_be_deleted_here( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + async with session_factory() as session: + admin = await _make_admin(session) + bridge_id = await _make_bridge(session) + room_id = await _make_room(session, bridge_id=bridge_id) + await _make_install( + session, bridge_id=bridge_id, status=INSTALL_ACTIVE, user_id=admin.id + ) + await session.commit() + + room_service = _RecordingRoomService() + lifecycle = _RecordingLifecycle() + async with session_factory() as session: + admin = await _make_admin(session) + with pytest.raises(HTTPException) as excinfo: + await delete_bridge( + bridge_id, + session, + _BRIDGE_STORE, + _ROOM_STORE, + room_service, # type: ignore[arg-type] + _INSTALL_STORE, + lifecycle, # type: ignore[arg-type] + admin, + ) + + assert excinfo.value.status_code == 409 + # The whole point of the ordering: nothing was destroyed on the way to the + # refusal, so retrying after disconnecting is a clean retry. + assert room_service.deleted == [] + assert lifecycle.removed == [] + + async with session_factory() as session: + assert await _BRIDGE_STORE.get(session, bridge_id) is not None + assert await _ROOM_STORE.get(session, room_id) is not None + + +async def test_the_refusal_names_the_workspace_to_disconnect( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + """An operator who cannot delete here has to be told where to go instead.""" + async with session_factory() as session: + admin = await _make_admin(session) + bridge_id = await _make_bridge(session) + install_id = await _make_install( + session, bridge_id=bridge_id, status=INSTALL_ACTIVE, user_id=admin.id + ) + await session.commit() + + async with session_factory() as session: + install = await _INSTALL_STORE.get(session, install_id=install_id) + workspace = install.external_workspace_id + with pytest.raises(HTTPException) as excinfo: + await delete_bridge( + bridge_id, + session, + _BRIDGE_STORE, + _ROOM_STORE, + _RecordingRoomService(), # type: ignore[arg-type] + _INSTALL_STORE, + _RecordingLifecycle(), # type: ignore[arg-type] + await _make_admin(session), + ) + + detail = str(excinfo.value.detail) + assert workspace in detail + assert "slack" in detail + assert "Disconnect" in detail + + +async def test_a_bridge_registered_by_hand_still_deletes( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + async with session_factory() as session: + bridge_id = await _make_bridge(session) + room_id = await _make_room(session, bridge_id=bridge_id) + await session.commit() + + room_service = _RecordingRoomService() + lifecycle = _RecordingLifecycle() + async with session_factory() as session: + result = await delete_bridge( + bridge_id, + session, + _BRIDGE_STORE, + _ROOM_STORE, + room_service, # type: ignore[arg-type] + _INSTALL_STORE, + lifecycle, # type: ignore[arg-type] + await _make_admin(session), + ) + + assert result == {"ok": True} + assert room_service.deleted == [room_id] + assert lifecycle.removed == [bridge_id] + + +async def test_a_bridge_whose_install_already_ended_still_deletes( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + """Disconnecting releases the pointer, so an ended install guards nothing. + + The row it leaves behind is a record, not a claim — and a bridge that + outlived its install is an ordinary bridge again. + """ + async with session_factory() as session: + admin = await _make_admin(session) + bridge_id = await _make_bridge(session) + await _make_install( + session, bridge_id=None, status=INSTALL_DISCONNECTED, user_id=admin.id + ) + await session.commit() + + lifecycle = _RecordingLifecycle() + async with session_factory() as session: + await delete_bridge( + bridge_id, + session, + _BRIDGE_STORE, + _ROOM_STORE, + _RecordingRoomService(), # type: ignore[arg-type] + _INSTALL_STORE, + lifecycle, # type: ignore[arg-type] + await _make_admin(session), + ) + + assert lifecycle.removed == [bridge_id] diff --git a/core/tests/switch_core/test_config_slack_app.py b/core/tests/switch_core/test_config_slack_app.py new file mode 100644 index 000000000..603bd1f45 --- /dev/null +++ b/core/tests/switch_core/test_config_slack_app.py @@ -0,0 +1,94 @@ +"""Half-configuring the distributed Slack app has to be a startup error. + +Each of the three credentials fails differently and none of them fails +usefully. Without the client id and secret the code exchange is refused by +Slack; without the signing secret there is nothing distinguishing a real event +from a post by anyone who found the URL. A deployment that sets two of three +looks configured, offers the button, and breaks at the worst moment — so it +does not start. +""" + +import pytest + +from switch_core.config import SwitchConfig + +_BASE_KWARGS = dict( + db_host="db", + db_port="5432", + db_user="postgres", + db_password="pw", + db_name="switch", + matrix_server_name="switch.local", + agent_registration_token="token", + jwt_secret_key="jwt", + gateway_admin_email="admin@example.com", + gateway_admin_password="pw", +) + +_APP = dict( + slack_app_client_id="1234.5678", + slack_app_client_secret="secret", + slack_app_signing_secret="signing", +) + + +def _config(**overrides: object) -> SwitchConfig: + return SwitchConfig(**{**_BASE_KWARGS, **overrides}) # type: ignore[arg-type] + + +def test_setting_none_of_them_is_the_ordinary_case() -> None: + assert _config().slack_app_client_id is None + + +def test_setting_all_three_with_a_public_origin_is_accepted() -> None: + config = _config(**_APP, messaging_public_url="https://switch.example") + assert config.slack_app_signing_secret == "signing" + + +@pytest.mark.parametrize("missing", sorted(_APP)) +def test_setting_some_of_them_raises(missing: str) -> None: + partial = {key: value for key, value in _APP.items() if key != missing} + with pytest.raises(ValueError, match="Partial distributed Slack app config"): + _config(**partial, messaging_public_url="https://switch.example") + + +def test_an_app_with_no_public_origin_raises() -> None: + """The redirect and the events URL are both built from it. + + Slack compares the redirect against the one registered with the app, so + building it against nothing is an install that fails at Slack with nothing + in our logs. + """ + with pytest.raises(ValueError, match="MESSAGING_PUBLIC_URL"): + _config(**_APP) + + +def test_the_gateway_url_does_not_stand_in_for_it() -> None: + """They name different hosts and only one of them is dialled by Slack. + + A deployment whose gateway is on a private network is the ordinary case, + and accepting that value here would register a redirect URL Slack cannot + reach — an install that fails at Slack with nothing in our logs. + """ + with pytest.raises(ValueError, match="MESSAGING_PUBLIC_URL"): + _config(**_APP, gateway_public_url="https://gateway.example") + + +def test_a_path_on_the_origin_is_refused() -> None: + """Slack compares the redirect byte for byte and says only that it failed.""" + with pytest.raises(ValueError, match=r"scheme \+ host only"): + _config(**_APP, messaging_public_url="https://switch.example/messaging") + + +def test_a_trailing_slash_is_allowed() -> None: + """The one path that is harmless: joining it with a rooted path is the same + string either way, and an operator who pastes a host with one should not be + refused for it.""" + config = _config(**_APP, messaging_public_url="https://switch.example/") + assert config.messaging_public_url == "https://switch.example/" + + +def test_an_http_origin_is_refused() -> None: + """Slack will not register one, so accepting it only defers the failure.""" + with pytest.raises(ValueError, match="must be https"): + _config(**_APP, messaging_public_url="http://switch.example") diff --git a/deploy/local/standalone-docker-compose.yml b/deploy/local/standalone-docker-compose.yml index f9ec593e0..9ff50cde2 100644 --- a/deploy/local/standalone-docker-compose.yml +++ b/deploy/local/standalone-docker-compose.yml @@ -207,6 +207,10 @@ services: GATEWAY_ADMIN_PASSWORD: ${GATEWAY_ADMIN_PASSWORD} FRONTEND_BASE_URL: ${FRONTEND_BASE_URL} GATEWAY_PUBLIC_URL: ${GATEWAY_PUBLIC_URL:-} + MESSAGING_PUBLIC_URL: ${MESSAGING_PUBLIC_URL:-} + SLACK_APP_CLIENT_ID: ${SLACK_APP_CLIENT_ID:-} + SLACK_APP_CLIENT_SECRET: ${SLACK_APP_CLIENT_SECRET:-} + SLACK_APP_SIGNING_SECRET: ${SLACK_APP_SIGNING_SECRET:-} depends_on: postgres: condition: service_healthy diff --git a/deploy/remote/helm/switch/samples/ingress.example.yaml b/deploy/remote/helm/switch/samples/ingress.example.yaml index 5fb7effd1..998f9fa6a 100644 --- a/deploy/remote/helm/switch/samples/ingress.example.yaml +++ b/deploy/remote/helm/switch/samples/ingress.example.yaml @@ -46,6 +46,12 @@ spec: - path: /.well-known pathType: Prefix backend: { service: { name: RELEASE-switch-core, port: { number: 8000 } } } + # The distributed Slack app's OAuth callback and event URLs. Needed + # only if you offer workspace installs; Slack dials these from the + # internet, so they must be reachable and on a trusted certificate. + - path: /messaging + pathType: Prefix + backend: { service: { name: RELEASE-switch-core, port: { number: 8000 } } } - path: /health pathType: Prefix backend: { service: { name: RELEASE-switch-core, port: { number: 8000 } } } diff --git a/deploy/remote/helm/switch/templates/_helpers.tpl b/deploy/remote/helm/switch/templates/_helpers.tpl index eac995c80..22f2bf51a 100644 --- a/deploy/remote/helm/switch/templates/_helpers.tpl +++ b/deploy/remote/helm/switch/templates/_helpers.tpl @@ -62,6 +62,10 @@ MATTERMOST_USER_PASSWORD: {{ .Values.secrets.mattermostUserPassword | default .V {{- if .Values.switchCore.oidc.enabled }} GATEWAY_OIDC_CLIENT_SECRET: {{ required "secrets.gatewayOidcClientSecret is required when switchCore.oidc.enabled" .Values.secrets.gatewayOidcClientSecret | b64enc | quote }} {{- end }} +{{- if .Values.switchCore.slackApp.enabled }} +SLACK_APP_CLIENT_SECRET: {{ required "secrets.slackAppClientSecret is required when switchCore.slackApp.enabled" .Values.secrets.slackAppClientSecret | b64enc | quote }} +SLACK_APP_SIGNING_SECRET: {{ required "secrets.slackAppSigningSecret is required when switchCore.slackApp.enabled" .Values.secrets.slackAppSigningSecret | b64enc | quote }} +{{- end }} {{- end }} {{/* @@ -604,6 +608,22 @@ Include with `nindent 12`. - name: GATEWAY_PUBLIC_URL value: {{ .Values.switchCore.gatewayPublicUrl | quote }} {{- end }} +{{- if .Values.switchCore.slackApp.enabled }} +- name: MESSAGING_PUBLIC_URL + value: {{ required "switchCore.slackApp.messagingPublicUrl is required when switchCore.slackApp.enabled" .Values.switchCore.slackApp.messagingPublicUrl | quote }} +- name: SLACK_APP_CLIENT_ID + value: {{ required "switchCore.slackApp.clientId is required when switchCore.slackApp.enabled" .Values.switchCore.slackApp.clientId | quote }} +- name: SLACK_APP_CLIENT_SECRET + valueFrom: + secretKeyRef: + name: {{ include "switch.secretName" . }} + key: SLACK_APP_CLIENT_SECRET +- name: SLACK_APP_SIGNING_SECRET + valueFrom: + secretKeyRef: + name: {{ include "switch.secretName" . }} + key: SLACK_APP_SIGNING_SECRET +{{- end }} {{- end }} {{/* diff --git a/deploy/remote/helm/switch/values.yaml b/deploy/remote/helm/switch/values.yaml index 75fffe8dc..a978886b6 100644 --- a/deploy/remote/helm/switch/values.yaml +++ b/deploy/remote/helm/switch/values.yaml @@ -32,6 +32,18 @@ ingress: # Emit ingress-nginx annotations that keep MCP/SSE streams alive (long read # timeouts, buffering off). Harmless on other controllers but ignored by them # — set false and add your controller's equivalents via annotations. + # + # Your controller almost certainly needs an equivalent, because the defaults + # elsewhere are short enough to cut an idle agent stream. An AWS ALB idles a + # connection out after 60 seconds; a GCE backend service after 30. On an ALB, + # add to annotations above: + # + # alb.ingress.kubernetes.io/load-balancer-attributes: idle_timeout.timeout_seconds=3600 + # + # — a property of the load balancer rather than of this Ingress, so Ingresses + # sharing a group must all agree on the value or the controller reconciles + # none of them. On GCE it is `timeoutSec` on a BackendConfig, which is a + # separate object this chart does not render. streamingAnnotations: true tls: enabled: false @@ -45,6 +57,7 @@ ingress: - /mcp - /oauth - /.well-known + - /messaging - /health # Path prefixes served by the Teams bridge listener rather than the FastAPI # app, because that listener is a separate HTTP server on its own port. Used @@ -89,6 +102,11 @@ secrets: mattermostUserPassword: "" # Gateway OIDC client secret (required when switchCore.oidc.enabled). gatewayOidcClientSecret: "" + # Distributed Slack app (required when switchCore.slackApp.enabled). The + # signing secret is the whole of what distinguishes a real Slack event from + # a post by anyone who learned the URL. + slackAppClientSecret: "" + slackAppSigningSecret: "" # ── Switch Core (FastAPI) ───────────────────────────────────────────────────── switchCore: @@ -149,6 +167,24 @@ switchCore: requireEmailVerified: true # Set false to disable password login entirely (OIDC-only). passwordLoginEnabled: true + # The distributed Slack app: the one a customer installs into their workspace + # by clicking a button, as opposed to an app an operator registers themselves + # and pastes a bot token for. Off unless you have registered such an app and + # are distributing it — a self-hosted install almost certainly wants the + # per-tenant Socket Mode bridge instead. + # + # Enabling it commits the deployment to being reachable from the internet: + # Slack dials messagingPublicUrl and gives up on anything it cannot resolve + # or cannot verify a certificate for. Add /messaging to ingress.agentApiPaths + # as well, or the URLs exist and nothing routes to them. + slackApp: + enabled: false + # Public origin Slack reaches Switch on — scheme + host, https, no path. + # Not the same thing as gatewayPublicUrl, which is where a person lands + # from a deeplink and may be private; setting this one to that host would + # move every deeplink along with it. + messagingPublicUrl: "" + clientId: "" # Bearer-token resolution runs before every authenticated request and agents # beat continuously, so a successful resolution is memoised in process for # this many seconds. Only successes are cached — an unknown or revoked token diff --git a/docs/old/bridges/README.md b/docs/old/bridges/README.md index fffa6b0bf..38a105e37 100644 --- a/docs/old/bridges/README.md +++ b/docs/old/bridges/README.md @@ -19,6 +19,12 @@ shared onboarding model, then the per-platform guide: | Discord | [`DISCORD_SETUP.md`](DISCORD_SETUP.md) | single bot app | Gateway WebSocket (outbound) | not required | | Telegram | [`TELEGRAM_SETUP.md`](TELEGRAM_SETUP.md) | single bot, agent named in the message body | long polling (outbound) | not required | +Every guide above describes an app the operator registers themselves and +supplies the credentials for. Slack additionally has a **distributed** app — +one we register and a customer installs by clicking a button, receiving events +over HTTPS rather than Socket Mode. It is a separate Slack app with different +requirements: see [`SLACK_DISTRIBUTED_APP.md`](SLACK_DISTRIBUTED_APP.md). + ## The onboarding model (same for every bridge) A bridge is an **unowned, workspace-wide integration** that holds platform diff --git a/docs/old/bridges/SLACK_DISTRIBUTED_APP.md b/docs/old/bridges/SLACK_DISTRIBUTED_APP.md new file mode 100644 index 000000000..d0e4f9da4 --- /dev/null +++ b/docs/old/bridges/SLACK_DISTRIBUTED_APP.md @@ -0,0 +1,213 @@ +# The distributed Slack app + +`SLACK_SETUP.md` describes the app **an operator registers for themselves**: +they create it, install it into their own workspace, and paste its tokens into +Switch. This page describes the other one — the app **we** register and +distribute, which a customer installs by clicking a button and which never +requires them to see a token at all. + +They are two separate Slack apps and they will both exist. Nothing here +replaces the other page. + +The install connects a workspace to a tenant that **already exists**. Creating +a tenant is Switch Console's job and is not reachable from Slack: the flow +begins with an authenticated admin inside the tenant they are installing into, +so no amount of clicking in Slack brings a tenant into being. + +## Why it cannot be the same app + +The self-registered app uses **Socket Mode**: Switch dials out to Slack and +events arrive down that connection, so the deployment needs no inbound route +from the internet. That is the right shape for a self-hosted operator and the +wrong shape here, for two independent reasons: + +- Slack does not permit Socket Mode for apps listed on the Marketplace. +- A Socket Mode connection is opened with one app-level token by one process. + Every installing workspace's events would arrive down that single socket, so + the moment Switch runs more than one replica the events land on whichever + replica happens to hold the connection. + +So the distributed app receives events over HTTPS instead, at a public URL. +That is the whole of the difference, and everything below follows from it. + +## The four URLs + +Every URL is the same host with a different path. The host is +**`MESSAGING_PUBLIC_URL`**: scheme and host, no path, https only, and the +origin Slack itself dials. + +It is deliberately not `GATEWAY_PUBLIC_URL`. That one is the host a *person* +lands on following an "Open in Switch Console" deeplink, and on many +deployments it is reachable only over a private network — which is fine for a +person and useless to Slack. Pointing it at an internet-facing host to satisfy +Slack would move every deeplink to that host as a side effect, so the two are +separate settings and a deployment may set either, both, or neither. + +| Slack setting | Path | +| --- | --- | +| **OAuth & Permissions → Redirect URLs** | `/messaging/slack/oauth/callback` | +| **Event Subscriptions → Request URL** | `/messaging/slack/events` | +| **Interactivity & Shortcuts → Request URL** | `/messaging/slack/interactive` | +| **Slash Commands → each command's URL** | `/messaging/slack/commands` | + +`/messaging` is a public prefix in its own right: it is unauthenticated by +nature, because a Slack event arrives with no credential of ours and an OAuth +callback arrives before there is anything to authenticate against. It is +deliberately not `/gateway` — that prefix is cookie-authenticated and is not +routed to this application from the outside — and deliberately not `/oauth`, +which already belongs to agents authenticating *to* Switch and would collide +in name only, confusingly. + +Reaching it from the internet needs the prefix added in two places: the +application's own public-path list, and the deployment's ingress path +allowlist. + +## Registering the app + +**Slack verifies the Request URL the moment you save it**, by posting a +challenge it expects the endpoint to echo back. So the order matters: you can +create the app and collect its credentials before the endpoints exist, but you +cannot fill in the URL fields until they are live and publicly reachable. + +1. → **Create New App** → **From an app + manifest**, in a workspace we control. Paste the manifest below, with the + host substituted. +2. **Basic Information → App Credentials.** Take the **Client ID**, **Client + Secret** and **Signing Secret**. The signing secret is what proves an + inbound webhook came from Slack; the client id and secret are what exchange + an install code for a bot token. There is no app-level token and no bot + token here — a bot token belongs to an installation, not to the app, and + arrives one per customer. +3. **Manage Distribution → Activate Public Distribution.** This is what makes + the app installable outside our own workspace and produces the *Add to + Slack* URL. Slack requires every hardcoded workspace reference to be removed + first. +4. Marketplace listing is a separate, reviewed submission, and is not required + for a customer to install by link. + +## Manifest + +Substitute the host in the four `url` fields. The scopes, slash commands and +bot events are identical to the self-registered app's, because the app does +the same job once installed — the differences are all in `settings` and +`oauth_config`. + +```json +{ + "display_information": { + "name": "Agent Switch" + }, + "features": { + "app_home": { + "home_tab_enabled": true, + "messages_tab_enabled": false, + "messages_tab_read_only_enabled": false + }, + "bot_user": { + "display_name": "Agent Switch", + "always_online": false + }, + "slash_commands": [ + { "command": "/admin", "url": "https://HOST/messaging/slack/commands", "description": "Toggle admin mode on/off for this room", "should_escape": false }, + { "command": "/help", "url": "https://HOST/messaging/slack/commands", "description": "Show the list of available in-room commands", "should_escape": false }, + { "command": "/reset", "url": "https://HOST/messaging/slack/commands", "description": "Reset a targeted agent's session (clears context, then reconnects)", "usage_hint": "@agent-name | @role (required)", "should_escape": true }, + { "command": "/reset-all-agents", "url": "https://HOST/messaging/slack/commands", "description": "Reset EVERY agent's session in this room", "should_escape": false }, + { "command": "/compact", "url": "https://HOST/messaging/slack/commands", "description": "Compact a targeted agent's session context", "usage_hint": "@agent-name | @role (required)", "should_escape": true }, + { "command": "/compact-all-agents", "url": "https://HOST/messaging/slack/commands", "description": "Compact EVERY agent's session context in this room", "should_escape": false }, + { "command": "/interrupt", "url": "https://HOST/messaging/slack/commands", "description": "Interrupt a targeted agent's current turn", "usage_hint": "@agent-name | @role (required)", "should_escape": true }, + { "command": "/interrupt-all-agents", "url": "https://HOST/messaging/slack/commands", "description": "Interrupt EVERY agent's current turn in this room", "should_escape": false }, + { "command": "/agents-status", "url": "https://HOST/messaging/slack/commands", "description": "Show each agent's presence and capabilities in this room", "should_escape": false }, + { "command": "/roles", "url": "https://HOST/messaging/slack/commands", "description": "List this room's roles and who currently holds each", "should_escape": false }, + { "command": "/list-agents", "url": "https://HOST/messaging/slack/commands", "description": "List the agents available in this room", "should_escape": false }, + { "command": "/list-switch-agents", "url": "https://HOST/messaging/slack/commands", "description": "List all agents registered on the Switch", "should_escape": false }, + { "command": "/list-documents", "url": "https://HOST/messaging/slack/commands", "description": "List the room's internal documents", "should_escape": false }, + { "command": "/list-references", "url": "https://HOST/messaging/slack/commands", "description": "List the room's references", "should_escape": false }, + { "command": "/list-aliases", "url": "https://HOST/messaging/slack/commands", "description": "List per-room agent aliases (@alias to agent)", "should_escape": false }, + { "command": "/set-alias", "url": "https://HOST/messaging/slack/commands", "description": "Give an agent a room alias", "usage_hint": "@agent-name @alias", "should_escape": true }, + { "command": "/remove-alias", "url": "https://HOST/messaging/slack/commands", "description": "Remove a room alias", "usage_hint": "@alias (or @agent-name)", "should_escape": true }, + { "command": "/invite-agent", "url": "https://HOST/messaging/slack/commands", "description": "Add an existing agent to this room", "usage_hint": "@agent-name", "should_escape": true }, + { "command": "/run-cmd", "url": "https://HOST/messaging/slack/commands", "description": "Show the terminal command to start a session for an agent", "usage_hint": "@agent-name [@role]", "should_escape": true }, + { "command": "/agents-greet", "url": "https://HOST/messaging/slack/commands", "description": "Have agents in the room introduce themselves", "should_escape": false }, + { "command": "/room-url", "url": "https://HOST/messaging/slack/commands", "description": "Show the frontend URL for this room", "should_escape": false } + ] + }, + "oauth_config": { + "redirect_urls": [ + "https://HOST/messaging/slack/oauth/callback" + ], + "scopes": { + "bot": [ + "files:read", + "files:write", + "assistant:write", + "channels:history", + "channels:manage", + "channels:read", + "chat:write", + "chat:write.customize", + "commands", + "groups:history", + "groups:read", + "groups:write", + "im:history", + "im:read", + "im:write", + "mpim:history", + "reactions:read", + "reactions:write", + "users:read", + "usergroups:read", + "usergroups:write" + ] + }, + "pkce_enabled": false + }, + "settings": { + "event_subscriptions": { + "request_url": "https://HOST/messaging/slack/events", + "bot_events": [ + "message.channels", + "message.groups", + "message.im", + "message.mpim" + ] + }, + "interactivity": { + "is_enabled": true, + "request_url": "https://HOST/messaging/slack/interactive" + }, + "org_deploy_enabled": false, + "socket_mode_enabled": false, + "token_rotation_enabled": false, + "is_mcp_enabled": false + } +} +``` + +## Three things left out on purpose + +**`agent_view`.** The self-registered app declares itself a Slack agent, which +is what turns DMs into threads and puts progress on the message. Declaring it +is irreversible per app, removes guest access from the workspace, and requires +re-review for a distributed app. It should be a deliberate later step with +that review budgeted, not something a customer discovers after installing. + +**`org_deploy_enabled`.** An Enterprise Grid org-wide install identifies itself +by enterprise id rather than by a single workspace id. `messaging_installs` is +unique on `(platform, external_workspace_id)` and an org install does not have +one of those, so enabling this is a schema question and not a checkbox. + +**`token_rotation_enabled`.** Rotating tokens means storing a refresh token, +refreshing before expiry, and handling a refresh that fails while events are +arriving. Worth doing, and not worth doing at the same time as everything +else; a non-rotating bot token is what the self-registered app already uses. + +## What a customer's install produces + +One row in `messaging_installs`: the workspace it was installed into, the bot +token that install granted (encrypted), the scopes Slack actually approved, +and the tenant and user who initiated it. `(platform, external_workspace_id)` +is unique across the whole deployment, because an inbound event carries a +workspace id and no tenant — a workspace claimed by two tenants would be an +event with two possible destinations. A second tenant attempting to claim an +already-claimed workspace is refused by the database. diff --git a/docs/old/bridges/SLACK_SETUP.md b/docs/old/bridges/SLACK_SETUP.md index da8182690..4f69fefc2 100644 --- a/docs/old/bridges/SLACK_SETUP.md +++ b/docs/old/bridges/SLACK_SETUP.md @@ -6,6 +6,11 @@ avatar overrides. Inbound events arrive over **Socket Mode** — an outbound WebSocket the bot opens to Slack — so **no public ingress is required**. Outbound messages go through the Slack Web API. +This page is about the app **you** register and hold the tokens for. There is +also a distributed app that we register and you install by clicking a button; +it is a different Slack app with different requirements, described in +[`SLACK_DISTRIBUTED_APP.md`](SLACK_DISTRIBUTED_APP.md). + ## Prerequisites - A Slack workspace where you can install a custom app (workspace admin approval diff --git a/gateway/src/data/api.ts b/gateway/src/data/api.ts index 0bae2bc28..065666d9d 100644 --- a/gateway/src/data/api.ts +++ b/gateway/src/data/api.ts @@ -816,11 +816,12 @@ export async function fetchAllExternalUsers(): Promise< return [...byId.values()]; } -export async function deleteBridge(bridgeId: string): Promise { - const res = await fetchJson<{ ok: boolean }>(`/collaborations/${bridgeId}`, { - method: "DELETE", - }); - return res?.ok ?? false; +// Throws rather than returning false: a connection created by installing the +// Switch app is refused here with a 409 saying to disconnect the app instead, +// and a caller that only sees "it didn't work" would leave the operator +// clicking Delete at a row that will never go. +export async function deleteBridge(bridgeId: string): Promise<{ ok: boolean }> { + return jsonRequest<{ ok: boolean }>(`/collaborations/${bridgeId}`, "DELETE"); } export interface BridgeUpdateInput { @@ -839,6 +840,62 @@ export async function updateBridge( }); } +// ── Installed apps ─────────────────────────────────────────────────────────── +// +// The other way a connection comes into being: instead of an operator +// registering their own app and pasting its credentials, they install this +// deployment's app into their workspace and the platform hands the credential +// back. Most deployments have no app of their own, so `fetchInstallablePlatforms` +// answering with an empty list is the ordinary case and not a failure. + +export interface InstalledApp { + id: string; + platform: string; + external_workspace_id: string; + // "active", "disconnected" (ended here) or "revoked" (ended at the + // platform). The last two are kept apart because an operator whose + // connection stopped working needs to know which of the two it was. + status: string; + // The platform's own spelling of what was granted, shown verbatim. + scopes: string; + bridge_id: string | null; + installed_at: string; + ended_at: string | null; +} + +export async function fetchInstallablePlatforms(): Promise { + const res = await fetchJson<{ platforms: string[] }>("/messaging-apps"); + return res === null ? null : res.platforms; +} + +export async function fetchInstalledApps(): Promise { + const res = await fetchJson<{ installs: InstalledApp[] }>( + "/messaging-apps/installs", + ); + return res === null ? null : res.installs; +} + +// Answers with a URL rather than redirecting, because the install has to begin +// in a top-level window on the platform's own domain — a redirect returned to +// this fetch would be followed by the fetch. +export async function beginAppInstall(platform: string): Promise { + const res = await jsonRequest<{ authorize_url: string }>( + `/messaging-apps/${platform}/install`, + "POST", + ); + return res.authorize_url; +} + +// Throwing, because 502 here means the platform refused to revoke, nothing was +// destroyed, and trying again is the right next move — all of which is in the +// message and none of which survives a boolean. +export async function disconnectApp(installId: string): Promise { + return jsonRequest( + `/messaging-apps/installs/${installId}`, + "DELETE", + ); +} + // ── Auth ──────────────────────────────────────────────────────────────────── export interface UserInfo { diff --git a/gateway/src/data/hooks.ts b/gateway/src/data/hooks.ts index c89af9d67..2f24101a8 100644 --- a/gateway/src/data/hooks.ts +++ b/gateway/src/data/hooks.ts @@ -10,6 +10,7 @@ import { type DocumentDetail, type DocumentSummary, type InboundLinkedRoomDetail, + type InstalledApp, type KnownAgentType, type LinkedRoomDetail, type RoomGraphData, @@ -35,6 +36,8 @@ import { fetchDocumentRooms, fetchDocuments, fetchInboundLinkedRooms, + fetchInstallablePlatforms, + fetchInstalledApps, fetchKnownAgentTypes, fetchLinkedRooms, fetchRoomGraph, @@ -155,6 +158,17 @@ export function useAllExternalUsers(): UseQueryResult { return useQuery(fetchAllExternalUsers); } +// Empty on every deployment that registered no app of its own, which is most +// of them — the page reads that as "there is nothing to install here", not as +// a failure. +export function useInstallablePlatforms(): UseQueryResult { + return useQuery(fetchInstallablePlatforms); +} + +export function useInstalledApps(): UseQueryResult { + return useQuery(fetchInstalledApps); +} + export function useKnownAgentTypes(): UseQueryResult { return useQuery(fetchKnownAgentTypes); } diff --git a/gateway/src/pages/collaborations/CollaborationsPage.tsx b/gateway/src/pages/collaborations/CollaborationsPage.tsx index 2ae99d390..fb2ddb86f 100644 --- a/gateway/src/pages/collaborations/CollaborationsPage.tsx +++ b/gateway/src/pages/collaborations/CollaborationsPage.tsx @@ -2,6 +2,7 @@ import AddLinkOutlined from "@mui/icons-material/AddLinkOutlined"; import AddOutlined from "@mui/icons-material/AddOutlined"; import DeleteOutline from "@mui/icons-material/DeleteOutline"; import { + Alert, Box, Button, Chip, @@ -25,6 +26,7 @@ import { useAuth } from "../../data/AuthContext"; import { useBridges } from "../../data/hooks"; import { formatDate, titleCase } from "../../theme/hootFormat"; import AddToChatDialog from "./AddToChatDialog"; +import InstalledAppsSection from "./InstalledAppsSection"; import RegisterMessagingAppDialog from "./RegisterMessagingAppDialog"; type BridgeRow = BridgeDetail & { id: string }; @@ -41,6 +43,7 @@ export default function CollaborationsPage() { const { data: bridges, loading, refetch } = useBridges(); const [deleteTarget, setDeleteTarget] = useState(null); const [deleting, setDeleting] = useState(false); + const [deleteError, setDeleteError] = useState(null); const [savingId, setSavingId] = useState(null); const [registerOpen, setRegisterOpen] = useState(false); const [installTarget, setInstallTarget] = useState(null); @@ -48,10 +51,19 @@ export default function CollaborationsPage() { const handleDelete = useCallback(async () => { if (!deleteTarget) return; setDeleting(true); - await deleteBridge(deleteTarget.bridge_id); - setDeleteTarget(null); - setDeleting(false); - refetch(); + setDeleteError(null); + try { + await deleteBridge(deleteTarget.bridge_id); + setDeleteTarget(null); + refetch(); + } catch (e) { + // A connection an install created is refused here, and the refusal says + // to disconnect the app instead. Closing the dialog on it would leave + // the operator clicking Delete at a row that will never go. + setDeleteError(e instanceof Error ? e.message : "Failed to delete"); + } finally { + setDeleting(false); + } }, [deleteTarget, refetch]); const handleToggleGreetings = useCallback( @@ -221,6 +233,8 @@ export default function CollaborationsPage() { )} + + setRegisterOpen(false)} @@ -232,7 +246,14 @@ export default function CollaborationsPage() { onClose={() => setInstallTarget(null)} /> - setDeleteTarget(null)}> + { + if (deleting) return; + setDeleteError(null); + setDeleteTarget(null); + }} + > Delete collaboration bridge @@ -241,9 +262,22 @@ export default function CollaborationsPage() { {deleteTarget?.room_count ?? 0} associated room {deleteTarget?.room_count === 1 ? "" : "s"} and external users. + {deleteError && ( + + {deleteError} + + )} - + + + + + ); +} diff --git a/gateway/src/pages/collaborations/InstalledAppsSection.tsx b/gateway/src/pages/collaborations/InstalledAppsSection.tsx new file mode 100644 index 000000000..61cd9ef5d --- /dev/null +++ b/gateway/src/pages/collaborations/InstalledAppsSection.tsx @@ -0,0 +1,235 @@ +import LinkOffOutlined from "@mui/icons-material/LinkOffOutlined"; +import { + Alert, + Box, + Button, + Chip, + CircularProgress, + IconButton, + Stack, + Tooltip, + Typography, +} from "@mui/material"; +import type { GridColDef } from "@mui/x-data-grid"; +import { useCallback, useMemo, useState } from "react"; +import DataTable from "../../components/DataTable"; +import { type InstalledApp, beginAppInstall } from "../../data/api"; +import { useInstallablePlatforms, useInstalledApps } from "../../data/hooks"; +import { EM_DASH, MONO_SX, formatDate, titleCase } from "../../theme/hootFormat"; +import DisconnectAppDialog from "./DisconnectAppDialog"; + +/** + * The other way a connection comes into being. Registering an app means + * creating one on the platform, choosing its scopes and pasting its token in + * here; installing means clicking a button and approving a consent screen, + * with this deployment's own app supplying the credentials. + * + * Both are shown because both exist, and an operator has to be able to tell + * which of their connections is which — a registered app's token is theirs to + * rotate and an installed one's is not, and only the second has to be removed + * through Disconnect. + * + * Renders nothing at all on a deployment that has no app of its own and has + * never had one, which is most of them: an empty section explaining a feature + * nobody here can use is worse than no section. + */ +type InstallRow = InstalledApp & { id: string }; + +const STATUS_COLOR: Record = { + active: "success", + // Ended at the platform rather than here — somebody removed the app, or its + // token was killed. Warned rather than greyed out, because unlike + // "disconnected" it is news. + revoked: "warning", + disconnected: "default", +}; + +interface Props { + isAdmin: boolean; + // Disconnecting removes the connection the install created, so the list of + // connections above this section is stale once it succeeds. + onConnectionsChanged: () => void; +} + +export default function InstalledAppsSection({ + isAdmin, + onConnectionsChanged, +}: Props) { + const { data: platforms } = useInstallablePlatforms(); + const { data: installs, loading, refetch } = useInstalledApps(); + const [starting, setStarting] = useState(null); + const [error, setError] = useState(null); + const [disconnectTarget, setDisconnectTarget] = useState( + null, + ); + + const handleInstall = useCallback(async (platform: string) => { + setStarting(platform); + setError(null); + try { + // A top-level navigation, not a popup and not an iframe: the platform's + // consent screen sets cookies on its own domain and refuses to be + // framed, and a popup opened after an await is what a browser blocks. + // `starting` is deliberately left set — the page is on its way out. + window.location.href = await beginAppInstall(platform); + } catch (e) { + setError(e instanceof Error ? e.message : "Failed to start the install"); + setStarting(null); + } + }, []); + + const handleDisconnected = useCallback(() => { + setDisconnectTarget(null); + refetch(); + onConnectionsChanged(); + }, [refetch, onConnectionsChanged]); + + const columns = useMemo[]>( + () => [ + { + field: "platform", + headerName: "Platform", + width: 130, + renderCell: ({ value }) => ( + + ), + }, + { + field: "external_workspace_id", + headerName: "Workspace", + width: 170, + renderCell: ({ value }) => ( + + {String(value)} + + ), + }, + { + field: "status", + headerName: "Status", + width: 140, + renderCell: ({ value }) => ( + + ), + }, + { + field: "scopes", + headerName: "Scopes", + flex: 1, + minWidth: 180, + // Verbatim, and in full on hover: a scope string that means nothing + // here is still the answer to why the platform refused a call. + renderCell: ({ value }) => ( + + + {String(value)} + + + ), + }, + { + field: "installed_at", + headerName: "Installed", + width: 130, + valueFormatter: (value) => formatDate(value as string), + }, + { + field: "ended_at", + headerName: "Ended", + width: 130, + valueFormatter: (value) => + value ? formatDate(value as string) : EM_DASH, + }, + ...(isAdmin + ? [ + { + field: "actions" as const, + headerName: "", + width: 70, + sortable: false, + filterable: false, + align: "right" as const, + renderCell: ({ row }: { row: InstallRow }) => + row.status === "active" ? ( + + setDisconnectTarget(row)} + > + + + + ) : null, + }, + ] + : []), + ], + [isAdmin], + ); + + const rows = useMemo(() => installs ?? [], [installs]); + + const installable = platforms ?? []; + if (installable.length === 0 && rows.length === 0) return null; + + return ( + + + Installed apps + {isAdmin && ( + + {installable.map((platform) => ( + + ))} + + )} + + + + Installing adds this Switch deployment's own app to your workspace. + The workspace grants it a token that Switch holds and you cannot see or + rotate, and the connection it creates receives messages over HTTPS + rather than the socket a registered app opens. Removing it has to go + through Disconnect, which revokes that token at the platform first. + + + {error && ( + setError(null)}> + {error} + + )} + + {loading ? ( + + ) : rows.length === 0 ? ( + + No workspaces yet. Use the button above to install the app into one. + + ) : ( + + )} + + setDisconnectTarget(null)} + onDisconnected={handleDisconnected} + /> + + ); +}