diff --git a/CHANGELOG.md b/CHANGELOG.md index fa1c32f6a..2ecd1baa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ All notable changes to Bambuddy will be documented in this file. - **Sort File Manager folder tree by recent activity (#1770, requested by @Kingbuzz0)** — Until now the folder tree was always sorted alphabetically by name, both backend (`order_by(LibraryFolder.name)`) and frontend. The reporter — a user with a lot of nested cad / slicer directories — wanted "find folders that just got a new 3MF" without scrolling the whole alphabet. **What changed.** The folder sidebar header gains a small dropdown (**By name** / **By recent activity**) plus an asc / desc arrow button, sitting alongside the existing Collapse + Wrap toggles. Choice persists per-browser via `localStorage` (`library-folder-sort-field`, `library-folder-sort-direction`) so the preference survives reloads. **Activity semantics.** `latest_activity_at` per folder = `MAX(folder.updated_at, MAX(immediate-child file.updated_at))`. The DB had the data — `LibraryFile.updated_at` is `onupdate=func.now()` and `LibraryFolder.updated_at` the same — but `LibraryFolder.updated_at` alone only bumps on rename / move, not on file-add inside the folder, which is exactly the wrong signal for "did I just drop a new model in here." The aggregate fixes that. Recursion across subfolders is intentionally **NOT** computed — a deeply nested new 3MF bubbles its immediate parent, not every ancestor up to the root. This keeps the route a single `GROUP BY` rather than a recursive CTE, matching the existing file_counts subquery shape sibling at `library.py:746`. A future Tier 3 follow-up could add the recursive-CTE variant if anyone reports deeply-nested updates not bubbling far enough. **Backend.** New `latest_activity_at: datetime | None` field on `FolderResponse` and `FolderTreeItem` schemas. The `/folders` tree route picks up a sibling `func.max(LibraryFile.updated_at)` group-by alongside the existing file-count subquery; resolves the field per row. The `/folders/by-project/{id}` and `/folders/by-archive/{id}` routes collapse their per-row file-count subquery to fetch `count + max` in one trip (one extra column, zero extra round-trips). All 5 single-folder constructors (POST `/folders`, GET `/folders/{id}`, PUT `/folders/{id}`, POST `/folders/external`, the create flows) populate the field with `max(folder.updated_at, latest_file)` or fall back to `folder.updated_at` when there are no files, so the API surface is consistent across every route that returns a folder. **External folders.** `LibraryFile` rows are created for scanned external files too (`library.py:526`), so the MAX aggregate works on them — but the timestamp reflects when Bambuddy last *scanned / re-indexed* the file, not the filesystem mtime. For a NAS that gets new files added outside Bambuddy, the activity-sort lags until the next scan. Documented in the file-manager wiki page rather than papered over with `os.stat()` on every list call, which would stall the route on slow mounts. **Frontend.** A new recursive `sortedFolders` `useMemo` applies the comparator uniformly to top-level + every nested `children` level so sort order is consistent at every depth. Comparator falls back to name when activity timestamps tie or are both null, so an empty folder never elbows a recently-used one to a random place — empties go to the end of the activity bucket regardless of direction. Both the desktop sidebar render and the mobile selector dropdown consume `sortedFolders` so the order is identical across breakpoints. The single-folder `findFolder()` traversal and `selectedFolder` memo still operate on the unsorted `folders` because they index by ID — sort-order-independent. **Recursion safety.** The sort creates fresh object refs at every level on every memo invocation; the `FolderTreeItem` keys stay ID-based (`${folder.id}-${collapseFoldersByDefault ? 'c' : 'e'}`) so React reconciliation by ID preserves folder expansion state across sort flips. **i18n.** 3 new keys in `fileManager.*` (`folderSort`, `folderSortByName`, `folderSortByActivity`) translated in all 11 locales (de / en / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW), no English fallback. Parity 5238 leaves per locale. **Tests.** 2 new backend integration cases in `test_library_api.py` (file-in-folder bubbles `latest_activity_at` to the file's timestamp, empty folder falls back to `folder.updated_at`). All 152 library + folder + trash + slice integration tests still pass; 51/51 FileManagerPage frontend tests still pass; 26/26 QueuePage tests still pass; `npm run build` clean; `ruff` clean; i18n parity green. ### Fixed +- **Enabling authentication silently disconnected Bambu Cloud (#2530, reporter @hburn7)** — Cloud credentials live in two different places depending on auth state: `get_stored_token()` reads the global `Settings` rows (`bambu_cloud_token` / `_email` / `_region`) when auth is off, and `User.cloud_token` when it's on. Completing `POST /auth/setup` flipped which store the `/cloud/*` routes consult, but nothing carried the token across — so an operator who linked their Bambu account *before* turning on auth found the freshly-created admin had `cloud_token = NULL`. `build_authenticated_cloud()` then returned `None` and every cloud route degraded: `get_filament_info` skipped its cloud phase entirely and answered `200` from the local-preset and built-in-name fallbacks, while `/cloud/devices` began returning `401`. Nothing surfaced the disconnect. The reporter observed the *symptom inverted* — cloud `400` warnings vanished after enabling auth — and reasonably read that as a fix; in fact the warnings stopped because Bambuddy had stopped calling the cloud at all. The tell is in their own timestamps: the pre-auth request spent ~960 ms on cloud round-trips, the post-auth one answered immediately. **Fix.** `setup_auth()` now migrates a globally-stored token onto the owning admin (and deletes the global rows, so a live credential isn't left at rest in a table nothing reads), and `disable_auth()` performs the mirror hand-off back to global storage. Both refuse to guess when ownership is ambiguous: setup migrates only when it creates the admin or exactly one admin already exists — with several admins it leaves the credential in place and logs a warning rather than handing one admin another's Bambu session; disable declines to overwrite a pre-existing global token. The `region` survives both hops rather than silently resetting to `global`. **Note for existing installs.** The migration runs at the auth on/off transition, so instances that already crossed it must re-link their Bambu account once from Settings → Bambu Cloud; the stranded `bambu_cloud_*` rows in `settings` can then be deleted. **Tests.** 7 integration cases pinning both directions, the two refuse-to-guess paths, the `auth_enabled=false` no-op, and region preservation. **Scope.** No DB migration, no schema change, no new permission, no i18n change. Separately tracked: the cloud `400 missing` warnings themselves are unrelated to auth and are logged at WARNING for an expected, already-handled fallback. - **Printer FTPS and MQTT connections inherited their TLS floor from the OpenSSL build instead of declaring one** — `ImplicitFTP_TLS` (`bambu_ftp.py`) and the MQTT client (`bambu_mqtt.py`) both built their context with `ssl.create_default_context()`, which leaves `minimum_version` at `MINIMUM_SUPPORTED`. What that resolves to is a property of the interpreter's OpenSSL build, not of Bambuddy: measured on identical `OpenSSL 3.5.6`, the `python:3.13-slim-trixie` Docker base reports `TLSVersion.TLSv1_2` while a bare-metal venv reports `MINIMUM_SUPPORTED` — so Docker users have always been floored at TLS 1.2, while bare-metal and appliance installs could in principle negotiate TLS 1.0 or 1.1 with a printer that offered them. **Fix.** Both contexts now set `minimum_version = ssl.TLSVersion.TLSv1_2` explicitly. On the two FTP profiles that also cap `maximum_version` (P2S, X2D — see #1401) this yields an exact TLS 1.2 pin rather than a ceiling over an inherited floor. **Verified against hardware, not just tests.** Probing an X1C and an H2D on both `:990` and `:8883`, each printer completes only on TLS 1.2 and rejects 1.0, 1.1 *and* 1.3 with a `handshake_failure` alert; a live FTPS login through the changed code path succeeds on both with `TLSv1.2` negotiated. Since the shipped Docker image already enforced this floor across the whole install base, no printer or firmware reachable today can be affected by making it explicit. **Also corrected** a stale comment in `ftp_profiles.py` claiming X1C / H2D installs "stay on the negotiated TLS 1.3" — both models refuse 1.3 outright, so `cap_tls_v1_2` is a no-op there; the P2S evidently does offer 1.3, which is why it alone surfaced the vsFTPd session-reuse bug. **Scope.** Two lines plus a comment. No behaviour change on Docker, no DB migration, no new permission, no i18n change; certificate verification is unchanged (printers use self-signed certs, so `check_hostname`/`CERT_NONE` remain by necessity). - **Backend failed to start on fastapi < 0.116: `AssertionError: Status code 204 must not have a response body`** — `uvicorn backend.app.main:app` aborted at import time while registering `DELETE /api/v1/library/tags/{tag_id}`. The route is declared `status_code=204` with a `-> None` return annotation, and `library_tags.py` uses `from __future__ import annotations` — so the annotation reaches FastAPI as the *string* `"None"`, which `get_typed_annotation()` resolves via `evaluate_forwardref()` to `NoneType`. `NoneType` is a class and therefore truthy, so `APIRoute.__init__` took the `if self.response_model:` branch and asserted that a 204 may carry no response body. fastapi **0.116** added an `if annotation is type(None): return None` guard that makes this benign, which is why CI and the Docker image (both resolve the top of the `>=0.109.0,<0.136.0` range) never saw it — only installs pinned to an older release inside that supported range, such as a venv created before the tag catalog landed in #1268, hit the crash. **Fix.** The route declares `response_model=None` explicitly, which short-circuits the annotation inference on every fastapi version in the supported range. The sibling 204 route (`DELETE /slicer/pipelines/{pipeline_id}`) is unaffected — its module has no `from __future__ import annotations` and no return annotation. **Scope.** Backend-only, one decorator. No behaviour change on fastapi >= 0.116, no DB migration, no new permission, no i18n change. Existing installs can equivalently unblock themselves with `pip install -U -r requirements.txt`. - **Dependency floors permitted resolutions the code can't run on: `sqlalchemy>=2.0.38`, exact ruff pin** — Two more instances of the same class of defect as the 204 crash above: `requirements.txt` declared floors low enough that a legitimate `pip install -r requirements.txt` could produce an environment Bambuddy fails to start or lint in. CI never caught either, because a fresh runner always resolves to the *top* of every range — only a longer-lived venv resolving lower hits them. **sqlalchemy.** `core/database._create_engine()` passes `pool_size` / `max_overflow` on the SQLite branch. SQLAlchemy **2.0.38** changed the aiosqlite dialect's default pool for file databases from `NullPool` (which rejects both kwargs) to `AsyncAdaptedQueuePool` (which accepts them); on 2.0.0-2.0.37 the module-level `engine = _create_engine()` raises `TypeError: Invalid argument(s) 'pool_size','max_overflow' sent to create_engine()` at import, taking down every SQLite install and the whole test suite (`conftest.py` imports the module). Postgres installs were never affected — `is_sqlite()` is False and the branch is dead. Floor raised to `sqlalchemy>=2.0.38`. **ruff.** The lint job ran a bare `pip install ruff` (always the newest release) while `requirements-dev.txt` said `ruff>=0.8.0`, so CI's linter and a contributor's were routinely *different programs enforcing different rule sets*. A venv holding ruff 0.8.4 reported 32 errors against a tree current ruff calls clean — 30 of them `UP038`, a rule ruff has since **removed** (PEP 604 syntax in `isinstance()` is slower than the tuple form it wanted you to replace). ruff is now pinned exactly (`ruff==0.15.20`) and the CI lint job installs that pin from `requirements-dev.txt`, so local and CI enforce the same rules and `format --check` can't disagree across machines. **Scope.** Packaging + CI only; no application code, no DB migration, no permission, no i18n change. Existing environments should re-run `pip install -U -r requirements.txt -r requirements-dev.txt`. diff --git a/backend/app/api/routes/auth.py b/backend/app/api/routes/auth.py index fb9b7987c..4a94b1bf0 100644 --- a/backend/app/api/routes/auth.py +++ b/backend/app/api/routes/auth.py @@ -295,6 +295,38 @@ async def setup_auth(request: SetupRequest, db: AsyncSession = Depends(get_db)): detail="Failed to create admin user", ) + if request.auth_enabled: + # Enabling auth flips cloud-credential storage from the global + # Settings rows to User.cloud_token. Carry any token linked while + # auth was off across to the owning admin, or /cloud/* silently + # degrades to local presets with no indication anything broke + # (#2530). Only migrate when there is exactly one obvious owner: + # handing another admin's session a Bambu credential is not a + # guess worth making. + from backend.app.api.routes.cloud import ( + get_stored_token, + migrate_global_cloud_token_to_user, + ) + + if admin_created: + cloud_owner = admin_user + elif len(existing_admin_users) == 1: + cloud_owner = existing_admin_users[0] + else: + cloud_owner = None + + if cloud_owner is not None: + if await migrate_global_cloud_token_to_user(db, cloud_owner): + logger.info("Migrated global Bambu Cloud credentials to admin '%s'", cloud_owner.username) + else: + global_token, _, _ = await get_stored_token(db, None) + if global_token: + logger.warning( + "A Bambu Cloud account is linked globally but %s admins exist; " + "leaving it unassigned. Re-link the account from Settings after login.", + len(existing_admin_users), + ) + # Set auth enabled and mark setup as completed await set_auth_enabled(db, request.auth_enabled) await set_setup_completed(db, True) @@ -349,6 +381,14 @@ async def disable_auth( ) try: + # Mirror of the migration in setup_auth: with auth off the cloud routes + # read the global Settings rows and never look at User.cloud_token, so + # hand this admin's credential over rather than stranding it (#2530). + from backend.app.api.routes.cloud import migrate_user_cloud_token_to_global + + if await migrate_user_cloud_token_to_global(db, user): + logger.info("Migrated Bambu Cloud credentials from admin '%s' to global storage", user.username) + await set_auth_enabled(db, False) await db.commit() logger.info("Authentication disabled by admin user: %s", user.username) diff --git a/backend/app/api/routes/cloud.py b/backend/app/api/routes/cloud.py index 50ba0009c..ae06abb3d 100644 --- a/backend/app/api/routes/cloud.py +++ b/backend/app/api/routes/cloud.py @@ -250,6 +250,69 @@ async def clear_token(db: AsyncSession, user: User | None = None) -> None: await db.commit() +async def migrate_global_cloud_token_to_user(db: AsyncSession, user: User) -> bool: + """Move a globally-stored cloud token onto ``user`` (auth being enabled). + + ``get_stored_token`` reads the global ``Settings`` rows when auth is off and + ``User.cloud_token`` when it's on. Enabling auth therefore switches which + column the cloud routes consult — without this migration the token linked + before setup is stranded in ``Settings``, ``build_authenticated_cloud`` + returns ``None``, and every ``/cloud/*`` route silently degrades (#2530). + + The global rows are deleted after the copy so the credential isn't left at + rest in a table nothing reads any more. Does **not** commit — the caller + owns the transaction. Returns True when a token was actually migrated. + """ + token, email, region = await get_stored_token(db, None) + if not token: + return False + + user.cloud_token = token + user.cloud_email = email + user.cloud_region = _normalise_region(region) + + result = await db.execute( + select(Settings).where(Settings.key.in_([CLOUD_TOKEN_KEY, CLOUD_EMAIL_KEY, CLOUD_REGION_KEY])) + ) + for setting in result.scalars().all(): + await db.delete(setting) + return True + + +async def migrate_user_cloud_token_to_global(db: AsyncSession, user: User) -> bool: + """Move ``user``'s cloud token into global storage (auth being disabled). + + The mirror of :func:`migrate_global_cloud_token_to_user`: once auth is off, + ``get_stored_token`` stops consulting ``User.cloud_token`` entirely, so the + admin who turns auth off would otherwise lose their own cloud link. + + Refuses to overwrite an existing global token — a stale row from a previous + no-auth stint is still someone's credential, and clobbering it silently is + worse than leaving this admin to re-link. Does **not** commit. Returns True + when a token was actually migrated. + """ + if not user.cloud_token: + return False + + existing, _, _ = await get_stored_token(db, None) + if existing: + return False + + for key, value in [ + (CLOUD_TOKEN_KEY, user.cloud_token), + (CLOUD_EMAIL_KEY, user.cloud_email), + (CLOUD_REGION_KEY, _normalise_region(user.cloud_region)), + ]: + if value is None: + continue + db.add(Settings(key=key, value=value)) + + user.cloud_token = None + user.cloud_email = None + user.cloud_region = None + return True + + def _assert_api_key_can_access_cloud(api_key: APIKey) -> None: """Reject API keys that aren't authorised to read cloud data. diff --git a/backend/tests/integration/test_cloud_token_auth_migration.py b/backend/tests/integration/test_cloud_token_auth_migration.py new file mode 100644 index 000000000..c4cc09462 --- /dev/null +++ b/backend/tests/integration/test_cloud_token_auth_migration.py @@ -0,0 +1,201 @@ +"""Cloud-credential migration across the auth on/off boundary (#2530). + +``get_stored_token`` reads global ``Settings`` rows when auth is disabled and +``User.cloud_token`` when it's enabled. Toggling auth therefore switches which +store the ``/cloud/*`` routes consult. Without an explicit hand-off the token +is stranded in the store nobody reads: ``build_authenticated_cloud`` returns +``None``, Phase 2 of ``get_filament_info`` is skipped entirely, and the caller +sees a ``200`` full of local-preset fallbacks with no sign the cloud was never +contacted. That silent degradation is what #2530 actually reported. + +These tests pin the hand-off in both directions, and — just as importantly — +pin the two cases where Bambuddy must refuse to guess who owns a credential. +""" + +import pytest +from httpx import AsyncClient +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.api.routes.cloud import ( + CLOUD_EMAIL_KEY, + CLOUD_REGION_KEY, + CLOUD_TOKEN_KEY, + get_stored_token, +) +from backend.app.core.auth import get_password_hash +from backend.app.models.settings import Settings +from backend.app.models.user import User + + +async def _seed_global_token(db: AsyncSession, token: str = "tok-global", region: str = "china") -> None: + db.add(Settings(key=CLOUD_TOKEN_KEY, value=token)) + db.add(Settings(key=CLOUD_EMAIL_KEY, value="owner@example.com")) + db.add(Settings(key=CLOUD_REGION_KEY, value=region)) + await db.commit() + + +async def _global_rows(db: AsyncSession) -> dict[str, str]: + rows = ( + ( + await db.execute( + select(Settings).where(Settings.key.in_([CLOUD_TOKEN_KEY, CLOUD_EMAIL_KEY, CLOUD_REGION_KEY])) + ) + ) + .scalars() + .all() + ) + return {r.key: r.value for r in rows} + + +async def _make_admin(db: AsyncSession, username: str) -> User: + user = User( + username=username, + password_hash=get_password_hash("AdminPass1!"), + role="admin", + is_active=True, + ) + db.add(user) + await db.commit() + await db.refresh(user) + return user + + +# --------------------------------------------------------------------------- +# auth OFF -> ON +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_setup_migrates_global_token_to_created_admin(async_client: AsyncClient, db_session: AsyncSession): + """The reporter's exact path: link cloud with auth off, then enable auth.""" + await _seed_global_token(db_session) + + resp = await async_client.post( + "/api/v1/auth/setup", + json={"auth_enabled": True, "admin_username": "admin", "admin_password": "AdminPass1!"}, + ) + assert resp.status_code == 200, resp.text + + admin = (await db_session.execute(select(User).where(User.role == "admin"))).scalar_one() + token, email, region = await get_stored_token(db_session, admin) + assert token == "tok-global" + assert email == "owner@example.com" + assert region == "china", "region must survive the hop, not silently reset to global" + + # Credential must not be left at rest in a table nothing reads any more. + assert await _global_rows(db_session) == {} + + +@pytest.mark.asyncio +async def test_setup_migrates_to_sole_pre_existing_admin(async_client: AsyncClient, db_session: AsyncSession): + """Re-enabling auth when exactly one admin already exists has one obvious owner.""" + admin = await _make_admin(db_session, "solo") + await _seed_global_token(db_session, token="tok-solo") + + resp = await async_client.post("/api/v1/auth/setup", json={"auth_enabled": True}) + assert resp.status_code == 200, resp.text + assert resp.json()["admin_created"] is False + + await db_session.refresh(admin) + token, _, _ = await get_stored_token(db_session, admin) + assert token == "tok-solo" + assert await _global_rows(db_session) == {} + + +@pytest.mark.asyncio +async def test_setup_refuses_to_guess_owner_when_multiple_admins(async_client: AsyncClient, db_session: AsyncSession): + """Two admins, one credential: handing it to either is a security decision we don't make.""" + a = await _make_admin(db_session, "admin_a") + b = await _make_admin(db_session, "admin_b") + await _seed_global_token(db_session, token="tok-ambiguous") + + resp = await async_client.post("/api/v1/auth/setup", json={"auth_enabled": True}) + assert resp.status_code == 200, resp.text + + await db_session.refresh(a) + await db_session.refresh(b) + assert a.cloud_token is None + assert b.cloud_token is None + # Left intact so the operator can re-link rather than lose it. + assert (await _global_rows(db_session))[CLOUD_TOKEN_KEY] == "tok-ambiguous" + + +@pytest.mark.asyncio +async def test_setup_with_auth_disabled_leaves_global_token_untouched( + async_client: AsyncClient, db_session: AsyncSession +): + """Completing setup while declining auth must not move anything.""" + await _seed_global_token(db_session, token="tok-stay") + + resp = await async_client.post("/api/v1/auth/setup", json={"auth_enabled": False}) + assert resp.status_code == 200, resp.text + + assert (await _global_rows(db_session))[CLOUD_TOKEN_KEY] == "tok-stay" + + +# --------------------------------------------------------------------------- +# auth ON -> OFF +# --------------------------------------------------------------------------- + + +async def _admin_bearer(async_client: AsyncClient, username: str = "admin") -> str: + await async_client.post( + "/api/v1/auth/setup", + json={"auth_enabled": True, "admin_username": username, "admin_password": "AdminPass1!"}, + ) + login = await async_client.post( + "/api/v1/auth/login", + json={"username": username, "password": "AdminPass1!"}, + ) + return login.json()["access_token"] + + +@pytest.mark.asyncio +async def test_disable_auth_migrates_admin_token_to_global(async_client: AsyncClient, db_session: AsyncSession): + bearer = await _admin_bearer(async_client) + admin = (await db_session.execute(select(User).where(User.role == "admin"))).scalar_one() + admin.cloud_token = "tok-user" + admin.cloud_email = "user@example.com" + admin.cloud_region = "china" + await db_session.commit() + + resp = await async_client.post("/api/v1/auth/disable", headers={"Authorization": f"Bearer {bearer}"}) + assert resp.status_code == 200, resp.text + + rows = await _global_rows(db_session) + assert rows[CLOUD_TOKEN_KEY] == "tok-user" + assert rows[CLOUD_REGION_KEY] == "china" + + await db_session.refresh(admin) + assert admin.cloud_token is None, "credential must not be duplicated across both stores" + + # And the no-auth read path now finds it. + token, _, _ = await get_stored_token(db_session, None) + assert token == "tok-user" + + +@pytest.mark.asyncio +async def test_disable_auth_does_not_clobber_existing_global_token(async_client: AsyncClient, db_session: AsyncSession): + """A stale global row is still somebody's credential — refuse rather than overwrite.""" + bearer = await _admin_bearer(async_client) + admin = (await db_session.execute(select(User).where(User.role == "admin"))).scalar_one() + admin.cloud_token = "tok-user" + await db_session.commit() + await _seed_global_token(db_session, token="tok-preexisting") + + resp = await async_client.post("/api/v1/auth/disable", headers={"Authorization": f"Bearer {bearer}"}) + assert resp.status_code == 200, resp.text + + assert (await _global_rows(db_session))[CLOUD_TOKEN_KEY] == "tok-preexisting" + await db_session.refresh(admin) + assert admin.cloud_token == "tok-user", "admin keeps their token when we decline to migrate" + + +@pytest.mark.asyncio +async def test_disable_auth_with_no_cloud_token_is_a_noop(async_client: AsyncClient, db_session: AsyncSession): + bearer = await _admin_bearer(async_client) + + resp = await async_client.post("/api/v1/auth/disable", headers={"Authorization": f"Bearer {bearer}"}) + assert resp.status_code == 200, resp.text + assert await _global_rows(db_session) == {}