From a1d6d26a529a2911debaffe54f582cdd89266944 Mon Sep 17 00:00:00 2001 From: Wojciech Wojtyniak Date: Fri, 11 Sep 2026 14:39:37 -0700 Subject: [PATCH 1/2] feat(authz): split the deployment-operator bit from tenant-scoped admin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `users.role == "admin"` was a global bypass and `tenant_members.role` was written at provisioning and never read. So making the creator of a workspace its owner would have granted nothing, and anything that set the global bit would have made that person an administrator of every workspace they later joined. The global bit now means what it honestly is — a deployment operator, never granted by anything self-service. `Principal.is_admin` becomes "may administer the tenant this request is bound to": the operator bypass, or an owner/admin membership row in that tenant. An agent still inherits exactly its owner's power, now in the bound tenant rather than everywhere. Each of the nine admin gates was classified as deployment-wide or workspace-scoped rather than converted uniformly; the two that enumerate or create deployment users stay operator-only. Design: docs/old/multi-tenancy-phase2-tenants.md section 2. --- core/switch_core/authz.py | 28 +- .../bridges/agent/protocol/service.py | 21 +- core/switch_core/db/stores/user_store.py | 32 +++ core/switch_core/gateway/agents.py | 35 ++- core/switch_core/gateway/auth.py | 45 ++- core/switch_core/gateway/collaborations.py | 14 +- core/switch_core/gateway/connectors.py | 10 +- core/switch_core/gateway/documents.py | 23 +- core/switch_core/gateway/packages.py | 39 ++- core/switch_core/gateway/references.py | 35 ++- core/switch_core/gateway/room_groups.py | 3 +- core/switch_core/gateway/rooms.py | 18 +- core/switch_core/gateway/templates.py | 5 +- .../8ef6d4038ecc_operator_owns_tenant_zero.py | 93 +++++++ .../test_agent_tenant_scoped_admin.py | 211 +++++++++++++++ .../protocol/test_list_all_references.py | 2 + .../protocol/test_reference_type_parity.py | 7 +- .../agent/protocol/test_room_roster_authz.py | 2 + .../db/test_tenant_admin_role_split.py | 229 ++++++++++++++++ .../gateway/test_collaborations_authz.py | 29 +- .../gateway/test_reference_types_routes.py | 13 +- core/tests/switch_core/test_authz.py | 21 ++ ...est_migration_operator_owns_tenant_zero.py | 256 ++++++++++++++++++ core/tests/switch_core/test_rooms_yaml.py | 10 +- 24 files changed, 1094 insertions(+), 87 deletions(-) create mode 100644 core/switch_core/migrations/versions/8ef6d4038ecc_operator_owns_tenant_zero.py create mode 100644 core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py create mode 100644 core/tests/switch_core/db/test_tenant_admin_role_split.py create mode 100644 core/tests/switch_core/test_migration_operator_owns_tenant_zero.py diff --git a/core/switch_core/authz.py b/core/switch_core/authz.py index 9c65903f8..f21eb4947 100644 --- a/core/switch_core/authz.py +++ b/core/switch_core/authz.py @@ -8,8 +8,14 @@ - The subject of every decision is a **user** (`Principal`). Agent-initiated requests resolve to the agent's owner (see the protocol service's `_resolve_acting_identity`); an agent inherits exactly its owner's - permissions. - - `User.role == "admin"` is a global bypass. + permissions in the tenant the request is bound to — never more than the + owner holds there. + - `Principal.is_admin` means "may administer the tenant bound to this + request" — the deployment operator bypass (`User.role == "admin"`, global, + never granted by anything self-service) OR an `owner`/`admin` membership + row for this user in the bound tenant (`tenant_members.role`, granted per + tenant, e.g. by creating it). `administers_tenant` below computes it; a + caller with a database session should use that rather than re-deriving it. - Owned entities carry a nullable owner plus independent `read_visibility` and `write_visibility` ("public" | "private"). @@ -38,12 +44,30 @@ class Principal: `id` may be ``None`` for an agent with no owner; such a principal owns nothing and is limited to public access. + + `is_admin` is tenant-scoped, not global — see `administers_tenant`. """ id: str | None is_admin: bool +# `tenant_members.role` is a checked string, not an enum type (`db/models.py`). +TENANT_ADMIN_ROLES = ("owner", "admin") + + +def administers_tenant(*, is_operator: bool, tenant_role: str | None) -> bool: + """Whether a caller may administer the tenant they hold `tenant_role` in. + + True for a deployment operator (the global bypass), regardless of + membership — an operator administers every tenant. Otherwise true only if + the membership role itself is `owner` or `admin`; a plain `member`, or no + membership row at all, is false. This is the one function that should ever + turn a role into an admin bit — see `Principal.is_admin`. + """ + return is_operator or tenant_role in TENANT_ADMIN_ROLES + + @runtime_checkable class Authorizable(Protocol): """Structural interface for any entity `can` arbitrates over. diff --git a/core/switch_core/bridges/agent/protocol/service.py b/core/switch_core/bridges/agent/protocol/service.py index cac0e1859..14d87b6c9 100644 --- a/core/switch_core/bridges/agent/protocol/service.py +++ b/core/switch_core/bridges/agent/protocol/service.py @@ -2308,13 +2308,9 @@ async def create_moderation_room( ) async with self.session_factory() as session: - agent = await self.agent_store.get(session, agent_id) - if agent is None: - raise ValueError(f"Unknown agent: {agent_id}") - owner_is_admin = False - if agent.owner_id is not None: - owner = await session.get(User, agent.owner_id) - owner_is_admin = owner is not None and owner.role == "admin" + agent, _owner_id, owner_is_admin = await self._resolve_acting_identity( + session, agent_id + ) group_id = await self._resolve_group_name(session, group_name) if (reference_ids or package_ids) and agent.owner_id is None: raise ValueError( @@ -2463,6 +2459,13 @@ async def _resolve_acting_identity( Returns ``(agent, owner_id, owner_is_admin)``. Used by moderation methods that perform resource-access checks on the agent's behalf. + + ``owner_is_admin`` is the owner's tenant-scoped administrative bit + (``UserStore.administers``), not the global operator flag alone: the + agent must not gain more than its owner holds in the tenant this + request is bound to. `session` is already scoped there by the time an + agent-bridge request reaches this method, so the read is the owner's + membership in the same tenant the agent itself belongs to. """ agent = await self.agent_store.get(session, agent_id) if agent is None: @@ -2470,7 +2473,9 @@ async def _resolve_acting_identity( owner_is_admin = False if agent.owner_id is not None: owner = await session.get(User, agent.owner_id) - owner_is_admin = owner is not None and owner.role == "admin" + owner_is_admin = owner is not None and await self.user_store.administers( + session, owner + ) return agent, agent.owner_id, owner_is_admin async def _require_room_action( diff --git a/core/switch_core/db/stores/user_store.py b/core/switch_core/db/stores/user_store.py index a0feea59f..d1e4f6122 100644 --- a/core/switch_core/db/stores/user_store.py +++ b/core/switch_core/db/stores/user_store.py @@ -6,12 +6,14 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession +from switch_core.authz import administers_tenant from switch_core.db.models import ( OidcIdentity, TenantMember, User, require_tenant_id, ) +from switch_core.tenant_context import current_tenant_id logger = logging.getLogger(__name__) @@ -297,3 +299,33 @@ async def _link_identity( async def get_all(self, session: AsyncSession) -> list[User]: result = await session.execute(select(User)) return list(result.scalars().all()) + + async def tenant_role( + self, session: AsyncSession, tenant_id: str, user_id: str + ) -> str | None: + """`user_id`'s membership role in `tenant_id`, or None if not a member. + + `TenantMember` is addressed by its whole primary key, so this is a + plain `session.get` rather than a query — same shape as + `ensure_membership`'s write. + """ + membership = await session.get(TenantMember, (tenant_id, user_id)) + return membership.role if membership is not None else None + + async def administers(self, session: AsyncSession, user: User) -> bool: + """Whether `user` may administer the tenant bound to `session`'s + context — the operator bypass, or an owner/admin membership in it. + + See `authz.administers_tenant`, which this composes with a read of the + one membership row that can answer "in *this* tenant". No tenant bound + answers with the operator bit alone: every gateway request binds one + by the time this is reachable, and nothing here should insist on a + precondition that authentication already enforces. + """ + tenant_id = current_tenant_id() + role = ( + None + if tenant_id is None + else await self.tenant_role(session, tenant_id, user.id) + ) + return administers_tenant(is_operator=user.role == "admin", tenant_role=role) diff --git a/core/switch_core/gateway/agents.py b/core/switch_core/gateway/agents.py index e5316c2e5..b2d908939 100644 --- a/core/switch_core/gateway/agents.py +++ b/core/switch_core/gateway/agents.py @@ -79,7 +79,10 @@ async def delete_agent_by_name( if agent is None: raise HTTPException(status_code=404, detail=f"Agent not found: {agent_name}") try: - require_manage(Principal(user.id, user.role == "admin"), agent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + agent.owner_id, + ) except PermissionError: raise HTTPException( status_code=403, @@ -106,7 +109,10 @@ async def delete_agent( if agent is None: raise HTTPException(status_code=404, detail=f"Agent not found: {agent_id}") try: - require_manage(Principal(user.id, user.role == "admin"), agent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + agent.owner_id, + ) except PermissionError: raise HTTPException( status_code=403, @@ -216,7 +222,10 @@ async def register_known_subagents( status_code=404, detail=f"Parent agent not found: {req.parent_agent_id}" ) try: - require_manage(Principal(user.id, user.role == "admin"), parent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + parent.owner_id, + ) except PermissionError as exc: raise HTTPException( status_code=403, @@ -317,7 +326,10 @@ async def update_agent_options( raise HTTPException(status_code=404, detail=f"Agent not found: {agent_id}") try: - require_manage(Principal(user.id, user.role == "admin"), agent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + agent.owner_id, + ) except PermissionError: raise HTTPException( status_code=403, @@ -370,7 +382,10 @@ async def update_agent_icon( raise HTTPException(status_code=404, detail=f"Agent not found: {agent_id}") try: - require_manage(Principal(user.id, user.role == "admin"), agent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + agent.owner_id, + ) except PermissionError: raise HTTPException( status_code=403, @@ -420,7 +435,10 @@ async def update_agent_display_name( raise HTTPException(status_code=404, detail=f"Agent not found: {agent_id}") try: - require_manage(Principal(user.id, user.role == "admin"), agent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + agent.owner_id, + ) except PermissionError: raise HTTPException( status_code=403, @@ -509,7 +527,10 @@ async def update_addressing_policy( raise HTTPException(status_code=404, detail=f"Agent not found: {agent_id}") try: - require_manage(Principal(user.id, user.role == "admin"), agent.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + agent.owner_id, + ) except PermissionError: raise HTTPException( status_code=403, diff --git a/core/switch_core/gateway/auth.py b/core/switch_core/gateway/auth.py index 61e14a694..5e4b4b8f6 100644 --- a/core/switch_core/gateway/auth.py +++ b/core/switch_core/gateway/auth.py @@ -234,11 +234,53 @@ async def get_current_user( async def require_admin( user: Annotated[User, Depends(get_current_user)], ) -> User: + """Raise 403 unless `user` is a deployment operator. + + ``User.role == "admin"`` is deliberately global and deliberately not + self-service: it names the person who runs the server, not a role any + tenant can grant. Gate deployment-wide actions on this — creating or + listing every user in the deployment — never a single tenant's resources. + Those use ``require_tenant_admin``. + """ if user.role != "admin": raise HTTPException(status_code=403, detail="Admin access required") return user +async def get_tenant_is_admin( + session: Annotated[AsyncSession, Depends(get_session)], + user: Annotated[User, Depends(get_current_user)], + user_store: Annotated[UserStore, Depends(get_user_store)], +) -> bool: + """Whether `user` may administer the tenant this request is bound to. + + This is the boolean every ``Principal.is_admin`` in a gateway request + should be built from — see ``UserStore.administers`` for what it actually + checks. + + Also callable directly (not just as a FastAPI dependency) from any + handler or service function that already holds a bound `session`, the + caller's `User`, and a `UserStore`. + """ + return await user_store.administers(session, user) + + +async def require_tenant_admin( + user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], +) -> User: + """Raise 403 unless `user` may administer the tenant this request is bound to. + + Unlike ``require_admin``, this is granted by ``tenant_members.role`` as + well as the operator bit — use it for routes that manage one tenant's + resources (a collaboration bridge, a room) rather than the deployment + itself. + """ + if not is_admin: + raise HTTPException(status_code=403, detail="Tenant admin access required") + return user + + async def require_room_access( session: AsyncSession, room_store: RoomStore, @@ -257,8 +299,9 @@ async def require_room_access( room = await room_store.get(session, room_id) if room is None: raise HTTPException(status_code=404, detail="Room not found") + is_admin = await UserStore().administers(session, user) try: - require(Principal(user.id, user.role == "admin"), action, room) + require(Principal(user.id, is_admin), action, room) except PermissionError as e: raise HTTPException(status_code=403, detail=str(e)) from e return room diff --git a/core/switch_core/gateway/collaborations.py b/core/switch_core/gateway/collaborations.py index 243d5066c..a7ae7361f 100644 --- a/core/switch_core/gateway/collaborations.py +++ b/core/switch_core/gateway/collaborations.py @@ -21,7 +21,7 @@ from switch_core.db.stores.external_user_store import ExternalUserStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user, require_admin +from switch_core.gateway.auth import get_current_user, require_tenant_admin from switch_core.gateway.dependencies import ( get_bridge_store, get_collab_lifecycle, @@ -164,7 +164,7 @@ async def create_bridge( # Admin-only: a bridge is an unowned, workspace-wide integration holding # platform secrets, so there is no owner to scope to (unlike connectors, # whose authz is owner-or-admin) — registering one is an admin action. - _user: Annotated[User, Depends(require_admin)], + _user: Annotated[User, Depends(require_tenant_admin)], ) -> BridgeDetail: try: bridge = await collab_lifecycle.register( @@ -202,7 +202,7 @@ async def set_default_bridge( collab_lifecycle: Annotated[ CollaborationBridgeLifecycleService, Depends(get_collab_lifecycle) ], - _user: Annotated[User, Depends(require_admin)], + _user: Annotated[User, Depends(require_tenant_admin)], ) -> BridgeDetail: """Nominate a bridge as the instance default, demoting the previous one.""" try: @@ -253,7 +253,7 @@ async def update_bridge( ], # Admin-only for the same reason as registering one: a bridge is an unowned, # workspace-wide integration, so there is no owner to scope mutation to. - _user: Annotated[User, Depends(require_admin)], + _user: Annotated[User, Depends(require_tenant_admin)], ) -> BridgeDetail: bridge = await bridge_store.get(session, bridge_id) if bridge is None: @@ -548,7 +548,7 @@ async def claim_bridge_identity( raise HTTPException(status_code=404, detail="Bridge not found") target_user_id = payload.user_id or user.id - if target_user_id != user.id and user.role != "admin": + if target_user_id != user.id and not await user_store.administers(session, user): raise HTTPException( status_code=403, detail="Only an admin may claim a messaging identity for another user", @@ -644,7 +644,7 @@ async def release_bridge_identity( raise HTTPException(status_code=404, detail="Identity not found") target_user_id = user_id or user.id - if target_user_id != user.id and user.role != "admin": + if target_user_id != user.id and not await user_store.administers(session, user): raise HTTPException( status_code=403, detail="Only an admin may release another user's messaging identity", @@ -676,7 +676,7 @@ async def delete_bridge( ], # Admin-only: deleting a bridge cascades into deleting every room on it, so # this is the most destructive operation on the router. - _user: Annotated[User, Depends(require_admin)], + _user: Annotated[User, Depends(require_tenant_admin)], ) -> dict[str, bool]: bridge = await bridge_store.get(session, bridge_id) if bridge is None: diff --git a/core/switch_core/gateway/connectors.py b/core/switch_core/gateway/connectors.py index 94b186edf..f21f7ea8f 100644 --- a/core/switch_core/gateway/connectors.py +++ b/core/switch_core/gateway/connectors.py @@ -12,6 +12,7 @@ from switch_core.db.models import User from switch_core.db.stores.api_key_store import ApiKeyStore from switch_core.db.stores.server_connector_store import ServerConnectorStore +from switch_core.db.stores.user_store import UserStore from switch_core.gateway.auth import get_current_user from switch_core.gateway.dependencies import ( get_api_key_store, @@ -137,10 +138,11 @@ async def delete_connector( raise HTTPException(status_code=404, detail="Connector not found") reg_key = await api_key_store.get(session, record.api_key_id) - if (reg_key is None or reg_key.user_id != user.id) and user.role != "admin": - raise HTTPException( - status_code=403, detail="Not authorized to delete this connector" - ) + if reg_key is None or reg_key.user_id != user.id: + if not await UserStore().administers(session, user): + raise HTTPException( + status_code=403, detail="Not authorized to delete this connector" + ) try: await connector_lifecycle.remove(connector_id) diff --git a/core/switch_core/gateway/documents.py b/core/switch_core/gateway/documents.py index 8a6301244..998b7ca17 100644 --- a/core/switch_core/gateway/documents.py +++ b/core/switch_core/gateway/documents.py @@ -10,7 +10,11 @@ from switch_core.db.stores.agent_store import AgentStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user, require_room_access +from switch_core.gateway.auth import ( + get_current_user, + get_tenant_is_admin, + require_room_access, +) from switch_core.gateway.dependencies import ( get_agent_store, get_resource_service, @@ -200,10 +204,11 @@ async def get_document( user_store: Annotated[UserStore, Depends(get_user_store)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> DocumentDetail: try: doc = await resource_service.get_document_for_user( - session, document_id, user.id, is_admin=user.role == "admin" + session, document_id, user.id, is_admin=is_admin ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -221,13 +226,14 @@ async def patch_document( user_store: Annotated[UserStore, Depends(get_user_store)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> DocumentDetail: try: doc = await resource_service.update_document( session, document_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=is_admin, name=req.name, description=req.description, instructions=req.instructions, @@ -249,6 +255,7 @@ async def delete_document( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> DocumentDeleteResponse: affected_packages = await resource_service.list_packages_for_document( session, document_id @@ -258,7 +265,7 @@ async def delete_document( session, document_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -278,10 +285,11 @@ async def list_rooms_for_document( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ResourceRoom]: try: await resource_service.get_document_for_user( - session, document_id, user.id, is_admin=user.role == "admin" + session, document_id, user.id, is_admin=is_admin ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -318,6 +326,7 @@ async def attach_document_to_room( agent_store: Annotated[AgentStore, Depends(get_agent_store)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> DocumentDetail: await require_room_access(session, room_store, room_id, user, "write") try: @@ -326,7 +335,7 @@ async def attach_document_to_room( room_id, document_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -334,7 +343,7 @@ async def attach_document_to_room( raise HTTPException(status_code=403, detail=str(e)) from e await session.commit() doc = await resource_service.get_document_for_user( - session, document_id, user.id, is_admin=user.role == "admin" + session, document_id, user.id, is_admin=is_admin ) return await _enrich_detail(session, doc, resource_service, user_store, agent_store) diff --git a/core/switch_core/gateway/packages.py b/core/switch_core/gateway/packages.py index 0ae22d56b..ed3cdc9b2 100644 --- a/core/switch_core/gateway/packages.py +++ b/core/switch_core/gateway/packages.py @@ -143,7 +143,10 @@ async def get_package( ) -> PackageDetail: try: pkg = await resource_service.get_package_for_user( - session, package_id, user.id, is_admin=user.role == "admin" + session, + package_id, + user.id, + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -166,7 +169,7 @@ async def patch_package( session, package_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), name=req.name, description=req.description, instructions=req.instructions, @@ -193,7 +196,7 @@ async def delete_package( session, package_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -212,7 +215,10 @@ async def list_rooms_for_package( ) -> list[ResourceRoom]: try: await resource_service.get_package_for_user( - session, package_id, user.id, is_admin=user.role == "admin" + session, + package_id, + user.id, + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -235,7 +241,10 @@ async def list_package_references( ) -> list[ReferenceDetail]: try: await resource_service.get_package_for_user( - session, package_id, user.id, is_admin=user.role == "admin" + session, + package_id, + user.id, + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -259,7 +268,7 @@ async def add_reference_to_package( package_id, reference_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -282,7 +291,7 @@ async def remove_reference_from_package( package_id, reference_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -308,7 +317,10 @@ async def list_package_documents( ) -> list[DocumentSummary]: try: await resource_service.get_package_for_user( - session, package_id, user.id, is_admin=user.role == "admin" + session, + package_id, + user.id, + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -334,7 +346,7 @@ async def add_document_to_package( package_id, document_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -357,7 +369,7 @@ async def remove_document_from_package( package_id, document_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -406,7 +418,7 @@ async def attach_package_to_room( room_id, package_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await UserStore().administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -414,7 +426,10 @@ async def attach_package_to_room( raise HTTPException(status_code=403, detail=str(e)) from e await session.commit() pkg = await resource_service.get_package_for_user( - session, package_id, user.id, is_admin=user.role == "admin" + session, + package_id, + user.id, + is_admin=await UserStore().administers(session, user), ) return await _enrich_one(session, pkg, resource_service, user_store) diff --git a/core/switch_core/gateway/references.py b/core/switch_core/gateway/references.py index 9b5e0979a..7d80b3fa2 100644 --- a/core/switch_core/gateway/references.py +++ b/core/switch_core/gateway/references.py @@ -167,7 +167,7 @@ async def list_reference_types( user: Annotated[User, Depends(get_current_user)], ) -> list[ReferenceTypeInfo]: views = await resource_service.list_reference_types_for_principal( - session, user_id=user.id, is_admin=user.role == "admin" + session, user_id=user.id, is_admin=await user_store.administers(session, user) ) owners = await _owner_names( session, user_store, {v.owner_id for v in views if v.owner_id is not None} @@ -186,7 +186,7 @@ async def list_owned_reference_types( user: Annotated[User, Depends(get_current_user)], ) -> list[ReferenceTypeDetail]: rows = await resource_service.list_owned_reference_types( - session, user_id=user.id, is_admin=user.role == "admin" + session, user_id=user.id, is_admin=await user_store.administers(session, user) ) owners = await _owner_names(session, user_store, {e.row.owner_id for e in rows}) return [_type_to_detail(e, owners.get(e.row.owner_id)) for e in rows] @@ -235,7 +235,7 @@ async def patch_reference_type( session, type, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await user_store.administers(session, user), display_name=req.display_name, instructions=req.instructions, value_hint=req.value_hint, @@ -259,11 +259,15 @@ async def delete_reference_type( type: str, session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], + user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], ) -> ReferenceTypeDeleteResponse: try: await resource_service.delete_reference_type( - session, type, user_id=user.id, is_admin=user.role == "admin" + session, + type, + user_id=user.id, + is_admin=await user_store.administers(session, user), ) except ReferenceTypeInUseError as e: raise HTTPException(status_code=409, detail=str(e)) from e @@ -298,7 +302,7 @@ async def create_reference( ref = await resource_service.create_reference( session, owner_id=user.id, - is_admin=user.role == "admin", + is_admin=await user_store.administers(session, user), read_visibility=req.read_visibility, write_visibility=req.write_visibility, type=req.type, @@ -323,7 +327,10 @@ async def get_reference( ) -> ReferenceDetail: try: ref = await resource_service.get_reference_for_user( - session, reference_id, user.id, is_admin=user.role == "admin" + session, + reference_id, + user.id, + is_admin=await user_store.administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -346,7 +353,7 @@ async def patch_reference( session, reference_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await user_store.administers(session, user), name=req.name, description=req.description, instructions=req.instructions, @@ -367,6 +374,7 @@ async def delete_reference( reference_id: str, session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], + user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], ) -> ReferenceDeleteResponse: affected_packages = await resource_service.list_packages_for_reference( @@ -377,7 +385,7 @@ async def delete_reference( session, reference_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=await user_store.administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -396,11 +404,15 @@ async def list_rooms_for_reference( reference_id: str, session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], + user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], ) -> list[ResourceRoom]: try: await resource_service.get_reference_for_user( - session, reference_id, user.id, is_admin=user.role == "admin" + session, + reference_id, + user.id, + is_admin=await user_store.administers(session, user), ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -436,12 +448,13 @@ async def attach_reference_to_room( ) -> ReferenceDetail: await require_room_access(session, room_store, room_id, user, "write") try: + is_admin = await user_store.administers(session, user) await resource_service.attach_reference_to_room( session, room_id, reference_id, user_id=user.id, - is_admin=user.role == "admin", + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -449,7 +462,7 @@ async def attach_reference_to_room( raise HTTPException(status_code=403, detail=str(e)) from e await session.commit() ref = await resource_service.get_reference_for_user( - session, reference_id, user.id, is_admin=user.role == "admin" + session, reference_id, user.id, is_admin=is_admin ) return await _enrich_one(session, ref, resource_service, user_store) diff --git a/core/switch_core/gateway/room_groups.py b/core/switch_core/gateway/room_groups.py index 70a3ca729..6d922673b 100644 --- a/core/switch_core/gateway/room_groups.py +++ b/core/switch_core/gateway/room_groups.py @@ -9,6 +9,7 @@ from switch_core.db.models import RoomGroup, User from switch_core.db.stores.room_group_store import RoomGroupStore from switch_core.db.stores.room_store import RoomStore +from switch_core.db.stores.user_store import UserStore from switch_core.gateway.auth import get_current_user from switch_core.gateway.dependencies import ( get_room_group_store, @@ -115,7 +116,7 @@ async def assign_rooms_to_group( if await room_group_store.get(session, group_id) is None: raise HTTPException(status_code=404, detail="Room group not found") - principal = Principal(user.id, user.role == "admin") + principal = Principal(user.id, await UserStore().administers(session, user)) forbidden: list[str] = [] allowed: list[str] = [] for room_id in req.room_ids: diff --git a/core/switch_core/gateway/rooms.py b/core/switch_core/gateway/rooms.py index 061b6faeb..5cdfad042 100644 --- a/core/switch_core/gateway/rooms.py +++ b/core/switch_core/gateway/rooms.py @@ -72,7 +72,11 @@ async def _require_room( if room is None: raise HTTPException(status_code=404, detail="Room not found") try: - require(Principal(user.id, user.role == "admin"), action, room) + require( + Principal(user.id, await UserStore().administers(session, user)), + action, + room, + ) except PermissionError as e: raise HTTPException(status_code=403, detail=str(e)) from e return room @@ -244,7 +248,7 @@ async def list_rooms( rooms = await room_store.list_readable( session, user.id, - is_admin=user.role == "admin", + is_admin=await user_store.administers(session, user), include_archived=include_archived, ) @@ -371,7 +375,9 @@ async def create_room( @router.post("/from-yaml", status_code=201) async def create_room_from_yaml( request: Request, + session: Annotated[AsyncSession, Depends(get_session)], rooms_yaml: Annotated[RoomYamlService, Depends(get_room_yaml_service)], + user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], ) -> ProvisionResult: """Provision a single room and its attachments from a YAML spec. @@ -400,7 +406,7 @@ async def create_room_from_yaml( inputs = None spec = rooms_yaml.parse(text, inputs=inputs) return await rooms_yaml.provision( - spec, user_id=user.id, is_admin=user.role == "admin" + spec, user_id=user.id, is_admin=await user_store.administers(session, user) ) except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) from e @@ -847,9 +853,10 @@ async def bulk_delete_rooms( session: Annotated[AsyncSession, Depends(get_session)], room_service: Annotated[RoomService, Depends(get_room_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], + user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], ) -> BulkDeleteResponse: - principal = Principal(user.id, user.role == "admin") + principal = Principal(user.id, await user_store.administers(session, user)) deleted = 0 for room_id in req.room_ids: room = await room_store.get(session, room_id) @@ -872,12 +879,13 @@ async def bulk_archive_rooms( session: Annotated[AsyncSession, Depends(get_session)], room_service: Annotated[RoomService, Depends(get_room_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], + user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], ) -> BulkArchiveResponse: """Archive or unarchive many rooms at once. Each room is authorized individually with the same `write` check as the single archive endpoint; rooms the caller cannot write (or that no longer exist) are skipped.""" - principal = Principal(user.id, user.role == "admin") + principal = Principal(user.id, await user_store.administers(session, user)) updated = 0 for room_id in req.room_ids: room = await room_store.get(session, room_id) diff --git a/core/switch_core/gateway/templates.py b/core/switch_core/gateway/templates.py index 43ac1abd5..60c4917c9 100644 --- a/core/switch_core/gateway/templates.py +++ b/core/switch_core/gateway/templates.py @@ -164,7 +164,10 @@ async def _load_for_management( status_code=404, detail=f"Template not found: {template_id}" ) try: - require_manage(Principal(user.id, user.role == "admin"), template.owner_id) + require_manage( + Principal(user.id, await UserStore().administers(session, user)), + template.owner_id, + ) except PermissionError as e: raise HTTPException(status_code=403, detail=str(e)) from e return template diff --git a/core/switch_core/migrations/versions/8ef6d4038ecc_operator_owns_tenant_zero.py b/core/switch_core/migrations/versions/8ef6d4038ecc_operator_owns_tenant_zero.py new file mode 100644 index 000000000..ac4150c07 --- /dev/null +++ b/core/switch_core/migrations/versions/8ef6d4038ecc_operator_owns_tenant_zero.py @@ -0,0 +1,93 @@ +"""operator/workspace-admin role split: the seeded administrator becomes an owner of tenant zero + +Revision ID: 8ef6d4038ecc +Revises: a3f61c02d5be +Create Date: 2026-09-11 00:00:00.000000 + +Phase 2 of multi-tenancy (`docs/old/multi-tenancy-phase2-tenants.md`, §2) +starts reading `tenant_members.role` instead of leaving it inert: +`authz.Principal.is_admin` now grants tenant-scoped administrative power to a +caller whose membership role in the bound tenant is `owner`/`admin`, not only +to the deployment operator bit (`users.role == "admin"`, still global, still +an unconditional bypass, still never self-service). + +That bypass is exactly why this migration is not a lockout fix. An operator +administers every tenant regardless of what `tenant_members` says about them, +before this change and after it — nothing here can strand an operator out of +their own deployment, because nothing here is what lets them in. What this +migration fixes is a *label*: a deployment whose operator's own membership +row still says `member` would show that operator as a guest of the tenant +they administer, in any future member listing or ownership check that reads +`tenant_members` instead of going through `authz`. Correcting it now, once, +means that surface can be built later without also auditing every deployment +that upgraded before it existed. + +Two ways an operator's row can already disagree with `owner`: + +- The Phase 1 schema migration (`8b276792ee30`) backfilled `owner` for every + user whose `role` was `admin` at the time it ran, and `member` for everyone + else. It cannot see a promotion that happened after. +- `UserStore.ensure_membership` writes a membership exactly once, at account + creation, deriving its role from `users.role` at that moment. Promoting an + account to `admin` afterwards — a direct database edit, since there is no + gateway endpoint for it (`multi-tenancy-phase2-tenants.md` §8) — never + revisits the row that promotion left behind. + +So every user with `users.role = 'admin'` gets an `owner` row in tenant zero: +updated in place if their row there is `member`, inserted if they have no row +there at all. The `INSERT` covers a state the two paths above should not be +able to produce today (`ensure_membership` runs inside `UserStore.create`, +which never leaves an account without one) but which nothing enforces at the +database level — cheaper to close here than to assume. + +Nothing else moves. A non-operator's row is left exactly as it is, matching +this phase's stated invariant that anyone with a single membership and no +operator bit sees no behavioural change from this release. Only tenant zero +is touched: it is the only tenant that can exist in a deployment old enough +to need this migration at all. + +Idempotent: re-running it updates nothing the first run did not already fix, +and inserts nothing that already exists. +""" + +from collections.abc import Sequence + +from alembic import op + +revision: str = "8ef6d4038ecc" +down_revision: str | None = "a3f61c02d5be" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +TENANT_ZERO_ID = "00000000-0000-0000-0000-000000000000" + + +def upgrade() -> None: + op.execute( + f""" + UPDATE tenant_members + SET role = 'owner' + WHERE tenant_id = '{TENANT_ZERO_ID}' + AND role NOT IN ('owner', 'admin') + AND user_id IN (SELECT id FROM users WHERE role = 'admin') + """ + ) + op.execute( + f""" + INSERT INTO tenant_members (tenant_id, user_id, role) + SELECT '{TENANT_ZERO_ID}', u.id, 'owner' + FROM users u + WHERE u.role = 'admin' + AND NOT EXISTS ( + SELECT 1 FROM tenant_members tm + WHERE tm.tenant_id = '{TENANT_ZERO_ID}' AND tm.user_id = u.id + ) + """ + ) + + +def downgrade() -> None: + # Not reversible: the rows this corrected were indistinguishable from an + # ordinary `owner` row the moment it ran, and rows it inserted have no + # prior state to restore — there was no row there before. + pass diff --git a/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py b/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py new file mode 100644 index 000000000..c684b3d6f --- /dev/null +++ b/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py @@ -0,0 +1,211 @@ +"""An agent inherits exactly its owner's tenant-scoped power, never more +(CHOO-2726 role split, phase 2 design §2). + +``_resolve_acting_identity`` used to compute ``owner_is_admin`` from +``owner.role == "admin"`` alone — the same global bit every other admin check +in the codebase used before this split. Once a person can hold ``owner`` in +one tenant and merely ``member`` (or nothing) in another, that global read is +wrong for an agent the same way it was wrong for a human acting directly: an +agent must not administer a tenant its owner does not administer, and must +administer one its owner does, purely because of the owner's role in *that* +tenant rather than any global bit. + +``test_agent_administers_the_tenant_its_owner_owns`` is the one that actually +distinguishes old from new: the owner here holds no global admin bit at all, +so the pre-split code (`owner.role == "admin"`) denies it regardless of the +`tenant_members` row, and only reading that row grants it. The other two +tests hold under both the old and new code — they pin down that the fix does +not *overreach* (a workspace owner's agent must not gain cross-tenant power, +and a global operator's agent must not lose its bypass) rather than proving a +new grant. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +import pytest +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from switch_core.bridges.agent.protocol.service import ProtocolService +from switch_core.bridges.agent.protocol.types import ( + IntegrationProfile, + TaskProtocolConfig, +) +from switch_core.db.models import ( + TENANT_ZERO_ID, + Client, + Room, + Tenant, + TenantMember, + User, +) +from switch_core.db.stores.agent_store import AgentStore +from switch_core.db.stores.api_key_store import ApiKeyStore +from switch_core.db.stores.room_store import RoomStore +from switch_core.db.stores.user_store import UserStore +from switch_core.tenant_context import tenant_scope + +TENANT_B = "tenant-b" + +_PROFILE = IntegrationProfile( + connection_model="session_passive", + message_exchange=True, + pre_invocation_mediation=[], + post_invocation_mediation=[], + event_reporting=[], + task_protocol=TaskProtocolConfig(can_delegate=False, can_accept=False), +) + + +class _FakeClientLifecycle: + def __init__(self, session_factory: async_sessionmaker[AsyncSession]) -> None: + self._session_factory = session_factory + + async def create_client(self, *, client_type: str, display_name: str) -> Client: + async with self._session_factory() as session: + client = Client( + matrix_user_id=f"@{display_name}:test", + display_name=display_name, + type=client_type, + ) + session.add(client) + await session.commit() + return client + + def start_client(self, client: Client) -> None: + pass + + +class _NoBridges: + def all_bridges(self) -> list[object]: + return [] + + +def _service(session_factory: async_sessionmaker[AsyncSession]) -> ProtocolService: + svc = object.__new__(ProtocolService) + svc.session_factory = session_factory # type: ignore[attr-defined] + svc.agent_store = AgentStore() # type: ignore[attr-defined] + svc.api_key_store = ApiKeyStore() # type: ignore[attr-defined] + svc.room_store = RoomStore() # type: ignore[attr-defined] + svc.user_store = UserStore() # type: ignore[attr-defined] + svc.client_lifecycle = _FakeClientLifecycle(session_factory) # type: ignore[attr-defined] + svc.collab_lifecycle = _NoBridges() # type: ignore[attr-defined] + svc.config = SimpleNamespace(jwt_secret_key="test-secret") # type: ignore[attr-defined] + return svc + + +async def _make_user( + session_factory: async_sessionmaker[AsyncSession], name: str, *, role: str = "user" +) -> str: + async with session_factory() as session: + user = User(name=name, email=f"{name}@test", role=role, password_hash="x") + session.add(user) + await session.commit() + return user.id + + +async def _make_tenant( + session_factory: async_sessionmaker[AsyncSession], tenant_id: str +) -> None: + async with session_factory() as session: + session.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id)) + await session.commit() + + +async def _membership( + session_factory: async_sessionmaker[AsyncSession], + tenant_id: str, + user_id: str, + role: str, +) -> None: + async with session_factory() as session: + session.add(TenantMember(tenant_id=tenant_id, user_id=user_id, role=role)) + await session.commit() + + +async def _register(svc: ProtocolService, name: str, owner_id: str) -> str: + result = await svc.register_agent( + name=name, + description=f"{name} desc", + connector_type="test", + integration_profile=_PROFILE, + owner_id=owner_id, + ) + return result.agent_id + + +async def _private_room( + session_factory: async_sessionmaker[AsyncSession], tenant_id: str, owner_id: str +) -> str: + async with session_factory() as session: + room = Room( + tenant_id=tenant_id, + matrix_room_id=f"!{tenant_id}-private:test", + name="private", + description="", + owner_id=owner_id, + read_visibility="private", + write_visibility="private", + ) + session.add(room) + await session.commit() + return room.id + + +class TestAgentInheritsOwnerTenantScopedAdmin: + async def test_agent_administers_the_tenant_its_owner_owns( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + """The owner holds no global admin bit — only `owner` in the bound + tenant. Only reading `tenant_members` (not `users.role`) can grant + this, so this is the one test that fails against the pre-split code. + """ + svc = _service(session_factory) + owner = await _make_user(session_factory, "workspace-owner") + await _membership(session_factory, TENANT_ZERO_ID, owner, "owner") + someone_else = await _make_user(session_factory, "tenant-zero-room-owner") + agent_id = await _register(svc, "owner-agent", owner) + room_id = await _private_room(session_factory, TENANT_ZERO_ID, someone_else) + + async with session_factory() as session: + room = await svc._require_room_action(session, agent_id, room_id, "write") + assert room.id == room_id + + async def test_agent_cannot_administer_a_tenant_where_owner_is_only_a_member( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + """Same owner, same agent: `owner` in tenant zero grants nothing in + tenant B, where that owner only holds `member`.""" + svc = _service(session_factory) + await _make_tenant(session_factory, TENANT_B) + owner = await _make_user(session_factory, "cross-tenant-owner") + await _membership(session_factory, TENANT_ZERO_ID, owner, "owner") + await _membership(session_factory, TENANT_B, owner, "member") + someone_else = await _make_user(session_factory, "tenant-b-room-owner") + agent_id = await _register(svc, "cross-tenant-agent", owner) + room_id = await _private_room(session_factory, TENANT_B, someone_else) + + async with session_factory() as session: + with tenant_scope(TENANT_B), pytest.raises(PermissionError): + await svc._require_room_action(session, agent_id, room_id, "write") + + async def test_operator_owned_agent_administers_every_tenant( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + """The deployment-operator bypass is unconditional and still applies + through an agent, in a tenant the operator holds no membership in at + all.""" + svc = _service(session_factory) + await _make_tenant(session_factory, TENANT_B) + operator = await _make_user(session_factory, "operator", role="admin") + someone_else = await _make_user(session_factory, "tenant-b-room-owner-2") + agent_id = await _register(svc, "operator-agent", operator) + room_id = await _private_room(session_factory, TENANT_B, someone_else) + + async with session_factory() as session: + with tenant_scope(TENANT_B): + room = await svc._require_room_action( + session, agent_id, room_id, "write" + ) + assert room.id == room_id diff --git a/core/tests/switch_core/bridges/agent/protocol/test_list_all_references.py b/core/tests/switch_core/bridges/agent/protocol/test_list_all_references.py index 592a1a080..28a9aca9e 100644 --- a/core/tests/switch_core/bridges/agent/protocol/test_list_all_references.py +++ b/core/tests/switch_core/bridges/agent/protocol/test_list_all_references.py @@ -26,6 +26,7 @@ from switch_core.db.stores.reference_store import ReferenceStore from switch_core.db.stores.reference_type_store import ReferenceTypeStore from switch_core.db.stores.room_link_store import RoomLinkStore +from switch_core.db.stores.user_store import UserStore T_OLD = datetime(2024, 1, 1, 12, 0, tzinfo=UTC) @@ -50,6 +51,7 @@ def _build_service( svc.connections = ConnectionRegistry() svc.session_factory = session_factory # type: ignore[assignment] svc.agent_store = _FakeAgentStore(owners) # type: ignore[assignment] + svc.user_store = UserStore() # type: ignore[assignment] svc.resource_service = ResourceService( reference_store=ReferenceStore(), reference_type_store=ReferenceTypeStore(), diff --git a/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py b/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py index c79dd480e..7ed73cae0 100644 --- a/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py +++ b/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py @@ -5,10 +5,10 @@ not the other is a drift bug that no other test would catch. `ProtocolService.__init__` takes 17 required collaborators, so the service is -built with `object.__new__` and the three attributes this method touches, per +built with `object.__new__` and the four attributes this method touches, per `test_agent_detail_mcp_tools.py`. `_resolve_acting_identity` loads the owner -with `session.get(User, ...)` on the live session rather than through -`user_store`, so no `user_store` is needed here. +with `session.get(User, ...)` on the live session, then reads +`user_store.administers` for the owner's tenant-scoped admin bit. """ from __future__ import annotations @@ -52,6 +52,7 @@ def _protocol_service( svc.session_factory = session_factory # type: ignore[attr-defined] svc.agent_store = AgentStore() # type: ignore[attr-defined] svc.resource_service = resource_service # type: ignore[attr-defined] + svc.user_store = _USER_STORE # type: ignore[attr-defined] return svc diff --git a/core/tests/switch_core/bridges/agent/protocol/test_room_roster_authz.py b/core/tests/switch_core/bridges/agent/protocol/test_room_roster_authz.py index d25f710ee..3ef918b1f 100644 --- a/core/tests/switch_core/bridges/agent/protocol/test_room_roster_authz.py +++ b/core/tests/switch_core/bridges/agent/protocol/test_room_roster_authz.py @@ -24,6 +24,7 @@ from switch_core.db.stores.agent_store import AgentStore from switch_core.db.stores.api_key_store import ApiKeyStore from switch_core.db.stores.room_store import RoomStore +from switch_core.db.stores.user_store import UserStore _PROFILE = IntegrationProfile( connection_model="session_passive", @@ -65,6 +66,7 @@ def _service(session_factory: async_sessionmaker[AsyncSession]) -> ProtocolServi svc.agent_store = AgentStore() # type: ignore[attr-defined] svc.api_key_store = ApiKeyStore() # type: ignore[attr-defined] svc.room_store = RoomStore() # type: ignore[attr-defined] + svc.user_store = UserStore() # type: ignore[attr-defined] svc.client_lifecycle = _FakeClientLifecycle(session_factory) # type: ignore[attr-defined] svc.collab_lifecycle = _NoBridges() # type: ignore[attr-defined] svc.config = SimpleNamespace(jwt_secret_key="test-secret") # type: ignore[attr-defined] diff --git a/core/tests/switch_core/db/test_tenant_admin_role_split.py b/core/tests/switch_core/db/test_tenant_admin_role_split.py new file mode 100644 index 000000000..c76cabcc1 --- /dev/null +++ b/core/tests/switch_core/db/test_tenant_admin_role_split.py @@ -0,0 +1,229 @@ +"""The operator/workspace-admin role split (CHOO-2726, phase 2 design §2). + +Before this change, `authz.Principal.is_admin` came from `users.role == +"admin"` alone — a global bit that `tenant_members.role` never fed into. So a +workspace's `owner` membership row granted nothing: every admin-gated route +checked the global bit, never the caller's role in the tenant the request is +bound to. + +These tests pin the four behavioural claims that split makes true: + +- a workspace `owner` can administer resources in *their own* tenant; +- the same person has no such power in a tenant they only hold a `member` + row in — `UserStore.administers` must read the *bound* tenant's row, not + any row belonging to the caller; +- a deployment operator (`users.role == "admin"`) keeps its unconditional + bypass regardless of what `tenant_members` says, in every tenant, with or + without a membership row at all; +- a plain `member` with no operator bit gains nothing. + +Each is proven both at the pure-decision level (`authz.administers_tenant`, +covered in `test_authz.py`) and here, against a real Postgres session and a +real `tenant_members` row, through `UserStore.administers` — the one place +that turns "who is this" plus "what tenant is bound" into that bit — and +through `authz.can` on a real `Room`, so the wiring from a membership row to +an actual resource decision is exercised end to end, not just the boolean. +""" + +from __future__ import annotations + +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from switch_core.authz import Principal, can +from switch_core.db.models import TENANT_ZERO_ID, Room, Tenant, TenantMember, User +from switch_core.db.stores.user_store import UserStore +from switch_core.tenant_context import tenant_scope + +TENANT_B = "tenant-b" + + +async def _make_user(session: AsyncSession, name: str, *, role: str = "user") -> User: + user = User( + name=name, email=f"{name}@example.invalid", role=role, password_hash="x" + ) + session.add(user) + await session.flush() + return user + + +async def _make_tenant(session: AsyncSession, tenant_id: str) -> Tenant: + tenant = Tenant(id=tenant_id, slug=tenant_id, name=tenant_id) + session.add(tenant) + await session.flush() + return tenant + + +async def _membership( + session: AsyncSession, tenant_id: str, user_id: str, role: str +) -> None: + session.add(TenantMember(tenant_id=tenant_id, user_id=user_id, role=role)) + await session.flush() + + +class TestUserStoreAdministers: + async def test_owner_administers_their_own_tenant( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + owner = await _make_user(session, "owner-of-zero") + await _membership(session, TENANT_ZERO_ID, owner.id, "owner") + await session.commit() + + # Ambient tenant is TENANT_ZERO_ID (see conftest.session_factory). + assert await store.administers(session, owner) is True + + async def test_owner_has_no_power_in_a_tenant_they_only_belong_to_as_member( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + # Two sessions, one per tenant: `TenantCheckedSession` stamps a + # session's transaction with whichever tenant is bound when it opens + # and refuses to let it drift mid-transaction + # (`db/tenant_session.py`), the same guard that would catch a request + # rebinding a session it did not open scoped. A real cross-tenant + # comparison goes through two sessions for the same reason. + store = UserStore() + async with session_factory() as session: + tenant_b = await _make_tenant(session, TENANT_B) + owner = await _make_user(session, "cross-tenant-owner") + await _membership(session, TENANT_ZERO_ID, owner.id, "owner") + await _membership(session, tenant_b.id, owner.id, "member") + await session.commit() + + assert await store.administers(session, owner) is True + + with tenant_scope(tenant_b.id): + async with session_factory() as session_b: + assert await store.administers(session_b, owner) is False + + async def test_admin_membership_role_also_administers( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + admin_member = await _make_user(session, "admin-of-zero") + await _membership(session, TENANT_ZERO_ID, admin_member.id, "admin") + await session.commit() + + assert await store.administers(session, admin_member) is True + + async def test_operator_bypasses_regardless_of_tenant_membership( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + tenant_b = await _make_tenant(session, TENANT_B) + operator = await _make_user(session, "operator", role="admin") + # A plain member in tenant zero... + await _membership(session, TENANT_ZERO_ID, operator.id, "member") + await session.commit() + + assert await store.administers(session, operator) is True + + # ...and no membership row at all in tenant B — a fresh session bound + # there, per the note above. + with tenant_scope(tenant_b.id): + async with session_factory() as session_b: + assert await store.administers(session_b, operator) is True + + async def test_plain_member_gains_nothing( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + member = await _make_user(session, "plain-member") + await _membership(session, TENANT_ZERO_ID, member.id, "member") + await session.commit() + + assert await store.administers(session, member) is False + + async def test_no_membership_row_in_the_bound_tenant_gains_nothing( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + stray = await _make_user(session, "no-membership") + await session.commit() + + assert await store.administers(session, stray) is False + + +class TestRoomAuthzThroughTenantRole: + """The same claims, one layer up: a real `authz.can` decision on a real + `Room` neither party personally owns, built from `UserStore.administers` + the way `gateway/rooms.py` and `gateway/auth.py` build it.""" + + async def test_workspace_owner_can_delete_a_room_they_do_not_personally_own( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + owner = await _make_user(session, "room-admin-owner") + someone_else = await _make_user(session, "room-creator") + await _membership(session, TENANT_ZERO_ID, owner.id, "owner") + room = Room( + tenant_id=TENANT_ZERO_ID, + matrix_room_id="!private-room:test", + name="private room", + description="desc", + owner_id=someone_else.id, + read_visibility="private", + write_visibility="private", + ) + session.add(room) + await session.commit() + + principal = Principal(owner.id, await store.administers(session, owner)) + assert can(principal, "delete", room) + + async def test_workspace_owner_cannot_delete_the_same_room_from_another_tenant( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + tenant_b = await _make_tenant(session, TENANT_B) + owner = await _make_user(session, "room-admin-owner-2") + someone_else = await _make_user(session, "room-creator-2") + await _membership(session, TENANT_ZERO_ID, owner.id, "owner") + await _membership(session, tenant_b.id, owner.id, "member") + room = Room( + tenant_id=TENANT_ZERO_ID, + matrix_room_id="!private-room-2:test", + name="private room", + description="desc", + owner_id=someone_else.id, + read_visibility="private", + write_visibility="private", + ) + session.add(room) + await session.commit() + + with tenant_scope(tenant_b.id): + async with session_factory() as session_b: + principal = Principal( + owner.id, await store.administers(session_b, owner) + ) + assert not can(principal, "delete", room) + + async def test_plain_member_cannot_delete_a_room_they_do_not_own( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + store = UserStore() + async with session_factory() as session: + member = await _make_user(session, "room-member") + someone_else = await _make_user(session, "room-creator-3") + await _membership(session, TENANT_ZERO_ID, member.id, "member") + room = Room( + tenant_id=TENANT_ZERO_ID, + matrix_room_id="!private-room-3:test", + name="private room", + description="desc", + owner_id=someone_else.id, + read_visibility="private", + write_visibility="private", + ) + session.add(room) + await session.commit() + + principal = Principal(member.id, await store.administers(session, member)) + assert not can(principal, "delete", room) diff --git a/core/tests/switch_core/gateway/test_collaborations_authz.py b/core/tests/switch_core/gateway/test_collaborations_authz.py index f71fb8186..400f3c417 100644 --- a/core/tests/switch_core/gateway/test_collaborations_authz.py +++ b/core/tests/switch_core/gateway/test_collaborations_authz.py @@ -10,9 +10,10 @@ These tests lock in three properties: 1. `/collab` is gone — not in the auth-bypass list, and its modules deleted. -2. Every collaboration write route on the gateway requires an admin (the - route dependency is `require_admin`), and every route requires at least - authentication. +2. Every collaboration write route on the gateway requires a tenant admin (the + route dependency is `require_tenant_admin` — a collaboration bridge is + tenant-scoped, so this is workspace administration, not the deployment + operator bit), and every route requires at least authentication. 3. The new set-default behaviour is correct against real Postgres. """ @@ -31,7 +32,7 @@ from switch_core.db.models import Client, CollaborationBridge, User from switch_core.db.stores.collaboration_bridge_store import CollaborationBridgeStore from switch_core.db.stores.room_store import RoomStore -from switch_core.gateway.auth import get_current_user, require_admin +from switch_core.gateway.auth import get_current_user, require_tenant_admin from switch_core.gateway.collaborations import ( _require_directory_account, claim_bridge_identity, @@ -91,9 +92,10 @@ def test_collab_api_module_is_gone() -> None: def test_bridge_write_routes_require_admin() -> None: - """EVERY state-changing route must be gated on require_admin — a bridge is an - unowned, workspace-wide integration holding platform secrets, so there is no - owner to scope mutation to. + """EVERY state-changing route must be gated on require_tenant_admin — a + bridge is an unowned, tenant-scoped integration holding platform secrets, + so there is no per-resource owner to scope mutation to, but it still + belongs to one workspace rather than the deployment as a whole. Derived from the router rather than a hand-listed set: a new write route that forgets the gate fails here instead of shipping open. @@ -103,10 +105,10 @@ def test_bridge_write_routes_require_admin() -> None: for route in router.routes if route.methods & WRITE_METHODS and route.path not in _OWNER_SCOPED_PATHS - and require_admin not in _dependency_calls(route.dependant) + and require_tenant_admin not in _dependency_calls(route.dependant) ) assert not unguarded, ( - f"collaboration write routes missing require_admin: {unguarded}" + f"collaboration write routes missing require_tenant_admin: {unguarded}" ) @@ -118,7 +120,7 @@ def test_owner_scoped_exemptions_name_real_routes() -> None: class TestIdentityRoutesAreSelfOrAdmin: - """The check the require_admin exemption trades away moved into the + """The check the require_tenant_admin exemption trades away moved into the handler; it did not disappear.""" async def test_claiming_for_another_user_is_refused(self) -> None: @@ -236,6 +238,9 @@ def __init__(self, value: object) -> None: async def get(self, *_args: object, **_kwargs: object) -> object: return self._value + async def administers(self, *_args: object, **_kwargs: object) -> bool: + return False + def _user(*, id: str, role: str) -> SimpleNamespace: return SimpleNamespace(id=id, role=role, name=id) @@ -253,10 +258,10 @@ def test_known_bridge_write_routes_are_present() -> None: def test_every_collaboration_route_requires_authentication() -> None: """No route may fall back to the old unauthenticated behaviour: each must - depend on get_current_user directly or via require_admin.""" + depend on get_current_user directly or via require_tenant_admin.""" for route in router.routes: calls = _dependency_calls(route.dependant) - assert get_current_user in calls or require_admin in calls, ( + assert get_current_user in calls or require_tenant_admin in calls, ( f"{sorted(route.methods)} {route.path} is not authenticated" ) diff --git a/core/tests/switch_core/gateway/test_reference_types_routes.py b/core/tests/switch_core/gateway/test_reference_types_routes.py index a792f7c60..7ffdaa416 100644 --- a/core/tests/switch_core/gateway/test_reference_types_routes.py +++ b/core/tests/switch_core/gateway/test_reference_types_routes.py @@ -142,7 +142,12 @@ def _app() -> FastAPI: # stubbed. It is never called: a request with no cookie is refused # before it is touched. app.dependency_overrides[get_session_factory] = lambda: None - app.dependency_overrides[get_user_store] = lambda: None + # A real store, not `None`: authorizing now reads `UserStore.administers` + # before the route reaches the resource service. With no tenant bound — + # this app never runs `get_current_user`'s `tenant_scope` — that resolves + # from the operator bit alone without touching the stubbed session, so a + # real instance is safe here and a bare `None` no longer is. + app.dependency_overrides[get_user_store] = lambda: UserStore() app.dependency_overrides[get_config] = lambda: None app.dependency_overrides[get_resource_service] = lambda: None return app @@ -352,7 +357,9 @@ async def test_delete_removes_an_unused_type( _create_request("notion"), session, svc, _USER_STORE, alice ) - response = await delete_reference_type("notion", session, svc, alice) + response = await delete_reference_type( + "notion", session, svc, _USER_STORE, alice + ) assert response.deleted_type == "notion" assert ( @@ -386,7 +393,7 @@ async def test_delete_of_a_type_in_use_is_409( ) with pytest.raises(HTTPException) as exc: - await delete_reference_type("notion", session, svc, alice) + await delete_reference_type("notion", session, svc, _USER_STORE, alice) assert exc.value.status_code == 409 assert "cannot be deleted" in exc.value.detail diff --git a/core/tests/switch_core/test_authz.py b/core/tests/switch_core/test_authz.py index 61c297fc2..ecf8d1182 100644 --- a/core/tests/switch_core/test_authz.py +++ b/core/tests/switch_core/test_authz.py @@ -6,6 +6,7 @@ from switch_core.authz import ( Principal, + administers_tenant, can, can_manage, require, @@ -106,6 +107,26 @@ def test_require_manage_raises_when_denied(self) -> None: require_manage(ALICE, "u-alice") # owner — no raise +class TestAdministersTenant: + """`administers_tenant` is the one function allowed to turn a role into + the admin bit (`authz.py` module docstring) — the operator bypass and the + per-tenant `owner`/`admin` membership role, combined.""" + + def test_operator_administers_regardless_of_tenant_role(self) -> None: + assert administers_tenant(is_operator=True, tenant_role=None) + assert administers_tenant(is_operator=True, tenant_role="member") + + def test_owner_or_admin_membership_administers(self) -> None: + assert administers_tenant(is_operator=False, tenant_role="owner") + assert administers_tenant(is_operator=False, tenant_role="admin") + + def test_plain_member_does_not_administer(self) -> None: + assert not administers_tenant(is_operator=False, tenant_role="member") + + def test_no_membership_does_not_administer(self) -> None: + assert not administers_tenant(is_operator=False, tenant_role=None) + + class TestValidateVisibilityPair: @pytest.mark.parametrize( "read,write", diff --git a/core/tests/switch_core/test_migration_operator_owns_tenant_zero.py b/core/tests/switch_core/test_migration_operator_owns_tenant_zero.py new file mode 100644 index 000000000..a9dfd8577 --- /dev/null +++ b/core/tests/switch_core/test_migration_operator_owns_tenant_zero.py @@ -0,0 +1,256 @@ +"""The operator/workspace-admin role split migration moves real data correctly. + +`test_migration_parity.py` replays the chain into an *empty* database, so it +can never catch a bug in what `8ef6d4038ecc` does to an existing deployment's +rows — a promoted-then-forgotten operator whose `tenant_members` row still +says `member` would pass every other test and only show up against real data. +This seeds tenant zero the way a deployment with that exact staleness would +look, stops the chain one revision short of the migration under test, runs +it, and inspects what came out. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any + +from alembic.config import Config +from alembic.runtime.environment import EnvironmentContext +from alembic.script import ScriptDirectory +from sqlalchemy import Connection, text +from sqlalchemy.ext.asyncio import create_async_engine + +_CORE = Path(__file__).resolve().parents[2] +_MIGRATION_DB = "migration_operator_owns_tenant_zero" + +_PRE_MIGRATION_REVISION = "a3f61c02d5be" +_MIGRATION_UNDER_TEST = "8ef6d4038ecc" +_TENANT_ZERO_ID = "00000000-0000-0000-0000-000000000000" + + +def _script_directory(config: Config) -> ScriptDirectory: + config.set_main_option("script_location", str(_CORE / "switch_core" / "migrations")) + return ScriptDirectory.from_config(config) + + +def _upgrade_to(revision: str) -> Any: + def upgrade(connection: Connection) -> None: + config = Config(str(_CORE / "alembic.ini")) + script = _script_directory(config) + + def do_upgrade(current_revision: str, context: Any) -> Any: + return script._upgrade_revs(revision, current_revision) + + with EnvironmentContext(config, script, fn=do_upgrade) as environment: + environment.configure(connection=connection) + with environment.begin_transaction(): + environment.run_migrations() + + return upgrade + + +async def _fixture_engine(postgres_url: str) -> Any: + admin = create_async_engine(postgres_url, isolation_level="AUTOCOMMIT") + async with admin.connect() as connection: + await connection.execute(text(f'DROP DATABASE IF EXISTS "{_MIGRATION_DB}"')) + await connection.execute(text(f'CREATE DATABASE "{_MIGRATION_DB}"')) + await admin.dispose() + base, _, _ = postgres_url.rpartition("/") + return create_async_engine(f"{base}/{_MIGRATION_DB}") + + +async def _drop_fixture(postgres_url: str) -> None: + admin = create_async_engine(postgres_url, isolation_level="AUTOCOMMIT") + async with admin.connect() as connection: + await connection.execute(text(f'DROP DATABASE IF EXISTS "{_MIGRATION_DB}"')) + await admin.dispose() + + +async def test_a_promoted_operators_stale_member_row_becomes_owner( + postgres_url: str, +) -> None: + """The gap the migration exists for: `users.role` was promoted to + `admin` by direct SQL after the `tenant_members` row already existed as + `member` — nothing before this migration ever revisited it.""" + engine = await _fixture_engine(postgres_url) + try: + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_PRE_MIGRATION_REVISION)) + + async with engine.begin() as connection: + await connection.execute( + text( + """ + INSERT INTO users (id, name, email, role, password_hash, metadata) + VALUES ('promoted-op', 'Promoted', 'promoted@example.com', + 'admin', 'bcrypt-hash', NULL) + """ + ) + ) + await connection.execute( + text( + f""" + INSERT INTO tenant_members (tenant_id, user_id, role) + VALUES ('{_TENANT_ZERO_ID}', 'promoted-op', 'member') + """ + ) + ) + + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_MIGRATION_UNDER_TEST)) + + async with engine.connect() as connection: + role = ( + await connection.execute( + text( + "SELECT role FROM tenant_members " + "WHERE tenant_id = :t AND user_id = 'promoted-op'" + ), + {"t": _TENANT_ZERO_ID}, + ) + ).scalar_one() + finally: + await engine.dispose() + await _drop_fixture(postgres_url) + + assert role == "owner" + + +async def test_an_operator_with_no_membership_row_at_all_gets_one( + postgres_url: str, +) -> None: + """`UserStore.ensure_membership` should make this unreachable, but + nothing at the database level enforces it — the migration closes the gap + rather than assuming application code always ran first.""" + engine = await _fixture_engine(postgres_url) + try: + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_PRE_MIGRATION_REVISION)) + + async with engine.begin() as connection: + await connection.execute( + text( + """ + INSERT INTO users (id, name, email, role, password_hash, metadata) + VALUES ('orphan-op', 'Orphan', 'orphan@example.com', + 'admin', 'bcrypt-hash', NULL) + """ + ) + ) + + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_MIGRATION_UNDER_TEST)) + + async with engine.connect() as connection: + role = ( + await connection.execute( + text( + "SELECT role FROM tenant_members " + "WHERE tenant_id = :t AND user_id = 'orphan-op'" + ), + {"t": _TENANT_ZERO_ID}, + ) + ).scalar_one() + finally: + await engine.dispose() + await _drop_fixture(postgres_url) + + assert role == "owner" + + +async def test_a_plain_members_row_is_left_exactly_as_it_is( + postgres_url: str, +) -> None: + """Nothing else moves: a non-operator's membership row is not this + migration's business, whatever role it happens to hold.""" + engine = await _fixture_engine(postgres_url) + try: + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_PRE_MIGRATION_REVISION)) + + async with engine.begin() as connection: + await connection.execute( + text( + """ + INSERT INTO users (id, name, email, role, password_hash, metadata) + VALUES ('plain-member', 'Plain', 'plain@example.com', + 'user', 'bcrypt-hash', NULL) + """ + ) + ) + await connection.execute( + text( + f""" + INSERT INTO tenant_members (tenant_id, user_id, role) + VALUES ('{_TENANT_ZERO_ID}', 'plain-member', 'member') + """ + ) + ) + + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_MIGRATION_UNDER_TEST)) + + async with engine.connect() as connection: + role = ( + await connection.execute( + text( + "SELECT role FROM tenant_members " + "WHERE tenant_id = :t AND user_id = 'plain-member'" + ), + {"t": _TENANT_ZERO_ID}, + ) + ).scalar_one() + finally: + await engine.dispose() + await _drop_fixture(postgres_url) + + assert role == "member" + + +async def test_running_it_twice_is_a_no_op_the_second_time( + postgres_url: str, +) -> None: + engine = await _fixture_engine(postgres_url) + try: + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_PRE_MIGRATION_REVISION)) + + async with engine.begin() as connection: + await connection.execute( + text( + """ + INSERT INTO users (id, name, email, role, password_hash, metadata) + VALUES ('idempotent-op', 'Idempotent', 'idempotent@example.com', + 'admin', 'bcrypt-hash', NULL) + """ + ) + ) + await connection.execute( + text( + f""" + INSERT INTO tenant_members (tenant_id, user_id, role) + VALUES ('{_TENANT_ZERO_ID}', 'idempotent-op', 'member') + """ + ) + ) + + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_MIGRATION_UNDER_TEST)) + async with engine.begin() as connection: + await connection.run_sync(_upgrade_to(_MIGRATION_UNDER_TEST)) + + async with engine.connect() as connection: + rows = ( + await connection.execute( + text( + "SELECT role FROM tenant_members " + "WHERE tenant_id = :t AND user_id = 'idempotent-op'" + ), + {"t": _TENANT_ZERO_ID}, + ) + ).all() + finally: + await engine.dispose() + await _drop_fixture(postgres_url) + + assert [r.role for r in rows] == ["owner"] diff --git a/core/tests/switch_core/test_rooms_yaml.py b/core/tests/switch_core/test_rooms_yaml.py index 064c5f461..f4ceed200 100644 --- a/core/tests/switch_core/test_rooms_yaml.py +++ b/core/tests/switch_core/test_rooms_yaml.py @@ -1052,10 +1052,12 @@ async def test_endpoint_json_body(env): from fastapi import HTTPException + from switch_core.db.stores.user_store import UserStore from switch_core.gateway.rooms import create_room_from_yaml svc = _svc(env) user_id = env["user_id"] + user_store = UserStore() user = User(name="alice", email="alice@example.com", role="member") # Poke the id to match the seeded user so provision works. object.__setattr__(user, "id", user_id) @@ -1071,12 +1073,14 @@ async def test_endpoint_json_body(env): request.headers = {"content-type": "application/json"} request.body.return_value = body - result = await create_room_from_yaml(request, svc, user) + async with env["session_factory"]() as session: + result = await create_room_from_yaml(request, session, svc, user_store, user) assert result.room_name == "carol local-deploy" # Non-string yaml value → 400. bad_body = json.dumps({"yaml": 123}).encode() request.body.return_value = bad_body - with pytest.raises(HTTPException) as exc_info: - await create_room_from_yaml(request, svc, user) + async with env["session_factory"]() as session: + with pytest.raises(HTTPException) as exc_info: + await create_room_from_yaml(request, session, svc, user_store, user) assert exc_info.value.status_code == 400 From 0fc9869571a46ef452db5e65ee27027f4e4c8225 Mon Sep 17 00:00:00 2001 From: Petr Bauch Date: Mon, 14 Sep 2026 08:27:06 -0400 Subject: [PATCH 2/2] fix(authz): resolve the tenant-admin bit once, in the dependency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review points on the role split, plus the collision the rebase onto main exposed. **The grant is wider than a list of routes.** `authz.can` and `can_manage` short-circuit on `Principal.is_admin` before ownership or visibility, so the moment `tenant_members.role` feeds that bit a workspace owner or admin holds read, write and delete on every owned resource in the workspace — private rooms included, via `require_room_access`, which is not an admin-gated route at all. That is a decision about what an administrator may see, not a consequence of a route list, so it is written down in the phase 2 note: accepted for this phase, with the narrower "administers configuration" bit left to whichever phase takes on resource-level sharing. **`UserStore.administers` refuses an unbound tenant** rather than answering on the operator bit alone. Half the question is a membership row that cannot be read without a tenant; answering anyway silently demotes a workspace owner, and the 403 that follows names no cause. Same call #442 made on the bridge-identity path. Two tests pin it, including for an operator, whose bypass is the one answer that would have been right and so the worst one to give. **No handler builds its own `UserStore` any more.** Twenty-five did, six while a real one sat in their signature. Now route handlers take `is_admin: Annotated[bool, Depends(get_tenant_is_admin)]` and non-route helpers take a plain `bool`, so authz I/O leaves the helpers entirely and a route cannot authorize against a different bit than its own dependencies resolved. Six `user_store` dependencies that the rewrite left with no reader are gone with them. Rebasing first paid for itself: this branch's test fake still implemented `all_bridges()`, which #442 removed when it merged — green apart, red together. Co-Authored-By: Claude Opus 5 --- core/switch_core/db/stores/user_store.py | 25 ++++-- core/switch_core/gateway/agents.py | 23 +++-- core/switch_core/gateway/auth.py | 11 ++- core/switch_core/gateway/collaborations.py | 12 ++- core/switch_core/gateway/connectors.py | 6 +- core/switch_core/gateway/documents.py | 11 ++- core/switch_core/gateway/packages.py | 49 +++++++---- core/switch_core/gateway/references.py | 46 ++++++---- core/switch_core/gateway/room_groups.py | 6 +- core/switch_core/gateway/room_links.py | 18 ++-- core/switch_core/gateway/rooms.py | 78 ++++++++++------- core/switch_core/gateway/templates.py | 16 ++-- .../test_agent_tenant_scoped_admin.py | 2 +- .../protocol/test_reference_type_parity.py | 3 +- .../db/test_tenant_admin_role_split.py | 43 ++++++++- .../gateway/agent_route_harness.py | 13 +++ .../gateway/test_agent_display_name_route.py | 14 ++- .../gateway/test_agent_icon_route.py | 15 +++- .../gateway/test_collaborations_authz.py | 2 + .../switch_core/gateway/test_connectors.py | 12 +++ .../gateway/test_reference_types_routes.py | 60 ++++++++++--- .../gateway/test_room_resource_read_authz.py | 33 ++++++- .../gateway/test_room_write_authz.py | 87 +++++++++++++++++-- .../gateway/test_template_routes.py | 23 ++++- core/tests/switch_core/test_rooms_yaml.py | 9 +- docs/old/multi-tenancy-phase2-tenants.md | 31 +++++++ 26 files changed, 507 insertions(+), 141 deletions(-) diff --git a/core/switch_core/db/stores/user_store.py b/core/switch_core/db/stores/user_store.py index d1e4f6122..3506b9741 100644 --- a/core/switch_core/db/stores/user_store.py +++ b/core/switch_core/db/stores/user_store.py @@ -317,15 +317,22 @@ async def administers(self, session: AsyncSession, user: User) -> bool: context — the operator bypass, or an owner/admin membership in it. See `authz.administers_tenant`, which this composes with a read of the - one membership row that can answer "in *this* tenant". No tenant bound - answers with the operator bit alone: every gateway request binds one - by the time this is reachable, and nothing here should insist on a - precondition that authentication already enforces. + one membership row that can answer "in *this* tenant". + + Raises: + RuntimeError: no tenant is bound. The question has no + tenant-independent answer: half of it is a membership row that + cannot be read without one. Answering on the operator bit + alone would quietly demote a workspace owner to a plain + member, and the symptom — a 403 on their own workspace — says + nothing about why. """ tenant_id = current_tenant_id() - role = ( - None - if tenant_id is None - else await self.tenant_role(session, tenant_id, user.id) - ) + if tenant_id is None: + raise RuntimeError( + "administers requires a bound tenant; whether someone may " + "administer a workspace is only answerable about a particular " + "one" + ) + role = await self.tenant_role(session, tenant_id, user.id) return administers_tenant(is_operator=user.role == "admin", tenant_role=role) diff --git a/core/switch_core/gateway/agents.py b/core/switch_core/gateway/agents.py index b2d908939..a2033b826 100644 --- a/core/switch_core/gateway/agents.py +++ b/core/switch_core/gateway/agents.py @@ -26,7 +26,7 @@ from switch_core.db.stores.agent_store import AgentStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user +from switch_core.gateway.auth import get_current_user, get_tenant_is_admin from switch_core.gateway.dependencies import ( get_agent_store, get_protocol, @@ -74,13 +74,14 @@ async def delete_agent_by_name( agent_store: Annotated[AgentStore, Depends(get_agent_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> dict[str, bool]: agent = await agent_store.get_by_name(session, agent_name) if agent is None: raise HTTPException(status_code=404, detail=f"Agent not found: {agent_name}") try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), agent.owner_id, ) except PermissionError: @@ -104,13 +105,14 @@ async def delete_agent( agent_store: Annotated[AgentStore, Depends(get_agent_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> dict[str, bool]: agent = await agent_store.get(session, agent_id) if agent is None: raise HTTPException(status_code=404, detail=f"Agent not found: {agent_id}") try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), agent.owner_id, ) except PermissionError: @@ -198,6 +200,7 @@ async def register_known_subagents( protocol: Annotated[ProtocolService, Depends(get_protocol)], session: Annotated[AsyncSession, Depends(get_session)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RegisterKnownSubagentsResponse: """Register many Claude Code subagents under one parent agent (session-authed). @@ -223,7 +226,7 @@ async def register_known_subagents( ) try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), parent.owner_id, ) except PermissionError as exc: @@ -308,6 +311,7 @@ async def update_agent_options( session: Annotated[AsyncSession, Depends(get_session)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> AgentSummary: """Replace a known-agent's options. @@ -327,7 +331,7 @@ async def update_agent_options( try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), agent.owner_id, ) except PermissionError: @@ -363,6 +367,7 @@ async def update_agent_icon( session: Annotated[AsyncSession, Depends(get_session)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> AgentSummary: """Set, change, or clear an agent's icon (CHOO-2171). @@ -383,7 +388,7 @@ async def update_agent_icon( try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), agent.owner_id, ) except PermissionError: @@ -419,6 +424,7 @@ async def update_agent_display_name( session: Annotated[AsyncSession, Depends(get_session)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> AgentSummary: """Set, change, or clear an agent's human display name. @@ -436,7 +442,7 @@ async def update_agent_display_name( try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), agent.owner_id, ) except PermissionError: @@ -516,6 +522,7 @@ async def update_addressing_policy( user_store: Annotated[UserStore, Depends(get_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> AgentDetail: """Set or clear an agent's scoped addressing policy (CHOO-1585). @@ -528,7 +535,7 @@ async def update_addressing_policy( try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), agent.owner_id, ) except PermissionError: diff --git a/core/switch_core/gateway/auth.py b/core/switch_core/gateway/auth.py index 5e4b4b8f6..2b9e7ccea 100644 --- a/core/switch_core/gateway/auth.py +++ b/core/switch_core/gateway/auth.py @@ -287,6 +287,7 @@ async def require_room_access( room_id: str, user: User, action: Action, + is_admin: bool, ) -> Room: """Load a room (404 if missing) and authorize `action` for `user`, raising HTTP 403 if denied. @@ -295,11 +296,19 @@ async def require_room_access( id (attaching references, linking rooms, …) so they cannot operate on a room the caller lacks access to. Mirrors the protocol layer's ``_require_room_action``. + + `is_admin` comes from ``get_tenant_is_admin``; it is a parameter rather + than a read of its own so that a route cannot end up authorizing against + a different admin bit than the one its own dependencies resolved. + + This is the widest of the tenant-admin gates: `authz.can` short-circuits + on the admin bit, so a workspace owner or admin passes it for every room + in their workspace, private ones included. See the phase 2 design note on + what that grant covers. """ room = await room_store.get(session, room_id) if room is None: raise HTTPException(status_code=404, detail="Room not found") - is_admin = await UserStore().administers(session, user) try: require(Principal(user.id, is_admin), action, room) except PermissionError as e: diff --git a/core/switch_core/gateway/collaborations.py b/core/switch_core/gateway/collaborations.py index a7ae7361f..902a5c974 100644 --- a/core/switch_core/gateway/collaborations.py +++ b/core/switch_core/gateway/collaborations.py @@ -21,7 +21,11 @@ from switch_core.db.stores.external_user_store import ExternalUserStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user, require_tenant_admin +from switch_core.gateway.auth import ( + get_current_user, + get_tenant_is_admin, + require_tenant_admin, +) from switch_core.gateway.dependencies import ( get_bridge_store, get_collab_lifecycle, @@ -529,6 +533,7 @@ async def claim_bridge_identity( CollaborationBridgeLifecycleService, Depends(get_collab_lifecycle) ], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ExternalUserSummary: """Claim a platform identity for a Switch user (CHOO-2137). @@ -548,7 +553,7 @@ async def claim_bridge_identity( raise HTTPException(status_code=404, detail="Bridge not found") target_user_id = payload.user_id or user.id - if target_user_id != user.id and not await user_store.administers(session, user): + if target_user_id != user.id and not is_admin: raise HTTPException( status_code=403, detail="Only an admin may claim a messaging identity for another user", @@ -635,6 +640,7 @@ async def release_bridge_identity( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], user_id: str | None = None, ) -> ExternalUserSummary: """Drop a claim on a platform account — your own, or someone else's if you @@ -644,7 +650,7 @@ async def release_bridge_identity( raise HTTPException(status_code=404, detail="Identity not found") target_user_id = user_id or user.id - if target_user_id != user.id and not await user_store.administers(session, user): + if target_user_id != user.id and not is_admin: raise HTTPException( status_code=403, detail="Only an admin may release another user's messaging identity", diff --git a/core/switch_core/gateway/connectors.py b/core/switch_core/gateway/connectors.py index f21f7ea8f..8dd425657 100644 --- a/core/switch_core/gateway/connectors.py +++ b/core/switch_core/gateway/connectors.py @@ -12,8 +12,7 @@ from switch_core.db.models import User from switch_core.db.stores.api_key_store import ApiKeyStore from switch_core.db.stores.server_connector_store import ServerConnectorStore -from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user +from switch_core.gateway.auth import get_current_user, get_tenant_is_admin from switch_core.gateway.dependencies import ( get_api_key_store, get_connector_lifecycle, @@ -132,6 +131,7 @@ async def delete_connector( ServerSideConnectorLifecycleService, Depends(get_connector_lifecycle) ], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> dict[str, bool]: record = await connector_store.get(session, connector_id) if record is None: @@ -139,7 +139,7 @@ async def delete_connector( reg_key = await api_key_store.get(session, record.api_key_id) if reg_key is None or reg_key.user_id != user.id: - if not await UserStore().administers(session, user): + if not is_admin: raise HTTPException( status_code=403, detail="Not authorized to delete this connector" ) diff --git a/core/switch_core/gateway/documents.py b/core/switch_core/gateway/documents.py index 998b7ca17..93a7dd362 100644 --- a/core/switch_core/gateway/documents.py +++ b/core/switch_core/gateway/documents.py @@ -308,8 +308,9 @@ async def list_room_documents( agent_store: Annotated[AgentStore, Depends(get_agent_store)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[DocumentSummary]: - await require_room_access(session, room_store, room_id, user, "read") + await require_room_access(session, room_store, room_id, user, "read", is_admin) docs = await resource_service.list_room_documents(session, room_id) return await _enrich_summaries( session, docs, resource_service, user_store, agent_store @@ -328,7 +329,7 @@ async def attach_document_to_room( user: Annotated[User, Depends(get_current_user)], is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> DocumentDetail: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) try: await resource_service.attach_document_to_room( session, @@ -356,10 +357,11 @@ async def detach_document_from_room( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> None: """For globally-owned docs: detach from the room. For room-scoped docs: hard-delete them entirely (since they live only in this room).""" - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) doc = await resource_service.get_room_scoped_document_or_none( session, room_id, document_id ) @@ -382,11 +384,12 @@ async def get_room_document( agent_store: Annotated[AgentStore, Depends(get_agent_store)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> DocumentDetail: """Fetch a document by id within the context of a room. Accepts both room-scoped documents (read-only) and globally-attached documents. Room read access is required and, once granted, implies access to its docs.""" - await require_room_access(session, room_store, room_id, user, "read") + await require_room_access(session, room_store, room_id, user, "read", is_admin) docs = await resource_service.list_room_documents(session, room_id) doc = next((d for d in docs if d.id == document_id), None) if doc is None: diff --git a/core/switch_core/gateway/packages.py b/core/switch_core/gateway/packages.py index ed3cdc9b2..4521f6d71 100644 --- a/core/switch_core/gateway/packages.py +++ b/core/switch_core/gateway/packages.py @@ -10,7 +10,11 @@ from switch_core.db.stores.agent_store import AgentStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user, require_room_access +from switch_core.gateway.auth import ( + get_current_user, + get_tenant_is_admin, + require_room_access, +) from switch_core.gateway.dependencies import ( get_agent_store, get_resource_service, @@ -140,13 +144,14 @@ async def get_package( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> PackageDetail: try: pkg = await resource_service.get_package_for_user( session, package_id, user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -163,13 +168,14 @@ async def patch_package( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> PackageDetail: try: pkg = await resource_service.update_package( session, package_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, name=req.name, description=req.description, instructions=req.instructions, @@ -190,13 +196,14 @@ async def delete_package( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> PackageDeleteResponse: try: detached = await resource_service.delete_package( session, package_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -212,13 +219,14 @@ async def list_rooms_for_package( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ResourceRoom]: try: await resource_service.get_package_for_user( session, package_id, user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -238,13 +246,14 @@ async def list_package_references( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ReferenceDetail]: try: await resource_service.get_package_for_user( session, package_id, user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -261,6 +270,7 @@ async def add_reference_to_package( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> None: try: await resource_service.add_reference_to_package( @@ -268,7 +278,7 @@ async def add_reference_to_package( package_id, reference_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -284,6 +294,7 @@ async def remove_reference_from_package( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> PackageMemberRemoveResponse: try: affected = await resource_service.remove_reference_from_package( @@ -291,7 +302,7 @@ async def remove_reference_from_package( package_id, reference_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -314,13 +325,14 @@ async def list_package_documents( user_store: Annotated[UserStore, Depends(get_user_store)], agent_store: Annotated[AgentStore, Depends(get_agent_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[DocumentSummary]: try: await resource_service.get_package_for_user( session, package_id, user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -339,6 +351,7 @@ async def add_document_to_package( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> None: try: await resource_service.add_document_to_package( @@ -346,7 +359,7 @@ async def add_document_to_package( package_id, document_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -362,6 +375,7 @@ async def remove_document_from_package( session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> PackageMemberRemoveResponse: try: affected = await resource_service.remove_document_from_package( @@ -369,7 +383,7 @@ async def remove_document_from_package( package_id, document_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -395,8 +409,9 @@ async def list_room_packages( room_store: Annotated[RoomStore, Depends(get_room_store)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[PackageDetail]: - await require_room_access(session, room_store, room_id, user, "read") + await require_room_access(session, room_store, room_id, user, "read", is_admin) pkgs = await resource_service.list_room_packages(session, room_id) return await _enrich(session, pkgs, resource_service, user_store) @@ -410,15 +425,16 @@ async def attach_package_to_room( room_store: Annotated[RoomStore, Depends(get_room_store)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> PackageDetail: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) try: await resource_service.attach_package_to_room( session, room_id, package_id, user_id=user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -429,7 +445,7 @@ async def attach_package_to_room( session, package_id, user.id, - is_admin=await UserStore().administers(session, user), + is_admin=is_admin, ) return await _enrich_one(session, pkg, resource_service, user_store) @@ -442,7 +458,8 @@ async def detach_package_from_room( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> None: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) await resource_service.detach_package_from_room(session, room_id, package_id) await session.commit() diff --git a/core/switch_core/gateway/references.py b/core/switch_core/gateway/references.py index 7d80b3fa2..c78b6e7d8 100644 --- a/core/switch_core/gateway/references.py +++ b/core/switch_core/gateway/references.py @@ -15,7 +15,11 @@ from switch_core.db.models import Reference, User from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user, require_room_access +from switch_core.gateway.auth import ( + get_current_user, + get_tenant_is_admin, + require_room_access, +) from switch_core.gateway.dependencies import ( get_resource_service, get_room_store, @@ -165,9 +169,10 @@ async def list_reference_types( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ReferenceTypeInfo]: views = await resource_service.list_reference_types_for_principal( - session, user_id=user.id, is_admin=await user_store.administers(session, user) + session, user_id=user.id, is_admin=is_admin ) owners = await _owner_names( session, user_store, {v.owner_id for v in views if v.owner_id is not None} @@ -184,9 +189,10 @@ async def list_owned_reference_types( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ReferenceTypeDetail]: rows = await resource_service.list_owned_reference_types( - session, user_id=user.id, is_admin=await user_store.administers(session, user) + session, user_id=user.id, is_admin=is_admin ) owners = await _owner_names(session, user_store, {e.row.owner_id for e in rows}) return [_type_to_detail(e, owners.get(e.row.owner_id)) for e in rows] @@ -229,13 +235,14 @@ async def patch_reference_type( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceTypeDetail: try: row = await resource_service.update_reference_type( session, type, user_id=user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, display_name=req.display_name, instructions=req.instructions, value_hint=req.value_hint, @@ -259,15 +266,15 @@ async def delete_reference_type( type: str, session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], - user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceTypeDeleteResponse: try: await resource_service.delete_reference_type( session, type, user_id=user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, ) except ReferenceTypeInUseError as e: raise HTTPException(status_code=409, detail=str(e)) from e @@ -297,12 +304,13 @@ async def create_reference( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceDetail: try: ref = await resource_service.create_reference( session, owner_id=user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, read_visibility=req.read_visibility, write_visibility=req.write_visibility, type=req.type, @@ -324,13 +332,14 @@ async def get_reference( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceDetail: try: ref = await resource_service.get_reference_for_user( session, reference_id, user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -347,13 +356,14 @@ async def patch_reference( resource_service: Annotated[ResourceService, Depends(get_resource_service)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceDetail: try: ref = await resource_service.update_reference( session, reference_id, user_id=user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, name=req.name, description=req.description, instructions=req.instructions, @@ -374,8 +384,8 @@ async def delete_reference( reference_id: str, session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], - user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceDeleteResponse: affected_packages = await resource_service.list_packages_for_reference( session, reference_id @@ -385,7 +395,7 @@ async def delete_reference( session, reference_id, user_id=user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -404,15 +414,15 @@ async def list_rooms_for_reference( reference_id: str, session: Annotated[AsyncSession, Depends(get_session)], resource_service: Annotated[ResourceService, Depends(get_resource_service)], - user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ResourceRoom]: try: await resource_service.get_reference_for_user( session, reference_id, user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, ) except ValueError as e: raise HTTPException(status_code=404, detail=str(e)) from e @@ -430,8 +440,9 @@ async def list_room_references( room_store: Annotated[RoomStore, Depends(get_room_store)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[ReferenceDetail]: - await require_room_access(session, room_store, room_id, user, "read") + await require_room_access(session, room_store, room_id, user, "read", is_admin) refs = await resource_service.list_room_references(session, room_id) return await _enrich(session, refs, resource_service, user_store) @@ -445,10 +456,10 @@ async def attach_reference_to_room( room_store: Annotated[RoomStore, Depends(get_room_store)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ReferenceDetail: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) try: - is_admin = await user_store.administers(session, user) await resource_service.attach_reference_to_room( session, room_id, @@ -475,7 +486,8 @@ async def detach_reference_from_room( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> None: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) await resource_service.detach_reference_from_room(session, room_id, reference_id) await session.commit() diff --git a/core/switch_core/gateway/room_groups.py b/core/switch_core/gateway/room_groups.py index 6d922673b..dca17f78d 100644 --- a/core/switch_core/gateway/room_groups.py +++ b/core/switch_core/gateway/room_groups.py @@ -9,8 +9,7 @@ from switch_core.db.models import RoomGroup, User from switch_core.db.stores.room_group_store import RoomGroupStore from switch_core.db.stores.room_store import RoomStore -from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user +from switch_core.gateway.auth import get_current_user, get_tenant_is_admin from switch_core.gateway.dependencies import ( get_room_group_store, get_room_store, @@ -110,13 +109,14 @@ async def assign_rooms_to_group( room_group_store: Annotated[RoomGroupStore, Depends(get_room_group_store)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomGroupAssignResponse: """Bulk-assign rooms into a group. Requires write access to each room; unknown room ids are skipped.""" if await room_group_store.get(session, group_id) is None: raise HTTPException(status_code=404, detail="Room group not found") - principal = Principal(user.id, await UserStore().administers(session, user)) + principal = Principal(user.id, is_admin) forbidden: list[str] = [] allowed: list[str] = [] for room_id in req.room_ids: diff --git a/core/switch_core/gateway/room_links.py b/core/switch_core/gateway/room_links.py index e219ee7fb..3c2e18a14 100644 --- a/core/switch_core/gateway/room_links.py +++ b/core/switch_core/gateway/room_links.py @@ -8,7 +8,11 @@ from switch_core.bridges.resource.service import ResourceService from switch_core.db.models import User from switch_core.db.stores.room_store import RoomStore -from switch_core.gateway.auth import get_current_user, require_room_access +from switch_core.gateway.auth import ( + get_current_user, + get_tenant_is_admin, + require_room_access, +) from switch_core.gateway.dependencies import ( get_resource_service, get_room_store, @@ -41,8 +45,9 @@ async def list_linked_rooms( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[LinkedRoomDetail]: - await require_room_access(session, room_store, room_id, user, "read") + await require_room_access(session, room_store, room_id, user, "read", is_admin) rows = await resource_service.list_linked_rooms_for_room(session, room_id) return [LinkedRoomDetail(**row) for row in rows] @@ -54,8 +59,9 @@ async def list_inbound_linked_rooms( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[InboundLinkedRoomDetail]: - await require_room_access(session, room_store, room_id, user, "read") + await require_room_access(session, room_store, room_id, user, "read", is_admin) rows = await resource_service.list_inbound_linked_rooms(session, room_id) return [InboundLinkedRoomDetail(**row) for row in rows] @@ -68,8 +74,9 @@ async def create_linked_room( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> LinkedRoomDetail: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) try: row = await resource_service.attach_linked_room( session, @@ -93,8 +100,9 @@ async def delete_linked_room( resource_service: Annotated[ResourceService, Depends(get_resource_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> Response: - await require_room_access(session, room_store, room_id, user, "write") + await require_room_access(session, room_store, room_id, user, "write", is_admin) removed = await resource_service.detach_linked_room( session, source_room_id=room_id, target_room_id=target_room_id ) diff --git a/core/switch_core/gateway/rooms.py b/core/switch_core/gateway/rooms.py index 5cdfad042..752326e05 100644 --- a/core/switch_core/gateway/rooms.py +++ b/core/switch_core/gateway/rooms.py @@ -19,7 +19,7 @@ from switch_core.db.stores.room_group_store import RoomGroupStore from switch_core.db.stores.room_store import RoomStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user +from switch_core.gateway.auth import get_current_user, get_tenant_is_admin from switch_core.gateway.dependencies import ( get_bridge_store, get_collab_lifecycle, @@ -65,6 +65,7 @@ async def _require_room( room_id: str, user: User, action: Action, + is_admin: bool, ) -> Room: """Load a room (404 if missing) and authorize `action` for `user`, raising HTTP 403 if denied.""" @@ -73,7 +74,7 @@ async def _require_room( raise HTTPException(status_code=404, detail="Room not found") try: require( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), action, room, ) @@ -242,13 +243,14 @@ async def list_rooms( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], search: Annotated[str | None, Query()] = None, include_archived: Annotated[bool, Query()] = False, ) -> list[RoomSummary]: rooms = await room_store.list_readable( session, user.id, - is_admin=await user_store.administers(session, user), + is_admin=is_admin, include_archived=include_archived, ) @@ -377,8 +379,8 @@ async def create_room_from_yaml( request: Request, session: Annotated[AsyncSession, Depends(get_session)], rooms_yaml: Annotated[RoomYamlService, Depends(get_room_yaml_service)], - user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> ProvisionResult: """Provision a single room and its attachments from a YAML spec. @@ -405,9 +407,7 @@ async def create_room_from_yaml( text = (await request.body()).decode("utf-8") inputs = None spec = rooms_yaml.parse(text, inputs=inputs) - return await rooms_yaml.provision( - spec, user_id=user.id, is_admin=await user_store.administers(session, user) - ) + return await rooms_yaml.provision(spec, user_id=user.id, is_admin=is_admin) except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) from e except PermissionError as e: @@ -423,6 +423,7 @@ async def export_room_yaml( room_store: Annotated[RoomStore, Depends(get_room_store)], rooms_yaml: Annotated[RoomYamlService, Depends(get_room_yaml_service)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], agents: Annotated[bool, Query()] = True, users: Annotated[bool, Query()] = True, references: Annotated[bool, Query()] = True, @@ -431,7 +432,7 @@ async def export_room_yaml( ) -> Response: """Export a room to YAML in the same surface ``/rooms/from-yaml`` accepts. Each section can be dropped via its boolean toggle (default included).""" - await _require_room(session, room_store, room_id, user, "read") + await _require_room(session, room_store, room_id, user, "read", is_admin) yaml_text = await rooms_yaml.export( room_id, agents=agents, @@ -452,8 +453,9 @@ async def get_room( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - room = await _require_room(session, room_store, room_id, user, "read") + room = await _require_room(session, room_store, room_id, user, "read", is_admin) return await _build_room_detail( session, room, room_store, bridge_store, external_user_store, protocol ) @@ -470,8 +472,9 @@ async def patch_room( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.update_room( room_id, @@ -500,8 +503,9 @@ async def list_room_roles( room_store: Annotated[RoomStore, Depends(get_room_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[RoomRoleDetail]: - await _require_room(session, room_store, room_id, user, "read") + await _require_room(session, room_store, room_id, user, "read", is_admin) return await _list_room_role_details(session, room_id, protocol) @@ -513,8 +517,9 @@ async def create_room_role( room_store: Annotated[RoomStore, Depends(get_room_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[RoomRoleDetail]: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await protocol.room_role_store.define_role( session, room_id, req.name, req.instructions, req.exclusive @@ -534,8 +539,9 @@ async def patch_room_role( room_store: Annotated[RoomStore, Depends(get_room_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[RoomRoleDetail]: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await protocol.room_role_store.edit_role( session, room_id, name, req.instructions, req.exclusive @@ -554,8 +560,9 @@ async def delete_room_role( room_store: Annotated[RoomStore, Depends(get_room_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> list[RoomRoleDetail]: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await protocol.room_role_store.delete_role(session, room_id, name) except ValueError as e: @@ -574,9 +581,10 @@ async def put_room_group( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: """Assign the room to a group, or make it standalone (`group_id=null`).""" - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_store.set_group(session, room_id, req.group_id) except ValueError as e: @@ -604,8 +612,9 @@ async def put_protection( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.update_protection_config(room_id, req.protection_config) except ValueError: @@ -630,8 +639,9 @@ async def put_observe( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.update_observe_config(room_id, req.observe_config) except ValueError: @@ -656,8 +666,9 @@ async def post_room_agents( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.add_agents_to_room( room_id, @@ -688,8 +699,9 @@ async def patch_room_agent( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.set_join_event_listeners( room_id, {agent_id: req.receives_join_events} @@ -715,8 +727,9 @@ async def delete_room_agent( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.remove_agents_from_room(room_id, [agent_id]) except ValueError as e: @@ -741,8 +754,9 @@ async def post_room_users( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.add_users_to_room(room_id, req.user_names) except ValueError as e: @@ -766,8 +780,9 @@ async def _set_archived( external_user_store: ExternalUserStore, protocol: ProtocolService, user: User, + is_admin: bool, ) -> RoomDetail: - await _require_room(session, room_store, room_id, user, "write") + await _require_room(session, room_store, room_id, user, "write", is_admin) try: await room_service.set_room_archived(room_id, archived) except ValueError: @@ -790,8 +805,9 @@ async def archive_room( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - """Archive a room: hide it from the default active list. Reversible and + """: hide it from the default active list. Reversible and metadata-only — the Matrix room, members, and bridge channel are intact.""" return await _set_archived( room_id, @@ -803,6 +819,7 @@ async def archive_room( external_user_store, protocol, user, + is_admin, ) @@ -816,8 +833,9 @@ async def unarchive_room( external_user_store: Annotated[ExternalUserStore, Depends(get_external_user_store)], protocol: Annotated[ProtocolService, Depends(get_protocol)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> RoomDetail: - """Unarchive a room: restore it to the active list.""" + """: restore it to the active list.""" return await _set_archived( room_id, False, @@ -828,6 +846,7 @@ async def unarchive_room( external_user_store, protocol, user, + is_admin, ) @@ -838,8 +857,9 @@ async def delete_room( room_service: Annotated[RoomService, Depends(get_room_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> dict[str, bool]: - await _require_room(session, room_store, room_id, user, "delete") + await _require_room(session, room_store, room_id, user, "delete", is_admin) try: await room_service.delete_room(room_id) except ValueError: @@ -853,10 +873,10 @@ async def bulk_delete_rooms( session: Annotated[AsyncSession, Depends(get_session)], room_service: Annotated[RoomService, Depends(get_room_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], - user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> BulkDeleteResponse: - principal = Principal(user.id, await user_store.administers(session, user)) + principal = Principal(user.id, is_admin) deleted = 0 for room_id in req.room_ids: room = await room_store.get(session, room_id) @@ -879,13 +899,13 @@ async def bulk_archive_rooms( session: Annotated[AsyncSession, Depends(get_session)], room_service: Annotated[RoomService, Depends(get_room_service)], room_store: Annotated[RoomStore, Depends(get_room_store)], - user_store: Annotated[UserStore, Depends(get_user_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> BulkArchiveResponse: """Archive or unarchive many rooms at once. Each room is authorized individually with the same `write` check as the single archive endpoint; rooms the caller cannot write (or that no longer exist) are skipped.""" - principal = Principal(user.id, await user_store.administers(session, user)) + principal = Principal(user.id, is_admin) updated = 0 for room_id in req.room_ids: room = await room_store.get(session, room_id) diff --git a/core/switch_core/gateway/templates.py b/core/switch_core/gateway/templates.py index 60c4917c9..44f9034b4 100644 --- a/core/switch_core/gateway/templates.py +++ b/core/switch_core/gateway/templates.py @@ -29,7 +29,7 @@ TemplateStore, ) from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user +from switch_core.gateway.auth import get_current_user, get_tenant_is_admin from switch_core.gateway.dependencies import ( get_config, get_session, @@ -155,7 +155,11 @@ def _require_within_size_limit(content: str, config: SwitchConfig) -> None: async def _load_for_management( - session: AsyncSession, store: TemplateStore, template_id: str, user: User + session: AsyncSession, + store: TemplateStore, + template_id: str, + user: User, + is_admin: bool, ) -> Template: """Fetch a template the caller is allowed to change, or fail saying why.""" template = await store.get(session, template_id) @@ -165,7 +169,7 @@ async def _load_for_management( ) try: require_manage( - Principal(user.id, await UserStore().administers(session, user)), + Principal(user.id, is_admin), template.owner_id, ) except PermissionError as e: @@ -306,8 +310,9 @@ async def patch_template( user_store: Annotated[UserStore, Depends(get_user_store)], config: Annotated[SwitchConfig, Depends(get_config)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> TemplateDetail: - await _load_for_management(session, template_store, template_id, user) + await _load_for_management(session, template_store, template_id, user, is_admin) if req.content is not None: _require_within_size_limit(req.content, config) _require_storable(req.content) @@ -336,8 +341,9 @@ async def delete_template( session: Annotated[AsyncSession, Depends(get_session)], template_store: Annotated[TemplateStore, Depends(get_template_store)], user: Annotated[User, Depends(get_current_user)], + is_admin: Annotated[bool, Depends(get_tenant_is_admin)], ) -> TemplateDeleteResponse: - await _load_for_management(session, template_store, template_id, user) + await _load_for_management(session, template_store, template_id, user, is_admin) try: await template_store.delete(session, template_id) except ValueError as e: diff --git a/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py b/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py index c684b3d6f..87b40b674 100644 --- a/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py +++ b/core/tests/switch_core/bridges/agent/protocol/test_agent_tenant_scoped_admin.py @@ -78,7 +78,7 @@ def start_client(self, client: Client) -> None: class _NoBridges: - def all_bridges(self) -> list[object]: + def bridges_for_tenant(self, tenant_id: str) -> list[object]: return [] diff --git a/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py b/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py index 7ed73cae0..b0656af7e 100644 --- a/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py +++ b/core/tests/switch_core/bridges/agent/protocol/test_reference_type_parity.py @@ -141,7 +141,8 @@ async def _seed( async def _slugs_from_route( session: AsyncSession, service: ResourceService, user: User ) -> set[str]: - listed = await route_list_types(session, service, _USER_STORE, user) + is_admin = await _USER_STORE.administers(session, user) + listed = await route_list_types(session, service, _USER_STORE, user, is_admin) return {info.type for info in listed} diff --git a/core/tests/switch_core/db/test_tenant_admin_role_split.py b/core/tests/switch_core/db/test_tenant_admin_role_split.py index c76cabcc1..9f87d889f 100644 --- a/core/tests/switch_core/db/test_tenant_admin_role_split.py +++ b/core/tests/switch_core/db/test_tenant_admin_role_split.py @@ -6,7 +6,7 @@ checked the global bit, never the caller's role in the tenant the request is bound to. -These tests pin the four behavioural claims that split makes true: +These tests pin the behavioural claims that split makes true: - a workspace `owner` can administer resources in *their own* tenant; - the same person has no such power in a tenant they only hold a `member` @@ -15,7 +15,9 @@ - a deployment operator (`users.role == "admin"`) keeps its unconditional bypass regardless of what `tenant_members` says, in every tenant, with or without a membership row at all; -- a plain `member` with no operator bit gains nothing. +- a plain `member` with no operator bit gains nothing; +- and with no tenant bound at all the question is refused rather than + answered on the operator bit alone, for operators too. Each is proven both at the pure-decision level (`authz.administers_tenant`, covered in `test_authz.py`) and here, against a real Postgres session and a @@ -27,12 +29,13 @@ from __future__ import annotations +import pytest from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker from switch_core.authz import Principal, can from switch_core.db.models import TENANT_ZERO_ID, Room, Tenant, TenantMember, User from switch_core.db.stores.user_store import UserStore -from switch_core.tenant_context import tenant_scope +from switch_core.tenant_context import no_tenant, tenant_scope TENANT_B = "tenant-b" @@ -147,6 +150,40 @@ async def test_no_membership_row_in_the_bound_tenant_gains_nothing( assert await store.administers(session, stray) is False + async def test_unbound_is_refused_rather_than_answered_on_the_operator_bit( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + # Answering here would mean answering a question that was not asked: + # "may this person administer" has no deployment-wide form. The + # tempting fallback — the operator bit alone — silently strips a + # workspace owner of their role, and the 403 that follows names no + # cause. Same call as the one #442 made for the bridge identities. + store = UserStore() + async with session_factory() as session: + owner = await _make_user(session, "unbound-owner") + await _membership(session, TENANT_ZERO_ID, owner.id, "owner") + await session.commit() + + with no_tenant(): + with pytest.raises(RuntimeError, match="requires a bound tenant"): + await store.administers(session, owner) + + async def test_unbound_is_refused_for_an_operator_too( + self, session_factory: async_sessionmaker[AsyncSession] + ) -> None: + # The operator bypass is the one answer that *would* have been right + # without a tenant, which is exactly why it must not be given: a + # caller that reaches here unbound has a bug, and returning True for + # the most privileged accounts is the worst moment to hide one. + store = UserStore() + async with session_factory() as session: + operator = await _make_user(session, "unbound-operator", role="admin") + await session.commit() + + with no_tenant(): + with pytest.raises(RuntimeError, match="requires a bound tenant"): + await store.administers(session, operator) + class TestRoomAuthzThroughTenantRole: """The same claims, one layer up: a real `authz.can` decision on a real diff --git a/core/tests/switch_core/gateway/agent_route_harness.py b/core/tests/switch_core/gateway/agent_route_harness.py index f39af1c7a..697437dbf 100644 --- a/core/tests/switch_core/gateway/agent_route_harness.py +++ b/core/tests/switch_core/gateway/agent_route_harness.py @@ -9,6 +9,19 @@ from sqlalchemy.ext.asyncio import AsyncSession from switch_core.db.models import Agent, ApiKey, Client, User +from switch_core.db.stores.user_store import UserStore + +_USER_STORE = UserStore() + + +async def is_admin(session: AsyncSession, user: User) -> bool: + """The bit `get_tenant_is_admin` hands a route, resolved the same way. + + A route takes it as an argument now, so a test calling the coroutine + directly has to supply one. Reading it through the store rather than + passing a literal keeps a `role="admin"` on the user row meaningful. + """ + return await _USER_STORE.administers(session, user) async def add_user(session: AsyncSession, *, name: str, role: str = "user") -> User: diff --git a/core/tests/switch_core/gateway/test_agent_display_name_route.py b/core/tests/switch_core/gateway/test_agent_display_name_route.py index d461993d2..474159bcc 100644 --- a/core/tests/switch_core/gateway/test_agent_display_name_route.py +++ b/core/tests/switch_core/gateway/test_agent_display_name_route.py @@ -15,7 +15,11 @@ from switch_core.db.stores.agent_store import AgentStore from switch_core.gateway.agents import update_agent_display_name from switch_core.gateway.schemas import UpdateAgentDisplayNameRequest -from tests.switch_core.gateway.agent_route_harness import add_agent, add_user +from tests.switch_core.gateway.agent_route_harness import ( + add_agent, + add_user, + is_admin, +) _AGENT_STORE = AgentStore() @@ -37,6 +41,7 @@ async def test_owner_can_set_a_display_name( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.display_name == _NAME @@ -59,6 +64,7 @@ async def test_surrounding_whitespace_is_trimmed( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.display_name == _NAME @@ -78,6 +84,7 @@ async def test_owner_can_change_an_existing_display_name( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.display_name == _OTHER_NAME @@ -100,6 +107,7 @@ async def test_null_or_blank_clears_it( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.display_name is None @@ -122,6 +130,7 @@ async def test_non_owner_is_refused( session, _AGENT_STORE, other, + await is_admin(session, other), ) assert exc.value.status_code == 403 @@ -143,6 +152,7 @@ async def test_admin_can_set_one_on_an_agent_they_do_not_own( session, _AGENT_STORE, admin, + await is_admin(session, admin), ) assert summary.display_name == _NAME @@ -160,6 +170,7 @@ async def test_unknown_agent_is_404( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert exc.value.status_code == 404 @@ -189,6 +200,7 @@ async def test_unsafe_names_are_400_not_stored( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert exc.value.status_code == 400 diff --git a/core/tests/switch_core/gateway/test_agent_icon_route.py b/core/tests/switch_core/gateway/test_agent_icon_route.py index 673541a56..4cc002405 100644 --- a/core/tests/switch_core/gateway/test_agent_icon_route.py +++ b/core/tests/switch_core/gateway/test_agent_icon_route.py @@ -15,7 +15,11 @@ from switch_core.db.stores.agent_store import AgentStore from switch_core.gateway.agents import update_agent_icon from switch_core.gateway.schemas import UpdateAgentIconRequest -from tests.switch_core.gateway.agent_route_harness import add_agent, add_user +from tests.switch_core.gateway.agent_route_harness import ( + add_agent, + add_user, + is_admin, +) _AGENT_STORE = AgentStore() @@ -37,6 +41,7 @@ async def test_owner_can_set_an_icon( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.icon_url == _ICON @@ -59,6 +64,7 @@ async def test_owner_can_change_an_existing_icon( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.icon_url == _OTHER_ICON @@ -80,6 +86,7 @@ async def test_null_clears_the_icon( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.icon_url is None @@ -102,6 +109,7 @@ async def test_blank_string_clears_rather_than_storing_empty( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert summary.icon_url is None @@ -121,6 +129,7 @@ async def test_non_owner_is_refused( session, _AGENT_STORE, other, + await is_admin(session, other), ) assert exc.value.status_code == 403 @@ -142,6 +151,7 @@ async def test_admin_can_set_an_icon_on_an_agent_they_do_not_own( session, _AGENT_STORE, admin, + await is_admin(session, admin), ) assert summary.icon_url == _ICON @@ -159,6 +169,7 @@ async def test_unknown_agent_is_404( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert exc.value.status_code == 404 @@ -192,6 +203,7 @@ async def test_unsafe_url_is_rejected_and_nothing_is_stored( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) assert exc.value.status_code == 400 @@ -215,6 +227,7 @@ async def test_a_rejected_change_leaves_the_previous_icon_intact( session, _AGENT_STORE, owner, + await is_admin(session, owner), ) stored = await _AGENT_STORE.get(session, agent.id) diff --git a/core/tests/switch_core/gateway/test_collaborations_authz.py b/core/tests/switch_core/gateway/test_collaborations_authz.py index 400f3c417..aeeeb8b53 100644 --- a/core/tests/switch_core/gateway/test_collaborations_authz.py +++ b/core/tests/switch_core/gateway/test_collaborations_authz.py @@ -138,6 +138,7 @@ async def test_claiming_for_another_user_is_refused(self) -> None: user_store=_StubGet(None), # type: ignore[arg-type] collab_lifecycle=object(), # type: ignore[arg-type] user=_user(id="me", role="user"), + is_admin=False, ) assert excinfo.value.status_code == 403 @@ -153,6 +154,7 @@ async def test_releasing_another_users_claim_is_refused(self) -> None: external_user_store=_StubGet(theirs), # type: ignore[arg-type] user_store=_StubGet(None), # type: ignore[arg-type] user=_user(id="me", role="user"), + is_admin=False, user_id="someone-else", ) assert excinfo.value.status_code == 403 diff --git a/core/tests/switch_core/gateway/test_connectors.py b/core/tests/switch_core/gateway/test_connectors.py index 39f95eff1..a1b19b58d 100644 --- a/core/tests/switch_core/gateway/test_connectors.py +++ b/core/tests/switch_core/gateway/test_connectors.py @@ -7,8 +7,16 @@ from switch_core.db.models import ApiKey, ServerConnector, User from switch_core.db.stores.api_key_store import ApiKeyStore from switch_core.db.stores.server_connector_store import ServerConnectorStore +from switch_core.db.stores.user_store import UserStore from switch_core.gateway.connectors import delete_connector +_USER_STORE = UserStore() + + +async def _is_admin(session: AsyncSession, user: User) -> bool: + """The bit `get_tenant_is_admin` hands the route, resolved the same way.""" + return await _USER_STORE.administers(session, user) + class _StubLifecycle: """Records remove() calls so we can assert whether deletion happened.""" @@ -78,6 +86,7 @@ async def test_non_owner_non_admin_is_forbidden( api_key_store=ApiKeyStore(), connector_lifecycle=lifecycle, # type: ignore[arg-type] user=other, + is_admin=await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -99,6 +108,7 @@ async def test_owner_can_delete( api_key_store=ApiKeyStore(), connector_lifecycle=lifecycle, # type: ignore[arg-type] user=owner, + is_admin=await _is_admin(session, owner), ) assert result == {"ok": True} @@ -121,6 +131,7 @@ async def test_admin_can_delete_others_connector( api_key_store=ApiKeyStore(), connector_lifecycle=lifecycle, # type: ignore[arg-type] user=admin, + is_admin=await _is_admin(session, admin), ) assert result == {"ok": True} @@ -142,6 +153,7 @@ async def test_missing_connector_is_not_found( api_key_store=ApiKeyStore(), connector_lifecycle=lifecycle, # type: ignore[arg-type] user=user, + is_admin=await _is_admin(session, user), ) assert exc.value.status_code == 404 diff --git a/core/tests/switch_core/gateway/test_reference_types_routes.py b/core/tests/switch_core/gateway/test_reference_types_routes.py index 7ffdaa416..58f81fb54 100644 --- a/core/tests/switch_core/gateway/test_reference_types_routes.py +++ b/core/tests/switch_core/gateway/test_reference_types_routes.py @@ -20,7 +20,7 @@ from switch_core.db.stores.reference_type_store import ReferenceTypeStore from switch_core.db.stores.room_link_store import RoomLinkStore from switch_core.db.stores.user_store import UserStore -from switch_core.gateway.auth import get_current_user +from switch_core.gateway.auth import get_current_user, get_tenant_is_admin from switch_core.gateway.dependencies import ( get_config, get_resource_service, @@ -49,6 +49,11 @@ _REFERENCE_STORE = ReferenceStore() +async def _is_admin(session: AsyncSession, user: User) -> bool: + """The bit `get_tenant_is_admin` hands the route, resolved the same way.""" + return await _USER_STORE.administers(session, user) + + def _resource_service( session_factory: async_sessionmaker[AsyncSession], ) -> ResourceService: @@ -142,12 +147,13 @@ def _app() -> FastAPI: # stubbed. It is never called: a request with no cookie is refused # before it is touched. app.dependency_overrides[get_session_factory] = lambda: None - # A real store, not `None`: authorizing now reads `UserStore.administers` - # before the route reaches the resource service. With no tenant bound — - # this app never runs `get_current_user`'s `tenant_scope` — that resolves - # from the operator bit alone without touching the stubbed session, so a - # real instance is safe here and a bare `None` no longer is. app.dependency_overrides[get_user_store] = lambda: UserStore() + # Authorizing reads the tenant-admin bit before the route reaches the + # resource service, and this app never runs `get_current_user`'s + # `tenant_scope`, so the real dependency would refuse rather than answer + # (`UserStore.administers`). These tests are about routing and body + # validation, so the bit is supplied directly. + app.dependency_overrides[get_tenant_is_admin] = lambda: False app.dependency_overrides[get_config] = lambda: None app.dependency_overrides[get_resource_service] = lambda: None return app @@ -219,7 +225,9 @@ async def test_create_then_list_and_own( assert created.shadowed_by_builtin is False assert created.value_schema["properties"]["urls"] - listed = await list_reference_types(session, svc, _USER_STORE, alice) + listed = await list_reference_types( + session, svc, _USER_STORE, alice, await _is_admin(session, alice) + ) by_slug = {t.type: t for t in listed} assert "notion" in by_slug assert by_slug["notion"].owner_name == "alice" @@ -228,7 +236,9 @@ async def test_create_then_list_and_own( assert by_slug["github"].owner_name is None assert by_slug["github"].value_hint - owned = await list_owned_reference_types(session, svc, _USER_STORE, alice) + owned = await list_owned_reference_types( + session, svc, _USER_STORE, alice, await _is_admin(session, alice) + ) assert [t.type for t in owned] == ["notion"] async def test_a_private_type_is_invisible_to_another_user( @@ -242,11 +252,16 @@ async def test_a_private_type_is_invisible_to_another_user( _create_request("notion"), session, svc, _USER_STORE, alice ) - listed = await list_reference_types(session, svc, _USER_STORE, bob) + listed = await list_reference_types( + session, svc, _USER_STORE, bob, await _is_admin(session, bob) + ) assert "notion" not in {t.type for t in listed} assert ( - await list_owned_reference_types(session, svc, _USER_STORE, bob) == [] + await list_owned_reference_types( + session, svc, _USER_STORE, bob, await _is_admin(session, bob) + ) + == [] ) async def test_invalid_slug_is_400( @@ -314,6 +329,7 @@ async def test_patch_updates_and_a_non_owner_is_403( svc, _USER_STORE, alice, + await _is_admin(session, alice), ) assert updated.display_name == "Notion Workspace" @@ -325,6 +341,7 @@ async def test_patch_updates_and_a_non_owner_is_403( svc, _USER_STORE, bob, + await _is_admin(session, bob), ) assert exc.value.status_code == 403 @@ -343,6 +360,7 @@ async def test_patch_of_an_unknown_slug_is_404( svc, _USER_STORE, alice, + await _is_admin(session, alice), ) assert exc.value.status_code == 404 @@ -358,12 +376,15 @@ async def test_delete_removes_an_unused_type( ) response = await delete_reference_type( - "notion", session, svc, _USER_STORE, alice + "notion", session, svc, alice, await _is_admin(session, alice) ) assert response.deleted_type == "notion" assert ( - await list_owned_reference_types(session, svc, _USER_STORE, alice) == [] + await list_owned_reference_types( + session, svc, _USER_STORE, alice, await _is_admin(session, alice) + ) + == [] ) async def test_delete_of_a_type_in_use_is_409( @@ -390,10 +411,13 @@ async def test_delete_of_a_type_in_use_is_409( svc, _USER_STORE, alice, + await _is_admin(session, alice), ) with pytest.raises(HTTPException) as exc: - await delete_reference_type("notion", session, svc, _USER_STORE, alice) + await delete_reference_type( + "notion", session, svc, alice, await _is_admin(session, alice) + ) assert exc.value.status_code == 409 assert "cannot be deleted" in exc.value.detail @@ -428,11 +452,19 @@ async def test_a_resolvable_slug_carries_its_display_name( svc, _USER_STORE, alice, + await _is_admin(session, alice), ) assert created.type_display_name == "Notion" - fetched = await get_reference(created.id, session, svc, _USER_STORE, alice) + fetched = await get_reference( + created.id, + session, + svc, + _USER_STORE, + alice, + await _is_admin(session, alice), + ) assert fetched.type_display_name == "Notion" async def test_an_unresolvable_slug_maps_to_none( diff --git a/core/tests/switch_core/gateway/test_room_resource_read_authz.py b/core/tests/switch_core/gateway/test_room_resource_read_authz.py index fd9dbce9b..d1d32fb20 100644 --- a/core/tests/switch_core/gateway/test_room_resource_read_authz.py +++ b/core/tests/switch_core/gateway/test_room_resource_read_authz.py @@ -36,6 +36,11 @@ _AGENT_STORE = AgentStore() +async def _is_admin(session: AsyncSession, user: User) -> bool: + """The bit `get_tenant_is_admin` hands the route, resolved the same way.""" + return await _USER_STORE.administers(session, user) + + def _svc(session_factory: async_sessionmaker[AsyncSession]) -> ResourceService: return ResourceService( reference_store=ReferenceStore(), @@ -88,6 +93,7 @@ async def test_non_owner_cannot_read_room_document( _AGENT_STORE, _ROOM_STORE, other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -109,6 +115,7 @@ async def test_non_owner_cannot_list_room_documents( _AGENT_STORE, _ROOM_STORE, other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -123,7 +130,13 @@ async def test_non_owner_cannot_detach_room_document( with pytest.raises(HTTPException) as exc: await detach_document_from_room( - room.id, "any-doc", session, svc, _ROOM_STORE, other + room.id, + "any-doc", + session, + svc, + _ROOM_STORE, + other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -138,7 +151,13 @@ async def test_non_owner_cannot_detach_reference( with pytest.raises(HTTPException) as exc: await detach_reference_from_room( - room.id, "any-ref", session, svc, _ROOM_STORE, other + room.id, + "any-ref", + session, + svc, + _ROOM_STORE, + other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -153,7 +172,13 @@ async def test_non_owner_cannot_detach_package( with pytest.raises(HTTPException) as exc: await detach_package_from_room( - room.id, "any-pkg", session, svc, _ROOM_STORE, other + room.id, + "any-pkg", + session, + svc, + _ROOM_STORE, + other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -176,6 +201,7 @@ async def test_owner_read_passes_authz( _AGENT_STORE, _ROOM_STORE, owner, + await _is_admin(session, owner), ) == [] ) @@ -189,5 +215,6 @@ async def test_owner_read_passes_authz( _AGENT_STORE, _ROOM_STORE, owner, + await _is_admin(session, owner), ) assert exc.value.status_code == 404 diff --git a/core/tests/switch_core/gateway/test_room_write_authz.py b/core/tests/switch_core/gateway/test_room_write_authz.py index 1b352078d..0cd97809a 100644 --- a/core/tests/switch_core/gateway/test_room_write_authz.py +++ b/core/tests/switch_core/gateway/test_room_write_authz.py @@ -31,6 +31,16 @@ _USER_STORE = UserStore() +async def _is_admin(session: AsyncSession, user: User) -> bool: + """The bit `get_tenant_is_admin` hands the route, resolved the same way. + + Passing a literal here would test the handler against an admin bit no + dependency could have produced; reading it through the store keeps the + role on the user row meaningful. + """ + return await _USER_STORE.administers(session, user) + + def _resource_service( session_factory: async_sessionmaker[AsyncSession], ) -> ResourceService: @@ -103,7 +113,14 @@ async def test_non_owner_cannot_attach_to_private_room( with pytest.raises(HTTPException) as exc: await attach_reference_to_room( - room.id, ref.id, session, svc, _ROOM_STORE, _USER_STORE, other + room.id, + ref.id, + session, + svc, + _ROOM_STORE, + _USER_STORE, + other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -119,7 +136,14 @@ async def test_owner_can_attach( svc = _resource_service(session_factory) detail = await attach_reference_to_room( - room.id, ref.id, session, svc, _ROOM_STORE, _USER_STORE, owner + room.id, + ref.id, + session, + svc, + _ROOM_STORE, + _USER_STORE, + owner, + await _is_admin(session, owner), ) assert detail.id == ref.id @@ -137,7 +161,14 @@ async def test_admin_can_attach_to_room_they_dont_own( svc = _resource_service(session_factory) detail = await attach_reference_to_room( - room.id, ref.id, session, svc, _ROOM_STORE, _USER_STORE, admin + room.id, + ref.id, + session, + svc, + _ROOM_STORE, + _USER_STORE, + admin, + await _is_admin(session, admin), ) assert detail.id == ref.id @@ -161,7 +192,14 @@ async def test_public_write_room_allows_non_owner( svc = _resource_service(session_factory) detail = await attach_reference_to_room( - room.id, ref.id, session, svc, _ROOM_STORE, _USER_STORE, other + room.id, + ref.id, + session, + svc, + _ROOM_STORE, + _USER_STORE, + other, + await _is_admin(session, other), ) assert detail.id == ref.id @@ -176,7 +214,14 @@ async def test_missing_room_is_404( with pytest.raises(HTTPException) as exc: await attach_reference_to_room( - "missing-room", ref.id, session, svc, _ROOM_STORE, _USER_STORE, user + "missing-room", + ref.id, + session, + svc, + _ROOM_STORE, + _USER_STORE, + user, + await _is_admin(session, user), ) assert exc.value.status_code == 404 @@ -196,7 +241,13 @@ async def test_non_owner_cannot_link( with pytest.raises(HTTPException) as exc: await create_linked_room( - source.id, req, session, svc, _ROOM_STORE, other + source.id, + req, + session, + svc, + _ROOM_STORE, + other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -213,7 +264,13 @@ async def test_owner_can_link( req = LinkedRoomCreateRequest(target_room_id=target.id, label="rel") detail = await create_linked_room( - source.id, req, session, svc, _ROOM_STORE, owner + source.id, + req, + session, + svc, + _ROOM_STORE, + owner, + await _is_admin(session, owner), ) assert detail.target_room_id == target.id @@ -237,7 +294,13 @@ async def test_non_owner_cannot_delete_link( with pytest.raises(HTTPException) as exc: await delete_linked_room( - source.id, target.id, session, svc, _ROOM_STORE, other + source.id, + target.id, + session, + svc, + _ROOM_STORE, + other, + await _is_admin(session, other), ) assert exc.value.status_code == 403 @@ -257,7 +320,13 @@ async def test_owner_can_delete_link( ) resp = await delete_linked_room( - source.id, target.id, session, svc, _ROOM_STORE, owner + source.id, + target.id, + session, + svc, + _ROOM_STORE, + owner, + await _is_admin(session, owner), ) assert resp.status_code == 204 diff --git a/core/tests/switch_core/gateway/test_template_routes.py b/core/tests/switch_core/gateway/test_template_routes.py index 490c37de6..4b42d6330 100644 --- a/core/tests/switch_core/gateway/test_template_routes.py +++ b/core/tests/switch_core/gateway/test_template_routes.py @@ -31,6 +31,12 @@ _TEMPLATE_STORE = TemplateStore() _USER_STORE = UserStore() + +async def _is_admin(session: AsyncSession, user: object) -> bool: + """The bit `get_tenant_is_admin` hands the route, resolved the same way.""" + return await _USER_STORE.administers(session, user) # type: ignore[arg-type] + + # Awkward on purpose: CRLF, a tab, trailing spaces, unicode, no final newline. _AWKWARD_DOCUMENT = ( "params:\r\n" @@ -290,6 +296,7 @@ async def test_deleting_someone_elses_template_is_refused( session, _TEMPLATE_STORE, bob, + await _is_admin(session, bob), ) assert exc.value.status_code == 403 assert await _TEMPLATE_STORE.get(session, created.id) is not None # type: ignore[attr-defined] @@ -312,6 +319,7 @@ async def test_editing_someone_elses_template_is_refused( _USER_STORE, _config(), bob, + await _is_admin(session, bob), ) assert exc.value.status_code == 403 @@ -349,6 +357,7 @@ async def test_an_owner_may_delete_their_own( session, _TEMPLATE_STORE, alice, + await _is_admin(session, alice), ) assert result.deleted_id == created.id # type: ignore[attr-defined] assert await _TEMPLATE_STORE.get(session, created.id) is None # type: ignore[attr-defined] @@ -367,6 +376,7 @@ async def test_an_admin_may_delete_anyones( session, _TEMPLATE_STORE, admin, + await _is_admin(session, admin), ) assert await _TEMPLATE_STORE.get(session, created.id) is None # type: ignore[attr-defined] @@ -387,6 +397,7 @@ async def test_an_admin_may_edit_anyones( _USER_STORE, _config(), admin, + await _is_admin(session, admin), ) assert updated.description == "tidied up by an admin" # Still Alice's — an admin edit is not a transfer of ownership. @@ -400,7 +411,13 @@ async def test_deleting_a_missing_template_is_a_404( await session.commit() with pytest.raises(HTTPException) as exc: - await delete_template("nope", session, _TEMPLATE_STORE, alice) + await delete_template( + "nope", + session, + _TEMPLATE_STORE, + alice, + await _is_admin(session, alice), + ) assert exc.value.status_code == 404 @@ -421,6 +438,7 @@ async def test_replacing_content_bumps_the_version( _USER_STORE, _config(), alice, + await _is_admin(session, alice), ) assert updated.version == 2 assert updated.content == "room:\n name: changed\n" @@ -441,6 +459,7 @@ async def test_a_metadata_edit_leaves_the_version_alone( _USER_STORE, _config(), alice, + await _is_admin(session, alice), ) assert updated.version == 1 assert updated.description == "clearer" @@ -462,6 +481,7 @@ async def test_an_oversize_replacement_is_refused( _USER_STORE, _config(template_max_bytes=100), alice, + await _is_admin(session, alice), ) assert exc.value.status_code == 413 stored = await _TEMPLATE_STORE.get(session, created.id) # type: ignore[attr-defined] @@ -485,5 +505,6 @@ async def test_renaming_onto_a_name_you_already_use_is_a_conflict( _USER_STORE, _config(), alice, + await _is_admin(session, alice), ) assert exc.value.status_code == 409 diff --git a/core/tests/switch_core/test_rooms_yaml.py b/core/tests/switch_core/test_rooms_yaml.py index f4ceed200..8aaa0d3b2 100644 --- a/core/tests/switch_core/test_rooms_yaml.py +++ b/core/tests/switch_core/test_rooms_yaml.py @@ -1052,12 +1052,13 @@ async def test_endpoint_json_body(env): from fastapi import HTTPException - from switch_core.db.stores.user_store import UserStore from switch_core.gateway.rooms import create_room_from_yaml svc = _svc(env) user_id = env["user_id"] - user_store = UserStore() + # Not an administrator of anything: this exercises body parsing, and the + # caller owns the room it creates. + is_admin = False user = User(name="alice", email="alice@example.com", role="member") # Poke the id to match the seeded user so provision works. object.__setattr__(user, "id", user_id) @@ -1074,7 +1075,7 @@ async def test_endpoint_json_body(env): request.body.return_value = body async with env["session_factory"]() as session: - result = await create_room_from_yaml(request, session, svc, user_store, user) + result = await create_room_from_yaml(request, session, svc, user, is_admin) assert result.room_name == "carol local-deploy" # Non-string yaml value → 400. @@ -1082,5 +1083,5 @@ async def test_endpoint_json_body(env): request.body.return_value = bad_body async with env["session_factory"]() as session: with pytest.raises(HTTPException) as exc_info: - await create_room_from_yaml(request, session, svc, user_store, user) + await create_room_from_yaml(request, session, svc, user, is_admin) assert exc_info.value.status_code == 400 diff --git a/docs/old/multi-tenancy-phase2-tenants.md b/docs/old/multi-tenancy-phase2-tenants.md index ebc636b93..931048675 100644 --- a/docs/old/multi-tenancy-phase2-tenants.md +++ b/docs/old/multi-tenancy-phase2-tenants.md @@ -70,6 +70,37 @@ This is a bigger change than the other three tickets put together, and it is the reason they cannot ship without it. It should be its own ticket, sequenced first. **This is the decision I want confirmed before anything is built.** +### What a workspace admin actually gets, which is more than a list of routes + +The tempting way to describe this grant is to enumerate the admin-gated routes +and classify each one. That description is incomplete, and incomplete in the +direction that matters. + +`authz.can()` and `can_manage()` both short-circuit on `Principal.is_admin` +*before* they look at ownership or visibility. The moment `tenant_members.role` +feeds that bit, an `owner` or `admin` of a workspace holds read, write and +delete on **every owned resource in it** — rooms, references, documents, +packages — and management of every agent in it, whoever owns them. No route +opts into this; it falls out of the one short-circuit. The widest path is +`require_room_access`, which is not an explicitly admin-gated route at all. + +**The least obvious case, and the one to decide on purpose: a private room +between a colleague and their agent.** A workspace admin can read it, write in +it, and delete it. "Workspace administrators can read private rooms in their +workspace" is a sentence someone should agree to rather than discover. + +This is invisible in tenant zero today. After the backfill migration the only +`owner`/`admin` rows belong to deployment operators, who held all of it through +the global bypass already. It becomes real the first time a person is invited +into a workspace as an admin — that is, with §5. + +**Chosen: accept it for this phase, and write it down here rather than leave it +implicit.** The alternative is a second, narrower bit — "administers the +workspace's configuration" as distinct from "may act on its contents" — which +is a real distinction and a real amount of work, and belongs to whichever phase +takes on resource-level sharing. What must not happen is shipping the wide +grant while the design describes the narrow one. + ## 3. How a request picks its tenant A request must act in exactly one tenant — Phase 1 depends on that and nothing