From 9beb001a17cba62f9afd7d80803e95a630b79a50 Mon Sep 17 00:00:00 2001 From: maziggy Date: Thu, 6 Aug 2026 11:06:15 +0200 Subject: [PATCH] Record who queued a file from the Library and the webhook API PrintQueueItem.created_by_id is what the queue:read_own / queue:update_own / queue:delete_own permissions filter on, but only three of the paths that create queue items were setting it. The Library's bulk "Add to queue" required Permission.QUEUE_CREATE and then bound the dependency to `_`, discarding the user, so every item it created was ownerless -- and invisible to the person who added it if their permissions are scoped to their own work. That is the one path built for adding many files at once, which is where it was hardest to notice. The webhook queue endpoint has no request user, but APIKey.user_id records the key's owner, which is the acting identity everywhere else the key is used, so its items are credited to that owner. Keys minted before per-user ownership have no user_id and their items stay ownerless. The virtual-printer path is left as-is on purpose. VirtualPrinter carries no owner, and the obvious substitute is wrong rather than incomplete: one admin typically configures the VP while everyone sends prints through it, so crediting those to the admin would make the "added by" column lie and put other people's jobs in the admin's own queue. Existing NULL rows are not backfilled -- there is no record of who created them, and the ownerless case is already handled throughout. Tests pin both fixed paths and the two cases that must stay ownerless (auth disabled, legacy key). --- CHANGELOG.md | 1 + backend/app/api/routes/library.py | 6 +- backend/app/api/routes/webhook.py | 4 + .../test_queue_creation_attribution.py | 211 ++++++++++++++++++ 4 files changed, 221 insertions(+), 1 deletion(-) create mode 100644 backend/tests/integration/test_queue_creation_attribution.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 160fb0e49..bc140470b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ All notable changes to Bambuddy will be documented in this file. - **Error and warning toasts now stay up twice as long** — Every pop-up notification disappeared after three seconds regardless of what it said. That is about right for "Settings saved", which confirms something you just did and is skimmed rather than read, but errors and warnings are a different kind of message: they carry a reason, often one relayed from the printer or the backend, and they run to a couple of lines. Three seconds was not long enough to finish reading one, and a missed error message is gone for good — there is no notification history to go back to. Errors and warnings now hold for six seconds. Success and informational toasts keep the three-second default, so the common case of clicking something and seeing it confirmed is unchanged, and the close button and the manual dismiss work exactly as before on all of them. The background print-dispatch toast is unaffected: it stays up while it has work in progress and clears itself shortly after the last job settles. Covered by frontend tests. ### Fixed +- **Files queued from the Library are no longer missing from their owner's queue** — On an installation with authentication turned on, a user whose permissions are scoped to their own work saw an empty queue after adding files from the Library, and adding more only added more nothing. The jobs were really there and really printed; they simply belonged to no one. Every queue item records who created it, and the "own queue" permissions decide what to show by comparing that against the signed-in user — but the Library's bulk **Add to queue** never wrote it down, so its items matched no one and were visible only to users who can see the whole queue. This affected the one path built for adding many files at once, which is where it was hardest to notice something was wrong: the file list on screen looked no different afterwards. The same omission applied to the queue endpoint of the webhook API, whose items are now credited to the owner of the API key that added them. Items that genuinely have no one behind them are unchanged and still belong to no one — jobs sent through a virtual printer, anything added while authentication is off, and keys created before API keys had owners. Covered by backend tests. - **The drying popover no longer starts a cycle under a material you did not pick (#2774)** — An AMS-HT loaded with Support for PLA/PETG offered PLA in the drying dialog's filament list, and the cycle that started was labelled Support for PLA/PETG on the printer's own screen. Opening the dialog prefills it from the spool that is loaded, and Bambu reports that spool's material as `PLA-S` — a name Bambuddy's table of drying temperatures does not carry. The temperature and duration fell back to PLA's 45°C for twelve hours, which is what the dialog showed and what was sent, but the material did not fall back with them: it stayed `PLA-S`, and the dropdown, handed a value that is not one of its options, displays its first option without saying so. So the list read PLA while `PLA-S` was what left for the printer. Anyone who opened the dialog and pressed **Start** without touching the material was affected; picking any entry from the list, even the same one it was already showing, made the two agree again. The same gap covered every composite — a spool of PETG-CF, PLA-CF, ABS-GF or PAHT-CF prefilled the dialog at PLA's 45°C, far short of what those materials want, and sent its full name as the material. Bambuddy now resolves a spool's material to an entry the list actually has before either value is set, so what the dialog shows and what the printer is told can no longer disagree. Support materials and composites resolve to the material they are built on, so PLA-S dries as PLA and PETG-CF as PETG at 65°C, and nylon is recognised under the several spellings Bambu gives it. Anything genuinely unrecognised still falls back to PLA, deliberately the coolest setting in the table — under-drying an exotic filament costs a cycle, where defaulting to the hottest would deform a PLA spool. This does not address the other half of that report: a printer that keeps showing the material set from its own screen even when Bambuddy names a different one, which needs a capture of what Bambu Studio sends before anything can sensibly be changed. Covered by frontend tests. - **Configuring an AMS slot shows up on the printer card straight away, without a page reload** — Setting a slot to a different filament from the printer card left the card showing the old one. Nothing was lost: the command reached the printer, the printer applied it, and reloading the page or waiting out the thirty-second fallback poll showed the new filament. It simply never arrived on its own. Bambuddy compares each status push from a printer against the last one it broadcast and stays quiet when nothing has changed, which is what keeps a machine mid-print from flooding every open browser tab several times a second. The comparison looked at each AMS tray's slot number, material and load state — and **Configure Slot** writes none of those. It writes the filament id, the colour, the profile name and the calibration profile. So changing PLA to a different brand or colour of PLA produced a push that looked identical to its predecessor and was discarded, while changing PLA to PETG came through immediately because the material had moved. That is also why **Reset** always worked: it clears the material. The comparison now covers the filament identity as well, so every kind of slot change reaches the card. Those fields only move when someone configures a slot or swaps a spool, so this adds no traffic during a print — the amount of filament left, which does tick down continuously, is still deliberately excluded. Covered by backend tests. - **"Any X2D" works on a printer that feeds from external spools instead of an AMS (#2771, reporter @Nick-C130)** — A fleet of five X2Ds with no AMS units, each printing PETG from its external spool holder, accepted a job sent to a named printer and refused the same job sent to **Any X2D**: the file uploaded, the printer answered "Failed to get AMS mapping table", and after three attempts the queue item failed. The two paths differ in one thing. A job queued for a named printer carries a filament mapping the browser worked out at the time you queued it; a job queued for a model has no printer yet, so the scheduler has to work the mapping out at dispatch — and its copy of that logic could not see an external spool on a dual-nozzle printer. On an X2D or H2D each filament in the sliced file names the nozzle it feeds, and Bambuddy will only offer a spool to the nozzle it is physically plumbed to. Which nozzle an external spool feeds was being read off a table the printer builds from its AMS units, so a printer with no AMS published an empty table, every external spool came back belonging to no nozzle at all, and the per-nozzle check discarded the only filament on the machine. Nothing matched, and the print went out claiming to use an AMS while carrying no mapping — which is the message the firmware was objecting to. The left and right external feeds identify themselves well enough to be routed without that table, and the printer reports its two nozzles directly, so both are now used. This is the same fault that was corrected in the browser last May for exactly this hardware; the scheduler kept the old logic, which is why the browser-resolved mapping worked and the scheduler-resolved one did not. Single-nozzle printers are untouched — they have no nozzle to route to and never took this branch. Separately, a job whose filament genuinely cannot be matched on a printer with no AMS now fails immediately and says which filament is missing and which nozzle wants it, instead of uploading several megabytes, collecting the firmware's error and failing anyway two retries later; where there *is* an AMS the firmware error still stands, because there the job can be recovered by loading a spool and pressing **Resume**. Covered by backend tests. diff --git a/backend/app/api/routes/library.py b/backend/app/api/routes/library.py index 59737894e..c338d61c2 100644 --- a/backend/app/api/routes/library.py +++ b/backend/app/api/routes/library.py @@ -2670,7 +2670,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)), + current_user: User | None = Depends(require_permission_if_auth_enabled(Permission.QUEUE_CREATE)), ): """Add library files to the print queue. @@ -2736,6 +2736,10 @@ async def add_files_to_queue( or (folder_projects.get(lib_file.folder_id) if lib_file.folder_id is not None else None), position=max_position, status="pending", + # Without this the row is ownerless, and `queue:read_own` filters + # on `created_by_id` — so the user who queued the file could not + # see it in their own queue. + created_by_id=current_user.id if current_user else None, ) db.add(queue_item) diff --git a/backend/app/api/routes/webhook.py b/backend/app/api/routes/webhook.py index 8912b77f0..4804de011 100644 --- a/backend/app/api/routes/webhook.py +++ b/backend/app/api/routes/webhook.py @@ -115,6 +115,10 @@ async def webhook_add_to_queue( scheduled_time=scheduled_time, require_previous_success=data.require_previous_success, auto_off_after=data.auto_off_after, + # Attribute to the key's owner so the item shows up under `queue:read_own` + # for the person whose key it is. Legacy keys predating per-user ownership + # have no `user_id`, and those rows stay ownerless. + created_by_id=api_key.user_id, ) db.add(queue_item) await db.flush() diff --git a/backend/tests/integration/test_queue_creation_attribution.py b/backend/tests/integration/test_queue_creation_attribution.py new file mode 100644 index 000000000..8dab62d0e --- /dev/null +++ b/backend/tests/integration/test_queue_creation_attribution.py @@ -0,0 +1,211 @@ +"""Regression tests: queue items created outside POST /queue/ lost their owner. + +`PrintQueueItem.created_by_id` is what the `queue:read_own` / `queue:update_own` / +`queue:delete_own` permissions filter on (`api/routes/print_queue.py`). Two +creation paths never set it, so the rows they produced were ownerless: + + - `POST /library/files/add-to-queue` — the bulk "Add to queue" action on the + Library page. The route already required `Permission.QUEUE_CREATE` but bound + the dependency to `_` and threw the user away, so a non-admin who queued + files from the Library could not then see them in their own queue. + - `POST /webhook/queue/add` — API-key inbound. `APIKey.user_id` records the + key's owner, which is the acting identity for everything else the key does. + +Ownerless rows are still legitimate for callers with no user behind them (auth +disabled, virtual-printer FTP uploads, legacy keys minted before per-user +ownership), so the tests pin those cases too — the fix must not invent a +placeholder user id. Compare `test_queue_start_user_attribution.py`, which pins +the same NULL-is-meaningful contract on the `/start` path. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest +from httpx import AsyncClient +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from backend.app.core.auth import generate_api_key +from backend.app.core.config import settings as app_settings +from backend.app.models.api_key import APIKey +from backend.app.models.print_queue import PrintQueueItem +from backend.app.models.user import User + + +async def _read_item(test_engine, item_id: int) -> PrintQueueItem: + """Fresh-session DB read — the `db_session` fixture's connection can look + stale after a route call dispatches through its own `Depends(get_db)` + session. Same helper shape as test_queue_start_user_attribution.py.""" + maker = async_sessionmaker(test_engine, class_=AsyncSession, expire_on_commit=False) + async with maker() as fresh: + return (await fresh.execute(select(PrintQueueItem).where(PrintQueueItem.id == item_id))).scalar_one() + + +async def _enable_auth_with_admin(client: AsyncClient, username: str) -> tuple[str, dict]: + """Boot auth setup and return (bearer_token, user_dict).""" + await client.post( + "/api/v1/auth/setup", + json={ + "auth_enabled": True, + "admin_username": username, + "admin_password": "AdminPass1!", + }, + ) + login = await client.post( + "/api/v1/auth/login", + json={"username": username, "password": "AdminPass1!"}, + ) + body = login.json() + return body["access_token"], body["user"] + + +@pytest.fixture +async def sliced_library_file(db_session): + """A library file that passes both of add-to-queue's gates: the filename + must look sliced, and the bytes must actually exist under `base_dir` (the + route rejects rows whose file is missing from disk).""" + from backend.app.models.library import LibraryFile + + rel_path = "archive/library/files/attribution_probe.gcode.3mf" + abs_path = Path(app_settings.base_dir) / rel_path + abs_path.parent.mkdir(parents=True, exist_ok=True) + abs_path.write_bytes(b"probe") + + lib_file = LibraryFile( + filename="attribution_probe.gcode.3mf", + file_path=rel_path, + file_size=5, + file_type="3mf", + ) + db_session.add(lib_file) + await db_session.commit() + await db_session.refresh(lib_file) + + yield lib_file + + abs_path.unlink(missing_ok=True) + + +class TestLibraryAddToQueueAttribution: + @pytest.mark.asyncio + @pytest.mark.integration + async def test_credits_the_authenticated_user(self, async_client: AsyncClient, test_engine, sliced_library_file): + """The Library bulk-add is the path a user reaches for when queueing + many files at once — exactly the case where losing attribution hurts.""" + token, user = await _enable_auth_with_admin(async_client, "libqueueadmin") + + response = await async_client.post( + "/api/v1/library/files/add-to-queue", + json={"file_ids": [sliced_library_file.id]}, + headers={"Authorization": f"Bearer {token}"}, + ) + assert response.status_code == 200 + added = response.json()["added"] + assert len(added) == 1 + + item = await _read_item(test_engine, added[0]["queue_item_id"]) + assert item.created_by_id == user["id"] + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_auth_disabled_leaves_item_ownerless( + self, async_client: AsyncClient, test_engine, sliced_library_file + ): + """With auth off the permission dep yields None. The row must stay + NULL rather than gaining a synthetic owner.""" + response = await async_client.post( + "/api/v1/library/files/add-to-queue", + json={"file_ids": [sliced_library_file.id]}, + ) + assert response.status_code == 200 + added = response.json()["added"] + assert len(added) == 1 + + item = await _read_item(test_engine, added[0]["queue_item_id"]) + assert item.created_by_id is None + + +class TestWebhookQueueAddAttribution: + @pytest.fixture + async def printer_and_archive(self, db_session): + from backend.app.models.archive import PrintArchive + from backend.app.models.printer import Printer + + printer = Printer( + name="Webhook Target", + ip_address="192.168.2.202", + serial_number="00M00A9876543210", + access_code="12345678", + model="P1S", + ) + archive = PrintArchive( + filename="Plate_1.gcode.3mf", + print_name="Plate 1", + file_path="/tmp/webhook_attribution.3mf", # nosec B108 + file_size=1024, + content_hash="webhookattributionhash", + status="completed", + ) + db_session.add_all([printer, archive]) + await db_session.commit() + await db_session.refresh(printer) + await db_session.refresh(archive) + return printer, archive + + async def _mint_key(self, db_session, owner_id: int | None) -> str: + full_key, key_hash, key_prefix = generate_api_key() + db_session.add( + APIKey( + name="attribution probe", + key_hash=key_hash, + key_prefix=key_prefix, + user_id=owner_id, + can_queue=True, + ) + ) + await db_session.commit() + return full_key + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_credits_the_key_owner(self, async_client: AsyncClient, db_session, test_engine, printer_and_archive): + printer, archive = printer_and_archive + + owner = User(username="keyowner", password_hash="x", is_active=True) + db_session.add(owner) + await db_session.commit() + await db_session.refresh(owner) + + key = await self._mint_key(db_session, owner.id) + + response = await async_client.post( + "/api/v1/webhook/queue/add", + json={"printer_id": printer.id, "archive_id": archive.id}, + headers={"X-API-Key": key}, + ) + assert response.status_code == 200 + + item = await _read_item(test_engine, response.json()["id"]) + assert item.created_by_id == owner.id + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_legacy_ownerless_key_leaves_item_ownerless( + self, async_client: AsyncClient, db_session, test_engine, printer_and_archive + ): + """`APIKey.user_id` is nullable only for keys minted before per-user + ownership existed. Those must produce an ownerless row, not a crash.""" + printer, archive = printer_and_archive + key = await self._mint_key(db_session, None) + + response = await async_client.post( + "/api/v1/webhook/queue/add", + json={"printer_id": printer.id, "archive_id": archive.id}, + headers={"X-API-Key": key}, + ) + assert response.status_code == 200 + + item = await _read_item(test_engine, response.json()["id"]) + assert item.created_by_id is None