Browse Source

fix(spoolbuddy): respect Spoolman mode end-to-end + multiple cache/UX/permission fixes

  Seven intertwined SpoolBuddy + Spoolman bugs from feature/spoolman-inventory-ui
  testing, fixed as one batch since they all live on the same path:

  1. /spoolbuddy/nfc/tag-scanned always tried local DB first and only
     consulted Spoolman as a fallback on local-DB miss. A stale local
     row silently won over the authoritative Spoolman record. Now gates
     on _get_spoolman_client_or_none() so the route uses Spoolman
     exclusively when enabled, local exclusively otherwise.

  2. Dashboard "Assign to AMS" button was a no-op when the matched
     spool wasn't yet in the cached spools query (newly created in
     Spoolman, or unarchived after page load). The card rendered via
     `displayedSpool ?? sbState.matchedSpool` fallback but the modal's
     stricter guard silently failed to mount. New effectiveModalSpool
     synthesises an InventorySpool-shaped object from the WebSocket-
     delivered MatchedSpool (9-field subset, sufficient for the modal
     since it only needs `id` to route the assign API).

  3. AMS-page slot picker explicitly returned null for the
     assign/unassign branch when a slot had a SpoolmanSlotAssignment
     but no tag-linked spool — only Configure stayed visible. Now
     resolves the assignment via spoolmanSlotAssignmentsAll +
     spoolmanInventorySpoolsCache, renders a "Assigned spool" info
     card, and exposes an Unassign button wired to a new
     unassignSpoolmanSlotMutation (DELETE
     /spoolman/inventory/slot-assignments/<id>).

  4. LinkSpoolModal showed "Unknown color" for every Spoolman spool
     because Spoolman doesn't standardise color_name — most installs
     only populate color_hex and filament.name (which often carries
     the colour, e.g. "PLA Basic Red"). _map_spoolman_spool now falls
     back to the filament's subtype (filament name minus material
     prefix) when color_name is empty, so spools are visually
     distinguishable. The NFC write-tag warning specifically checks
     the raw filament.color_name (not the mapped value) so the
     "tag encodes empty color name" warning still fires on installs
     that genuinely lack the field.

  5. Writing a tag for spool B didn't clear the same tag from spool A,
     so a single NFC UID could map to two spools at once and
     find_spool_by_tag returned whichever came first in the cached
     list. nfc_write_result now searches Spoolman for any other spool
     currently bound to the target UID and clears its extra.tag
     (best-effort: cleanup failure logs a warning but doesn't block
     the write, since the chip is already written).

  6. The kiosk display held stale spoolmanSlotAssignments cache
     permanently because a long-running browser window has no
     focus/remount triggers to fire a refetch. Adds
     refetchInterval: 3_000 so the kiosk picks up changes from another
     client (Bambuddy main UI, direct Spoolman edit) within seconds.

  7. Kiosk QuickMenu System buttons (Restart Daemon / Restart Browser /
     Reboot / Shutdown) all 403'd silently. /system/command was gated
     on Permission.SETTINGS_UPDATE (T-Gap 2 from a prior security
     audit) but every other kiosk-scoped device route uses
     INVENTORY_UPDATE; the kiosk operator's session has the latter,
     not the former. Lowered to INVENTORY_UPDATE so operators can
     recover the kiosk from the kiosk. Risk is bounded — only the 4
     named commands are accepted (no RCE), reboot/shutdown require
     physical-access recovery anyway, the same operator already
     controls printers + weighs spools. /update keeps SETTINGS_UPDATE
     because it can replace the daemon binary.
maziggy 2 weeks ago
parent
commit
ef7fd4fa5c

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 8 - 1
backend/app/api/routes/spoolbuddy.py

@@ -1123,7 +1123,14 @@ async def queue_system_command(
     device_id: str,
     req: SystemCommandRequest,
     db: AsyncSession = Depends(get_db),
-    _: User | None = RequirePermissionIfAuthEnabled(Permission.SETTINGS_UPDATE),
+    # Aligns with the rest of the kiosk-scoped device routes (calibration,
+    # display, cancel-write, command-result — all INVENTORY_UPDATE). The
+    # previous SETTINGS_UPDATE gate locked operators out of the QuickMenu's
+    # Restart-Daemon / Restart-Browser / Reboot / Shutdown buttons even
+    # though they had access to every other operation on the same device.
+    # Reboot and shutdown remain recoverable via physical access — the
+    # operator already has the kiosk in front of them.
+    _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_UPDATE),
 ):
     """Queue a system command (reboot, shutdown, restart_daemon, restart_browser) for the SpoolBuddy device."""
     if req.command not in VALID_SYSTEM_COMMANDS:

+ 19 - 3
backend/tests/integration/test_settings_api_key_scrubbing.py

@@ -107,7 +107,15 @@ class TestSettingsScrubForApiKey:
 
 
 class TestRceEndpointPermissions:
-    """T-Gap 2: System command endpoints require SETTINGS_UPDATE permission."""
+    """T-Gap 2 (revised): system_command was originally gated on SETTINGS_UPDATE
+    but that locked out kiosk operators (who hold INVENTORY_UPDATE-only keys)
+    from the QuickMenu's Restart-Daemon / Restart-Browser / Reboot / Shutdown
+    buttons — the only way to recover the kiosk from the kiosk itself. Risk
+    is bounded: only the 4 named commands are accepted (no RCE), reboot and
+    shutdown require physical-access recovery, and the same operator already
+    controls printers + weighs spools on the same device. The /update route
+    (full firmware upgrade) keeps SETTINGS_UPDATE because it can replace the
+    daemon binary, which is a different threat surface."""
 
     @pytest.fixture
     async def auth_enabled(self, db_session):
@@ -151,7 +159,7 @@ class TestRceEndpointPermissions:
 
     @pytest.mark.asyncio
     @pytest.mark.integration
-    async def test_system_command_requires_settings_update(
+    async def test_system_command_accepts_inventory_update(
         self,
         async_client: AsyncClient,
         db_session,
@@ -159,12 +167,20 @@ class TestRceEndpointPermissions:
         inventory_only_api_key,
         spoolbuddy_device,
     ):
+        """T-Gap 2 (revised): system_command was lowered from SETTINGS_UPDATE
+        to INVENTORY_UPDATE so kiosk operators can use the QuickMenu buttons.
+        An inventory-only key must NOT 403 — it should reach the route's
+        device-state check (and 409 for offline device, since the test
+        fixture doesn't set last_seen).
+        """
         resp = await async_client.post(
             f"/api/v1/spoolbuddy/devices/{spoolbuddy_device.device_id}/system/command",
             json={"command": "reboot"},
             headers={"X-API-Key": inventory_only_api_key},
         )
-        assert resp.status_code == 403
+        # Permission accepted — fails on device-state, not on auth.
+        assert resp.status_code == 409
+        assert "offline" in resp.json()["detail"].lower()
 
     @pytest.mark.asyncio
     @pytest.mark.integration

Some files were not shown because too many files changed in this diff