From effd662798e5a361d384c1610f018829c34b8a38 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 30 Oct 2026 09:01:29 +0100 Subject: [PATCH] fix: lower-priority findings from the 2026-10-28 scan (#698) - Deprecate auth.deps.require_permission (remove_in 0.1.0); the README points at RequiresPermission. - Files and Branding menus are gated on their page permissions, not role names. - Feature-flag tenant overrides are cleared from every worker's registry on TenantDeleted (rest of #639). - Bulk invite no longer shows raw exception text (SQL and parameters). - The audit-log JSON list filters actor and end date like the browse screen and CSV export. - Leases on Postgres lock the slot row (an expired placeholder when missing) before reading the clock, so a lock wait can no longer grant or refresh a lease that already expired. Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- CHANGELOG.md | 12 +++++ docs/framework/leases.md | 2 +- docs/modules/audit_log.md | 7 +++ docs/modules/branding.md | 4 ++ docs/modules/feature_flags.md | 7 +++ docs/modules/file_storage.md | 4 ++ docs/modules/users.md | 4 ++ .../core/simple_module_core/feature_flags.py | 6 +++ framework/db/simple_module_db/leases.py | 39 +++++++++++---- framework/db/tests/test_db_lease_lock_wait.py | 39 +++++++++++++++ host/tests/test_deprecation_expiry.py | 2 + modules/audit_log/audit_log/endpoints/api.py | 8 ++-- modules/audit_log/audit_log/filters.py | 13 +++-- .../tests/test_audit_log_api_filter_parity.py | 48 +++++++++++++++++++ .../audit_log/tests/test_date_range_filter.py | 3 +- modules/auth/README.md | 18 +++++-- modules/auth/auth/deps.py | 17 ++++--- .../tests/test_auth_permission_deprecation.py | 13 +++++ modules/branding/branding/module.py | 2 +- .../tests/test_branding_menu_permission.py | 17 +++++++ .../feature_flags/invalidation.py | 9 +++- modules/feature_flags/feature_flags/module.py | 6 +++ .../test_feature_flags_tenant_cleanup.py | 35 ++++++++++++++ modules/file_storage/file_storage/module.py | 4 +- .../test_file_storage_menu_permission.py | 18 +++++++ .../test_users_bulk_invite_error_privacy.py | 24 ++++++++++ modules/users/users/invites/bulk.py | 12 +---- 27 files changed, 322 insertions(+), 51 deletions(-) create mode 100644 framework/db/tests/test_db_lease_lock_wait.py create mode 100644 modules/audit_log/tests/test_audit_log_api_filter_parity.py create mode 100644 modules/auth/tests/test_auth_permission_deprecation.py create mode 100644 modules/branding/tests/test_branding_menu_permission.py create mode 100644 modules/file_storage/tests/test_file_storage_menu_permission.py create mode 100644 modules/users/tests/invites/test_users_bulk_invite_error_privacy.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8373db74c..0f2c18a2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,18 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] ### Fixed +- Deprecate `auth.deps.require_permission` for removal in 0.1.0; recommend + `RequiresPermission` and enforce the removal release in the shim ledger (#698). +- **Behaviour change:** gate Files and Branding navigation by their page + permissions instead of hardcoded roles (#698). +- Clear deleted tenants' feature-flag registry overrides locally and on remote + tenant-deletion invalidations, preserving other scopes (#639, #698). +- Keep unexpected bulk-invite exception details in logs and return a generic + failure message instead of exposing SQL and parameters (#698). +- **Behaviour change:** align audit-log JSON list actor search and inclusive + end-of-day filtering with the browse screen and CSV export (#698). +- Start database lease TTLs after locking each slot on Postgres, and recheck expiry after + lock waits when refreshing or releasing (#698). - **Behaviour change:** password resets revoke all browser sessions, access tokens and refresh tokens, sharing revocation with password changes and sign-out-everywhere (#686). - Defer single admin create/invite events and invitation-related emails until the diff --git a/docs/framework/leases.md b/docs/framework/leases.md index bf8e6b5ea..7ca10b6b1 100644 --- a/docs/framework/leases.md +++ b/docs/framework/leases.md @@ -50,7 +50,7 @@ The backend is process-global because a Celery worker has no `app.state`. `creat | Configured | Backend | Notes | |---|---|---| | `SM_REDIS_URL` | `RedisLeaseBackend` (`simple_module_hosting`) | `SET NX PX` plus Lua compare-and-set for refresh and release; a semaphore is a sorted set scored by expiry against Redis `TIME`; `INCR :fence` for the fence. | -| a database, no Redis | `DbLeaseBackend` (`simple_module_db`) | Rows in `sm_lease (name, slot, holder, fence, expires_at)`, primary key `(name, slot)`. Every comparison uses the database clock, so clock skew between hosts does not matter. | +| a database, no Redis | `DbLeaseBackend` (`simple_module_db`) | Rows in `sm_lease (name, slot, holder, fence, expires_at)`, primary key `(name, slot)`. Every comparison uses the database clock, so clock skew between hosts does not matter; on Postgres the clock is read after the slot row is locked, so a lock wait never shortens a TTL. | | neither | `InProcessLeaseBackend` | One process only. Also what tests and eager Celery get when nothing else is configured. | Backend errors propagate. A lease that the backend could not confirm is never reported as held. This is unlike the rate limiter, which falls back to per-process counters. diff --git a/docs/modules/audit_log.md b/docs/modules/audit_log.md index 799d8ca31..6e0b0d85a 100644 --- a/docs/modules/audit_log.md +++ b/docs/modules/audit_log.md @@ -111,6 +111,13 @@ GET /api/audit_log/ Requires `audit_log.view`. Returns `AuditEntryList`. Query filters (all optional): `entity_type`, `entity_id`, `action`, `user_id`, `from_date`, `to_date`, `page` (≥1), `page_size` (1–200, default 50). Results are ordered newest-first (`created_at` desc). +The JSON list, browse screen and CSV export share actor and date-range semantics +(#698): `user_id` accepts an exact UUID or a name/email substring (an exact +stored actor id when the directory cannot search). `to_date` includes the whole +day it names, even if supplied with a time. `from_date` remains the supplied +lower bound. Service callers needing an exact upper timestamp can use +`AuditLogService.list_entries` or construct `EntryFilters` directly. + ### View | Method + path | Inertia component | Permission | diff --git a/docs/modules/branding.md b/docs/modules/branding.md index fbb45892c..00a57c154 100644 --- a/docs/modules/branding.md +++ b/docs/modules/branding.md @@ -306,3 +306,7 @@ edits show at once, in every worker once a transport is installed. - Branding has no table: the system identity is SYSTEM-scope settings, a tenant's overrides are TENANT-scope settings (see [Per-tenant branding](#per-tenant-branding)). - The primary colour overrides the `--primary` / `--sidebar-primary` CSS variables from a single hex; the full OKLCH colour scale is not regenerated. + +The Branding navigation entry requires `branding.view`, matching its page guard, +rather than the `admin` role (#698). Custom roles with that permission see the +entry; changing branding still requires `branding.manage`. diff --git a/docs/modules/feature_flags.md b/docs/modules/feature_flags.md index 468b35147..a2afd6cf2 100644 --- a/docs/modules/feature_flags.md +++ b/docs/modules/feature_flags.md @@ -132,3 +132,10 @@ Top-level keys in `feature_flags/locales/en.json`: - The `FeatureFlagService` keeps the in-memory registry in sync after every DB mutation, so `is_enabled()` is always cheap. - Tenant-scope endpoints use a non-empty `scope_id`; the system scope uses `""`. Don't pass `None` — the unique index treats `NULL` as distinct on Postgres and you'll end up with duplicate rows. - The `Browse` page lists tenants that have any override attached, so you can jump from system view → a tenant view that actually has overrides without typing IDs. + +Tenant deletion durably removes persisted tenant overrides and clears the local +registry through a `TenantDeleted` subscriber (#639, #698). Workers also forget +the tenant on the existing `tenants.deleted` invalidation broadcast; system and +other tenants' overrides are preserved, including overrides for unknown flags. +The periodic override reload remains the fallback for missed broadcasts after +durable cleanup completes. diff --git a/docs/modules/file_storage.md b/docs/modules/file_storage.md index c47c19945..0858065da 100644 --- a/docs/modules/file_storage.md +++ b/docs/modules/file_storage.md @@ -444,3 +444,7 @@ and worker processes, and verify authenticated/public downloads, ranged reads, thumbnails and deletion before resuming writes. Keep the source and backups until verification is complete, then remove source objects explicitly. There is no automatic migration command or historical-backend registry. + +The Files navigation entry requires `file_storage.download`, matching the browse +route (#698). It is available to permitted ordinary users on single-tenant +installs as well as tenant members; role names do not independently gate it. diff --git a/docs/modules/users.md b/docs/modules/users.md index 2feeaac44..ce7a810b2 100644 --- a/docs/modules/users.md +++ b/docs/modules/users.md @@ -420,6 +420,10 @@ Trade-offs to know about: `mailer/` ships one implementation of the `Mailer` protocol, `TemplatedMailer`. It renders the bundled verify / reset / invite templates and sends each as a `MailMessage` through the framework mail sender (`simple_module_core.mail.current_mail_sender()`), looked up at send time. The transport (SMTP or console), its credentials and the `mail.mailer` health check belong to the [`mail` module](./mail.md); the former `mailer` and `smtp_*` users settings no longer exist. +Bulk invite failures return a known, safe reason for an existing account and a +generic retry message for unexpected errors (#698). SQL statements, parameters +and backend exception messages stay in the application log. + `delivers_email` reflects the installed sender, so screens that need to hand the one-time link back (reset-sent card, add people, resend and bulk invite) do so whenever the sender only logs. `build_mailer(settings)` returns the `TemplatedMailer`; a third-party `Mailer` can still be assigned to `app.state.users.mailer`. ## OAuth providers diff --git a/framework/core/simple_module_core/feature_flags.py b/framework/core/simple_module_core/feature_flags.py index da76223d1..5fafe499c 100644 --- a/framework/core/simple_module_core/feature_flags.py +++ b/framework/core/simple_module_core/feature_flags.py @@ -71,6 +71,12 @@ def clear_override(self, name: str, tenant_id: str | None = None) -> None: else: self._tenant_overrides.pop((name, tenant_id), None) + def clear_tenant_overrides(self, tenant_id: str) -> None: + """Forget every override for a deleted tenant, including unknown flags.""" + self._tenant_overrides = { + key: value for key, value in self._tenant_overrides.items() if key[1] != tenant_id + } + def replace_overrides( self, system: dict[str, bool], tenants: dict[tuple[str, str], bool] ) -> None: diff --git a/framework/db/simple_module_db/leases.py b/framework/db/simple_module_db/leases.py index f30f62099..51d2ba664 100644 --- a/framework/db/simple_module_db/leases.py +++ b/framework/db/simple_module_db/leases.py @@ -106,19 +106,40 @@ async def _tx(self) -> AsyncGenerator[tuple[AsyncConnection, datetime]]: await conn.execute(_SQLITE_WRITE_LOCK) yield conn, await _db_now(conn) + async def _locked_now(self, conn: AsyncConnection, *where: Any) -> datetime: + """Lock the matching rows on Postgres, then read the clock (#698). + + Postgres computes an UPDATE's new values before it waits on a row lock, + so a time read first could grant (or "refresh") a lease that expired + during the wait. SQLite holds the database write lock already. + """ + await conn.execute(select(_T.c.slot).where(*where).with_for_update()) + return await _db_now(conn) + async def _take_slot( self, conn: AsyncConnection, name: str, slot: int, token: str, - expires: datetime, + ttl: float, now: datetime, ) -> int | None: """Claim *slot* if free (missing or expired) or already ours; return its fence.""" - values = {"holder": token, "expires_at": expires} - ins = _insert(conn.dialect.name).values(name=name, slot=slot, fence=1, **values) - taken = (await conn.execute(ins.on_conflict_do_nothing())).rowcount == 1 + if self._sqlite: + values = {"holder": token, "expires_at": now + timedelta(seconds=ttl)} + ins = _insert(conn.dialect.name).values(name=name, slot=slot, fence=1, **values) + taken = (await conn.execute(ins.on_conflict_do_nothing())).rowcount == 1 + else: + # An expired placeholder gives a missing slot a row to lock; the + # UPDATE below then claims it exactly like an expired lease (fence 1). + placeholder = _insert(conn.dialect.name).values( + name=name, slot=slot, holder=_FREE, fence=0, expires_at=_EPOCH + ) + await conn.execute(placeholder.on_conflict_do_nothing()) + now = await self._locked_now(conn, _row(name, slot)) + values = {"holder": token, "expires_at": now + timedelta(seconds=ttl)} + taken = False if not taken: stmt = ( update(_T) @@ -144,15 +165,14 @@ async def _next_counter(self, conn: AsyncConnection, name: str) -> int: async def acquire(self, name: str, token: str, ttl: float) -> int | None: async with self._tx() as (conn, now): - return await self._take_slot(conn, name, 0, token, now + timedelta(seconds=ttl), now) + return await self._take_slot(conn, name, 0, token, ttl, now) async def semaphore_acquire(self, name: str, token: str, slots: int, ttl: float) -> int | None: async with self._tx() as (conn, now): - expires = now + timedelta(seconds=ttl) mine = await self._slot_of(conn, name, token, now) order = [mine] if mine is not None else range(slots) for slot in order: - if await self._take_slot(conn, name, slot, token, expires, now) is not None: + if await self._take_slot(conn, name, slot, token, ttl, now) is not None: return await self._next_counter(conn, name) return None @@ -169,8 +189,11 @@ async def _held_update( ) -> bool: """Extend (``ttl``) or end (``None``) *token*'s unexpired hold.""" async with self._tx() as (conn, now): - where = [_T.c.name == name, _T.c.holder == token, _T.c.expires_at > now] + where = [_T.c.name == name, _T.c.holder == token] where.append(_T.c.slot >= 0 if any_slot else _T.c.slot == 0) + if not self._sqlite: + now = await self._locked_now(conn, *where) + where.append(_T.c.expires_at > now) expires = now if ttl is None else now + timedelta(seconds=ttl) result = await conn.execute(update(_T).where(*where).values(expires_at=expires)) return result.rowcount > 0 diff --git a/framework/db/tests/test_db_lease_lock_wait.py b/framework/db/tests/test_db_lease_lock_wait.py new file mode 100644 index 000000000..5688d847d --- /dev/null +++ b/framework/db/tests/test_db_lease_lock_wait.py @@ -0,0 +1,39 @@ +"""A lease taken after waiting on a row lock is still valid when granted (#698).""" + +import asyncio +import uuid + +import pytest +from simple_module_db.leases import DbLeaseBackend, LeaseRow +from sqlalchemy import func, select + +_T = LeaseRow.__table__ + + +@pytest.mark.postgres +@pytest.mark.parametrize("operation", ["acquire", "refresh"]) +async def test_postgres_lock_wait_does_not_consume_ttl(engine, operation): + async with engine.begin() as conn: + await conn.run_sync(LeaseRow.metadata.create_all) + backend = DbLeaseBackend(engine) + name = f"lockwait-{uuid.uuid4().hex}" + holder = "old" if operation == "acquire" else "me" + ttl = 0.01 if operation == "acquire" else 60 + assert await backend.acquire(name, holder, ttl) is not None + await asyncio.sleep(0.05) # intentional-sleep: let the 10 ms lease expire + + async with engine.connect() as blocker: + await blocker.begin() + row = (_T.c.name == name) & (_T.c.slot == 0) + await blocker.execute(select(_T.c.fence).where(row).with_for_update()) + if operation == "acquire": + pending = asyncio.create_task(backend.acquire(name, "new", 1.0)) + else: + pending = asyncio.create_task(backend.refresh(name, "me", 1.0)) + await asyncio.sleep(1.5) # intentional-sleep: hold the lock past the 1 s TTL + await blocker.commit() + assert await pending + + async with engine.connect() as conn: + live = await conn.execute(select(_T.c.expires_at > func.clock_timestamp()).where(row)) + assert live.scalar_one() is True diff --git a/host/tests/test_deprecation_expiry.py b/host/tests/test_deprecation_expiry.py index ade2bba85..35f9d8e90 100644 --- a/host/tests/test_deprecation_expiry.py +++ b/host/tests/test_deprecation_expiry.py @@ -21,6 +21,7 @@ # Modules whose import registers a deprecation (shim modules and call-site shims). _SHIM_MODULES = ( + "auth.deps", "host.routes_legacy", "permissions.deps", "settings.registration", @@ -31,6 +32,7 @@ "simple_module_core.versioning", ) _EXPECTED = { + "auth.deps.require_permission", "host.routes_legacy (pre-/admin view URL redirects)", "permissions.deps.invalidate_grants_on_commit", "settings.registration", diff --git a/modules/audit_log/audit_log/endpoints/api.py b/modules/audit_log/audit_log/endpoints/api.py index 76ff4ddff..cbca180f6 100644 --- a/modules/audit_log/audit_log/endpoints/api.py +++ b/modules/audit_log/audit_log/endpoints/api.py @@ -26,6 +26,7 @@ @router.get("/", response_model=AuditEntryList, dependencies=_VIEW) async def list_audit_entries( + request: Request, service: AuditLogServiceDep, entity_type: str | None = Query(default=None), entity_id: str | None = Query(default=None), @@ -38,18 +39,17 @@ async def list_audit_entries( page: int = Query(default=1, ge=1, le=MAX_PAGE), page_size: int = Query(default=DEFAULT_PAGE_SIZE, ge=1, le=MAX_PER_PAGE), ) -> AuditEntryList: - return await service.list_entries( + filters = EntryFilters.for_date_only_range( entity_type=entity_type, entity_id=entity_id, action=action, - user_id=user_id, + actor_match=actor_filter(get_user_directory(request.app), user_id or ""), correlation_id=correlation_id, tenant_id=tenant_id, from_date=from_date, to_date=to_date, - page=page, - page_size=page_size, ) + return await service.list_filtered(filters, page=page, page_size=page_size) @router.get("/export.csv", response_model=None, dependencies=_VIEW) diff --git a/modules/audit_log/audit_log/filters.py b/modules/audit_log/audit_log/filters.py index 98f7d0024..85331b1c2 100644 --- a/modules/audit_log/audit_log/filters.py +++ b/modules/audit_log/audit_log/filters.py @@ -34,10 +34,10 @@ def end_of_day(moment: datetime | None) -> datetime | None: class EntryFilters: """A narrowing of the audit log, independent of paging. - ``user_id`` is an exact match on the stored id — what the JSON list - endpoint takes. ``actor_match`` is the predicate an Actor *search* compiles - to (see ``resolve.actor_filter``), which may match by name or email and is - therefore a subquery rather than a value. + ``user_id`` is an exact match on the stored id for service callers. + ``actor_match`` is the predicate an Actor search compiles to (see + ``resolve.actor_filter``), which may match by name or email and is therefore + a subquery rather than a value. """ entity_type: str | None = None @@ -68,12 +68,11 @@ def for_date_only_range( The upper bound is stretched over the whole day it names — see :func:`end_of_day`. A named constructor rather than a step a caller has - to remember: the browse view and the CSV export both build filters from + to remember: the browse view, JSON list and CSV export build filters from the same query string, and one of them forgetting would mean the file and the page disagreed about which day the range ended on. - Plain construction keeps exact ``datetime`` bounds, which is what the - JSON list endpoint's documented ``from_date``/``to_date`` mean. + Plain construction keeps exact ``datetime`` bounds for service callers. """ return cls( entity_type=entity_type, diff --git a/modules/audit_log/tests/test_audit_log_api_filter_parity.py b/modules/audit_log/tests/test_audit_log_api_filter_parity.py new file mode 100644 index 000000000..53b43ad6a --- /dev/null +++ b/modules/audit_log/tests/test_audit_log_api_filter_parity.py @@ -0,0 +1,48 @@ +"""JSON list and CSV export interpret the same query identically (#698).""" + +import csv +import io +import uuid +from datetime import UTC, datetime + +import pytest +from audit_log.constants import ACTION_UPDATED +from audit_log.models import AuditEntry +from users.models import User + + +@pytest.mark.parametrize("actor", ["", "Filter Person", "filter-person@example.com", "id"]) +async def test_audit_log_list_matches_export_actor_and_day(app, authenticated_client, actor): + user_id = uuid.uuid4() + async with app.state.sm.db.session_factory() as db: + db.add( + User( + id=user_id, + email="filter-person@example.com", + full_name="Filter Person", + hashed_password="unused", + ) + ) + await db.flush() + for entity_id, day in [("included", 19), ("excluded", 20)]: + db.add( + AuditEntry( + entity_type="FilterParity", + entity_id=entity_id, + action=ACTION_UPDATED, + changes=[], + user_id=str(user_id), + created_at=datetime(2026, 8, day, 14, tzinfo=UTC), + ) + ) + await db.commit() + params = { + "entity_type": "FilterParity", + "to_date": "2026-08-19", + "user_id": str(user_id) if actor == "id" else actor, + } + listing = await authenticated_client.get("/api/audit_log/", params=params) + export = await authenticated_client.get("/api/audit_log/export.csv", params=params) + assert listing.status_code == export.status_code == 200 + assert [row["entity_id"] for row in csv.DictReader(io.StringIO(export.text))] == ["included"] + assert [row["entity_id"] for row in listing.json()["items"]] == ["included"] diff --git a/modules/audit_log/tests/test_date_range_filter.py b/modules/audit_log/tests/test_date_range_filter.py index edb1e0be8..e975b53e2 100644 --- a/modules/audit_log/tests/test_date_range_filter.py +++ b/modules/audit_log/tests/test_date_range_filter.py @@ -44,8 +44,7 @@ def test_the_screen_constructor_stretches_the_upper_bound(self) -> None: assert filters.to_date == datetime(2026, 8, 19, 23, 59, 59, 999999, tzinfo=UTC) def test_plain_construction_keeps_the_exact_bound(self) -> None: - """The JSON list endpoint documents ``to_date`` as a timestamp; only - the screen's date-only control needs stretching.""" + """Service callers can request an exact bound through plain construction.""" exact = datetime(2026, 8, 19, 10, 30, tzinfo=UTC) assert EntryFilters(to_date=exact).to_date == exact diff --git a/modules/auth/README.md b/modules/auth/README.md index 8e51c3e82..bbbf0074d 100644 --- a/modules/auth/README.md +++ b/modules/auth/README.md @@ -1,6 +1,6 @@ # simple_module_auth -Pluggable authentication core for [simple_module](https://github.com/antosubash/simple_module_python) apps. Owns the stable public contracts every other module imports — `UserContext`, the `AuthProvider` protocol, the `PrincipalResolver` chain, and the `get_current_user` / `CurrentUser` / `require_permission` dependencies — plus the `AuthMiddleware` that resolves the current principal on every request. +Pluggable authentication core for [simple_module](https://github.com/antosubash/simple_module_python) apps. Owns the stable public contracts every other module imports — `UserContext`, the `AuthProvider` protocol, the `PrincipalResolver` chain, and the `get_current_user` / `CurrentUser` dependencies — plus the `AuthMiddleware` that resolves the current principal on every request. **Heads up:** for most apps you don't install this directly — an auth-provider module (`simple_module_users` for email+password, `simple_module_keycloak` for OIDC) pulls it in and registers itself on `app.state.auth.auth_provider`. @@ -16,7 +16,7 @@ pip install simple_module_auth - `AuthProvider` protocol — the swappable-backend contract (`resolve_user`, `get_login_url`, `get_logout_url`, `is_bearer_request`); exactly one provider module registers an implementation on `app.state.auth.auth_provider`. - `PrincipalResolver` chain — async `(Request) -> UserContext | None` callables apps append to `app.state.auth.principal_resolvers` (e.g. PAT/bearer-token or API-key auth), consulted after the session path. - `AuthMiddleware` — delegates to the provider + resolver chain on every request and populates the request principal. -- `get_current_user` / `CurrentUser` dependency and the `require_permission(*permissions)` dependency factory. +- `get_current_user` / `CurrentUser` dependency. - Anonymous-access is declared via the framework's method-aware `register_public_routes` hook (with the `SM_AUTH_PUBLIC_PATHS` host-level escape hatch). ## Usage @@ -34,18 +34,26 @@ async def me(user: CurrentUser): return {"user_id": user.id, "email": user.email} ``` -Routes that need a specific permission use the `require_permission(...)` dependency factory: +Routes that need a specific permission use `RequiresPermission` from hosting: ```python from fastapi import Depends -from auth.deps import require_permission +from simple_module_hosting.permissions import RequiresPermission +from .constants import PERM_PRODUCTS_CREATE -@router.post("/", dependencies=[Depends(require_permission("products.create"))]) + +@router.post("/", dependencies=[Depends(RequiresPermission(PERM_PRODUCTS_CREATE))]) async def create_product(): ... ``` +`auth.deps.require_permission` is deprecated and will be removed in **0.1.0** +(#698). It retains its legacy any-of semantics, role-based admin bypass and cached +permission check. Migrate to `RequiresPermission(PERM_KEY)`; for several required +permissions, declare a dependency for each. Unlike the legacy helper, +`RequiresPermission` rechecks administrators demoted during the request. + ## Depends on - `simple_module_core`, `simple_module_db` diff --git a/modules/auth/auth/deps.py b/modules/auth/auth/deps.py index b330c79b6..3dfd5bb5b 100644 --- a/modules/auth/auth/deps.py +++ b/modules/auth/auth/deps.py @@ -5,15 +5,14 @@ from typing import Annotated from fastapi import Depends, HTTPException, Request -from simple_module_core.permissions import grants +from simple_module_core.deprecation import deprecated +from simple_module_core.permissions import ADMIN_ROLE, grants from simple_module_hosting.i18n_deps import TranslatorDep from simple_module_hosting.permissions import ensure_resolved_permissions from simple_module_hosting.token_guard import GATE_PERMISSION, mark_token_gate from auth.contracts.schemas import UserContext -_ADMIN_ROLE = "admin" - async def get_current_user(request: Request, t: TranslatorDep) -> UserContext: """Extract the authenticated user from request state. @@ -29,13 +28,13 @@ async def get_current_user(request: Request, t: TranslatorDep) -> UserContext: CurrentUser = Annotated[UserContext, Depends(get_current_user)] +@deprecated(remove_in="0.1.0", use="simple_module_hosting.permissions.RequiresPermission") def require_permission(*permissions: str): - """Create a dependency that checks if the user has required permissions. - - Usage:: + """Legacy any-of gate, removed in 0.1.0; use ``RequiresPermission`` instead. - @router.post("/", dependencies=[Depends(require_permission("products.create"))]) - async def create_product(...): ... + Retains the role-based admin bypass and cached permission behaviour for + compatibility. ``RequiresPermission`` rechecks a principal demoted during + the request; this helper does not. """ async def check( @@ -46,7 +45,7 @@ async def check( # Admin role bypasses permission checks — but not for a token: an # admin's PAT is worth only the scopes it carries, and the resolved # set below already applies that cap. - if _ADMIN_ROLE in user.roles and user.scopes is None: + if ADMIN_ROLE in user.roles and user.scopes is None: return # The same resolution RequiresPermission reads: the registry's role map diff --git a/modules/auth/tests/test_auth_permission_deprecation.py b/modules/auth/tests/test_auth_permission_deprecation.py new file mode 100644 index 000000000..2ace99181 --- /dev/null +++ b/modules/auth/tests/test_auth_permission_deprecation.py @@ -0,0 +1,13 @@ +"""The legacy any-of permission helper carries a migration warning (#698).""" + +import pytest +from auth.deps import require_permission +from simple_module_core.deprecation import registered_deprecations + + +def test_auth_require_permission_warns_and_registers_removal(): + with pytest.warns(DeprecationWarning, match="RequiresPermission"): + dependency = require_permission("products.view") + assert callable(dependency.dependency) + entry = next(d for d in registered_deprecations() if d.name == "auth.deps.require_permission") + assert entry.remove_in == "0.1.0" diff --git a/modules/branding/branding/module.py b/modules/branding/branding/module.py index 5f911daa9..12a941114 100644 --- a/modules/branding/branding/module.py +++ b/modules/branding/branding/module.py @@ -92,7 +92,7 @@ def register_menu_items(self, registry: MenuRegistry) -> None: section=MenuSection.ADMIN_SIDEBAR, group="Appearance", group_key="ui.nav_groups.appearance", - roles=["admin"], + permissions=[constants.PERM_VIEW], ) ) diff --git a/modules/branding/tests/test_branding_menu_permission.py b/modules/branding/tests/test_branding_menu_permission.py new file mode 100644 index 000000000..e12fd047b --- /dev/null +++ b/modules/branding/tests/test_branding_menu_permission.py @@ -0,0 +1,17 @@ +"""Branding navigation follows the page guard rather than role names (#698).""" + +import pytest +from branding.constants import MENU_URL, PERM_VIEW +from branding.module import BrandingModule +from simple_module_core.menu import MenuRegistry, MenuSection + + +@pytest.mark.parametrize("granted", [False, True]) +@pytest.mark.parametrize("role", ["admin", "designer"]) +def test_branding_menu_follows_view_permission(granted, role): + registry = MenuRegistry() + BrandingModule().register_menu_items(registry) + items = registry.get_for_user( + is_authenticated=True, roles=[role], permissions=[PERM_VIEW] if granted else [] + )[MenuSection.ADMIN_SIDEBAR] + assert any(item["url"] == MENU_URL for item in items) is granted diff --git a/modules/feature_flags/feature_flags/invalidation.py b/modules/feature_flags/feature_flags/invalidation.py index da325a58a..938fe595c 100644 --- a/modules/feature_flags/feature_flags/invalidation.py +++ b/modules/feature_flags/feature_flags/invalidation.py @@ -1,8 +1,9 @@ """Re-read flag overrides after commit or a remote invalidation.""" from fastapi import FastAPI -from simple_module_core.invalidation import InvalidationBus +from simple_module_core.invalidation import Invalidation, InvalidationBus from simple_module_core.invalidation_refresh import InvalidationRefresh +from simple_module_core.tenant_events import TENANT_DELETED_CHANNEL from feature_flags.service import FeatureFlagService @@ -10,6 +11,12 @@ def subscribe(bus: InvalidationBus, app: FastAPI) -> InvalidationRefresh: + def forget_tenant(message: Invalidation) -> None: + if message.key is not None: + app.state.sm.feature_flags.clear_tenant_overrides(message.key) + + bus.subscribe(TENANT_DELETED_CHANNEL, forget_tenant) + async def reload(_key: str | None) -> None: async with app.state.sm.db.session_factory() as db: await FeatureFlagService(db).hydrate_registry(app.state.sm.feature_flags) diff --git a/modules/feature_flags/feature_flags/module.py b/modules/feature_flags/feature_flags/module.py index 3fc05652a..e5217028c 100644 --- a/modules/feature_flags/feature_flags/module.py +++ b/modules/feature_flags/feature_flags/module.py @@ -75,6 +75,12 @@ def register_event_handlers(self, bus: EventBus, app: FastAPI | None = None) -> from feature_flags.tenant_cleanup import purge_tenant_data bus.subscribe_durable(TenantDeleted, purge_tenant_data) + if app is not None: + + async def forget_tenant(event: TenantDeleted) -> None: + app.state.sm.feature_flags.clear_tenant_overrides(event.tenant_id) + + bus.subscribe(TenantDeleted, forget_tenant) def register_invalidations(self, bus: InvalidationBus, app: FastAPI) -> None: from feature_flags.invalidation import subscribe diff --git a/modules/feature_flags/tests/test_feature_flags_tenant_cleanup.py b/modules/feature_flags/tests/test_feature_flags_tenant_cleanup.py index 595459cac..99e13646e 100644 --- a/modules/feature_flags/tests/test_feature_flags_tenant_cleanup.py +++ b/modules/feature_flags/tests/test_feature_flags_tenant_cleanup.py @@ -32,3 +32,38 @@ async def test_durable_tenant_cleanup_is_scoped_idempotent_and_transactional(db_ (SCOPE_TENANT, "other"), (SCOPE_SYSTEM, "deleted"), } + + +async def test_feature_flags_tenant_event_clears_only_deleted_tenant(): + from types import SimpleNamespace + + from fastapi import FastAPI + from simple_module_core.feature_flags import FeatureFlagRegistry + + app = FastAPI() + registry = FeatureFlagRegistry() + app.state.sm = SimpleNamespace(feature_flags=registry) + registry.set_override("unknown.flag", True, tenant_id="deleted") + registry.set_override("unknown.flag", True, tenant_id="other") + registry.set_override("unknown.flag", False) + bus = EventBus() + FeatureFlagsModule().register_event_handlers(bus, app) + event = TenantDeleted(tenant_id="deleted", slug="deleted", name="Deleted") + await bus.publish(event) + await bus.publish(event) + assert registry.tenant_override("unknown.flag", "deleted") is None + assert registry.tenant_override("unknown.flag", "other") is True + assert registry.system_override("unknown.flag") is False + + +async def test_feature_flags_remote_tenant_deletion_clears_registry(app): + from simple_module_core.invalidation import Invalidation + from simple_module_core.tenant_events import TENANT_DELETED_CHANNEL + + registry = app.state.sm.feature_flags + registry.set_override("unknown.flag", True, tenant_id="deleted") + registry.set_override("unknown.flag", True, tenant_id="other") + message = Invalidation(channel=TENANT_DELETED_CHANNEL, key="deleted", origin="other") + await app.state.sm.invalidation.deliver(message.to_wire()) + assert registry.tenant_override("unknown.flag", "deleted") is None + assert registry.tenant_override("unknown.flag", "other") is True diff --git a/modules/file_storage/file_storage/module.py b/modules/file_storage/file_storage/module.py index 9072ee903..67e22c72d 100644 --- a/modules/file_storage/file_storage/module.py +++ b/modules/file_storage/file_storage/module.py @@ -165,9 +165,7 @@ def register_menu_items(self, registry: MenuRegistry) -> None: icon=constants.MENU_ICON, order=constants.MENU_ORDER, section=MenuSection.SIDEBAR, - # Platform admins, and every member of the active organisation - # (the files screen is where a tenant's own uploads live). - roles=[constants.ADMIN_ROLE, *(tenant_role(r) for r in TenantRole)], + permissions=[constants.Permission.DOWNLOAD], group="Content", group_key="ui.nav_groups.content", ) diff --git a/modules/file_storage/tests/test_file_storage_menu_permission.py b/modules/file_storage/tests/test_file_storage_menu_permission.py new file mode 100644 index 000000000..035062b0a --- /dev/null +++ b/modules/file_storage/tests/test_file_storage_menu_permission.py @@ -0,0 +1,18 @@ +"""Files navigation follows the browse permission, including without tenants (#698).""" + +import pytest +from file_storage.constants import ROUTE_PREFIX_VIEW, Permission +from file_storage.module import FileStorageModule +from simple_module_core.menu import MenuRegistry, MenuSection + + +@pytest.mark.parametrize("granted", [False, True]) +def test_file_storage_menu_follows_download_permission(granted): + registry = MenuRegistry() + FileStorageModule().register_menu_items(registry) + items = registry.get_for_user( + is_authenticated=True, + roles=["user"], + permissions=[Permission.DOWNLOAD] if granted else [], + )[MenuSection.SIDEBAR] + assert any(item["url"] == f"{ROUTE_PREFIX_VIEW}/" for item in items) is granted diff --git a/modules/users/tests/invites/test_users_bulk_invite_error_privacy.py b/modules/users/tests/invites/test_users_bulk_invite_error_privacy.py new file mode 100644 index 000000000..2e4751274 --- /dev/null +++ b/modules/users/tests/invites/test_users_bulk_invite_error_privacy.py @@ -0,0 +1,24 @@ +"""Invite responses never expose database statements or exception details (#698).""" + +import pytest +from fastapi_users.exceptions import UserAlreadyExists +from sqlalchemy.exc import IntegrityError +from users.invites.bulk import _failure_detail + + +@pytest.mark.parametrize( + "error", + [ + IntegrityError( + "INSERT INTO users VALUES (:password)", {"password": "secret"}, Exception("db") + ), + RuntimeError("private backend details"), + Exception(), + ], +) +def test_users_bulk_invite_unknown_error_is_generic(error): + assert _failure_detail(error) == "The invitation could not be created. Please try again." + + +def test_users_bulk_invite_already_exists_has_safe_message(): + assert _failure_detail(UserAlreadyExists()) == "Already a member of this workspace" diff --git a/modules/users/users/invites/bulk.py b/modules/users/users/invites/bulk.py index 5a2bd9df7..8f47c25ca 100644 --- a/modules/users/users/invites/bulk.py +++ b/modules/users/users/invites/bulk.py @@ -55,18 +55,10 @@ def _failure_detail(exc: Exception) -> str: - """Human-readable reason for one address failing. - - ``str(UserAlreadyExists())`` is the empty string — fastapi-users carries the - meaning in the exception *type*, not its message. Passing that straight - through renders the single most common failure as a bare red address with no - reason beside it, which is exactly the question the per-address results exist - to answer. The final fallback names the exception class rather than leaving - the cell blank: a class name is a poor message, but it is still a lead. - """ + """Only known errors have specific messages; backend details stay in logs.""" if isinstance(exc, UserAlreadyExists): return "Already a member of this workspace" - return str(exc) or type(exc).__name__ + return "The invitation could not be created. Please try again." def invite_link(request: Request, token: str) -> str: