From 45d1c664929fb3b4249d828444314505b91f4640 Mon Sep 17 00:00:00 2001 From: Petr Bauch Date: Fri, 11 Sep 2026 14:07:23 +0200 Subject: [PATCH 1/2] feat(db): messaging_installs and the lookup that routes a workspace to a tenant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The table behind the official messaging app: one row per external workspace a tenant has installed us into, holding the token that install granted. Slack is the first platform to use it; the shape is deliberately platform-agnostic because Teams, Discord and Telegram each need the same row. (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 and no way to choose. The database decides it, because a read-then-insert in application code cannot be made atomic. The cost is that a tenant claiming a claimed workspace learns it is claimed; the alternative is a silent second claim, discovered when a customer's messages arrive in somebody else's rooms. tenant_of_messaging_install is an addition to the closed set of SECURITY DEFINER functions that are exempt from row-level security, and is meant to be argued with rather than waved through. The argument: the webhook is unauthenticated by nature and holds nothing but a workspace id, what comes back 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. It is the first lookup to take two arguments, so TenantLookup carries a tuple of them and each bind is named after the argument it fills. 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. bridge_id is nullable: the install row is written before anything is built on it, and removing a bridge should not force the credential to be re-granted. Without the column the webhook would have to find its bridge by string-matching a workspace id inside JSON. encrypted_bot_token uses the same key as every other credential this schema stores, 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. Two frozen-copy comparisons needed the other direction added. 9c41a7b0e5d8 froze the lookups and 265ed188ad6f froze the scoped-table list, and both tests compared their copy against the live module exactly; a lookup or a table added afterwards is legitimately absent from them. _ADDED_SINCE and _POLICIED_SINCE name what was added and the revision that installs it, and each is paired with a test that the named revision really renders the same DDL — loosening either comparison to a subset check would pass just as happily for a function or a policy that exists in create_all and in no migration at all. Co-Authored-By: Claude Opus 5 --- core/switch_core/db/models.py | 74 +++++++++ core/switch_core/db/tenant_lookup.py | 127 ++++++++++----- .../c8a4e21f6d30_messaging_installs.py | 126 +++++++++++++++ .../db/test_messaging_install_claim.py | 151 ++++++++++++++++++ .../switch_core/db/test_row_level_security.py | 14 +- .../switch_core/db/test_tenant_lookup.py | 114 ++++++++++--- 6 files changed, 547 insertions(+), 59 deletions(-) create mode 100644 core/switch_core/migrations/versions/c8a4e21f6d30_messaging_installs.py create mode 100644 core/tests/switch_core/db/test_messaging_install_claim.py diff --git a/core/switch_core/db/models.py b/core/switch_core/db/models.py index f61bfb12f..2ecd40132 100644 --- a/core/switch_core/db/models.py +++ b/core/switch_core/db/models.py @@ -1131,6 +1131,80 @@ 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 + ) + + # ── 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/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..c6a1a6ca8 100644 --- a/core/tests/switch_core/db/test_row_level_security.py +++ b/core/tests/switch_core/db/test_row_level_security.py @@ -46,6 +46,8 @@ GLOBAL_TABLES, POLICY_NAME, REQUIRE_TENANT_FUNCTION_NAME, + create_policy_ddl, + enable_rls_ddl, scoped_tables, unscoped_tables, ) @@ -328,8 +330,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 +340,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." From 438ac75db9eef472dd9d65d2b49373d7d5d70a06 Mon Sep 17 00:00:00 2001 From: Petr Bauch Date: Fri, 11 Sep 2026 14:52:51 +0200 Subject: [PATCH 2/2] feat(bridges): install protocol for the distributed Slack app (CHOO-2626) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything an app install needs that does not depend on the public hostname, which is still undecided. The host appears only as `GATEWAY_PUBLIC_URL` joined to a path computed at runtime, and as the literal placeholder `HOST` in the walkthrough. The install layer is generic and the Slack implementation is the only one: `MessagingAppInstaller` is per deployment and holds *our* app's credentials, where an adapter is per bridge and holds a customer's. The seam between them is `connection_config` — an installer's last act renders the grant into the dict the adapter already takes, so the lifecycle service registers, validates and starts an installed bridge exactly as it does one an operator typed in. Registration is the feature flag: an installer exists when its credentials are configured, and the endpoints refuse when it does not. `messaging_install_states` records one in-flight install. The row is not what carries the tenant across — the signed state is — so the flow needs no ninth `SECURITY DEFINER` lookup and the closed list stays as short as it is. What the row adds is single use, which a signature cannot give: without it, replaying a captured state installs an attacker's workspace against the victim's tenant and injects messages into their rooms. Slack takes events two incompatible ways, so `SlackConnectionConfig` gains a hidden `event_delivery` discriminator and a validator refusing both half-states. A Socket Mode bridge with no app token would send fine and receive nothing — the shape of failure that reads as "Slack is quiet today" for a week. Two guards were tripped on purpose and updated deliberately: - `/messaging` joins the unauthenticated allowlist. Its callers are Slack and a browser mid-redirect, neither of which holds a credential of ours; nothing under it discloses a version, and every route proves the platform's signature before it acts. - `app_token` is no longer required by the Slack config schema, because a webhook bridge has none. The model validator, not the form, is what enforces it under Socket Mode. The walkthrough is checked against the code rather than trusted: a test parses the manifest out of the markdown and compares scopes, redirect, request URLs and every slash command against what the installer actually asks for and the paths this application actually serves. Co-Authored-By: Claude Opus 5 --- core/switch_core/bridges/agent/auth.py | 8 + .../bridges/collaboration/install.py | 220 +++++++++++++ .../bridges/collaboration/slack/adapter.py | 109 ++++++- .../bridges/collaboration/slack/install.py | 153 +++++++++ core/switch_core/config.py | 44 +++ core/switch_core/db/models.py | 53 ++++ .../d3f6b0c95a17_messaging_install_states.py | 87 +++++ .../agent/test_version_is_not_public.py | 6 + .../test_bridge_type_registry.py | 6 +- .../test_slack_distributed_app.py | 122 +++++++ .../collaboration/test_slack_installer.py | 299 ++++++++++++++++++ .../switch_core/db/test_row_level_security.py | 2 - .../switch_core/test_config_slack_app.py | 63 ++++ docs/old/bridges/README.md | 6 + docs/old/bridges/SLACK_DISTRIBUTED_APP.md | 202 ++++++++++++ docs/old/bridges/SLACK_SETUP.md | 5 + 16 files changed, 1365 insertions(+), 20 deletions(-) create mode 100644 core/switch_core/bridges/collaboration/install.py create mode 100644 core/switch_core/bridges/collaboration/slack/install.py create mode 100644 core/switch_core/migrations/versions/d3f6b0c95a17_messaging_install_states.py create mode 100644 core/tests/switch_core/bridges/collaboration/test_slack_distributed_app.py create mode 100644 core/tests/switch_core/bridges/collaboration/test_slack_installer.py create mode 100644 core/tests/switch_core/test_config_slack_app.py create mode 100644 docs/old/bridges/SLACK_DISTRIBUTED_APP.md 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 2ecd40132..ddc33ca2d 100644 --- a/core/switch_core/db/models.py +++ b/core/switch_core/db/models.py @@ -1205,6 +1205,59 @@ class MessagingInstall(TenantScoped, Base): ) +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/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_row_level_security.py b/core/tests/switch_core/db/test_row_level_security.py index c6a1a6ca8..0a3945552 100644 --- a/core/tests/switch_core/db/test_row_level_security.py +++ b/core/tests/switch_core/db/test_row_level_security.py @@ -46,8 +46,6 @@ GLOBAL_TABLES, POLICY_NAME, REQUIRE_TENANT_FUNCTION_NAME, - create_policy_ddl, - enable_rls_ddl, scoped_tables, unscoped_tables, ) 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