From e4e37fb99e5b5772b5ebcfc960bed79f1873185e Mon Sep 17 00:00:00 2001 From: maziggy Date: Mon, 2 Feb 2026 08:01:42 +0100 Subject: [PATCH] Issue #224: File Manager Permissions The File Manager (Library) backend had no permission enforcement - endpoints were returning data to any authenticated user regardless of their group permissions. Closes #224 --- CHANGELOG.md | 8 + backend/app/api/routes/library.py | 105 +++++++-- backend/tests/integration/test_library_api.py | 207 ++++++++++++++++++ 3 files changed, 303 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca9659c38..3b5621927 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,14 @@ All notable changes to Bambuddy will be documented in this file. - Removed ~2000 lines of legacy JSON-based backup/restore code ### Fixes +- **File Manager permissions not enforced** (Issue #224) - Fixed backend not checking `library:read` permission for File Manager endpoints: + - Added `library:read` permission check to all list/view endpoints (files, folders, stats) + - Added `library:upload` permission check to upload and folder creation endpoints + - Added `queue:create` permission check to add-to-queue endpoint + - Added `printers:control` permission check to direct print endpoint + - Added ownership-based permission checks to file move operation + - Users without `library:read` permission can no longer view files in the File Manager + - Users can now only delete/update their own files unless they have `*_all` permissions - **JWT secret key not persistent across restarts** - Fixed JWT secret key generation to properly use data directory, ensuring tokens remain valid across container restarts - **Images/thumbnails returning 401 when auth enabled** - Fixed auth middleware to allow public access to image/media endpoints (thumbnails, photos, QR codes, timelapses, camera streams) since browser elements like `` don't send Authorization headers - **Library thumbnails missing after restore** - Fixed library files using absolute paths that break after restore on different systems: diff --git a/backend/app/api/routes/library.py b/backend/app/api/routes/library.py index 957e1a396..62334e8c1 100644 --- a/backend/app/api/routes/library.py +++ b/backend/app/api/routes/library.py @@ -16,7 +16,6 @@ from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload from backend.app.core.auth import ( - require_auth_if_enabled, require_ownership_permission, require_permission_if_auth_enabled, ) @@ -237,7 +236,11 @@ IMAGE_EXTENSIONS = {".png", ".jpg", ".jpeg", ".gif", ".webp", ".bmp", ".tiff", " @router.get("/folders", response_model=list[FolderTreeItem]) @router.get("/folders/", response_model=list[FolderTreeItem]) -async def list_folders(response: Response, db: AsyncSession = Depends(get_db)): +async def list_folders( + response: Response, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get all folders as a tree structure.""" # Prevent browser caching of folder list response.headers["Cache-Control"] = "no-cache, no-store, must-revalidate" @@ -289,7 +292,11 @@ async def list_folders(response: Response, db: AsyncSession = Depends(get_db)): @router.get("/folders/by-project/{project_id}", response_model=list[FolderResponse]) -async def get_folders_by_project(project_id: int, db: AsyncSession = Depends(get_db)): +async def get_folders_by_project( + project_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get all folders linked to a specific project.""" result = await db.execute( select(LibraryFolder, Project.name) @@ -326,7 +333,11 @@ async def get_folders_by_project(project_id: int, db: AsyncSession = Depends(get @router.get("/folders/by-archive/{archive_id}", response_model=list[FolderResponse]) -async def get_folders_by_archive(archive_id: int, db: AsyncSession = Depends(get_db)): +async def get_folders_by_archive( + archive_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get all folders linked to a specific archive.""" result = await db.execute( select(LibraryFolder, PrintArchive.print_name) @@ -364,7 +375,11 @@ async def get_folders_by_archive(archive_id: int, db: AsyncSession = Depends(get @router.post("/folders", response_model=FolderResponse) @router.post("/folders/", response_model=FolderResponse) -async def create_folder(data: FolderCreate, db: AsyncSession = Depends(get_db)): +async def create_folder( + data: FolderCreate, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_UPLOAD)), +): """Create a new folder.""" # Verify parent exists if specified if data.parent_id is not None: @@ -415,7 +430,11 @@ async def create_folder(data: FolderCreate, db: AsyncSession = Depends(get_db)): @router.get("/folders/{folder_id}", response_model=FolderResponse) -async def get_folder(folder_id: int, db: AsyncSession = Depends(get_db)): +async def get_folder( + folder_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get a folder by ID.""" result = await db.execute( select(LibraryFolder, Project.name, PrintArchive.print_name) @@ -449,8 +468,17 @@ async def get_folder(folder_id: int, db: AsyncSession = Depends(get_db)): @router.put("/folders/{folder_id}", response_model=FolderResponse) -async def update_folder(folder_id: int, data: FolderUpdate, db: AsyncSession = Depends(get_db)): - """Update a folder.""" +async def update_folder( + folder_id: int, + data: FolderUpdate, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_UPDATE_ALL)), +): + """Update a folder. + + Note: Folders require library:update_all permission since they don't have + ownership tracking. + """ result = await db.execute(select(LibraryFolder).where(LibraryFolder.id == folder_id)) folder = result.scalar_one_or_none() @@ -595,6 +623,7 @@ async def list_files( folder_id: int | None = None, include_root: bool = True, db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), ): """List files, optionally filtered by folder. @@ -669,7 +698,7 @@ async def upload_file( folder_id: int | None = None, generate_stl_thumbnails: bool = Query(default=True), db: AsyncSession = Depends(get_db), - current_user: User | None = Depends(require_auth_if_enabled), + current_user: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_UPLOAD)), ): """Upload a file to the library.""" try: @@ -805,7 +834,7 @@ async def extract_zip_file( create_folder_from_zip: bool = Query(default=False), generate_stl_thumbnails: bool = Query(default=True), db: AsyncSession = Depends(get_db), - current_user: User | None = Depends(require_auth_if_enabled), + current_user: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_UPLOAD)), ): """Upload and extract a ZIP file to the library. @@ -1064,9 +1093,13 @@ async def extract_zip_file( async def batch_generate_stl_thumbnails( request: BatchThumbnailRequest, db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_UPDATE_ALL)), ): """Generate thumbnails for STL files in batch. + Note: Requires library:update_all permission since this is a batch operation + that may affect files owned by different users. + Can generate thumbnails for: - Specific file IDs (file_ids) - All STL files in a folder (folder_id) @@ -1188,6 +1221,7 @@ def is_sliced_file(filename: str) -> bool: async def add_files_to_queue( request: AddToQueueRequest, db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.QUEUE_CREATE)), ): """Add library files to the print queue. @@ -1266,6 +1300,7 @@ async def add_files_to_queue( async def get_library_file_plates( file_id: int, db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), ): """Get available plates from a multi-plate 3MF library file. @@ -1477,6 +1512,7 @@ async def get_library_file_filament_requirements( file_id: int, plate_id: int | None = None, db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), ): """Get filament requirements from a library file. @@ -1599,6 +1635,7 @@ async def print_library_file( printer_id: int, body: FilePrintRequest | None = None, db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.PRINTERS_CONTROL)), ): """Print a library file directly. @@ -1786,7 +1823,11 @@ async def print_library_file( @router.get("/files/{file_id}", response_model=FileResponseSchema) -async def get_file(file_id: int, db: AsyncSession = Depends(get_db)): +async def get_file( + file_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get a file by ID with full details.""" result = await db.execute( select(LibraryFile).options(selectinload(LibraryFile.created_by)).where(LibraryFile.id == file_id) @@ -1961,7 +2002,11 @@ async def delete_file( @router.get("/files/{file_id}/download") -async def download_file(file_id: int, db: AsyncSession = Depends(get_db)): +async def download_file( + file_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Download a file.""" result = await db.execute(select(LibraryFile).where(LibraryFile.id == file_id)) file = result.scalar_one_or_none() @@ -2008,7 +2053,11 @@ async def get_thumbnail(file_id: int, db: AsyncSession = Depends(get_db)): @router.get("/files/{file_id}/gcode") -async def get_gcode(file_id: int, db: AsyncSession = Depends(get_db)): +async def get_gcode( + file_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get gcode for a file (for preview).""" result = await db.execute(select(LibraryFile).where(LibraryFile.id == file_id)) file = result.scalar_one_or_none() @@ -2046,8 +2095,22 @@ async def get_gcode(file_id: int, db: AsyncSession = Depends(get_db)): @router.post("/files/move") -async def move_files(data: FileMoveRequest, db: AsyncSession = Depends(get_db)): - """Move multiple files to a folder.""" +async def move_files( + data: FileMoveRequest, + db: AsyncSession = Depends(get_db), + auth_result: tuple[User | None, bool] = Depends( + require_ownership_permission( + Permission.LIBRARY_UPDATE_ALL, + Permission.LIBRARY_UPDATE_OWN, + ) + ), +): + """Move multiple files to a folder. + + Files not owned by the user are skipped (unless user has *_all permission). + """ + user, can_modify_all = auth_result + # Verify folder exists if specified if data.folder_id is not None: folder_result = await db.execute(select(LibraryFolder).where(LibraryFolder.id == data.folder_id)) @@ -2056,14 +2119,19 @@ async def move_files(data: FileMoveRequest, db: AsyncSession = Depends(get_db)): # Update files moved = 0 + skipped = 0 for file_id in data.file_ids: result = await db.execute(select(LibraryFile).where(LibraryFile.id == file_id)) file = result.scalar_one_or_none() if file: + # Ownership check + if not can_modify_all and file.created_by_id != user.id: + skipped += 1 + continue file.folder_id = data.folder_id moved += 1 - return {"status": "success", "moved": moved} + return {"status": "success", "moved": moved, "skipped": skipped} @router.post("/bulk-delete", response_model=BulkDeleteResponse) @@ -2133,7 +2201,10 @@ async def bulk_delete( @router.get("/stats") -async def get_library_stats(db: AsyncSession = Depends(get_db)): +async def get_library_stats( + db: AsyncSession = Depends(get_db), + _: User | None = Depends(require_permission_if_auth_enabled(Permission.LIBRARY_READ)), +): """Get library statistics.""" # Total files total_files_result = await db.execute(select(func.count(LibraryFile.id))) diff --git a/backend/tests/integration/test_library_api.py b/backend/tests/integration/test_library_api.py index bacd4888d..45341e9b6 100644 --- a/backend/tests/integration/test_library_api.py +++ b/backend/tests/integration/test_library_api.py @@ -848,3 +848,210 @@ class TestLibraryPathHelpers: assert to_absolute_path(None) is None assert to_absolute_path("") is None + + +class TestLibraryPermissions: + """Tests for library permission enforcement.""" + + @pytest.fixture + async def auth_setup(self, db_session): + """Set up auth with users of different permission levels.""" + from backend.app.core.auth import create_access_token, get_password_hash + from backend.app.models.group import Group + from backend.app.models.settings import Settings + from backend.app.models.user import User + + # Enable auth + settings = Settings(key="auth_enabled", value="true") + db_session.add(settings) + await db_session.commit() + + # Groups are auto-seeded during db init, but we need to commit them + await db_session.commit() + + # Get groups + from sqlalchemy import select + + admin_group = (await db_session.execute(select(Group).where(Group.name == "Administrators"))).scalar_one() + operator_group = (await db_session.execute(select(Group).where(Group.name == "Operators"))).scalar_one() + viewer_group = (await db_session.execute(select(Group).where(Group.name == "Viewers"))).scalar_one() + + password_hash = get_password_hash("password") + + # Create users + admin_user = User(username="admin_lib", password_hash=password_hash, role="admin", is_active=True) + admin_user.groups.append(admin_group) + + operator_user = User(username="operator_lib", password_hash=password_hash, is_active=True) + operator_user.groups.append(operator_group) + + viewer_user = User(username="viewer_lib", password_hash=password_hash, is_active=True) + viewer_user.groups.append(viewer_group) + + db_session.add_all([admin_user, operator_user, viewer_user]) + await db_session.commit() + + # Create tokens + admin_token = create_access_token(data={"sub": admin_user.username}) + operator_token = create_access_token(data={"sub": operator_user.username}) + viewer_token = create_access_token(data={"sub": viewer_user.username}) + + return { + "admin_user": admin_user, + "operator_user": operator_user, + "viewer_user": viewer_user, + "admin_token": admin_token, + "operator_token": operator_token, + "viewer_token": viewer_token, + } + + @pytest.fixture + async def test_file(self, db_session, auth_setup): + """Create a test file owned by the operator user.""" + from backend.app.models.library import LibraryFile + + operator_user = auth_setup["operator_user"] + lib_file = LibraryFile( + filename="test.txt", + file_path="data/archive/library/files/test.txt", + file_type="txt", + file_size=100, + created_by_id=operator_user.id, + ) + db_session.add(lib_file) + await db_session.commit() + await db_session.refresh(lib_file) + return lib_file + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_list_files_requires_library_read(self, async_client: AsyncClient, db_session, auth_setup): + """Verify list_files requires library:read permission.""" + viewer_token = auth_setup["viewer_token"] + + # Viewers have library:read, should succeed + response = await async_client.get("/api/v1/library/files", headers={"Authorization": f"Bearer {viewer_token}"}) + assert response.status_code == 200 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_list_files_denied_without_permission(self, async_client: AsyncClient, db_session): + """Verify list_files denied without auth when auth is enabled.""" + from backend.app.models.settings import Settings + + # Enable auth + settings = Settings(key="auth_enabled", value="true") + db_session.add(settings) + await db_session.commit() + + # Request without token should fail + response = await async_client.get("/api/v1/library/files") + assert response.status_code == 401 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_delete_file_own_by_owner(self, async_client: AsyncClient, db_session, auth_setup, test_file): + """Verify operator can delete their own files.""" + from pathlib import Path + + # Create actual file on disk so delete doesn't fail + from backend.app.core.config import settings as app_settings + + file_path = Path(app_settings.base_dir) / test_file.file_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text("test content") + + operator_token = auth_setup["operator_token"] + + response = await async_client.delete( + f"/api/v1/library/files/{test_file.id}", headers={"Authorization": f"Bearer {operator_token}"} + ) + assert response.status_code == 200 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_delete_file_own_denied_for_others_file(self, async_client: AsyncClient, db_session, auth_setup): + """Verify operator cannot delete files owned by others.""" + # Create another operator user with a file + from sqlalchemy import select + + from backend.app.core.auth import create_access_token + from backend.app.models.group import Group + from backend.app.models.library import LibraryFile + from backend.app.models.user import User + + operator_group = (await db_session.execute(select(Group).where(Group.name == "Operators"))).scalar_one() + + from backend.app.core.auth import get_password_hash as get_pw_hash + + other_user = User(username="other_op", password_hash=get_pw_hash("password"), is_active=True) + other_user.groups.append(operator_group) + db_session.add(other_user) + await db_session.commit() + await db_session.refresh(other_user) + + # Create file owned by other user + other_file = LibraryFile( + filename="other.txt", + file_path="data/archive/library/files/other.txt", + file_type="txt", + file_size=100, + created_by_id=other_user.id, + ) + db_session.add(other_file) + await db_session.commit() + await db_session.refresh(other_file) + + # Original operator should not be able to delete it + operator_token = auth_setup["operator_token"] + response = await async_client.delete( + f"/api/v1/library/files/{other_file.id}", headers={"Authorization": f"Bearer {operator_token}"} + ) + assert response.status_code == 403 + assert "your own files" in response.json()["detail"].lower() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_delete_file_admin_can_delete_any(self, async_client: AsyncClient, db_session, auth_setup): + """Verify admin can delete any file.""" + from pathlib import Path + + from backend.app.core.config import settings as app_settings + from backend.app.models.library import LibraryFile + + # Create file owned by operator + operator_user = auth_setup["operator_user"] + lib_file = LibraryFile( + filename="admin_can_delete.txt", + file_path="data/archive/library/files/admin_can_delete.txt", + file_type="txt", + file_size=100, + created_by_id=operator_user.id, + ) + db_session.add(lib_file) + await db_session.commit() + await db_session.refresh(lib_file) + + # Create actual file on disk + file_path = Path(app_settings.base_dir) / lib_file.file_path + file_path.parent.mkdir(parents=True, exist_ok=True) + file_path.write_text("test content") + + # Admin should be able to delete it + admin_token = auth_setup["admin_token"] + response = await async_client.delete( + f"/api/v1/library/files/{lib_file.id}", headers={"Authorization": f"Bearer {admin_token}"} + ) + assert response.status_code == 200 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_viewer_cannot_delete_files(self, async_client: AsyncClient, db_session, auth_setup, test_file): + """Verify viewer cannot delete any files.""" + viewer_token = auth_setup["viewer_token"] + + response = await async_client.delete( + f"/api/v1/library/files/{test_file.id}", headers={"Authorization": f"Bearer {viewer_token}"} + ) + # Viewers don't have delete_own or delete_all permissions + assert response.status_code == 403