From 93cae4dddd7f0c5154fa9b7d5b1f112327e6e8b3 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sat, 27 Jun 2026 09:47:35 +0200 Subject: [PATCH] fix(auth): API keys with Manage Library can curate library files (#1832) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit require_ownership_permission gates API keys on `all_perm` only — the comment at auth.py:1659 says OWN and ALL "both map to the same scope flag" for queue / archives / etc., so checking `all_perm` is the correct gate. Library deliberately broke that: LIBRARY_UPDATE_OWN / LIBRARY_DELETE_OWN mapped to can_manage_library, but the ALL variants were in _APIKEY_DENIED_PERMISSIONS. Result — every API-key request to DELETE /library/files/{id}, PUT /library/files/{id} (rename), or POST /library/files/move hit "administrative operations" 403, even for keys with can_manage_library=True. Only slice worked, because it doesn't go through require_ownership_permission. The "ALL stays admin-only because it crosses the user boundary" intent was internally inconsistent. API keys have no per-row ownership identity (user=None), so the route's `file.created_by_id != user.id` ownership check would AttributeError on a key acting under OWN anyway — the only working path is can_modify_all=True, which `all_perm` denial blocked outright. Fix folds LIBRARY_UPDATE_ALL and LIBRARY_DELETE_ALL into _APIKEY_SCOPE_BY_PERMISSION under can_manage_library, matching the can_queue precedent (QUEUE_UPDATE_OWN and QUEUE_UPDATE_ALL both map to can_queue for the same per-key-identity reason). Both removed from _APIKEY_DENIED_PERMISSIONS. LIBRARY_PURGE stays denied — it bypasses the soft-delete window and is genuinely destructive. --- CHANGELOG.md | 2 + backend/app/core/auth.py | 21 ++- .../integration/test_auth_apikey_rbac.py | 11 +- backend/tests/integration/test_library_api.py | 134 ++++++++++++++++++ 4 files changed, 161 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d35fd4507..3089e8e4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.5b1] - Unreleased ### Fixed +- **API keys with Manage Library permission can now rename / delete / move library files (#1832, reporter @MorganMLGman)** — `require_ownership_permission` gates API keys on `all_perm` only (line 1668) — the comment block at line 1659 says OWN and ALL "both map to the same scope flag" for queue / archives / etc., so checking `all_perm` is the correct gate. Library deliberately broke that invariant by putting `LIBRARY_UPDATE_ALL` / `LIBRARY_DELETE_ALL` in `_APIKEY_DENIED_PERMISSIONS` while only the OWN variants were allowlisted under `can_manage_library`. Net effect: every library curation route (DELETE `/library/files/{id}`, PUT `/library/files/{id}` rename, POST `/library/files/move`) returned `403 "API keys cannot be used for administrative operations"` for keys with `can_manage_library=True`, contradicting the wiki docs that explicitly list "rename and delete your own library entries" under that scope. Only `POST /library/files/{id}/slice` worked (it doesn't go through `require_ownership_permission`). The "ALL stays admin-only because it crosses the user boundary" comment was internally inconsistent: API keys have no per-row ownership identity (`user=None`), so the route's `file.created_by_id != user.id` ownership check would `AttributeError` on a key acting under OWN anyway — the only path that ever worked was `can_modify_all=True`, which `all_perm` denial blocked outright. **Fix.** Fold `LIBRARY_UPDATE_ALL` and `LIBRARY_DELETE_ALL` into `_APIKEY_SCOPE_BY_PERMISSION` mapping to `can_manage_library` (matching the `can_queue` precedent — both `QUEUE_UPDATE_OWN` and `QUEUE_UPDATE_ALL` map to `can_queue` for the same per-key-identity reason). Remove both from `_APIKEY_DENIED_PERMISSIONS`. `LIBRARY_PURGE` deliberately stays denied — it bypasses the soft-delete window and is genuinely destructive, the kind of cross-boundary op the denylist exists for. **Tests.** 5 new cases in `TestLibraryPermissions` pinning the route-level contract — `test_apikey_with_manage_library_can_delete_file`, `test_apikey_with_manage_library_can_rename_file`, `test_apikey_with_manage_library_can_move_file`, `test_apikey_without_manage_library_still_blocked` (regression guard that the fix widens the allowed-permission set, not the per-key scope check), and `test_apikey_with_manage_library_still_cannot_purge` (LIBRARY_PURGE stays admin-only). The matrix drift-detection in `test_auth_apikey_rbac.py` updated to include `LIBRARY_UPDATE_OWN`, `LIBRARY_UPDATE_ALL`, `LIBRARY_DELETE_ALL` under `can_manage_library` and removes `LIBRARY_DELETE_ALL` from `_ADMIN_CASES`. `pytest -n 30 backend/tests/unit backend/tests/integration` green (6494). Ruff clean. **Scope.** No DB migration. No schema change. No frontend change. The wiki entry for API key permissions at `/features/api-keys/#available-permissions` now matches actual behaviour. + - **HMS Action buttons now reach the printer (#1830, H2D/H2C wrong-plate verification)** — The HMS Actions feature shipped in #1743 looked correct at the publish layer but the firmware silently dropped the commands at the printer, so clicking "Stop printing", "Problem solved and resume", or "Ignore and resume" did nothing visible on the live H2D — the modal kept reappearing, the print stayed paused, and the route still returned `200 OK`. Three independent bugs combined into one user-facing failure. **(1) Wrong command shape for resume / stop.** `hms_resume()` and `hms_stop()` sent the documented-but-not-actually-used `{"err": , "param": "reserve", "job_id": , ...}` shape that BambuStudio never produces. Bambu firmware rejects this silently — verified by injecting candidate shapes on `device//request` against a live H2D paused on a wrong-plate HMS: the `err`-bearing shape held PAUSE → PAUSE for the full window, the plain `{"print":{"command":"stop","param":"","sequence_id":"0"}}` transitioned PAUSE → FAILED in 1.7s, the same plain `resume` transitioned PAUSE → RUNNING in <2s. Fix: both helpers send the plain shape now, no `err`, no `job_id`, no `param:"reserve"`. **(2) `IGNORE_RESUME` mapped to the wrong command for paused prints.** The original mapping dispatched `idle_ignore` for both `IGNORE_RESUME` and `NO_REMINDER_NEXT_TIME`. `idle_ignore` is BambuStudio's "dismiss this warning" command and only works for non-pause warnings — verified against the H2D, idle_ignore on a paused print is silently rejected regardless of `err`. `hms_ignore()` now branches on `self.state.gcode_state == "PAUSE"`: paused → dispatch plain `resume` (which is what the button actually means on a paused print), running/idle → keep `idle_ignore` with the `type=0/1` persistence flag. `DONT_REMIND_NEXT_TIME` on PAUSE degrades to resume too — the "don't remind" flag can't ride along on a resume but the user's clicked-action intent (continue printing) is honoured. **(3) 64-bit `hms[]`-array faults truncated to a non-matching `err` (#1830 §(1)).** The hms[] parser at line 2740 built the short code as `f"{(attr >> 16) & 0xFFFF:04X}_{code & 0xFFFF:04X}"`, discarding 32 of the 64 bits of the fault identifier. For codes whose full form is e.g. `0C00_0300_0002_000C`, the truncated `0C00000C` doesn't match what the firmware compares against in `idle_ignore`. New `HMSError.full_code` field carries the canonical hex identifier — 16 chars `f"{attr:08X}{code:08X}"` for hms[]-sourced faults, 8 chars `f"{print_error:08X}"` for print_error-sourced faults (which are already 32-bit). Catalog lookup tries the 16-char form first and falls back to the 8-char short code so existing entries keep matching. Frontend echoes `error.full_code` back as `HmsActionBody.print_error` instead of recomputing the short code; the schema's pattern relaxes to `^[0-9A-Fa-f]{8}([0-9A-Fa-f]{8})?$` to accept both lengths. **(4) Masking failure — publish-success returned as printer-ack (#1830 §(3)).** `execute_hms_action` returned True the moment the publish succeeded, so any of the three bugs above produced `200 OK` while the printer ignored the command and the modal kept popping. The `/hms/execute-action` route now snapshots `(gcode_state, print_error, hms_errors count)` before dispatch, awaits `HMS_ACTION_ACK_WAIT_SECONDS` (default 2.5s, module-level so tests override), and returns `502 "Printer did not acknowledge HMS action within 2.5s"` if none of those moved. Every accepted HMS action mutates at least one of the three, so this is a clean signal. **Empirical verification.** A test harness on `device/0948BB540200427/request` confirmed each shape against the live H2D: a print sent with deliberately-wrong build plate raises `print_error=0x05008051` ("Detected build plate is not the same as the Gcode file"), the printer enters `gcode_state=PAUSE`, and the new command shapes transition out correctly. The current Bambuddy code (before this fix) failed to act on every button. **Tests.** `test_hms_actions.py` shape assertions rewritten — `test_resume_is_plain_no_err_no_job_id`, `test_stop_is_plain_no_err_no_job_id`, `test_ignore_resume_dispatches_resume_when_print_paused`, `test_ignore_resume_uses_idle_ignore_when_not_paused`, `test_dont_remind_dispatches_resume_when_paused`, `test_dont_remind_uses_idle_ignore_type_one_when_not_paused`, `test_idle_ignore_accepts_16_char_full_code`. New `TestHMSFullCode` class in `test_bambu_mqtt.py` pins the parser contract — `test_hms_array_path_populates_16_char_full_code`, `test_print_error_path_populates_8_char_full_code`, `test_hms_array_catalog_lookup_tries_16_char_first`, `test_hms_array_catalog_falls_back_to_8_char`. New integration cases in `test_printers_api.py` — `test_execute_hms_action_no_printer_ack_returns_502`, `test_execute_hms_action_accepts_16_char_full_code`. The malformed-input test now covers 9- and 15-char rejections (the relaxed pattern accepts 8 OR 16, nothing in between). `pytest -n 30 backend/tests/unit/services/test_hms_actions.py backend/tests/unit/services/test_bambu_mqtt.py backend/tests/unit/services/test_printer_manager.py backend/tests/integration/test_printers_api.py` green (509 + 181). `ruff check` clean. Frontend `npm run build` clean. **Scope.** No DB migration. No new permission. No new i18n key — the frontend toast on action failure already uses the existing `hmsErrors.actionFailed` string, which now gets the more accurate "Printer did not acknowledge" message instead of "Failed to send action". The `HMSError.full_code` field defaults to `""` so old in-memory state surviving a backend upgrade (without an MQTT reconnect) degrades to the existing 8-char short code via the frontend's `||` fallback. ### Added diff --git a/backend/app/core/auth.py b/backend/app/core/auth.py index 527789dfb..b54e49e3b 100644 --- a/backend/app/core/auth.py +++ b/backend/app/core/auth.py @@ -112,13 +112,21 @@ _APIKEY_SCOPE_BY_PERMISSION: dict[Permission, str] = { Permission.PRINTERS_AMS_RFID: "can_control_printer", Permission.PRINTERS_CLEAR_PLATE: "can_control_printer", Permission.SMART_PLUGS_CONTROL: "can_control_printer", - # can_manage_library — file-manager scope (upload/rename/delete OWN library + # can_manage_library — file-manager scope (upload/rename/delete library # entries + MakerWorld import which downloads files into the library). - # Bulk/ALL-ownership library ops (UPDATE_ALL / DELETE_ALL / PURGE) stay - # admin-only because they cross the user boundary. + # OWN and ALL ownership variants map to the same scope so the + # `require_ownership_permission` checker (which gates on `all_perm`) + # passes the API key through. This matches `can_queue` and the + # archives/inventory scopes — API keys have no per-row ownership identity + # (line 1663), so splitting OWN/ALL across allowlist/denylist made the + # whole library curation surface unreachable for API keys (#1832). + # LIBRARY_PURGE stays admin-only as a genuinely destructive op that + # bypasses the soft-delete window. Permission.LIBRARY_UPLOAD: "can_manage_library", Permission.LIBRARY_UPDATE_OWN: "can_manage_library", + Permission.LIBRARY_UPDATE_ALL: "can_manage_library", Permission.LIBRARY_DELETE_OWN: "can_manage_library", + Permission.LIBRARY_DELETE_ALL: "can_manage_library", Permission.MAKERWORLD_IMPORT: "can_manage_library", # can_manage_inventory — inventory write scope. Covers the documented # spool/catalog/forecast write surface AND the SpoolBuddy kiosk endpoints @@ -183,8 +191,11 @@ _APIKEY_DENIED_PERMISSIONS: frozenset[Permission] = frozenset( Permission.ARCHIVES_DELETE_OWN, Permission.ARCHIVES_DELETE_ALL, Permission.ARCHIVES_PURGE, - Permission.LIBRARY_UPDATE_ALL, - Permission.LIBRARY_DELETE_ALL, + # LIBRARY_UPDATE_ALL / LIBRARY_DELETE_ALL moved to the allowlist + # under `can_manage_library` (#1832) — split between allow/deny made + # the whole library curation surface unreachable for API keys via + # `require_ownership_permission`. Purge stays denied as a genuinely + # destructive op. Permission.LIBRARY_PURGE, Permission.PROJECTS_CREATE, Permission.PROJECTS_UPDATE, diff --git a/backend/tests/integration/test_auth_apikey_rbac.py b/backend/tests/integration/test_auth_apikey_rbac.py index 4f84e5f95..1ef956b5a 100644 --- a/backend/tests/integration/test_auth_apikey_rbac.py +++ b/backend/tests/integration/test_auth_apikey_rbac.py @@ -300,9 +300,15 @@ class TestCheckApiKeyPermissionsMatrix: ("PRINTERS_CONTROL", "can_control_printer", "start/stop print"), ("PRINTERS_FILES", "can_control_printer", "send file to printer"), ("SMART_PLUGS_CONTROL", "can_control_printer", "smart plug on/off"), - # can_manage_library + # can_manage_library — OWN and ALL ownership variants both fold into + # the same scope (#1832): API keys have no per-row ownership identity, + # so splitting OWN/ALL across allowlist/denylist made the curation + # surface unreachable. PURGE stays admin-only. ("LIBRARY_UPLOAD", "can_manage_library", "upload library file"), + ("LIBRARY_UPDATE_OWN", "can_manage_library", "rename own library file"), + ("LIBRARY_UPDATE_ALL", "can_manage_library", "rename any library file"), ("LIBRARY_DELETE_OWN", "can_manage_library", "delete own library file"), + ("LIBRARY_DELETE_ALL", "can_manage_library", "delete any library file"), ("MAKERWORLD_IMPORT", "can_manage_library", "import from MakerWorld"), # can_manage_inventory ("INVENTORY_CREATE", "can_manage_inventory", "create spool record"), @@ -321,7 +327,8 @@ class TestCheckApiKeyPermissionsMatrix: "FIRMWARE_UPDATE", # Unmapped administrative (allowlist fail-closed catches these too) "PRINTERS_CREATE", - "LIBRARY_DELETE_ALL", + # LIBRARY_DELETE_ALL / LIBRARY_UPDATE_ALL moved to can_manage_library + # under #1832 — covered by the _SCOPE_CASES matrix above. "LIBRARY_PURGE", "DISCOVERY_SCAN", ] diff --git a/backend/tests/integration/test_library_api.py b/backend/tests/integration/test_library_api.py index db6cb4cf5..dac8a0f8e 100644 --- a/backend/tests/integration/test_library_api.py +++ b/backend/tests/integration/test_library_api.py @@ -1349,6 +1349,140 @@ class TestLibraryPermissions: # Viewers don't have delete_own or delete_all permissions assert response.status_code == 403 + # ---------- #1832: API-key curation under can_manage_library ---------- + # + # require_ownership_permission gates API keys on `all_perm`, but the + # library deliberately split UPDATE_OWN/DELETE_OWN (allowed under + # can_manage_library) from UPDATE_ALL/DELETE_ALL (previously denied). + # That made the entire curation surface (DELETE, PUT rename, POST move) + # unreachable for API keys, including for files the key's owner uploaded. + # The fix folds UPDATE_ALL/DELETE_ALL into can_manage_library so the + # checker passes; LIBRARY_PURGE stays admin-only. + + @pytest.fixture + async def manage_library_key(self, db_session, auth_setup): + """Mint an API key owned by the admin user with can_manage_library.""" + from backend.app.core.auth import generate_api_key + from backend.app.models.api_key import APIKey + + admin = auth_setup["admin_user"] + full_key, key_hash, key_prefix = generate_api_key() + row = APIKey( + name="lib-curation", + key_hash=key_hash, + key_prefix=key_prefix, + user_id=admin.id, + can_manage_library=True, + ) + db_session.add(row) + await db_session.commit() + return full_key + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_apikey_with_manage_library_can_delete_file( + self, async_client: AsyncClient, db_session, auth_setup, test_file, manage_library_key + ): + """Pre-#1832 this 403'd with "administrative operations" because + LIBRARY_DELETE_ALL wasn't in _APIKEY_SCOPE_BY_PERMISSION.""" + from pathlib import Path + + from backend.app.core.config import settings as app_settings + + # Materialise the file on disk so the delete handler doesn't 500 on + # the path it tries to unlink. + 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") + + response = await async_client.delete( + f"/api/v1/library/files/{test_file.id}", + headers={"X-API-Key": manage_library_key}, + ) + assert response.status_code == 200, response.text + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_apikey_with_manage_library_can_rename_file( + self, async_client: AsyncClient, db_session, auth_setup, test_file, manage_library_key + ): + """PUT /library/files/{id} is gated on LIBRARY_UPDATE_ALL/OWN. Same + #1832 path as delete.""" + response = await async_client.put( + f"/api/v1/library/files/{test_file.id}", + headers={"X-API-Key": manage_library_key}, + json={"filename": "renamed.txt"}, + ) + assert response.status_code == 200, response.text + assert response.json()["filename"] == "renamed.txt" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_apikey_with_manage_library_can_move_file( + self, async_client: AsyncClient, db_session, auth_setup, test_file, manage_library_key + ): + """POST /library/files/move (bulk) is gated on LIBRARY_UPDATE_ALL/OWN + — same checker, same #1832 path.""" + # Create a target folder the move can land in. + from backend.app.models.library import LibraryFolder + + folder = LibraryFolder(name="target") + db_session.add(folder) + await db_session.commit() + await db_session.refresh(folder) + + response = await async_client.post( + "/api/v1/library/files/move", + headers={"X-API-Key": manage_library_key}, + json={"file_ids": [test_file.id], "folder_id": folder.id}, + ) + assert response.status_code == 200, response.text + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_apikey_without_manage_library_still_blocked( + self, async_client: AsyncClient, db_session, auth_setup, test_file + ): + """Regression guard: a key WITHOUT can_manage_library must still get + 403 — the fix widens the allowed-permission set, it doesn't bypass + the per-key scope check.""" + from backend.app.core.auth import generate_api_key + from backend.app.models.api_key import APIKey + + admin = auth_setup["admin_user"] + full_key, key_hash, key_prefix = generate_api_key() + row = APIKey( + name="read-only", + key_hash=key_hash, + key_prefix=key_prefix, + user_id=admin.id, + can_read_status=True, + can_manage_library=False, + ) + db_session.add(row) + await db_session.commit() + + response = await async_client.delete( + f"/api/v1/library/files/{test_file.id}", + headers={"X-API-Key": full_key}, + ) + assert response.status_code == 403 + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_apikey_with_manage_library_still_cannot_purge( + self, async_client: AsyncClient, db_session, auth_setup, manage_library_key + ): + """LIBRARY_PURGE deliberately stays in _APIKEY_DENIED_PERMISSIONS as + a genuinely destructive op that bypasses the soft-delete window. + can_manage_library does NOT grant it.""" + response = await async_client.post( + "/api/v1/library/purge", + headers={"X-API-Key": manage_library_key}, + json={"days_in_trash": 30}, + ) + assert response.status_code == 403 + class TestPrintFileUploadValidation: """#1401: pre-flight rejection of unprintable uploads at the library +