diff --git a/CHANGELOG.md b/CHANGELOG.md index c30a5d942..d6fa76626 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ All notable changes to Bambuddy will be documented in this file. - **VP FTP upload capped at 4 GiB (DoS guard)** — `cmd_STOR` now rejects an upload that crosses `MAX_UPLOAD_BYTES = 4 GiB`, deletes the partial file, and replies 426. Without the cap a runaway or malicious client could drive RSS or disk to exhaustion; 4 GiB is well above any realistic multi-plate `.gcode.3mf`. Same code path adds the streaming rewrite (see Changed section for details). ### Fixed +- **OIDC auto-provisioning now reads the standard `email` claim for `User.email` when `Email Claim` is set to a non-email identity claim (#1569, reported by @anderl1969)** — Reporter configured Authentik with `Email Claim = preferred_username` to drive username from the preferred_username claim and expected the standard `email` claim (which the ID token also carries) to populate the user's email field. Result: username was correctly set from `preferred_username`, but `User.email` came out empty. Cause: `backend/app/api/routes/mfa.py::_resolve_provider_email` reads only `claims[provider.email_claim]`. With `email_claim="preferred_username"` and `preferred_username="jdoe"`, the value fails the SEC-2 email shape check (no `@`) and returns `None`. The auto-create-users branch then constructs `User(email=None, …)` and stores `UserOIDCLink(provider_email=None)` even though `claims["email"]` carries a perfectly valid `jdoe@example.com`. **Fix**: new helper `_resolve_standard_email_for_user_record(provider, claims, provider_sub)` reads the standard `email` claim independently and applies the same Fall A/B logic (shape check, `require_email_verified` strict / permissive split, explicit `email_verified=False` drop). The auto-create-users branch in `oidc_callback` now resolves `user_email_for_storage = provider_email or _resolve_standard_email_for_user_record(...)` and uses that for both `new_user.email` and the `UserOIDCLink.provider_email` record. **Scope is deliberately narrow**: the fallback is invoked only when `provider.email_claim != "email"` AND the primary resolver returned `None` AND the auto-create-users branch is taken. The auto-link-existing-accounts gate above remains on the primary `provider_email` — it does NOT consult the fallback. This preserves every existing GHSA-shape guard: Fall-B (`email_claim='email'` + `require_email_verified=False`) is still rejected at schema level when paired with auto-link; Fall-C (custom claim) auto-link still depends on the custom claim's shape, never on the standard `email` claim. New email fallback path runs the same shape + `email_verified` enforcement as Fall-A/B for the standard `email` claim, so an attacker-controlled IdP that sets `email_verified=False` or sends a malformed value gets dropped exactly like it would on the primary path. **Tests**: 4 in `TestOIDCStandardEmailFallback` (`backend/tests/integration/test_mfa_api.py`) — `email_claim=preferred_username` with both claims present → username from `preferred_username`, email from standard `email`; `email_claim=preferred_username` with no standard `email` claim → email stays `None` (behaviour unchanged); standard `email` with `email_verified=False` → fallback drops, email stays `None`; `email_claim="email"` (default) with `email_verified` absent → fallback path does NOT fire (Fall-A semantics preserved). Full 5478-test backend suite green. Backend ruff clean. - **Sliced `.gcode.3mf` files now render in the 3D preview and expose a Preview-3D action in the file row (#1543, reported by @Vlado-Tarakan)** — Reporter exported a multi-plate `.gcode.3mf` from Bambu Studio to the shared folder Bambuddy watches and the 3D preview tab came up empty; if he re-uploaded the same file via the file manager, the preview worked. Root cause: two paths classify `file_type` differently. `backend/app/api/routes/library.py:1343-1348` (the shared-folder scan path) does a compound-extension check and tags the file `gcode.3mf`; the upload path at the same file's `1588` does a single `ext[1:]` and tags it `3mf`. Then `frontend/src/components/ModelViewerModal.tsx:71-73` had `hasModel = normalizedType === '3mf' || 'stl'` and `hasGcode = normalizedType === 'gcode' || '3mf'` — neither matched `gcode.3mf`, so the capabilities object landed with both flags false and the modal rendered an empty bed. `FileManagerPage.tsx:858` also gated the Preview-3D context action on `file_type === '3mf' || 'gcode' || 'stl'`, so for shared-folder files the entry didn't even appear, and the type pill at `765-770` had no colour case for `gcode.3mf` so it fell through to the generic gray. **Fix** (frontend-only, no backend churn): `ModelViewerModal.tsx` introduces an `isThreeMfFamily = normalizedType === '3mf' || normalizedType === 'gcode.3mf'` predicate used in two places — the capabilities branch (`hasModel = isThreeMfFamily || 'stl'`, `hasGcode = isThreeMfFamily || 'gcode'`) and the plates-loading branch that previously hard-gated on `!== '3mf'` and would have returned `setPlatesData(null)` for the shared-folder file. `FileManagerPage.tsx` adds `gcode.3mf` to the Preview-3D action gate and shares the gcode blue type-pill colour so sliced-output files are visually distinguishable from source 3MFs. The compound `gcode.3mf` classification on the backend is intentionally preserved — it carries useful "this is a sliced output" semantics that other UI surfaces could use later. The `canOpenInSlicer` and `sliceableType` checks at `ModelViewerModal.tsx:269, 277-280` are deliberately left alone — a sliced output isn't openable in the slicer, and `sliceableType` already explicitly excludes `.gcode` and `.gcode.3mf` per the comment "the file type can't be sliced". **Out of scope** (separate Bambu-Studio format limitation, not a Bambuddy bug): Vlado's secondary observation that the upload-path 3D preview "shows only one plate" even though his project has 5 plates — Bambu Studio's `.gcode.3mf` export contains the g-code and model data for the active plate only, not the entire multi-plate project. The print picker enumerates plates via `gcode_*.gcode` entries inside the zip (a separate code path), which is why the user can still pick the plate at print time. The empty-bed fix is the data point that closes the user-visible bug. **Tests**: existing full 2043-test frontend suite green; no test asserted on the unsupported `gcode.3mf` capabilities branch (the change is additive — `3mf` and `stl` and `gcode` behaviours are unchanged). Frontend build clean. - **Connected-edge reconciliation closes the missed-PRINT-COMPLETE loop that produced ghost replays on smart-plug power cycles (#1542 follow-up, reported by @vixussrl-ui)** — Reporter ran a fresh trace after the doubled-extension fix landed and found a distinct second cause behind his ghost prints, hitting 4-of-4 of his A1s. Timeline: 22:50 PRINT START → print runs all night → MQTT disconnects multiple times (A1's keepalives are unstable on his network) → print finishes during one of those disconnect windows so PRINT COMPLETE is never observed → smart plug cuts power on idle → power resumes for the next scheduled print → firmware auto-replays the leftover `.3mf` from the SD card → Bambuddy reconnects to a fresh PRINT START for the ghost. The existing IDLE-after-RUNNING completion check at `backend/app/services/bambu_mqtt.py:3022` was meant to catch the simple disconnect-then-finish case via `_previous_gcode_state` preserved across reconnects, but with multiple disconnect/reconnect cycles + a smart-plug power-off that Bambuddy can't distinguish from any other transient drop, the IDLE window that branch needs simply never reaches it. The SD `.3mf` lingers, the firmware ghost-replays every power cycle, and the loop repeats until the operator notices. **Fix**: a new connected-edge reconciliation pass — new `reconcile_stale_active_prints(printer_id)` in `backend/app/main.py` queries archives in `status="printing"` for the printer at MQTT (re)connect time and synthesises `on_print_complete(status="aborted")` for any whose print can't actually be running anymore. The decision is made by a pure `_is_active_archive_stale(archive, state)` function with three triggers: (1) current printer state is terminal (IDLE / FINISH / FAILED) — covers the clean disconnect-then-finish case the existing #3022 branch was already trying to handle; (2) printer is running but with a different `subtask_id` than the archive — Bambu firmware mints a fresh `subtask_id` for each print including the ghost-replay it runs after a power cycle, so a mismatch is unambiguous evidence the in-DB archive is no longer the print on the printer; (3) printer is running but `subtask_name` is empty — the printer doesn't know what it's running, archive reference is broken. PAUSE / PREPARE / SLICING / RUNNING with matching subtask are intentionally left alone — false positives there cost a single misreported "aborted" status that the real PRINT COMPLETE would have overwritten anyway, while a false negative is the ghost-print loop being reported. The synthesised `on_print_complete` reuses the existing chain (SD cleanup, status update, usage tracker, notifications) — no reimplementation, no duplicate event when real completion later fires (the second call sees `status != "printing"` and falls through). Status `"aborted"` is the conservative label; we have no progress evidence to promote to `"completed"`. **Wiring**: new `_printer_reconciled_since_connect: dict[int, bool]` edge tracker at module scope, checked at the start of `on_printer_status_change` — when `state.connected` flips False → True (which covers both Bambuddy startup with no prior connection AND a mid-session MQTT reconnect), reconciliation fires exactly once for that connection. Setting the edge to True BEFORE the spawned task starts prevents concurrent status updates within the same connection from re-triggering it. **Concurrency**: reconciliation runs as `asyncio.create_task` so it doesn't block the WebSocket dedup / broadcast logic that on_printer_status_change is the hot path for. **Ghost-print collateral worth being explicit about**: if the ghost is already running when reconciliation fires, the synthesised SD-cleanup will hit 550-file-locked (firmware locks the file during print, same cause as the #1542 first case). The cleanup retries 3× then logs "lingering" — same as any other in-print cleanup attempt. The ghost runs to completion, its own end-of-print cleanup deletes the file, and the next power cycle has nothing to replay. The loop breaks even when reconciliation can't physically delete the file mid-ghost. A perfect cancel would require sending a `print_stop` MQTT command to the printer, which is invasive and explicitly out of scope. **Tests**: 21 in `test_reconcile_stale_active_prints.py` — `TestIsActiveArchiveStale` covers all three stale triggers with case-insensitive state matching, the four healthy-no-op cases (RUNNING / PAUSE / PREPARE / SLICING with matching subtask), the IDLE-overrides-subtask-match precedence, and the missing-subtask_id edge cases that fall through to the subtask_name check. `TestReconcileStaleActivePrints` covers the orchestrator: no-status, disconnected-status, and no-active-archives all short-circuit; a stale archive produces a synthesised `on_print_complete(status="aborted", _reconciled=True)` payload with the archive filename; a healthy in-flight archive doesn't fire any completion; an exception inside one archive's synthesis doesn't block the rest or propagate to the caller. Full 5399-test backend suite green (5378 + 21 new). Backend ruff clean. - **Fallback-archive MQTT filament extraction now actually fires for real prints (#1533 follow-up, reported by @JmanB52D)** — Reporter updated to 0.2.5b1 expecting the #1533 fix to populate filament fields on his P2S virtual-printer prints when the .3mf is locked. His support bundle showed Bambuddy still creating fallback archives with NULL filament fields even though the print-start log line proved AMS-0-T0 had PETG loaded at the moment the helper should have read it (`AMS 0: T0(type=PETG, color=FFFFFFFF, …)`). Cause: the #1533 helper `_extract_filament_data_from_mqtt(data)` in `backend/app/main.py` only looked at `data["ams"]`, but the dict that `on_print_start` actually receives at runtime is the wrapper shape `{"filename", "subtask_name", "remaining_time", "raw_data": , "ams_mapping"}` that `backend/app/services/bambu_mqtt.py:2971-2980` constructs — so `data["ams"]` was undefined on every real call and the helper silently returned `{}`, leaving the fallback archive's `filament_type` / `filament_color` NULL. The 15 unit tests that shipped with #1533 all passed the bare inner shape directly and never exercised the callback wiring, so the regression slipped through the green build. **Fix**: the helper now resolves `data["raw_data"]["ams"]` first (the callback shape) and only falls back to `data["ams"]` when the wrapper isn't present (preserves the inner-shape callers from the existing tests). Defensive: a non-dict `raw_data` (e.g. partial MQTT decode failure) falls through to the inner lookup instead of crashing. **Tests**: 5 new in `TestOnPrintStartCallbackShape` (`backend/tests/unit/test_fallback_archive_mqtt_filament.py`) — wrapper payload with ams_mapping resolves to the inner data; wrapper with no ams_mapping lists all loaded slots; the existing inner-shape callers still work after the additive wrapper lookup; missing `raw_data` returns `{}` instead of raising; junk `raw_data` (string) doesn't shadow a present inner `ams`. Full 5378-test backend suite green. Backend ruff clean. **What this does NOT fix**: per-filament gram usage still needs the actual .3mf — the printer locks it during print (P-line firmware behaviour, not a Bambuddy bug), and the existing 19 FTP candidate paths + directory probes are expected to 550 in that window. Per-print filament type and colour are the data point that drives the AMS-expansion planning the reporter explicitly called out, so this is the fix that moves the needle for him. diff --git a/backend/app/api/routes/mfa.py b/backend/app/api/routes/mfa.py index 88e81cd08..1fb6fecca 100644 --- a/backend/app/api/routes/mfa.py +++ b/backend/app/api/routes/mfa.py @@ -562,6 +562,57 @@ def _resolve_provider_email(provider: OIDCProvider, claims: dict, provider_sub: return raw_email +def _resolve_standard_email_for_user_record(provider: OIDCProvider, claims: dict, provider_sub: str) -> str | None: + """Resolve the standard 'email' claim for populating a newly-created User.email. + + Issue #1569: when an operator sets email_claim to a non-email identity claim + (e.g. preferred_username on Authentik), the primary _resolve_provider_email + returns None because the identity value isn't email-shaped. This helper lets + the auto-create-users path still capture the user's real email from the + standard 'email' claim that the IdP usually sends alongside. + + This is NOT a substitute for _resolve_provider_email and does NOT feed + auto_link_existing_accounts — that gate stays on the primary resolver, so + the GHSA Fall-B/C security guards remain intact. + + Applies the same Fall A/B shape + email_verified logic as the primary + resolver does for the standard 'email' claim. + """ + raw_claim_value = claims.get("email") + if raw_claim_value is not None and not isinstance(raw_claim_value, str): + logger.warning( + "OIDC provider %d: standard 'email' claim has unexpected type %s for sub=%r, ignoring", + provider.id, + type(raw_claim_value).__name__, + provider_sub, + ) + return None + raw_email = raw_claim_value.lower().strip() if raw_claim_value else None + if not raw_email: + return None + if not _is_valid_email_shaped(raw_email): + logger.warning( + "OIDC provider %d: standard 'email' claim failed shape check for sub=%r, ignoring", + provider.id, + provider_sub, + ) + return None + email_verified = claims.get("email_verified") + if provider.require_email_verified: + if email_verified is True: + return raw_email + logger.info( + "OIDC provider %d: ignoring fallback email for sub=%r because email_verified=%r", + provider.id, + provider_sub, + email_verified, + ) + return None + if email_verified is False: + return None + return raw_email + + # --------------------------------------------------------------------------- # Settings helpers (email 2FA flag) # --------------------------------------------------------------------------- @@ -1905,6 +1956,18 @@ async def oidc_callback( raw = provider_sub[:30] candidate = re.sub(r"[^a-zA-Z0-9._-]", "", raw)[:30] or "oidcuser" + # Issue #1569: when email_claim is configured to a non-email + # identity claim (e.g. preferred_username on Authentik), the + # primary resolver returns None for the email field because the + # identity value isn't email-shaped. Fall back to the standard + # 'email' claim for User.email so the operator can split + # username-from-preferred_username and email-from-email. + # The auto-link gate above stays on provider_email, so the + # GHSA Fall-B/C guards remain intact. + user_email_for_storage = provider_email + if user_email_for_storage is None and provider.email_claim != "email": + user_email_for_storage = _resolve_standard_email_for_user_record(provider, claims, provider_sub) + username = candidate counter = 1 while True: @@ -1932,7 +1995,7 @@ async def oidc_callback( new_user = User( username=username, - email=provider_email, + email=user_email_for_storage, # M-1: auth_source="oidc" prevents local password-reset flow # for users who should only authenticate via OIDC. auth_source="oidc", @@ -1949,7 +2012,7 @@ async def oidc_callback( user_id=new_user.id, provider_id=provider_id, provider_user_id=provider_sub, - provider_email=provider_email, + provider_email=user_email_for_storage, ) ) await db.commit() diff --git a/backend/tests/integration/test_mfa_api.py b/backend/tests/integration/test_mfa_api.py index 19d0f4366..a393d4b1c 100644 --- a/backend/tests/integration/test_mfa_api.py +++ b/backend/tests/integration/test_mfa_api.py @@ -4241,6 +4241,217 @@ class TestOIDCEmailResolutionExtra: assert upd2.json()["email_claim"] == "preferred_username" +# =========================================================================== +# Issue #1569: standard 'email' claim fallback for User.email during +# auto-provisioning when email_claim is a non-email identity claim. +# =========================================================================== + + +class TestOIDCStandardEmailFallback: + """Issue #1569: when email_claim is set to a non-email identity claim + (e.g. preferred_username on Authentik) and the IdP token also carries a + standard 'email' claim, auto-created users get User.email from the + standard claim — without affecting the auto-link gate.""" + + async def _get_user_and_link(self, db_session: AsyncSession, provider_id: int, sub: str): + from sqlalchemy import select + + from backend.app.models.oidc_provider import UserOIDCLink + from backend.app.models.user import User as UserModel + + link_result = await db_session.execute( + select(UserOIDCLink) + .where(UserOIDCLink.provider_id == provider_id) + .where(UserOIDCLink.provider_user_id == sub) + ) + link = link_result.scalar_one_or_none() + if link is None: + return None, None + user_result = await db_session.execute(select(UserModel).where(UserModel.id == link.user_id)) + return user_result.scalar_one_or_none(), link + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_email_claim_preferred_username_falls_back_to_standard_email( + self, + async_client: AsyncClient, + db_session: AsyncSession, + ): + """email_claim=preferred_username + both claims → username from preferred_username, + email populated from the standard 'email' claim.""" + private_pem, jwks_data = _make_test_rsa_key() + issuer = "https://issue1569-both.example" + client_id = "issue1569-both-client" + + admin_token = await _setup_and_login(async_client, "i1569b_adm", "I1569b123!") + provider_id = await _create_provider_via_api( + async_client, + admin_token, + issuer, + client_id, + email_claim="preferred_username", + require_email_verified=False, + suffix="i1569both", + ) + + sub = f"sub-i1569-both-{secrets.token_hex(6)}" + await _run_oidc_callback( + async_client, + db_session, + provider_id=provider_id, + claims={ + "sub": sub, + "preferred_username": "jdoe", + "email": "jdoe@example.com", + "email_verified": True, + }, + private_pem=private_pem, + jwks_data=jwks_data, + issuer=issuer, + client_id=client_id, + ) + db_session.expire_all() + user, link = await self._get_user_and_link(db_session, provider_id, sub) + assert user is not None and link is not None + assert user.username == "jdoe" + assert user.email == "jdoe@example.com" + assert link.provider_email == "jdoe@example.com" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_email_claim_preferred_username_no_standard_email_keeps_email_none( + self, + async_client: AsyncClient, + db_session: AsyncSession, + ): + """email_claim=preferred_username + no standard 'email' claim → behaviour unchanged: + username derived from preferred_username, User.email stays None.""" + private_pem, jwks_data = _make_test_rsa_key() + issuer = "https://issue1569-noemail.example" + client_id = "issue1569-noemail-client" + + admin_token = await _setup_and_login(async_client, "i1569n_adm", "I1569n123!") + provider_id = await _create_provider_via_api( + async_client, + admin_token, + issuer, + client_id, + email_claim="preferred_username", + require_email_verified=False, + suffix="i1569none", + ) + + sub = f"sub-i1569-none-{secrets.token_hex(6)}" + await _run_oidc_callback( + async_client, + db_session, + provider_id=provider_id, + claims={"sub": sub, "preferred_username": "jdoe2"}, + private_pem=private_pem, + jwks_data=jwks_data, + issuer=issuer, + client_id=client_id, + ) + db_session.expire_all() + user, link = await self._get_user_and_link(db_session, provider_id, sub) + assert user is not None and link is not None + assert user.username == "jdoe2" + assert user.email is None + assert link.provider_email is None + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_fallback_respects_email_verified_false( + self, + async_client: AsyncClient, + db_session: AsyncSession, + ): + """email_claim=preferred_username + standard 'email' claim with email_verified=False + → fallback drops the email (User.email None) — matches Fall B semantics.""" + private_pem, jwks_data = _make_test_rsa_key() + issuer = "https://issue1569-evfalse.example" + client_id = "issue1569-evfalse-client" + + admin_token = await _setup_and_login(async_client, "i1569f_adm", "I1569f123!") + provider_id = await _create_provider_via_api( + async_client, + admin_token, + issuer, + client_id, + email_claim="preferred_username", + require_email_verified=False, + suffix="i1569evfalse", + ) + + sub = f"sub-i1569-evfalse-{secrets.token_hex(6)}" + await _run_oidc_callback( + async_client, + db_session, + provider_id=provider_id, + claims={ + "sub": sub, + "preferred_username": "jdoe3", + "email": "jdoe3@example.com", + "email_verified": False, + }, + private_pem=private_pem, + jwks_data=jwks_data, + issuer=issuer, + client_id=client_id, + ) + db_session.expire_all() + user, link = await self._get_user_and_link(db_session, provider_id, sub) + assert user is not None and link is not None + assert user.username == "jdoe3" + assert user.email is None + assert link.provider_email is None + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_fallback_skipped_when_email_claim_is_email( + self, + async_client: AsyncClient, + db_session: AsyncSession, + ): + """email_claim='email' (default) — fallback path must NOT fire. Primary + resolver result is authoritative; this guards against the fallback + accidentally widening Fall-A/B semantics.""" + private_pem, jwks_data = _make_test_rsa_key() + issuer = "https://issue1569-default.example" + client_id = "issue1569-default-client" + + admin_token = await _setup_and_login(async_client, "i1569d_adm", "I1569d123!") + provider_id = await _create_provider_via_api( + async_client, + admin_token, + issuer, + client_id, + email_claim="email", + require_email_verified=True, + suffix="i1569default", + ) + + sub = f"sub-i1569-default-{secrets.token_hex(6)}" + # email_verified absent → Fall A drops it. Fallback must NOT + # re-instate the email since email_claim='email' is already the + # primary path. + await _run_oidc_callback( + async_client, + db_session, + provider_id=provider_id, + claims={"sub": sub, "email": "jdoe4@example.com"}, + private_pem=private_pem, + jwks_data=jwks_data, + issuer=issuer, + client_id=client_id, + ) + db_session.expire_all() + user, link = await self._get_user_and_link(db_session, provider_id, sub) + assert user is not None and link is not None + assert user.email is None + assert link.provider_email is None + + # =========================================================================== # E2E: Fall C (custom email claim) auto-link actually links existing user # ===========================================================================