From 7d4579540825e42c03bd5f66d080e8ccd42e904e Mon Sep 17 00:00:00 2001 From: zurdi Date: Fri, 26 Jun 2026 22:10:00 +0000 Subject: [PATCH] fix(permissions): address gantoine review + Postgres-safe single migration Visibility-coverage gaps (404-mask hidden entities, mirroring the existing delete/read paths): - update_rom (PUT /roms/{id}) and update_rom_user (PUT /roms/{id}/props) - add_firmware: platform-hide now cascades to firmware uploads - patch_rom: resolve the parent rom of both the base and patch files and 404 when hidden, so a hidden rom's bytes can no longer be streamed back - activity feeds (get_all_activity / get_rom_activity): drop sessions whose rom is hidden from the caller Migration: make the role enum -> varchar narrowing Postgres-safe. The cast now uses postgresql_using, the orphaned native role type is dropped on upgrade, and downgrade recreates it explicitly (create_type=False) before re-typing the column. Verified up/down/up on Postgres 16 and MariaDB. Also collapses the two permission migrations into a single 0092 and notes the override own_only replacement granularity limit in the resolver. AI assistance: implemented with Claude Code (review-fix pass). Co-Authored-By: Claude Opus 4.8 --- .../0092_narrow_role_to_admin_user.py | 56 ---------------- ...on_system.py => 0092_permission_system.py} | 67 +++++++++++++++++-- backend/endpoints/activity.py | 21 +++++- backend/endpoints/firmware.py | 5 ++ backend/endpoints/roms/__init__.py | 8 +++ backend/endpoints/roms/patch.py | 19 ++++++ backend/handler/auth/permissions.py | 4 ++ .../endpoints/test_permissions_visibility.py | 31 +++++++++ 8 files changed, 147 insertions(+), 64 deletions(-) delete mode 100644 backend/alembic/versions/0092_narrow_role_to_admin_user.py rename backend/alembic/versions/{0091_permission_system.py => 0092_permission_system.py} (78%) diff --git a/backend/alembic/versions/0092_narrow_role_to_admin_user.py b/backend/alembic/versions/0092_narrow_role_to_admin_user.py deleted file mode 100644 index 85050d470..000000000 --- a/backend/alembic/versions/0092_narrow_role_to_admin_user.py +++ /dev/null @@ -1,56 +0,0 @@ -"""Narrow the user role vocabulary from viewer/editor/admin to user/admin. - -The viewer/editor distinction now lives entirely in permission groups, so the -`role` column collapses to two kinds: `admin` (bypasses everything) and `user`. - -The column is converted from a native enum to a portable VARCHAR (matching the -rest of the permission vocabulary, `native_enum=False`) and existing values are -normalized: ADMIN -> admin, VIEWER/EDITOR -> user. Stored values are lowercase. - -Revision ID: 0092_narrow_role_to_admin_user -Revises: 0091_permission_system -Create Date: 2026-06-25 18:00:00.000000 - -""" - -import sqlalchemy as sa -from alembic import op - -# revision identifiers, used by Alembic. -revision = "0092_narrow_role_to_admin_user" -down_revision = "0091_permission_system" -branch_labels = None -depends_on = None - - -def upgrade() -> None: - # Drop the native 3-value enum in favour of a plain VARCHAR so the - # vocabulary can change without a destructive cross-dialect ALTER TYPE. - with op.batch_alter_table("users") as batch_op: - batch_op.alter_column( - "role", - existing_type=sa.Enum("VIEWER", "EDITOR", "ADMIN", name="role"), - type_=sa.String(length=20), - existing_nullable=False, - ) - - # Collapse to the two-value lowercase vocabulary. UPPER() guards against - # either casing being present (native enums stored the uppercase name). - op.execute("UPDATE users SET role = 'admin' WHERE UPPER(role) = 'ADMIN'") - op.execute( - "UPDATE users SET role = 'user' " - "WHERE UPPER(role) IN ('VIEWER', 'EDITOR', 'USER') OR role IS NULL" - ) - - -def downgrade() -> None: - # Best-effort reverse: map user -> viewer, keep admin, restore the enum. - op.execute("UPDATE users SET role = 'VIEWER' WHERE LOWER(role) = 'user'") - op.execute("UPDATE users SET role = 'ADMIN' WHERE LOWER(role) = 'admin'") - with op.batch_alter_table("users") as batch_op: - batch_op.alter_column( - "role", - existing_type=sa.String(length=20), - type_=sa.Enum("VIEWER", "EDITOR", "ADMIN", name="role"), - existing_nullable=False, - ) diff --git a/backend/alembic/versions/0091_permission_system.py b/backend/alembic/versions/0092_permission_system.py similarity index 78% rename from backend/alembic/versions/0091_permission_system.py rename to backend/alembic/versions/0092_permission_system.py index 7a8311c3a..bcc135690 100644 --- a/backend/alembic/versions/0091_permission_system.py +++ b/backend/alembic/versions/0092_permission_system.py @@ -1,8 +1,8 @@ -"""Granular permission system: groups, grants, per-user overrides, hidden entities. +"""Granular permission system: groups, grants, overrides, hidden entities, and +the narrowing of the user role vocabulary to admin/user. -Single migration for the whole permissions feature (tables, group colour, and -the legacy-group backfill). Backfills so that NO user gains or loses access on -upgrade: +Single migration for the whole permissions feature. Backfills so that NO user +gains or loses access on upgrade: * "Viewer (legacy)" group reproduces the old non-kiosk default user (WRITE_SCOPES): read the library, manage only own collections/assets/devices. @@ -11,11 +11,15 @@ upgrade: editors can already delete -- preserved here as explicit delete grants). * Existing users are assigned to the matching group by their current role. Admins are left unassigned (they bypass groups entirely). + * The `role` column is then narrowed from the viewer/editor/admin enum to a + portable VARCHAR holding just `admin`/`user` (the viewer/editor distinction + now lives entirely in permission groups). The group assignment above MUST + happen before this, since it reads the old VIEWER/EDITOR values. The seeded grant matrix is the frozen twin of ``handler/auth/permissions_map.py``; ``tests/handler/auth/test_permissions_parity.py`` keeps the two in lockstep. -Revision ID: 0091_permission_system +Revision ID: 0092_permission_system Revises: 0091_unique_platform_fs_name Create Date: 2026-06-22 00:00:00.000000 @@ -23,9 +27,12 @@ Create Date: 2026-06-22 00:00:00.000000 import sqlalchemy as sa from alembic import op +from sqlalchemy.dialects.postgresql import ENUM as PostgresEnum + +from utils.database import is_postgresql # revision identifiers, used by Alembic. -revision = "0091_permission_system" +revision = "0092_permission_system" down_revision = "0091_unique_platform_fs_name" branch_labels = None depends_on = None @@ -61,6 +68,9 @@ EDITOR_GRANTS = VIEWER_GRANTS + EDITOR_EXTRA VIEWER_COLOR = "#3b82f6" EDITOR_COLOR = "#f59e0b" +# The pre-narrowing role enum, used by the role alter_column in both directions. +_ROLE_ENUM = sa.Enum("VIEWER", "EDITOR", "ADMIN", name="role") + def _timestamp_cols() -> tuple[sa.Column, sa.Column]: # Fresh Column objects per call: a Column instance can only belong to one @@ -217,8 +227,30 @@ def upgrade() -> None: if_not_exists=True, ) + # Seed + assign by current role while `role` still holds VIEWER/EDITOR/ADMIN. _seed_legacy_groups() + # Now collapse the role vocabulary to the two-value lowercase set. Convert + # the native enum to a portable VARCHAR first, then normalize the values. + # On Postgres the cast needs an explicit USING and the orphaned enum type + # must be dropped so a later downgrade can recreate it cleanly. + connection = op.get_bind() + with op.batch_alter_table("users") as batch_op: + batch_op.alter_column( + "role", + existing_type=_ROLE_ENUM, + type_=sa.String(length=20), + existing_nullable=False, + postgresql_using="role::text", + ) + op.execute("UPDATE users SET role = 'admin' WHERE UPPER(role) = 'ADMIN'") + op.execute( + "UPDATE users SET role = 'user' " + "WHERE UPPER(role) IN ('VIEWER', 'EDITOR', 'USER') OR role IS NULL" + ) + if is_postgresql(connection): + PostgresEnum(name="role").drop(connection, checkfirst=True) + def _seed_legacy_groups() -> None: conn = op.get_bind() @@ -300,6 +332,29 @@ def _seed_legacy_groups() -> None: def downgrade() -> None: + # Restore the role enum (reverse of the narrowing) before tearing down the + # permission infrastructure. Values are restored while still VARCHAR. + connection = op.get_bind() + op.execute("UPDATE users SET role = 'VIEWER' WHERE LOWER(role) = 'user'") + op.execute("UPDATE users SET role = 'ADMIN' WHERE LOWER(role) = 'admin'") + # On Postgres the native enum type was dropped on upgrade; recreate it + # explicitly (create_type=False keeps the alter from re-issuing CREATE TYPE) + # and cast the column back with an explicit USING. + role_type: sa.Enum = _ROLE_ENUM + if is_postgresql(connection): + role_type = PostgresEnum( + "VIEWER", "EDITOR", "ADMIN", name="role", create_type=False + ) + role_type.create(connection, checkfirst=True) + with op.batch_alter_table("users") as batch_op: + batch_op.alter_column( + "role", + existing_type=sa.String(length=20), + type_=role_type, + existing_nullable=False, + postgresql_using="role::role", + ) + with op.batch_alter_table("users") as batch_op: batch_op.drop_constraint("fk_users_permission_group_id", type_="foreignkey") batch_op.drop_index("ix_users_permission_group_id", if_exists=True) diff --git a/backend/endpoints/activity.py b/backend/endpoints/activity.py index eb0a7afe6..930d83179 100644 --- a/backend/endpoints/activity.py +++ b/backend/endpoints/activity.py @@ -7,6 +7,7 @@ from decorators.auth import protected_route from endpoints.responses.activity import ActivityClearSchema, ActivityEntrySchema from handler.activity_handler import ActivityEntry, activity_handler from handler.auth.constants import Scope +from handler.auth.dependencies import get_permissions from handler.database import db_device_handler, db_rom_handler, db_save_handler from handler.socket_handler import socket_handler from logger.logger import log @@ -19,6 +20,22 @@ router = APIRouter( ) +def _visible_activity( + request: Request, entries: list[ActivityEntry] +) -> list[ActivityEntrySchema]: + """Drop sessions whose ROM is hidden from the caller (platform or rom hide).""" + perms = get_permissions(request) + if not perms.is_admin and (perms.hidden_platform_ids or perms.hidden_rom_ids): + rom_ids = [e["rom_id"] for e in entries] + hidden = db_rom_handler.get_hidden_rom_ids_among( + rom_ids, + list(perms.hidden_platform_ids), + list(perms.hidden_rom_ids), + ) + entries = [e for e in entries if e["rom_id"] not in hidden] + return [ActivityEntrySchema(**e) for e in entries] + + class DeviceHeartbeatPayload(BaseModel): rom_id: int = Field(ge=1) device_id: str = Field(min_length=1, max_length=255) @@ -28,14 +45,14 @@ class DeviceHeartbeatPayload(BaseModel): async def get_all_activity(request: Request) -> list[ActivityEntrySchema]: """Return every currently active play session across all users.""" entries = await activity_handler.get_all_active() - return [ActivityEntrySchema(**e) for e in entries] + return _visible_activity(request, entries) @protected_route(router.get, "/rom/{rom_id}", [Scope.ROMS_USER_READ]) async def get_rom_activity(request: Request, rom_id: int) -> list[ActivityEntrySchema]: """Return all active play sessions for a specific ROM.""" entries = await activity_handler.get_active_for_rom(rom_id) - return [ActivityEntrySchema(**e) for e in entries] + return _visible_activity(request, entries) @protected_route(router.post, "/heartbeat", [Scope.ROMS_USER_WRITE]) diff --git a/backend/endpoints/firmware.py b/backend/endpoints/firmware.py index 4f405419e..8993b4310 100644 --- a/backend/endpoints/firmware.py +++ b/backend/endpoints/firmware.py @@ -51,6 +51,11 @@ async def add_firmware( log.error(error) raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=error) + # Platform-hide cascades to firmware; 404-mask uploads to hidden platforms. + if not get_permissions(request).can_see_platform(db_platform.id): + error = f"Platform with ID {platform_id} not found" + raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail=error) + uploaded_firmware = [] firmware_path = fs_firmware_handler.get_firmware_fs_structure(db_platform.fs_slug) diff --git a/backend/endpoints/roms/__init__.py b/backend/endpoints/roms/__init__.py index f557559b0..ead7c539a 100644 --- a/backend/endpoints/roms/__init__.py +++ b/backend/endpoints/roms/__init__.py @@ -1318,6 +1318,10 @@ async def update_rom( if not rom: raise RomNotFoundInDatabaseException(id) + # 404-mask roms hidden from the caller rather than letting them edit. + if not get_permissions(request).can_see_rom(rom.id, rom.platform_id): + raise RomNotFoundInDatabaseException(id) + if unmatch_metadata: db_rom_handler.update_rom( id, @@ -1830,6 +1834,10 @@ async def update_rom_user( if not rom: raise RomNotFoundInDatabaseException(id) + # 404-mask roms hidden from the caller rather than confirming existence. + if not get_permissions(request).can_see_rom(rom.id, rom.platform_id): + raise RomNotFoundInDatabaseException(id) + db_rom_user = db_rom_handler.get_rom_user( id, request.user.id ) or db_rom_handler.add_rom_user(id, request.user.id) diff --git a/backend/endpoints/roms/patch.py b/backend/endpoints/roms/patch.py index 86187c479..ed7ec5755 100644 --- a/backend/endpoints/roms/patch.py +++ b/backend/endpoints/roms/patch.py @@ -14,6 +14,7 @@ from starlette.responses import FileResponse from config import ROM_PATCHER_MAX_FILE_SIZE_BYTES from decorators.auth import protected_route from handler.auth.constants import Scope +from handler.auth.dependencies import get_permissions from handler.database import db_rom_handler from handler.filesystem import fs_rom_handler from logger.formatter import BLUE @@ -64,12 +65,21 @@ async def patch_rom( request.user.username if request.user.is_authenticated else "unknown" ) + perms = get_permissions(request) + rom_file = db_rom_handler.get_rom_file_by_id(id) if not rom_file: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, detail=f"ROM file with id {id} not found", ) + # 404-mask file bytes of roms hidden from the caller. + base_rom = db_rom_handler.get_rom(rom_file.rom_id) + if not base_rom or not perms.can_see_rom(base_rom.id, base_rom.platform_id): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"ROM file with id {id} not found", + ) if rom_file.missing_from_fs: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, @@ -82,6 +92,15 @@ async def patch_rom( status_code=status.HTTP_404_NOT_FOUND, detail=f"Patch file with id {patch_request.patch_file_id} not found", ) + # The patch file's bytes are read too; mask it if its rom is hidden. + patch_rom_parent = db_rom_handler.get_rom(patch_file.rom_id) + if not patch_rom_parent or not perms.can_see_rom( + patch_rom_parent.id, patch_rom_parent.platform_id + ): + raise HTTPException( + status_code=status.HTTP_404_NOT_FOUND, + detail=f"Patch file with id {patch_request.patch_file_id} not found", + ) if patch_file.missing_from_fs: raise HTTPException( status_code=status.HTTP_404_NOT_FOUND, diff --git a/backend/handler/auth/permissions.py b/backend/handler/auth/permissions.py index d6acaf5c2..d4aa4a0b7 100644 --- a/backend/handler/auth/permissions.py +++ b/backend/handler/auth/permissions.py @@ -94,6 +94,10 @@ def _resolve_grant_map( base[(g.entity, g.action)] = g.own_only # Per-user overrides win over the group: grant adds, revoke removes. + # Override identity is (entity, action) only, so a grant override fully + # replaces the group's own_only for that key rather than merging. Both + # directions are admin-initiated and the narrowing case fails closed, so + # this is a granularity limit, not a privilege leak. if user.id is not None: for ov in db_permission_handler.get_user_overrides(user.id, session=session): if ov.granted: diff --git a/backend/tests/endpoints/test_permissions_visibility.py b/backend/tests/endpoints/test_permissions_visibility.py index 44a86633f..b18cf9ebd 100644 --- a/backend/tests/endpoints/test_permissions_visibility.py +++ b/backend/tests/endpoints/test_permissions_visibility.py @@ -120,6 +120,37 @@ def test_hidden_rom_cannot_be_downloaded_by_id(client, viewer_user, rom): assert content_resp.status_code == status.HTTP_404_NOT_FOUND +def test_hidden_rom_update_is_404_masked(client, editor_user, rom): + # Editor holds library-wide roms write (passes the coarse gate), so the + # hidden rom must be 404-masked instead of being editable. + _hide(PermEntity.ROMS, rom.id, editor_user.id) + resp = client.put( + f"/api/roms/{rom.id}", headers=_auth(editor_user), data={"name": "x"} + ) + assert resp.status_code == status.HTTP_404_NOT_FOUND + + +def test_hidden_rom_props_update_is_404_masked(client, viewer_user, rom): + # ROMS_USER_WRITE is a self-service scope every user holds, so the coarse + # gate passes; the hidden rom must still be masked, not confirmed. + _hide(PermEntity.ROMS, rom.id, viewer_user.id) + resp = client.put( + f"/api/roms/{rom.id}/props", headers=_auth(viewer_user), json={"rating": 5} + ) + assert resp.status_code == status.HTTP_404_NOT_FOUND + + +def test_hidden_rom_patch_is_404_masked(client, viewer_user, rom, rom_file): + # patch_rom streams file bytes back; a hidden rom's bytes must not leak. + _hide(PermEntity.ROMS, rom.id, viewer_user.id) + resp = client.post( + f"/api/roms/{rom_file.id}/patch", + headers=_auth(viewer_user), + json={"patch_file_id": rom_file.id}, + ) + assert resp.status_code == status.HTTP_404_NOT_FOUND + + def test_delete_requires_delete_grant_even_with_write_scope( client, viewer_user, platform ):