diff --git a/docs/framework/multi-tenancy.md b/docs/framework/multi-tenancy.md index 09e451fa..c2cca08d 100644 --- a/docs/framework/multi-tenancy.md +++ b/docs/framework/multi-tenancy.md @@ -211,6 +211,22 @@ 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). +## Feature flags + +`is_flag_enabled`, `flag_enabled` and `require_flag` read the request's tenant +(`request.state.tenant_id`), so a per-tenant override beats the system value, +which beats the definition default. Boot hydration loads every tenant's +overrides with no tenant bound — deliberately cross-tenant (the table is not +`MultiTenantMixin`), so keep it that way. No tenant role holds +`feature_flags.manage`; the tenant-override admin screens are platform-only. + +Screens that take a tenant id from the URL can vet it without importing +`tenants`: the module publishes `app.state.tenant_exists`, and +`await simple_module_core.tenancy.tenant_exists(app, tenant_id)` answers +`True`/`False`, or `None` when no module can say (the id is then accepted). +`feature_flags` uses it to 404 on an unknown tenant when setting or listing +overrides; clearing stays unvalidated so a stale override can still be removed. + ## Testing The `simple_module_test` plugin ships `tenant_client` (needs the `users` and diff --git a/framework/core/simple_module_core/tenancy.py b/framework/core/simple_module_core/tenancy.py index 11323388..00a79424 100644 --- a/framework/core/simple_module_core/tenancy.py +++ b/framework/core/simple_module_core/tenancy.py @@ -13,7 +13,9 @@ from __future__ import annotations +from collections.abc import Awaitable, Callable from enum import StrEnum +from typing import Any TENANT_ROLE_PREFIX = "tenant:" @@ -46,4 +48,29 @@ def is_tenant_role(role: str) -> bool: return role.startswith(TENANT_ROLE_PREFIX) -__all__ = ["TENANT_ROLE_PREFIX", "TenantRole", "is_tenant_role", "tenant_role"] +TenantExists = Callable[[str], Awaitable[bool]] +"""``async (tenant_id) -> bool``: whether a tenant with that id exists.""" + + +async def tenant_exists(app: Any, tenant_id: str) -> bool | None: + """Whether ``tenant_id`` names a known tenant, or ``None`` when nobody can say. + + A module that owns tenants (``tenants``) publishes ``app.state.tenant_exists`` + (a :data:`TenantExists`). Platform screens that take a tenant id from the + URL ask through this, so they can refuse a typo without importing that + module; with none installed the answer is ``None`` and they accept the id. + """ + check: TenantExists | None = getattr(app.state, "tenant_exists", None) + if check is None: + return None + return bool(await check(tenant_id)) + + +__all__ = [ + "TENANT_ROLE_PREFIX", + "TenantExists", + "TenantRole", + "is_tenant_role", + "tenant_exists", + "tenant_role", +] diff --git a/framework/core/tests/test_tenant_exists.py b/framework/core/tests/test_tenant_exists.py new file mode 100644 index 00000000..d7998958 --- /dev/null +++ b/framework/core/tests/test_tenant_exists.py @@ -0,0 +1,21 @@ +"""``tenant_exists``: the soft lookup platform screens use to vet a tenant id.""" + +from __future__ import annotations + +from types import SimpleNamespace + +from simple_module_core.tenancy import tenant_exists + + +async def test_none_when_no_module_publishes_a_directory(): + app = SimpleNamespace(state=SimpleNamespace()) + assert await tenant_exists(app, "t1") is None + + +async def test_delegates_to_the_published_callable(): + async def exists(tenant_id: str) -> bool: + return tenant_id == "t1" + + app = SimpleNamespace(state=SimpleNamespace(tenant_exists=exists)) + assert await tenant_exists(app, "t1") is True + assert await tenant_exists(app, "t2") is False diff --git a/modules/feature_flags/feature_flags/deps.py b/modules/feature_flags/feature_flags/deps.py index 84f87ca9..8d928b96 100644 --- a/modules/feature_flags/feature_flags/deps.py +++ b/modules/feature_flags/feature_flags/deps.py @@ -4,8 +4,9 @@ from typing import Annotated -from fastapi import Depends, Request +from fastapi import Depends, HTTPException, Request from simple_module_core.feature_flags import FeatureFlagRegistry +from simple_module_core.tenancy import tenant_exists from simple_module_db.deps import get_db from sqlalchemy.ext.asyncio import AsyncSession @@ -23,5 +24,18 @@ def get_feature_flag_registry(request: Request) -> FeatureFlagRegistry: return request.app.state.sm.feature_flags +async def require_known_tenant(request: Request, tenant_id: str | None = None) -> None: + """404 when a tenant-scope screen names a tenant that does not exist. + + Without it a typo in the URL persists an override for a tenant nobody has, + which nothing ever reads. The lookup goes through core's ``tenant_exists`` + (``app.state.tenant_exists``, published by the ``tenants`` module), so this + module does not depend on ``tenants``; with none installed any id is + accepted, as before. No id (system scope) is always fine. + """ + if tenant_id and await tenant_exists(request.app, tenant_id) is False: + raise HTTPException(status_code=404, detail="Tenant not found") + + FeatureFlagServiceDep = Annotated[FeatureFlagService, Depends(get_feature_flag_service)] FeatureFlagRegistryDep = Annotated[FeatureFlagRegistry, Depends(get_feature_flag_registry)] diff --git a/modules/feature_flags/feature_flags/endpoints/api.py b/modules/feature_flags/feature_flags/endpoints/api.py index ae5f1818..76e1d7ba 100644 --- a/modules/feature_flags/feature_flags/endpoints/api.py +++ b/modules/feature_flags/feature_flags/endpoints/api.py @@ -12,7 +12,11 @@ SCOPE_TENANT, ) from feature_flags.contracts.schemas import FeatureFlagView, ToggleRequest -from feature_flags.deps import FeatureFlagRegistryDep, FeatureFlagServiceDep +from feature_flags.deps import ( + FeatureFlagRegistryDep, + FeatureFlagServiceDep, + require_known_tenant, +) router = APIRouter() @@ -87,7 +91,10 @@ async def clear_override( @router.get( "/tenant/{tenant_id}", response_model=list[FeatureFlagView], - dependencies=[Depends(RequiresPermission(PERM_FEATURE_FLAGS_VIEW))], + dependencies=[ + Depends(RequiresPermission(PERM_FEATURE_FLAGS_VIEW)), + Depends(require_known_tenant), + ], ) async def list_flags_for_tenant( tenant_id: str, @@ -100,7 +107,10 @@ async def list_flags_for_tenant( @router.put( "/tenant/{tenant_id}/{name}", response_model=FeatureFlagView, - dependencies=[Depends(RequiresPermission(PERM_FEATURE_FLAGS_MANAGE))], + dependencies=[ + Depends(RequiresPermission(PERM_FEATURE_FLAGS_MANAGE)), + Depends(require_known_tenant), + ], ) async def set_tenant_override( tenant_id: str, @@ -118,6 +128,8 @@ async def set_tenant_override( return view +# Clearing is not validated: an override left behind for a tenant that has +# since gone must stay removable. @router.delete( "/tenant/{tenant_id}/{name}", status_code=204, diff --git a/modules/feature_flags/feature_flags/endpoints/views.py b/modules/feature_flags/feature_flags/endpoints/views.py index ab8e9ff5..b481aa7d 100644 --- a/modules/feature_flags/feature_flags/endpoints/views.py +++ b/modules/feature_flags/feature_flags/endpoints/views.py @@ -26,7 +26,11 @@ SCOPE_TENANT, SYSTEM_SCOPE_ID, ) -from feature_flags.deps import FeatureFlagRegistryDep, FeatureFlagServiceDep +from feature_flags.deps import ( + FeatureFlagRegistryDep, + FeatureFlagServiceDep, + require_known_tenant, +) router = APIRouter() @@ -61,7 +65,10 @@ def _scope_args(tenant_id: str | None) -> dict[str, str]: @router.get( "/", response_model=None, - dependencies=[Depends(RequiresPermission(PERM_FEATURE_FLAGS_VIEW))], + dependencies=[ + Depends(RequiresPermission(PERM_FEATURE_FLAGS_VIEW)), + Depends(require_known_tenant), + ], ) async def browse( request: Request, @@ -86,7 +93,10 @@ async def browse( @router.post( "/{name}/toggle", response_model=None, - dependencies=[Depends(RequiresPermission(PERM_FEATURE_FLAGS_MANAGE))], + dependencies=[ + Depends(RequiresPermission(PERM_FEATURE_FLAGS_MANAGE)), + Depends(require_known_tenant), + ], ) async def toggle_action( name: str, diff --git a/modules/feature_flags/tests/conftest.py b/modules/feature_flags/tests/conftest.py new file mode 100644 index 00000000..d7812f8c --- /dev/null +++ b/modules/feature_flags/tests/conftest.py @@ -0,0 +1,25 @@ +"""Fixtures for tenant-scope flag tests, which need real tenants to name.""" + +from __future__ import annotations + +import pytest + + +@pytest.fixture +def make_tenant(app): + """``await make_tenant("acme")`` — a tenants row with that id.""" + + async def make(tenant_id: str) -> str: + from tenants.models import Tenant + + async with app.state.sm.db.session_factory() as session: + session.add(Tenant(id=tenant_id, slug=tenant_id, name=tenant_id.title())) + await session.commit() + return tenant_id + + return make + + +@pytest.fixture +async def acme_tenant(make_tenant) -> str: + return await make_tenant("acme") diff --git a/modules/feature_flags/tests/test_feature_flags_api.py b/modules/feature_flags/tests/test_feature_flags_api.py index 5e21ad6a..5dc1ec30 100644 --- a/modules/feature_flags/tests/test_feature_flags_api.py +++ b/modules/feature_flags/tests/test_feature_flags_api.py @@ -3,6 +3,7 @@ from __future__ import annotations import httpx +import pytest class TestFeatureFlagsAPI: @@ -58,6 +59,7 @@ async def test_get_flag_returns_view(self, authenticated_client: httpx.AsyncClie assert "overridden" in body +@pytest.mark.usefixtures("acme_tenant") class TestFeatureFlagsTenantAPI: async def test_set_tenant_override_creates_tenant_specific_row( self, authenticated_client: httpx.AsyncClient diff --git a/modules/feature_flags/tests/test_feature_flags_tenancy.py b/modules/feature_flags/tests/test_feature_flags_tenancy.py new file mode 100644 index 00000000..4b3bce92 --- /dev/null +++ b/modules/feature_flags/tests/test_feature_flags_tenancy.py @@ -0,0 +1,162 @@ +"""Feature flags under multi-tenancy (#375). + +Request-time checks (``require_flag`` / ``is_flag_enabled``) already resolve +``request.state.tenant_id``; boot hydration is deliberately cross-tenant; the +admin tenant-override endpoints refuse a tenant that does not exist. +""" + +from __future__ import annotations + +import pytest +from fastapi import Depends +from feature_flags.constants import PERM_FEATURE_FLAGS_MANAGE, PERM_FEATURE_FLAGS_VIEW +from feature_flags.service import FeatureFlagService +from simple_module_core.feature_flags import FeatureFlagRegistry, require_flag +from simple_module_core.tenancy import TenantRole, tenant_role + +FLAG = "file_storage.public_uploads" # default off +SYSTEM = f"/api/feature_flags/{FLAG}" + + +def tenant_url(tenant_id: str) -> str: + return f"/api/feature_flags/tenant/{tenant_id}/{FLAG}" + + +@pytest.fixture +def gated(app): + """``GET /__gated`` — 200 when ``FLAG`` is on for the caller's tenant, else 404.""" + + @app.get("/__gated", dependencies=[Depends(require_flag(FLAG))]) + async def _gated() -> dict: + return {"ok": True} + + return "/__gated" + + +async def _put(client, url, enabled): + resp = await client.put(url, json={"enabled": enabled}) + assert resp.status_code == 200, resp.text + + +class TestRequireFlagResolvesTheRequestTenant: + async def test_two_tenants_with_different_overrides( + self, authenticated_client, tenant_client, gated + ): + async with tenant_client() as a, tenant_client() as b: + await _put(authenticated_client, tenant_url(a.tenant_id), True) + await _put(authenticated_client, tenant_url(b.tenant_id), False) + assert (await a.client.get(gated)).status_code == 200 + assert (await b.client.get(gated)).status_code == 404 + + async def test_tenant_override_beats_system_in_both_directions( + self, authenticated_client, tenant_client, gated + ): + async with tenant_client() as a, tenant_client() as b, tenant_client() as c: + await _put(authenticated_client, SYSTEM, True) + await _put(authenticated_client, tenant_url(a.tenant_id), False) + # system on: A overridden off, B inherits on + assert (await a.client.get(gated)).status_code == 404 + assert (await b.client.get(gated)).status_code == 200 + await _put(authenticated_client, SYSTEM, False) + await _put(authenticated_client, tenant_url(c.tenant_id), True) + # system off: C overridden on, B inherits off, A keeps its own off + assert (await c.client.get(gated)).status_code == 200 + assert (await b.client.get(gated)).status_code == 404 + + async def test_default_applies_when_nothing_is_overridden(self, tenant_client, gated): + async with tenant_client() as a: + assert (await a.client.get(gated)).status_code == 404 + + async def test_clearing_a_tenant_override_falls_back_to_system( + self, authenticated_client, tenant_client, gated + ): + async with tenant_client() as a: + await _put(authenticated_client, SYSTEM, True) + await _put(authenticated_client, tenant_url(a.tenant_id), False) + assert (await a.client.get(gated)).status_code == 404 + cleared = await authenticated_client.delete(tenant_url(a.tenant_id)) + assert cleared.status_code == 204 + assert (await a.client.get(gated)).status_code == 200 + + +class TestBootHydration: + async def test_loads_every_tenants_overrides_with_no_tenant_bound(self, app, tenant_client): + assert app.state.sm.db.tenant_strict # strict: an unscoped tenant read would raise + async with tenant_client() as a, tenant_client() as b: + async with app.state.sm.db.session_factory() as db: + service = FeatureFlagService(db) + await service.set_override(FLAG, True, scope="tenant", scope_id=a.tenant_id) + await service.set_override(FLAG, False, scope="tenant", scope_id=b.tenant_id) + await db.commit() + + fresh = FeatureFlagRegistry() + async with app.state.sm.db.session_factory() as db: + loaded = await FeatureFlagService(db).hydrate_registry(fresh) + + assert loaded >= 2 + assert fresh.tenant_override(FLAG, a.tenant_id) is True + assert fresh.tenant_override(FLAG, b.tenant_id) is False + + +class TestTenantRolesNeverManageFlags: + @pytest.mark.parametrize("role", list(TenantRole)) + def test_no_tenant_role_holds_flag_permissions(self, app, role): + mapped = set(app.state.sm.permissions.role_map[tenant_role(role)]) + assert PERM_FEATURE_FLAGS_MANAGE not in mapped + assert PERM_FEATURE_FLAGS_VIEW not in mapped + + @pytest.mark.parametrize("role", list(TenantRole)) + async def test_tenant_member_cannot_reach_the_admin_endpoints(self, tenant_client, role): + async with tenant_client(role) as m: + own = tenant_url(m.tenant_id) + assert (await m.client.put(own, json={"enabled": True})).status_code == 403 + assert (await m.client.delete(own)).status_code == 403 + listing = await m.client.get(f"/api/feature_flags/tenant/{m.tenant_id}") + assert listing.status_code == 403 + + +class TestUnknownTenantIsRejected: + async def test_set_override_for_an_unknown_tenant_404s_and_persists_nothing( + self, authenticated_client, app + ): + resp = await authenticated_client.put(tenant_url("no-such-tenant"), json={"enabled": True}) + assert resp.status_code == 404 + assert app.state.sm.feature_flags.tenant_override(FLAG, "no-such-tenant") is None + async with app.state.sm.db.session_factory() as db: + known = await FeatureFlagService(db).list_tenants_with_overrides() + assert "no-such-tenant" not in known + + async def test_listing_an_unknown_tenant_404s(self, authenticated_client): + resp = await authenticated_client.get("/api/feature_flags/tenant/no-such-tenant") + assert resp.status_code == 404 + + async def test_known_tenant_is_accepted(self, authenticated_client, tenant_client): + async with tenant_client() as a: + await _put(authenticated_client, tenant_url(a.tenant_id), True) + listing = await authenticated_client.get(f"/api/feature_flags/tenant/{a.tenant_id}") + assert listing.status_code == 200 + + async def test_browse_view_rejects_an_unknown_tenant(self, authenticated_client): + resp = await authenticated_client.get("/admin/feature-flags/?tenant_id=nope") + assert resp.status_code == 404 + assert (await authenticated_client.get("/admin/feature-flags/")).status_code == 200 + + async def test_an_orphaned_override_can_still_be_cleared(self, authenticated_client, app): + async with app.state.sm.db.session_factory() as db: + await FeatureFlagService(db).set_override( + FLAG, True, registry=app.state.sm.feature_flags, scope="tenant", scope_id="gone" + ) + await db.commit() + resp = await authenticated_client.delete(tenant_url("gone")) + assert resp.status_code == 204 + + +async def test_without_a_tenant_directory_any_id_is_accepted(authenticated_client, app): + """No ``tenants`` module installed means nobody can say a tenant is unknown.""" + saved = app.state.tenant_exists + del app.state.tenant_exists + try: + resp = await authenticated_client.put(tenant_url("anything"), json={"enabled": True}) + finally: + app.state.tenant_exists = saved + assert resp.status_code == 200 diff --git a/modules/tenants/tenants/module.py b/modules/tenants/tenants/module.py index 38545fdf..747d2875 100644 --- a/modules/tenants/tenants/module.py +++ b/modules/tenants/tenants/module.py @@ -23,10 +23,22 @@ from fastapi import APIRouter, FastAPI from simple_module_core.invalidation import InvalidationBus from simple_module_core.permissions import PermissionRegistry + from simple_module_core.tenancy import TenantExists logger = logging.getLogger(__name__) +def _tenant_exists(app: FastAPI) -> TenantExists: + """The ``app.state.tenant_exists`` callable core's ``tenant_exists()`` reads.""" + from tenants.models import Tenant + + async def exists(tenant_id: str) -> bool: + async with app.state.sm.db.session_factory() as db: + return await db.get(Tenant, tenant_id) is not None + + return exists + + class TenantsModule(ModuleBase): meta = ModuleMeta( name=c.DISPLAY_NAME, @@ -53,6 +65,7 @@ def register_settings(self, app: FastAPI) -> None: app, c.MODULE_PACKAGE, TenantsSettings, lambda s: TenantsServices(settings=s) ) app.state.tenant_resolver = resolve_tenant + app.state.tenant_exists = _tenant_exists(app) register_inertia_shared_provider(app, tenant_shared_props) def register_exception_handlers(self, app: FastAPI) -> None: