diff --git a/CHANGELOG.md b/CHANGELOG.md index e2358a15b..aef09d606 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.4.7] - 2026-06-14 +### 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 - **Sponsor-prompt thresholds lowered to fire for typical new installs** — The in-app sponsor toast in `useSponsorPrompt` was calibrated for power users: the lowest print milestone was `100`, the lowest archive milestone was `50`, the lowest filament-cost milestone was `100`. A check of recent Matomo data showed the toast firing very rarely (`?from=app-toast-prints-100` = 4 visits, `?from=app-toast-archives-50` = 3 visits in a 7-day window) — most installs simply never reach those bars, especially with the install base ~doubling since March. Calibration widened: `PRINT_MILESTONES` now `(10, 25, 100, 500, 1000, 2500, 5000)`, `ARCHIVE_MILESTONES` now `(5, 10, 50, 250, 1000)`, `COST_MILESTONES` now `(25, 50, 100, 500, 1000)`. The existing priority order (anniversary → prints → archives → cost → version-update) and 14-day cross-family cooldown are unchanged, so a user still sees at most one toast per fortnight. The "fire highest unseen milestone" logic in `_check_prints` / `_check_archives` / `_check_cost` is unchanged — a user already at 200 prints still gets `prints-100` first (they crossed it earlier in the timeline). The existing toast copy uses `{count}` / `{total}` interpolation in all 11 locales — no new i18n keys needed; "You've completed 10 prints with Bambuddy" reads as fluently as the 100 variant. **Tests.** `test_failed_prints_dont_count` and `test_fires_when_cost_sum_crosses_100` rebalanced (5 completed prints instead of 50; 5 prints × 21 cost-each instead of 30 × 3.5) so they still test "below the lowest threshold" semantics with the new lower bars. New `test_fires_at_lowest_threshold` pins `prints-10` as the new minimum trigger. `pytest -n 30 backend/tests/unit/test_sponsor_prompt_service.py backend/tests/integration/test_sponsor_prompt_api.py` green (25/25). `ruff check` clean. **Scope.** No DB migration. No new permission. No frontend change. The change is opt-in by virtue of the existing toast cooldown — installs that already saw a recent toast see no behaviour change; installs that never crossed the old 100-print bar become eligible the first time they pass 10 prints (subject to the 14-day cooldown after any other family fires first). diff --git a/backend/app/api/routes/printers.py b/backend/app/api/routes/printers.py index 5a7053f4d..646bd94d6 100644 --- a/backend/app/api/routes/printers.py +++ b/backend/app/api/routes/printers.py @@ -63,6 +63,11 @@ from backend.app.utils.http import build_content_disposition logger = logging.getLogger(__name__) router = APIRouter(prefix="/printers", tags=["printers"]) +# Seconds the /hms/execute-action route waits for a printer status push +# confirming the command landed before reporting 502 to the UI. Module-level +# so tests can monkeypatch a near-zero value instead of mocking asyncio.sleep. +HMS_ACTION_ACK_WAIT_SECONDS = 2.5 + async def _caller_can_view_printer_secrets(user: User | None, db: AsyncSession) -> bool: """Whether the caller is trusted enough to see ``access_code`` on a printer @@ -458,7 +463,13 @@ async def get_printer_status( # Convert HMS errors to response format hms_errors = [ HMSErrorResponse( - code=e.code, attr=e.attr, module=e.module, severity=e.severity, actions=e.actions, job_id=e.job_id + code=e.code, + attr=e.attr, + module=e.module, + severity=e.severity, + actions=e.actions, + job_id=e.job_id, + full_code=e.full_code, ) for e in (state.hms_errors or []) ] @@ -3809,8 +3820,37 @@ async def execute_hms_action( if not client: raise HTTPException(400, "Printer not connected") + # Snapshot pre-state so we can verify the printer actually acted on the + # command. publish() success is NOT the same as printer-ack: Bambu's + # firmware silently rejects malformed HMS commands at QoS 1 (the broker + # ACKs the publish, but the printer drops it). Verified end-to-end against + # a live H2D — see #1830 §(3). We sample (gcode_state, hms_errors length) + # because every accepted HMS action mutates at least one of them. + # + # PrinterState.state carries the MQTT `gcode_state` value verbatim (see + # bambu_mqtt.py line 2144); the raw `print_error` int isn't preserved on + # state, only the derived HMSError entries are. + pre_gcode = client.state.state + pre_hms_count = len(client.state.hms_errors) + success = client.execute_hms_action(body.print_error, body.action, body.job_id) if not success: raise HTTPException(400, "Failed to execute HMS action") + # Give the printer time to push a state update. The dispatch helper already + # publishes a pushall after every command, so a fresh status should arrive + # within ~1s; the default 2.5s covers slower firmware variants without + # making the UI feel hung. Plain sleep is fine — paho's MQTT callback + # runs in its own thread and updates state regardless of whether this + # coroutine is awaiting. + await asyncio.sleep(HMS_ACTION_ACK_WAIT_SECONDS) + + acked = client.state.state != pre_gcode or len(client.state.hms_errors) != pre_hms_count + if not acked: + # Publish succeeded but the printer's state didn't move. Almost always + # firmware-side silent rejection (err mismatch, command/state mismatch). + # 502 makes it visible at the UI instead of the 200-but-broken loop + # #1830 reported. + raise HTTPException(502, "Printer did not acknowledge HMS action within 2.5s") + return {"success": True, "message": "HMS action executed"} 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/app/schemas/printer.py b/backend/app/schemas/printer.py index 3cd2d0b25..76afd17f2 100644 --- a/backend/app/schemas/printer.py +++ b/backend/app/schemas/printer.py @@ -155,6 +155,13 @@ class HMSErrorResponse(BaseModel): severity: int # 1=fatal, 2=serious, 3=common, 4=info actions: list[str] = [] # List of user-facing action keys (e.g. "CHECK_FILAMENT") job_id: str | None = None # Optional job ID for actions that require it (e.g. "CHECK_ASSISTANT") + # Canonical hex identifier the firmware uses to match HMS-related commands. + # 16 chars for `hms[]`-array faults (full 64-bit attr+code), 8 chars for + # `print_error` faults. The frontend echoes this back as + # HmsActionBody.print_error so we send the firmware-recognised key, not the + # truncated short_code that historically caused silent command rejection + # (#1830, H2D wrong-plate verification). + full_code: str = "" class AMSTray(BaseModel): @@ -219,9 +226,11 @@ class AmsLabelBody(BaseModel): class HmsActionBody(BaseModel): - # 8-char hex short code without separator (e.g. "05000070") — frontend strips - # the underscore from the displayed `MMMM_EEEE` before sending. - print_error: str = Field(..., min_length=8, max_length=8, pattern=r"^[0-9A-Fa-f]{8}$") + # Canonical hex identifier (HMSErrorResponse.full_code): 8 chars for + # `print_error`-sourced faults, 16 chars for `hms[]`-array faults whose + # full 64-bit code is the firmware's matching key. Length-bounded to + # those two valid shapes to keep stray input from reaching the dispatcher. + print_error: str = Field(..., min_length=8, max_length=16, pattern=r"^[0-9A-Fa-f]{8}([0-9A-Fa-f]{8})?$") # One of the HMSAction enum values. Length-capped to keep stray input from # reaching the dispatcher's `match` statement. action: str = Field(..., min_length=1, max_length=64) diff --git a/backend/app/services/bambu_mqtt.py b/backend/app/services/bambu_mqtt.py index 61882a5ee..214f14860 100644 --- a/backend/app/services/bambu_mqtt.py +++ b/backend/app/services/bambu_mqtt.py @@ -180,6 +180,13 @@ class HMSError: # The `subtask_id` snapshotted from PrinterState when this error surfaced; Bambu's # HMS-aware commands echo it back as `job_id`. None for idle errors with no job. job_id: str | None = None + # Canonical hex identifier for the firmware's `err` matching: 16 chars for the + # 64-bit `hms[]` array path (`f"{attr:08X}{code:08X}"`), 8 chars for the + # 32-bit `print_error` path. The frontend echoes this back to + # execute_hms_action; the truncated 8-char short code that `_parse_status` + # used to send caused the firmware to silently reject HMS commands on H2C + # (#1830) and on `hms[]`-sourced faults generally. + full_code: str = "" # HMS short codes the firmware emits during normal user-cancel sequences. @@ -2740,7 +2747,15 @@ class BambuMQTTClient: short_code = f"{(attr >> 16) & 0xFFFF:04X}_{code & 0xFFFF:04X}" if short_code in _HMS_USER_ACTION_CODES: continue - actions = get_actions_for_error_code(self.serial_number[:3], short_code.replace("_", "")) + # Catalog has both 8-char keys (base class) and 16-char keys + # (specific variants). The full 16-char identifier preserves + # the 32 bits of `attr_low` + `code_high` that the short_code + # discards — that's the firmware's matching key, so try it + # first and fall back to the short form. + full_code = f"{attr:08X}{code:08X}" + actions = get_actions_for_error_code(self.serial_number[:3], full_code) + if not actions: + actions = get_actions_for_error_code(self.serial_number[:3], short_code.replace("_", "")) self.state.hms_errors.append( HMSError( code=f"0x{code:x}" if code else "0x0", @@ -2749,6 +2764,7 @@ class BambuMQTTClient: severity=severity if severity > 0 else 2, actions=actions, job_id=self.state.subtask_id, + full_code=full_code, ) ) @@ -2820,6 +2836,9 @@ class BambuMQTTClient: severity=3, # Warning level for print_error actions=actions, job_id=job_id, + # print_error is already 32-bit — `f"{print_error:08X}"` + # is the firmware's matching key with no truncation. + full_code=f"{print_error:08X}", ) ) @@ -5417,13 +5436,17 @@ class BambuMQTTClient: """Dispatch the user's choice from the HMS-error modal as a printer command. Args: - print_error: 8-char hex short code with no separator (e.g. "05000070"). - The frontend strips the underscore from the displayed `MMMM_EEEE` - before sending. + print_error: Canonical hex identifier for the fault — 8 chars for the + 32-bit `print_error` path, 16 chars for the 64-bit `hms[]` path + (HMSError.full_code). Carried through unchanged from the route. + Only the `idle_ignore` branch puts it on the wire; resume / stop + use BambuStudio's plain shape (verified against a live H2D, the + `err`-bearing shape is silently rejected by the firmware). action: One of HMSAction's string values. job_id: The `subtask_id` snapshotted onto the HMSError at parse-time. - Bambu's HMS-aware commands echo it back as `job_id`. May be None - for idle errors that never had a job. + Preserved for symmetry with the catalog but no longer sent — + BambuStudio's actual resume/stop commands are plain and the + firmware doesn't echo `job_id` back on the response either. Returns False when the MQTT client is offline or when `action` is unknown so the route surfaces it as a 4xx rather than a silent no-op. @@ -5442,34 +5465,52 @@ class BambuMQTTClient: ) def hms_resume(): + # BambuStudio's actual shape — plain resume, no err / no job_id. + # The `err`-bearing shape (`err`, `param: "reserve"`, `job_id`) is + # silently rejected by Bambu firmware on print_error- and hms[]-sourced + # faults alike; verified by injecting candidate shapes against a live + # H2D paused on a wrong-plate HMS. See #1830 §(2). publish( { "print": { "command": "resume", - "err": print_error, - "param": "reserve", - "job_id": job_id, + "param": "", "sequence_id": "0", } } ) def hms_stop(): + # Same as hms_resume — BambuStudio's actual shape is plain. The + # `err`-bearing variant is silently rejected; verified on the H2D. publish( { "print": { "command": "stop", - "err": print_error, - "param": "reserve", - "job_id": job_id, + "param": "", "sequence_id": "0", } } ) def hms_ignore(persistent: bool = False): - # `idle_ignore` is BambuStudio's "dismiss this warning" command. - # type=0 dismisses once, type=1 hides the same warning permanently. + # `idle_ignore` is BambuStudio's "dismiss this warning" command for + # non-pause warnings. type=0 dismisses once, type=1 hides the same + # warning permanently. + # + # For HMS-paused state, `idle_ignore` is silently rejected by the + # firmware regardless of `err` (verified on a live H2D — see + # #1830 §(2)). The user-facing intent of "Ignore and resume" on a + # paused print is to continue, so we dispatch a plain resume + # instead. The `persistent` flag is informational in that branch — + # firmware can't honour "don't remind" through a resume — but the + # button still does what the user expects. + # + # NB: PrinterState's `state` field carries the MQTT `gcode_state` + # value verbatim — line 2144 stores `data["gcode_state"]` onto it. + if self.state.state == "PAUSE": + hms_resume() + return publish( { "print": { diff --git a/backend/app/services/printer_manager.py b/backend/app/services/printer_manager.py index de544bd1d..3d6240dde 100644 --- a/backend/app/services/printer_manager.py +++ b/backend/app/services/printer_manager.py @@ -1144,6 +1144,7 @@ def printer_state_to_dict( "severity": e.severity, "actions": e.actions, "job_id": e.job_id, + "full_code": e.full_code, } for e in (state.hms_errors or []) ], 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 + diff --git a/backend/tests/integration/test_printers_api.py b/backend/tests/integration/test_printers_api.py index a6d9bf317..7e0dc4b49 100644 --- a/backend/tests/integration/test_printers_api.py +++ b/backend/tests/integration/test_printers_api.py @@ -1770,13 +1770,32 @@ class TestExecuteHMSActionAPI: @pytest.mark.asyncio @pytest.mark.integration async def test_execute_hms_action_success(self, async_client: AsyncClient, printer_factory): - """200 happy path — dispatcher returns True, body forwarded verbatim.""" + """200 happy path — dispatcher returns True AND printer state moves + within the ack-wait window. The state delta is the firmware's only + proof that the command landed (publish success is necessary but not + sufficient; see #1830 §(3)).""" printer = await printer_factory(name="Test Printer") mock_client = MagicMock() - mock_client.execute_hms_action.return_value = True + # Pre-action state — paused with a fault. + mock_client.state.state = "PAUSE" + mock_client.state.print_error = 0x05008051 + mock_client.state.hms_errors = [object()] - with patch("backend.app.api.routes.printers.printer_manager") as mock_pm: + def _act(*_a, **_kw): + # Simulate the printer accepting the command and clearing the fault + # by the time the ack-wait expires. + mock_client.state.state = "FAILED" + mock_client.state.print_error = 0 + mock_client.state.hms_errors = [] + return True + + mock_client.execute_hms_action.side_effect = _act + + with ( + patch("backend.app.api.routes.printers.printer_manager") as mock_pm, + patch("backend.app.api.routes.printers.HMS_ACTION_ACK_WAIT_SECONDS", 0.01), + ): mock_pm.get_client.return_value = mock_client body = {"print_error": "07008029", "action": "FILAMENT_EXTRUDED", "job_id": "task-7"} @@ -1808,17 +1827,82 @@ class TestExecuteHMSActionAPI: assert response.status_code == 400 assert "failed" in response.json()["detail"].lower() + @pytest.mark.asyncio + @pytest.mark.integration + async def test_execute_hms_action_no_printer_ack_returns_502(self, async_client: AsyncClient, printer_factory): + """502 when publish succeeded but printer state didn't move within the + ack-wait window. This is the silent-rejection failure mode #1830 + identifies: the broker ACKs the publish at QoS 1 but the firmware + drops the command (err mismatch, wrong shape, state mismatch). + Surfacing this as 502 instead of 200 stops the UI from claiming + success while the modal sticks.""" + printer = await printer_factory(name="Test Printer") + + mock_client = MagicMock() + mock_client.state.state = "PAUSE" + mock_client.state.print_error = 0x05008051 + mock_client.state.hms_errors = [object()] + mock_client.execute_hms_action.return_value = True # publish "succeeded" + # Crucially: state does NOT change → ack-wait detects no movement. + + with ( + patch("backend.app.api.routes.printers.printer_manager") as mock_pm, + patch("backend.app.api.routes.printers.HMS_ACTION_ACK_WAIT_SECONDS", 0.01), + ): + mock_pm.get_client.return_value = mock_client + + response = await async_client.post( + f"/api/v1/printers/{printer.id}/hms/execute-action", json=self._VALID_BODY + ) + + assert response.status_code == 502 + assert "acknowledge" in response.json()["detail"].lower() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_execute_hms_action_accepts_16_char_full_code(self, async_client: AsyncClient, printer_factory): + """200 for a 16-char full_code (hms[]-array-sourced fault). The + schema's relaxed pattern allows both 8-char (print_error) and + 16-char (hms[]) shapes.""" + printer = await printer_factory(name="Test Printer") + + mock_client = MagicMock() + mock_client.state.state = "RUNNING" + mock_client.state.print_error = 0 + mock_client.state.hms_errors = [object()] + + def _act(*_a, **_kw): + mock_client.state.hms_errors = [] + return True + + mock_client.execute_hms_action.side_effect = _act + + with ( + patch("backend.app.api.routes.printers.printer_manager") as mock_pm, + patch("backend.app.api.routes.printers.HMS_ACTION_ACK_WAIT_SECONDS", 0.01), + ): + mock_pm.get_client.return_value = mock_client + + body = {"print_error": "0C00030000020010", "action": "IGNORE_RESUME"} + response = await async_client.post(f"/api/v1/printers/{printer.id}/hms/execute-action", json=body) + + assert response.status_code == 200 + mock_client.execute_hms_action.assert_called_once_with("0C00030000020010", "IGNORE_RESUME", None) + @pytest.mark.asyncio @pytest.mark.integration async def test_execute_hms_action_rejects_malformed_print_error(self, async_client: AsyncClient, printer_factory): - """422 when print_error fails the ^[0-9A-Fa-f]{8}$ pattern — stray + """422 when print_error fails the relaxed pattern (8 OR 16 hex chars). + Lengths in between (9-15) and outside (7, 17+) are invalid; stray input can't reach the dispatcher's match statement.""" printer = await printer_factory(name="Test Printer") bad_bodies = [ {"print_error": "0300_8070", "action": "OK_BUTTON"}, # underscore - {"print_error": "0300807", "action": "OK_BUTTON"}, # 7 chars - {"print_error": "030080700", "action": "OK_BUTTON"}, # 9 chars + {"print_error": "0300807", "action": "OK_BUTTON"}, # 7 chars (too short) + {"print_error": "030080700", "action": "OK_BUTTON"}, # 9 chars (between) + {"print_error": "030080700300807", "action": "OK_BUTTON"}, # 15 chars (between) + {"print_error": "0300807003008070A", "action": "OK_BUTTON"}, # 17 chars (too long) {"print_error": "0300GGGG", "action": "OK_BUTTON"}, # non-hex ] for body in bad_bodies: diff --git a/backend/tests/unit/services/test_bambu_mqtt.py b/backend/tests/unit/services/test_bambu_mqtt.py index 935230513..e48d964b1 100644 --- a/backend/tests/unit/services/test_bambu_mqtt.py +++ b/backend/tests/unit/services/test_bambu_mqtt.py @@ -4928,6 +4928,83 @@ class TestHMSUserActionFiltering: assert mqtt_client.state.hms_errors[0].code == "0x8061" +class TestHMSFullCode: + """full_code is the firmware-matching key for HMS-related commands. + Truncating it to the 8-char short code is what caused #1830's silent + rejection on H2C, and the H2D wrong-plate path needs the print_error + 32-bit form. Both branches must populate full_code consistently.""" + + @pytest.fixture + def mqtt_client(self): + from backend.app.services.bambu_mqtt import BambuMQTTClient + + return BambuMQTTClient( + ip_address="192.168.1.100", + serial_number="TEST_FULLCODE", + access_code="12345678", + ) + + def test_hms_array_path_populates_16_char_full_code(self, mqtt_client): + """hms[] entries carry a 64-bit identifier (attr + code, 32 bits each). + The full 16-char hex is what BambuStudio uses to match err on + idle_ignore — the truncated short_code drops 32 bits and the firmware + silently rejects (#1830). Verifies the parser preserves the full + identifier on HMSError.full_code.""" + # 0x07FF0200 / 0x8011 → displayed as 07FF_0200_0000_8011 in the wiki + mqtt_client._update_state({"hms": [{"attr": 0x07FF0200, "code": 0x8011}]}) + assert len(mqtt_client.state.hms_errors) == 1 + assert mqtt_client.state.hms_errors[0].full_code == "07FF02000000" + "8011" + + def test_print_error_path_populates_8_char_full_code(self, mqtt_client): + """print_error is already 32 bits — no truncation. full_code is the + 8-char hex form, which is exactly what the firmware matches against.""" + mqtt_client._update_state({"print_error": 0x05008051}) + assert len(mqtt_client.state.hms_errors) == 1 + assert mqtt_client.state.hms_errors[0].full_code == "05008051" + + def test_hms_array_catalog_lookup_tries_16_char_first(self, mqtt_client, monkeypatch): + """When the catalog has both an 8-char and a 16-char entry for the + same fault family, the 16-char (specific variant) wins. The 8-char + is the fallback for codes that aren't in the long-form catalog.""" + from backend.app.services import bambu_mqtt as mod + + calls = [] + + def fake_lookup(device, code): + calls.append((device, code)) + if len(code) == 16: + return ["RESUME_PRINTING"] + return [] + + monkeypatch.setattr(mod, "get_actions_for_error_code", fake_lookup) + # SN prefix "TES" — irrelevant for the test, we mocked the lookup. + mqtt_client._update_state({"hms": [{"attr": 0x07FF0200, "code": 0x8011}]}) + # 16-char lookup attempted first, then 8-char only if 16-char missed. + assert calls[0][1] == "07FF020000008011" + assert mqtt_client.state.hms_errors[0].actions == ["RESUME_PRINTING"] + + def test_hms_array_catalog_falls_back_to_8_char(self, mqtt_client, monkeypatch): + """If the catalog has no 16-char entry, fall back to the 8-char short + code — that's where base-class HMS codes live.""" + from backend.app.services import bambu_mqtt as mod + + calls = [] + + def fake_lookup(device, code): + calls.append((device, code)) + if len(code) == 16: + return [] # no specific variant + return ["CHECK_ASSISTANT"] # base class hit + + monkeypatch.setattr(mod, "get_actions_for_error_code", fake_lookup) + mqtt_client._update_state({"hms": [{"attr": 0x07FF0200, "code": 0x8011}]}) + # Two lookups: 16-char miss, then 8-char hit. + assert len(calls) == 2 + assert calls[0][1] == "07FF020000008011" + assert calls[1][1] == "07FF8011" + assert mqtt_client.state.hms_errors[0].actions == ["CHECK_ASSISTANT"] + + class TestForceReconnectRouting: """#1136 — force_reconnect_stale_session routes between hard-reset (full paho-client teardown, wipes the QoS 1 queue) and socket-close (the legacy diff --git a/backend/tests/unit/services/test_hms_actions.py b/backend/tests/unit/services/test_hms_actions.py index b3722b12c..89bc24ece 100644 --- a/backend/tests/unit/services/test_hms_actions.py +++ b/backend/tests/unit/services/test_hms_actions.py @@ -88,7 +88,11 @@ class TestExecuteHmsActionDispatch: # tail — just confirm no command went out by inspecting the helper. assert self._published_commands(client) == [] - def test_resume_carries_err_param_and_job_id(self, client): + def test_resume_is_plain_no_err_no_job_id(self, client): + # Verified against a live H2D — the `err`-bearing shape is silently + # rejected by Bambu firmware. BambuStudio sends a plain resume; we + # match that. job_id is accepted on the call for symmetry with the + # catalog but deliberately dropped from the wire. See #1830 §(2). ok = client.execute_hms_action("03008070", HMSAction.RESUME_PRINTING, job_id="task-42") assert ok is True cmds = self._published_commands(client) @@ -96,27 +100,56 @@ class TestExecuteHmsActionDispatch: { "print": { "command": "resume", - "err": "03008070", - "param": "reserve", - "job_id": "task-42", + "param": "", "sequence_id": "0", } } ] + assert "err" not in cmds[0]["print"] + assert "job_id" not in cmds[0]["print"] def test_proceed_falls_through_to_resume(self, client): client.execute_hms_action("03008070", HMSAction.PROCEED, job_id="task-1") cmds = self._published_commands(client) assert cmds[0]["print"]["command"] == "resume" - assert cmds[0]["print"]["err"] == "03008070" + # Same plain shape as RESUME_PRINTING — no err. + assert "err" not in cmds[0]["print"] - def test_stop_carries_err_and_job_id(self, client): + def test_stop_is_plain_no_err_no_job_id(self, client): + # Same firmware silent-rejection class as resume — the `err` variant + # was confirmed broken on H2D-1 (PAUSE → PAUSE), the plain shape + # transitions to FAILED within ~2s. client.execute_hms_action("03008070", HMSAction.STOP_PRINTING, job_id="task-1") cmds = self._published_commands(client) - assert cmds[0]["print"]["command"] == "stop" - assert cmds[0]["print"]["job_id"] == "task-1" + assert cmds[0] == { + "print": { + "command": "stop", + "param": "", + "sequence_id": "0", + } + } + assert "err" not in cmds[0]["print"] + assert "job_id" not in cmds[0]["print"] - def test_ignore_resume_uses_idle_ignore_type_zero(self, client): + def test_ignore_resume_dispatches_resume_when_print_paused(self, client): + # Verified on H2D: idle_ignore is silently rejected while gcode_state + # is PAUSE. The user's intent on a paused HMS modal is to continue, + # so IGNORE_RESUME dispatches a plain resume instead. See #1830 §(2). + client.state.state = "PAUSE" + client.execute_hms_action("03008070", HMSAction.IGNORE_RESUME) + cmds = self._published_commands(client) + assert cmds[0] == { + "print": { + "command": "resume", + "param": "", + "sequence_id": "0", + } + } + + def test_ignore_resume_uses_idle_ignore_when_not_paused(self, client): + # For non-pause warnings (e.g. AMS-side prompts during printing), + # idle_ignore IS the correct command and the firmware honours it. + client.state.state = "RUNNING" client.execute_hms_action("03008070", HMSAction.IGNORE_RESUME) cmds = self._published_commands(client) assert cmds[0] == { @@ -128,14 +161,32 @@ class TestExecuteHmsActionDispatch: } } - def test_dont_remind_uses_idle_ignore_type_one(self, client): - # DONT_REMIND_NEXT_TIME and IGNORE_NO_REMINDER_NEXT_TIME are the - # persistent variants — Bambu hides the warning for future prints. + def test_dont_remind_dispatches_resume_when_paused(self, client): + # The persistent variant still degrades to resume on a paused print — + # the "don't remind" flag can't ride along on a resume, but the user + # clicked an action whose top-level intent is to continue, so we + # honour that. The behavioural contract is documented in hms_ignore. + client.state.state = "PAUSE" + client.execute_hms_action("03008070", HMSAction.DONT_REMIND_NEXT_TIME) + cmds = self._published_commands(client) + assert cmds[0]["print"]["command"] == "resume" + + def test_dont_remind_uses_idle_ignore_type_one_when_not_paused(self, client): + client.state.state = "RUNNING" client.execute_hms_action("03008070", HMSAction.DONT_REMIND_NEXT_TIME) cmds = self._published_commands(client) assert cmds[0]["print"]["command"] == "idle_ignore" assert cmds[0]["print"]["type"] == 1 + def test_idle_ignore_accepts_16_char_full_code(self, client): + # hms[]-array faults carry a 16-char full identifier. The firmware + # matches against the full 64-bit code; the truncated 8-char form + # (used pre-#1830) was silently rejected on H2C. + client.state.state = "RUNNING" + client.execute_hms_action("0C00030000020010", HMSAction.IGNORE_RESUME) + cmds = self._published_commands(client) + assert cmds[0]["print"]["err"] == "0C00030000020010" + def test_filament_extruded_sends_ams_done(self, client): client.execute_hms_action("07008029", HMSAction.FILAMENT_EXTRUDED) cmds = self._published_commands(client) diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 77a5433e8..021406dbd 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -335,6 +335,11 @@ export interface HMSError { severity: number; // 1=fatal, 2=serious, 3=common, 4=info actions?: string[]; // List of user-facing action keys (e.g. "CHECK_FILAMENT") job_id?: string; // Optional job ID for actions that require it (e.g. "CHECK_ASSISTANT") + // Canonical hex identifier the firmware matches against — 8 chars for + // print_error-sourced faults, 16 chars for hms[]-array-sourced faults. Send + // this back as HmsActionBody.print_error so we don't truncate the 64-bit + // identifier into the silent-rejection short code (#1830). + full_code?: string; } export interface HMSActionBody { diff --git a/frontend/src/components/HMSErrorModal.tsx b/frontend/src/components/HMSErrorModal.tsx index 847a65f8f..1230a927b 100644 --- a/frontend/src/components/HMSErrorModal.tsx +++ b/frontend/src/components/HMSErrorModal.tsx @@ -1023,9 +1023,14 @@ export function HMSErrorModal({ printerName, errors, onClose, printerId, hasPerm