From 3427efb78aa97691233d0e6ace4524207c1e34fc Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Mon, 5 Oct 2026 12:37:57 +0200 Subject: [PATCH 1/2] fix(permissions,branding): tenancy-adoption review follow-ups (#396) - permissions: keep code-registered role grants when the editor saves a role (separate DB overlay on PermissionRegistry, replaced not popped) - branding reaper: commit the soft-delete before dropping the backend object - branding: retrieve failed in-flight read exceptions (in-flight map is already per-app) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../core/simple_module_core/permissions.py | 21 +++- modules/branding/branding/reaper.py | 6 +- modules/branding/branding/tenant_branding.py | 6 +- .../branding/tests/test_asset_lifecycle.py | 17 ++- .../tests/test_branding_reap_ordering.py | 111 ++++++++++++++++++ modules/branding/tests/test_logo_dark.py | 17 ++- modules/file_storage/file_storage/service.py | 19 ++- modules/permissions/permissions/service.py | 11 +- .../tests/test_permissions_role_overlay.py | 43 +++++++ 9 files changed, 231 insertions(+), 20 deletions(-) create mode 100644 modules/branding/tests/test_branding_reap_ordering.py create mode 100644 modules/permissions/tests/test_permissions_role_overlay.py diff --git a/framework/core/simple_module_core/permissions.py b/framework/core/simple_module_core/permissions.py index d79fdab8..09684545 100644 --- a/framework/core/simple_module_core/permissions.py +++ b/framework/core/simple_module_core/permissions.py @@ -64,6 +64,9 @@ class PermissionRegistry: def __init__(self) -> None: self._groups: dict[str, PermissionGroup] = {} self._role_map: dict[str, set[str]] = {} + # Admin-editor grants (DB-backed). Kept apart from ``_role_map`` (code + # mappings) so replacing a role's DB keys never drops a code grant. + self._role_overlay: dict[str, set[str]] = {} self._all_permissions_cache: list[str] | None = None self._role_map_cache: dict[str, list[str]] | None = None @@ -116,6 +119,16 @@ def map_role(self, role: str, permissions: list[str]) -> None: self._role_map[role].update(permissions) self._invalidate() + def set_role_overlay(self, role: str, permissions: Collection[str]) -> None: + """Replace *role*'s persisted (admin-editor) grants. + + Unlike :meth:`map_role` this is a replace, so removals take effect; the + code-registered mappings from :meth:`map_role` are left untouched and + stay merged into :attr:`role_map`. + """ + self._role_overlay[role] = set(permissions) + self._invalidate() + @property def role_map(self) -> dict[str, list[str]]: """Merged role→permission mapping (``DEFAULT_ROLE_PERMISSIONS`` + module maps).""" @@ -123,11 +136,9 @@ def role_map(self) -> dict[str, list[str]]: merged: dict[str, list[str]] = { role: list(perms) for role, perms in DEFAULT_ROLE_PERMISSIONS.items() } - for role, perms in self._role_map.items(): - if role in merged: - merged[role] = list(set(merged[role]) | perms) - else: - merged[role] = list(perms) + for source in (self._role_map, self._role_overlay): + for role, perms in source.items(): + merged[role] = list(set(merged.get(role, ())) | perms) self._role_map_cache = merged return self._role_map_cache diff --git a/modules/branding/branding/reaper.py b/modules/branding/branding/reaper.py index b393fb6a..3c8f8d4a 100644 --- a/modules/branding/branding/reaper.py +++ b/modules/branding/branding/reaper.py @@ -72,8 +72,12 @@ async def reap(app: FastAPI, file_id: str, *, tenant_id: str | None) -> None: return services = app.state.file_storage storage = FileStorageService(db, services.backend, services.settings) - await storage.delete(parsed, platform=tenant_id is None) + # Soft-delete and commit first, drop the bytes after: a failed + # commit must never leave a live row with no object. A failure + # after the commit only orphans an object, which is acceptable. + row = await storage.delete(parsed, platform=tenant_id is None, drop_object=False) await db.commit() + await storage.drop_object(row) except Exception: # Deliberately broad: a cleanup failure is never a reason to fail (or, # running after commit, to misreport) a rebrand that succeeded. diff --git a/modules/branding/branding/tenant_branding.py b/modules/branding/branding/tenant_branding.py index 38d3be9e..818561d7 100644 --- a/modules/branding/branding/tenant_branding.py +++ b/modules/branding/branding/tenant_branding.py @@ -188,7 +188,11 @@ def _shared_read( entry = (cache.epoch, read) cache.inflight[tenant_id] = entry - def _done(_: asyncio.Future[dict[str, str]]) -> None: + def _done(fut: asyncio.Future[dict[str, str]]) -> None: + # Retrieve the outcome so a failed read whose every waiter was + # cancelled never logs "exception was never retrieved". + if not fut.cancelled(): + fut.exception() if cache.inflight.get(tenant_id) is entry: del cache.inflight[tenant_id] diff --git a/modules/branding/tests/test_asset_lifecycle.py b/modules/branding/tests/test_asset_lifecycle.py index 0f1880e5..86df2ef4 100644 --- a/modules/branding/tests/test_asset_lifecycle.py +++ b/modules/branding/tests/test_asset_lifecycle.py @@ -47,7 +47,11 @@ async def fake_upload( return row async def fake_delete( - self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + self: FileStorageService, + file_id: uuid.UUID, + *, + platform: bool = False, + drop_object: bool = True, ) -> StoredFile: assert platform, "branding must act on platform-owned files" if file_id in store.deleted: @@ -65,6 +69,7 @@ async def fake_delete( monkeypatch.setattr(FileStorageService, "upload", fake_upload, raising=True) monkeypatch.setattr(FileStorageService, "delete", fake_delete, raising=True) + monkeypatch.setattr(FileStorageService, "drop_object", _noop_drop, raising=True) @pytest.fixture @@ -143,7 +148,11 @@ async def test_a_failed_cleanup_does_not_fail_the_rebrand( await _upload(authenticated_client, "logo") async def boom( - self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + self: FileStorageService, + file_id: uuid.UUID, + *, + platform: bool = False, + drop_object: bool = True, ) -> StoredFile: raise RuntimeError("backend unavailable") @@ -153,3 +162,7 @@ async def boom( assert resp.status_code == 200, resp.text assert resp.json()["logo_url"] is None + + +async def _noop_drop(self: FileStorageService, row: StoredFile) -> None: + """The fake rows have no backend object to drop.""" diff --git a/modules/branding/tests/test_branding_reap_ordering.py b/modules/branding/tests/test_branding_reap_ordering.py new file mode 100644 index 00000000..4159da3c --- /dev/null +++ b/modules/branding/tests/test_branding_reap_ordering.py @@ -0,0 +1,111 @@ +"""The reaper commits the soft-delete before it drops the backend object (#396). + +Also: the tenant-branding in-flight read map is per app and its failures are +always retrieved. +""" + +from __future__ import annotations + +import asyncio +import gc + +from branding import reaper, tenant_branding +from branding.tenant_branding import TenantCache +from file_storage.models import StoredFile +from settings.contracts.schemas import SettingScope +from settings.service import SettingService +from simple_module_db import all_tenants +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +_PNG = b"\x89PNG\r\n\x1a\n" + b"\x00" * 32 + + +async def _upload_then_unset(app, a) -> str: + resp = await a.client.post( + "/api/branding/tenant/logo", files={"file": ("l.png", _PNG, "image/png")} + ) + assert resp.status_code == 200, resp.text + async with app.state.sm.db.session_factory() as db: + service = SettingService(db) + row = await service.get_scoped(SettingScope.TENANT, a.tenant_id, "branding.logo_file_id") + file_id = row.value + await service.delete_scoped(SettingScope.TENANT, a.tenant_id, "branding.logo_file_id") + await db.commit() + return file_id + + +async def _row(app, file_id: str) -> StoredFile: + with all_tenants(): + async with app.state.sm.db.session_factory() as db: + stmt = select(StoredFile).execution_options(include_deleted=True) + return next(r for r in (await db.execute(stmt)).scalars() if str(r.id) == file_id) + + +async def test_failed_commit_keeps_the_bytes(app, tenant_client, monkeypatch): + async with tenant_client() as a: + file_id = await _upload_then_unset(app, a) + key = (await _row(app, file_id)).key + backend = app.state.file_storage.backend + + async def boom(self): + raise RuntimeError("commit failed") + + monkeypatch.setattr(AsyncSession, "commit", boom) + await reaper.reap(app, file_id, tenant_id=a.tenant_id) # logs, never raises + monkeypatch.undo() + + assert (await _row(app, file_id)).is_deleted is False + assert await backend.exists(key) + + +async def test_bytes_are_dropped_only_after_the_commit(app, tenant_client, monkeypatch): + events: list[str] = [] + orig_commit = AsyncSession.commit + + async def commit(self): + events.append("commit") + await orig_commit(self) + + async with tenant_client() as a: + file_id = await _upload_then_unset(app, a) + backend = app.state.file_storage.backend + orig_delete = backend.delete + + async def delete(key): + events.append("delete") + await orig_delete(key) + + monkeypatch.setattr(AsyncSession, "commit", commit) + monkeypatch.setattr(backend, "delete", delete) + await reaper.reap(app, file_id, tenant_id=a.tenant_id) + + assert events == ["commit", "delete"] + assert (await _row(app, file_id)).is_deleted is True + + +async def test_failed_unawaited_read_is_still_retrieved(app, monkeypatch): + cache = TenantCache() + started = asyncio.Event() + + async def boom(_app, _tid): + started.set() + raise RuntimeError("read failed") + + monkeypatch.setattr(tenant_branding, "read_overrides", boom) + entry = tenant_branding._shared_read(app, cache, "t1") + await started.wait() + await asyncio.sleep(0) + assert entry[1].done() + assert cache.inflight == {} + + seen: list[dict] = [] + asyncio.get_running_loop().set_exception_handler(lambda _l, ctx: seen.append(ctx)) + del entry + gc.collect() + assert not seen + + +def test_inflight_is_per_app_state(): + assert not hasattr(tenant_branding, "_INFLIGHT") + assert TenantCache().inflight is not TenantCache().inflight diff --git a/modules/branding/tests/test_logo_dark.py b/modules/branding/tests/test_logo_dark.py index c4580afc..989a761e 100644 --- a/modules/branding/tests/test_logo_dark.py +++ b/modules/branding/tests/test_logo_dark.py @@ -48,7 +48,11 @@ async def fake_upload( return row async def fake_download( - self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + self: FileStorageService, + file_id: uuid.UUID, + *, + platform: bool = False, + drop_object: bool = True, ) -> StreamDownload: assert platform, "branding must act on platform-owned files" @@ -58,7 +62,11 @@ async def body() -> AsyncIterator[bytes]: return StreamDownload(file=rows[file_id], body=body()) async def fake_delete( - self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False + self: FileStorageService, + file_id: uuid.UUID, + *, + platform: bool = False, + drop_object: bool = True, ) -> StoredFile: assert platform, "branding must act on platform-owned files" return rows.pop(file_id) @@ -66,6 +74,7 @@ async def fake_delete( monkeypatch.setattr(FileStorageService, "upload", fake_upload, raising=True) monkeypatch.setattr(FileStorageService, "download", fake_download, raising=True) monkeypatch.setattr(FileStorageService, "delete", fake_delete, raising=True) + monkeypatch.setattr(FileStorageService, "drop_object", _noop_drop, raising=True) return rows @@ -177,3 +186,7 @@ async def test_an_unset_dark_logo_is_a_404(client: httpx.AsyncClient) -> None: # The frontend never requests it in this state — it falls back to logoUrl — # but a hand-typed URL must not 500. assert (await client.get("/api/branding/logo-dark")).status_code == 404 + + +async def _noop_drop(self: FileStorageService, row: StoredFile) -> None: + """The fake rows have no backend object to drop.""" diff --git a/modules/file_storage/file_storage/service.py b/modules/file_storage/file_storage/service.py index b969856f..c7b04724 100644 --- a/modules/file_storage/file_storage/service.py +++ b/modules/file_storage/file_storage/service.py @@ -243,7 +243,15 @@ async def delete_many(self, file_ids: Sequence[uuid.UUID]) -> list[StoredFile]: ) return rows - async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredFile: + async def delete( + self, file_id: uuid.UUID, *, platform: bool = False, drop_object: bool = True + ) -> StoredFile: + """Soft-delete the row and (by default) drop the backend object. + + ``drop_object=False`` only flushes the soft-delete: a caller that must + commit first (so a failed commit never leaves a live row with no bytes) + then calls :meth:`drop_object` with the returned row. + """ # One scope around read + write: its opening flush runs before the # platform row is touched, so the guard never sees that change. async with platform_scope(self.db, platform): @@ -253,10 +261,15 @@ async def delete(self, file_id: uuid.UUID, *, platform: bool = False) -> StoredF row.is_deleted = True row.deleted_at = datetime.now(UTC) await self.db.flush() - # Object is acceptably absent — eg. a previous delete partially succeeded. + if drop_object: + await self.drop_object(row) + return row + + async def drop_object(self, row: StoredFile) -> None: + """Delete ``row``'s backend object; an already-absent object is fine.""" + # Acceptably absent — eg. a previous delete partially succeeded. with contextlib.suppress(StorageNotFoundError): await self.backend.delete(row.key) - return row def _generate_key(tenant_id: str, filename: str) -> str: diff --git a/modules/permissions/permissions/service.py b/modules/permissions/permissions/service.py index f5290fc4..f87c066c 100644 --- a/modules/permissions/permissions/service.py +++ b/modules/permissions/permissions/service.py @@ -112,10 +112,9 @@ async def set_role_permissions( ) await self.db.flush() - # `map_role` is additive — reset the role entry so removals take effect - # without a restart. No public replace API on PermissionRegistry yet. - self.registry._role_map.pop(role.name, None) - self.registry.map_role(role.name, sorted(wanted)) + # Replace only the DB overlay: code-registered mappings stay in effect + # and removed DB keys drop immediately, without a restart. + self.registry.set_role_overlay(role.name, wanted) return RolePermissionsOut(role=role, permissions=sorted(wanted)) @@ -273,7 +272,7 @@ async def load_all_into_registry(self) -> None: for name, key in result.all(): by_role.setdefault(name, []).append(key) for name, keys in by_role.items(): - self.registry.map_role(name, keys) + self.registry.set_role_overlay(name, keys) async def sync_admin_all_permissions(self, assigned_by: str | None = None) -> None: """Ensure the admin role holds every registered permission key. @@ -295,4 +294,4 @@ async def sync_admin_all_permissions(self, assigned_by: str | None = None) -> No for key in missing ) await self.db.flush() - self.registry.map_role(ADMIN_ROLE_NAME, sorted(registered)) + self.registry.set_role_overlay(ADMIN_ROLE_NAME, registered) diff --git a/modules/permissions/tests/test_permissions_role_overlay.py b/modules/permissions/tests/test_permissions_role_overlay.py new file mode 100644 index 00000000..888afbf4 --- /dev/null +++ b/modules/permissions/tests/test_permissions_role_overlay.py @@ -0,0 +1,43 @@ +"""Saving a role in the editor keeps code-registered grants effective (#396).""" + +from __future__ import annotations + +import httpx +from fastapi import FastAPI +from permissions.constants import PERM_VIEW +from simple_module_core.permissions import PermissionRegistry + + +def test_overlay_replace_keeps_code_mapping_and_drops_removed_db_keys(): + reg = PermissionRegistry() + reg.map_role("user", ["a.code"]) + reg.set_role_overlay("user", ["a.db", "b.db"]) + assert set(reg.role_map["user"]) == {"a.code", "a.db", "b.db"} + reg.set_role_overlay("user", ["b.db"]) + assert set(reg.role_map["user"]) == {"a.code", "b.db"} + reg.set_role_overlay("user", []) + assert set(reg.role_map["user"]) == {"a.code"} + + +async def test_saving_a_role_keeps_its_code_mapped_grants( + authenticated_client: httpx.AsyncClient, app: FastAPI +): + from users.constants import USER_ROLE_ID, USER_ROLE_NAME + from users.models import Role + + registry = app.state.sm.permissions + registry.add("zz_demo.code_granted") + registry.map_role(USER_ROLE_NAME, ["zz_demo.code_granted"]) + async with app.state.sm.db.session_factory() as db: + if await db.get(Role, USER_ROLE_ID) is None: + db.add(Role(id=USER_ROLE_ID, name=USER_ROLE_NAME, description="Standard user")) + await db.commit() + + for keys in ([PERM_VIEW], []): + resp = await authenticated_client.put( + f"/api/permissions/roles/{USER_ROLE_ID}", json={"permissions": keys} + ) + assert resp.status_code == 200 + granted = set(registry.role_map[USER_ROLE_NAME]) + assert "zz_demo.code_granted" in granted + assert (PERM_VIEW in granted) == bool(keys) From 9b76effd5e8ec595661903734300536308941532 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Tue, 6 Oct 2026 14:07:43 +0200 Subject: [PATCH 2/2] fix: address code review findings (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01M9neheZZEe3sVpDi2S3zT4 --- .../tests/test_branding_reap_ordering.py | 17 ++++++++--------- modules/branding/tests/test_logo_dark.py | 6 +----- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/modules/branding/tests/test_branding_reap_ordering.py b/modules/branding/tests/test_branding_reap_ordering.py index 4159da3c..2d8468e0 100644 --- a/modules/branding/tests/test_branding_reap_ordering.py +++ b/modules/branding/tests/test_branding_reap_ordering.py @@ -100,12 +100,11 @@ async def boom(_app, _tid): assert cache.inflight == {} seen: list[dict] = [] - asyncio.get_running_loop().set_exception_handler(lambda _l, ctx: seen.append(ctx)) - del entry - gc.collect() - assert not seen - - -def test_inflight_is_per_app_state(): - assert not hasattr(tenant_branding, "_INFLIGHT") - assert TenantCache().inflight is not TenantCache().inflight + loop = asyncio.get_running_loop() + loop.set_exception_handler(lambda _l, ctx: seen.append(ctx)) + try: + del entry + gc.collect() + assert not seen + finally: + loop.set_exception_handler(None) diff --git a/modules/branding/tests/test_logo_dark.py b/modules/branding/tests/test_logo_dark.py index 989a761e..aae6a17e 100644 --- a/modules/branding/tests/test_logo_dark.py +++ b/modules/branding/tests/test_logo_dark.py @@ -48,11 +48,7 @@ async def fake_upload( return row async def fake_download( - self: FileStorageService, - file_id: uuid.UUID, - *, - platform: bool = False, - drop_object: bool = True, + self: FileStorageService, file_id: uuid.UUID, *, platform: bool = False ) -> StreamDownload: assert platform, "branding must act on platform-owned files"