feat(feature_flags): validate tenant ids, pin tenant flag behaviour (#375) [stack 4/11]

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

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",
+]

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

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)]

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,

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,

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")

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