diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 0d616cd1..09e451fa 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -204,6 +204,13 @@ there for code that inspects a principal's roles. A role maps only what it is given — map `owner` and `admin` too if they should hold a member's permissions. +Role→permission grants stay global and combine with these mappings: a user's +platform roles and their active `tenant:` resolve through the same +registry, additively. The `tenant:` prefix is reserved — the `users` `Role` +model refuses a name that starts with it, so no platform role (and no +`RolePermission` row, and nothing `sync_admin_all_permissions` writes) can +collide with a tenant role (#377). + ## Testing The `simple_module_test` plugin ships `tenant_client` (needs the `users` and diff --git a/modules/tenants/tests/test_tenant_role_permissions.py b/modules/tenants/tests/test_tenant_role_permissions.py new file mode 100644 index 00000000..81f15a29 --- /dev/null +++ b/modules/tenants/tests/test_tenant_role_permissions.py @@ -0,0 +1,113 @@ +"""Tenant roles never alias platform roles in the permission chain (#377). + +A tenant membership role reaches the principal as ``tenant:``; the +registry maps that onto tenant permissions only, and the platform ``admin`` +wildcard stays a different principal entirely. +""" + +from __future__ import annotations + +import pytest +from fastapi import Request +from simple_module_core.permissions import ADMIN_ROLE, WILDCARD +from simple_module_core.tenancy import TENANT_ROLE_PREFIX, TenantRole, is_tenant_role, tenant_role +from simple_module_hosting.permissions import resolved_permissions_for +from tenants.constants import ROLE_PERMISSIONS +from users.models import Role + + +@pytest.fixture +def probe(app): + """``GET /__probe`` -> the roles and resolved permissions the request holds.""" + + @app.get("/__probe") + async def _probe(request: Request): + user = request.state.user + return { + "roles": list(user.roles), + "permissions": sorted(resolved_permissions_for(request)), + } + + return "/__probe" + + +class TestReservedRoleNames: + @pytest.mark.parametrize("name", ["tenant:admin", "tenant:owner", "tenant:custom", "TENANT:x"]) + def test_constructing_a_role_in_the_namespace_is_refused(self, name): + with pytest.raises(ValueError, match="reserved for tenant roles"): + Role(name=name) + + def test_renaming_a_role_into_the_namespace_is_refused(self): + role = Role(name="editor") + with pytest.raises(ValueError, match="reserved for tenant roles"): + role.name = f"{TENANT_ROLE_PREFIX}admin" + assert role.name == "editor" + + def test_lookalikes_outside_the_namespace_are_fine(self): + for name in ("admin", "tenant", "tenants:admin", "my-tenant:admin"): + assert Role(name=name).name == name + + +class TestTenantAdminIsNotPlatformAdmin: + def test_tenant_roles_hold_no_wildcard_and_are_distinct_from_admin(self, app): + role_map = app.state.sm.permissions.role_map + assert WILDCARD in role_map[ADMIN_ROLE] + for role in TenantRole: + key = tenant_role(role) + assert key != ADMIN_ROLE and is_tenant_role(key) + assert WILDCARD not in role_map[key] + assert set(role_map[key]) == set(ROLE_PERMISSIONS[role]) + assert not any(".platform." in p for p in role_map[key]) + + async def test_tenant_admin_is_refused_platform_routes(self, tenant_client): + async with tenant_client("admin") as admin: + assert (await admin.client.get("/api/tenants/admin/")).status_code == 403 + assert (await admin.client.get("/api/tenants/current/members")).status_code == 200 + + async def test_platform_admin_still_holds_everything(self, authenticated_client): + assert (await authenticated_client.get("/api/tenants/admin/")).status_code == 200 + + +class TestSyncAdminAllPermissions: + async def test_never_touches_tenant_roles(self, app, db_session): + from permissions.models import RolePermission + from permissions.service import PermissionService + from sqlalchemy import select + from users.constants import ADMIN_ROLE_NAME + + registry = app.state.sm.permissions + before = {r: sorted(p) for r, p in registry.role_map.items() if is_tenant_role(r)} + assert before # the tenants module mapped them + + await PermissionService(db_session, registry).sync_admin_all_permissions() + await db_session.flush() + + rows = (await db_session.execute(select(RolePermission.role_name).distinct())).scalars() + assert set(rows) == {ADMIN_ROLE_NAME} + after = {r: sorted(p) for r, p in registry.role_map.items() if is_tenant_role(r)} + assert after == before + + +class TestEffectivePermissions: + @pytest.mark.parametrize("role", list(TenantRole)) + async def test_member_gets_registry_mapping_and_no_platform_extras( + self, tenant_client, probe, app, role + ): + role_map = app.state.sm.permissions.role_map + async with tenant_client(role) as m: + body = (await m.client.get(probe)).json() + assert tenant_role(role) in body["roles"] + assert ADMIN_ROLE not in body["roles"] + platform = [r for r in body["roles"] if not is_tenant_role(r)] + expected = set(role_map[tenant_role(role)]) + for r in platform: # the fixture user's ordinary ``user`` role, if mapped + expected |= set(role_map.get(r, [])) + assert set(body["permissions"]) == expected + assert WILDCARD not in body["permissions"] + + async def test_only_the_active_tenants_role_applies(self, tenant_client, probe): + async with tenant_client("owner") as a, tenant_client("member") as b: + a_roles = (await a.client.get(probe)).json()["roles"] + b_roles = (await b.client.get(probe)).json()["roles"] + assert [r for r in a_roles if is_tenant_role(r)] == ["tenant:owner"] + assert [r for r in b_roles if is_tenant_role(r)] == ["tenant:member"] diff --git a/modules/users/users/models/role.py b/modules/users/users/models/role.py index f1d69160..ad41c81a 100644 --- a/modules/users/users/models/role.py +++ b/modules/users/users/models/role.py @@ -6,7 +6,9 @@ import uuid from fastapi_users_db_sqlalchemy.generics import GUID +from simple_module_core.tenancy import TENANT_ROLE_PREFIX from simple_module_db.mixins import AuditMixin +from sqlalchemy.orm import validates from sqlmodel import Field, Relationship from users.models._base import Base @@ -32,3 +34,17 @@ class Role(Base, AuditMixin, table=True): # ty: ignore[unsupported-base] back_populates="roles", sa_relationship_kwargs={"lazy": "noload"}, ) + + @validates("name") + def _reject_tenant_namespace(self, _key: str, value: str) -> str: + """``tenant:*`` is the synthetic per-tenant principal namespace (#377). + + A platform ``Role`` row with that prefix would be indistinguishable from + the active tenant's membership role on a principal, so the name is + refused at the model, whichever path (service, CLI, seed) sets it. + """ + if isinstance(value, str) and value.casefold().startswith(TENANT_ROLE_PREFIX): + raise ValueError( + f"Role names starting with {TENANT_ROLE_PREFIX!r} are reserved for tenant roles" + ) + return value