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/install.py b/core/switch_core/bridges/collaboration/install.py new file mode 100644 index 000000000..ff9e4039e --- /dev/null +++ b/core/switch_core/bridges/collaboration/install.py @@ -0,0 +1,220 @@ +"""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 ClassVar + +#: 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 `GATEWAY_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. + """ + + +@dataclass(frozen=True) +class InstallGrant: + """What the platform handed back when a workspace installed us. + + Deliberately three 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`. + + `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 + 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 + 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 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:`WebhookAuthenticityError` for a payload that names no + workspace: an event we cannot route is not an event we may guess at. + """ + + @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/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..cf63742ff --- /dev/null +++ b/core/switch_core/bridges/collaboration/slack/install.py @@ -0,0 +1,153 @@ +"""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 logging +from collections.abc import Mapping +from typing import ClassVar +from urllib.parse import 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 ( + InstallGrant, + MessagingAppInstaller, + MessagingInstallError, + WebhookAuthenticityError, +) + +logger = logging.getLogger(__name__) + +AUTHORIZE_URL = "https://slack.com/oauth/v2/authorize" + +#: 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, + bot_token=access_token, + scopes=response.get("scope") or "", + ) + + 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 workspace_of_event(self, payload: Mapping[str, object]) -> str: + workspace_id = payload.get("team_id") + if not isinstance(workspace_id, str) or not workspace_id: + raise WebhookAuthenticityError("Slack event names no workspace") + return workspace_id + + def connection_config(self, grant: InstallGrant) -> dict[str, object]: + return { + "bot_token": grant.bot_token, + "workspace_id": grant.external_workspace_id, + } diff --git a/core/switch_core/config.py b/core/switch_core/config.py index ab01ca1f4..93683bbd9 100644 --- a/core/switch_core/config.py +++ b/core/switch_core/config.py @@ -178,6 +178,22 @@ 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 + # 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. @@ -421,6 +437,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.gateway_public_url: + raise ValueError( + "A distributed Slack app is configured but GATEWAY_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..ddc33ca2d 100644 --- a/core/switch_core/db/models.py +++ b/core/switch_core/db/models.py @@ -1131,6 +1131,133 @@ 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, not per tenant**, 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. + + That constraint is also the one place a tenant learns something about + another: claiming a workspace somebody else already claimed 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. + + `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. + + `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__ = ( + UniqueConstraint( + "platform", + "external_workspace_id", + name="uq_messaging_installs_workspace", + ), + 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] = mapped_column(Text, nullable=False) + 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 + ) + + +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 + ) + + # ── Server-Side Connectors ──────────────────────────────────────────────────── diff --git a/core/switch_core/db/tenant_lookup.py b/core/switch_core/db/tenant_lookup.py index dcb3fbcfe..adcd78dfd 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,19 @@ 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. + 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 +174,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 +197,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 +229,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 +242,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 +252,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 +265,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 +274,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 +284,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 +300,22 @@ 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" + ), + 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. Unique by " + "constraint on exactly this pair, so it answers at most once." + ), + ), ) TENANT_LOOKUPS_BY_NAME: dict[str, TenantLookup] = { @@ -297,9 +331,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 +370,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 +415,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 +424,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 +504,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/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_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..aa7e6499a --- /dev/null +++ b/core/tests/switch_core/bridges/collaboration/test_slack_installer.py @@ -0,0 +1,299 @@ +"""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 time + +import pytest +from slack_sdk.web.async_client import AsyncWebClient + +from switch_core.bridges.collaboration.install import ( + InstallGrant, + MessagingInstallerRegistry, + MessagingInstallError, + WebhookAuthenticityError, + 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 TestWorkspaceOfEvent: + def test_it_reads_the_team_id(self, installer: SlackAppInstaller) -> None: + assert installer.workspace_of_event({"team_id": "T123"}) == "T123" + + @pytest.mark.parametrize("payload", [{}, {"team_id": ""}, {"team_id": 7}]) + def test_an_event_naming_no_workspace_is_refused( + self, installer: SlackAppInstaller, payload: dict + ) -> None: + """An event we cannot route is not an event to guess at. + + There is no sensible default here: picking any tenant would deliver a + stranger's message into somebody's rooms. + """ + with pytest.raises(WebhookAuthenticityError): + installer.workspace_of_event(payload) + + +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_three_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", + bot_token="xoxb-granted", + scopes="chat:write,commands", + ) + + 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 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.""" + rendered = installer.connection_config( + InstallGrant( + external_workspace_id="T123", + bot_token="xoxb-granted", + scopes="chat:write", + ) + ) + config = SlackConnectionConfig.model_validate( + {**rendered, "event_delivery": "webhook"} + ) + assert config.bot_token == "xoxb-granted" + assert config.workspace_id == "T123" + 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/test_messaging_install_claim.py b/core/tests/switch_core/db/test_messaging_install_claim.py new file mode 100644 index 000000000..1319744ae --- /dev/null +++ b/core/tests/switch_core/db/test_messaging_install_claim.py @@ -0,0 +1,151 @@ +"""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. +""" + +from __future__ import annotations + +import uuid + +import pytest +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import 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 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 + ) 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_lookup.py b/core/tests/switch_core/db/test_tenant_lookup.py index 70cb344bc..5e39f0bf9 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,14 @@ # 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", +} def _revision_module(revision: str) -> ModuleType: @@ -128,6 +133,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 +158,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 +235,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 +473,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 +685,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 +711,30 @@ 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. """ - 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] + module = _revision_module(revision) + assert getattr(module, f"CREATE_{name.upper()}") == create_lookup_ddl( + lookup + ), ( + f"revision {revision} would install {name} with different DDL " + "from the one db/tenant_lookup.py builds." + ) + assert getattr(module, 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 +790,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/test_config_slack_app.py b/core/tests/switch_core/test_config_slack_app.py new file mode 100644 index 000000000..5c9c18222 --- /dev/null +++ b/core/tests/switch_core/test_config_slack_app.py @@ -0,0 +1,63 @@ +"""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, gateway_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, gateway_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="GATEWAY_PUBLIC_URL"): + _config(**_APP) 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..3294bde14 --- /dev/null +++ b/docs/old/bridges/SLACK_DISTRIBUTED_APP.md @@ -0,0 +1,202 @@ +# 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. + +## 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 the public +origin of the deployment — the same value as `GATEWAY_PUBLIC_URL`, which is +already validated as scheme-and-host with no path and already serves the +deeplink redirect from the same application. + +| 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