From 78408856cdd5d9dd0a5e762f5136d66dca3d059f Mon Sep 17 00:00:00 2001 From: Sn0rrii <4687675+netscout2001@users.noreply.github.com> Date: Tue, 28 Apr 2026 17:37:48 +0200 Subject: [PATCH] fix(oidc): Allow `auto_link_existing_accounts` with custom email claims (Azure Entra ID) (#1142) chore(i18n): extend parity gate to all locales with strict/info tiers --- CHANGELOG.md | 2 + UPDATING.md | 19 +- backend/app/api/routes/mfa.py | 13 +- backend/app/core/database.py | 115 +++++++- backend/app/models/oidc_provider.py | 8 +- backend/app/schemas/auth.py | 21 +- backend/tests/integration/test_mfa_api.py | 226 +++++++++++++++- backend/tests/unit/test_db_dialect.py | 252 ++++++++++++++++-- frontend/scripts/check-i18n-parity.mjs | 15 +- .../components/OIDCProviderSettings.test.tsx | 29 +- .../src/components/OIDCProviderSettings.tsx | 7 +- frontend/src/i18n/locales/de.ts | 1 + frontend/src/i18n/locales/en.ts | 1 + frontend/src/i18n/locales/fr.ts | 1 + frontend/src/i18n/locales/it.ts | 1 + frontend/src/i18n/locales/ja.ts | 1 + frontend/src/i18n/locales/pt-BR.ts | 1 + frontend/src/i18n/locales/zh-CN.ts | 1 + frontend/src/i18n/locales/zh-TW.ts | 1 + 19 files changed, 628 insertions(+), 87 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 41ffb6138..e5102bfd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,8 @@ All notable changes to Bambuddy will be documented in this file. - **Per-request trace ID column on every log line, plumbed through HTTP access log + application logs + response headers** — Builds on the new uvicorn-access-log-into-bambuddy.log change below: the access line tells you *who* called an endpoint, but until now there was no way to tie that line to the application records emitted on the server side while handling that request. A new FastAPI middleware (`trace_id_middleware` in `main.py`, sourced from `backend.app.core.trace`) stamps each request with a fresh 8-char hex ID (or honours a sane inbound `X-Trace-Id` header for cross-system correlation), stores it in a `ContextVar` so any code in the request's call stack can read it, echoes it on the response as `X-Trace-Id`, and a new `TraceIDFilter` injects it into every `LogRecord` so the format string `[%(trace_id)s]` resolves to the right ID for the right request. ContextVars (rather than `request.state`) are the right plumbing here because asyncio copies the current context into every `asyncio.create_task`, so background work spawned from inside a request inherits the trace ID without explicit threading; the logging filter has no access to the FastAPI request object regardless. Records emitted outside any request scope (startup, MQTT callbacks, scheduler) get a stable `-` placeholder so the column stays visually aligned and missing values are obvious in `grep`. Inbound `X-Trace-Id` is hard-validated against a strict whitelist (`[A-Za-z0-9_-]+`, max 64 chars) before being honoured — a hostile or buggy caller cannot smuggle log-injection payloads (newlines, control chars, megabyte blobs) into `bambuddy.log` via the trace-ID column; values that fail the gate silently trigger a freshly minted server-side ID rather than failing the request. Middleware is decorated AFTER `auth_middleware` on purpose: Starlette stacks `@app.middleware` decorators LIFO so the last-decorated runs first inbound, making trace stamp the OUTERMOST layer — auth log lines and every record emitted on the way down to and back from the route handler all carry the same ID. Output now looks like `2026-04-26 09:51:39,152 INFO [uvicorn.access] [a4f3b1e7] 192.168.1.42:54812 - "POST /api/v1/printers/1/print/stop HTTP/1.1" 200` paired with the route handler's `2026-04-26 09:51:39,158 INFO [bambu_mqtt] [a4f3b1e7] [SERIAL] Sent stop print command` — one `grep a4f3b1e7` away from the full causality chain. 30 new tests across `tests/unit/test_trace.py` (placeholder when no request scope, filter copies ContextVar value onto records, ID propagates into spawned tasks via asyncio context copy, concurrent requests don't leak IDs into each other, generator produces unique hex IDs, hostile payloads rejected by validator, max-length boundary, dash/underscore variants accepted) plus `tests/integration/test_trace_middleware.py` (X-Trace-Id header echoed on response, body and header IDs match, each request gets a unique ID, generator format stays short hex, safe inbound IDs honoured, hostile inbound IDs replaced, overlong inbound IDs replaced, ContextVar reset cleanly after request). ### Fixed +- **OIDC `auto_link_existing_accounts` now works with custom email claims (Azure Entra ID)** ([#1088](https://github.com/maziggy/bambuddy/issues/1088)) — `auto_link_existing_accounts` was previously blocked unless both `email_claim='email'` and `require_email_verified=True`. This also rejected Azure Entra ID configurations using `preferred_username` or `upn` as the email claim — the recommended setup for that provider, which does not send `email_verified`. The guard now only blocks the genuinely unsafe combination (Fall B): `email_claim='email'` + `require_email_verified=False`. Custom-claim configurations (Fall C) never consult `email_verified` at all, so there is no verification-bypass risk on that path. All five enforcement layers (DB CHECK constraint, schema validators for create and update, route combined-state guard, DB migration for existing installations) have been updated consistently. **Security note:** custom claims are safe for auto-link only when the claim value is tenant-administered. If your IdP allows end users to self-assert the claim's value, do not enable auto-link. An in-app warning is shown in the OIDC provider form when this combination is configured. +- **OIDC settings form: "Require email verified" toggle no longer jumps layout when auto-link is enabled** — When `Auto-link existing accounts` was toggled on, the shorter description text caused the `Require email verified` toggle to reflow next to `Auto-link` in the flex container instead of staying on its own row. Both toggles now have `w-full` and always occupy a full row regardless of description length. - **P1P print dispatch failed with `0500_4003 "can't parse print file"` when the printer was slow to acknowledge** ([#1150](https://github.com/maziggy/bambuddy/issues/1150), reported by @d3ni3) — On a P1P at firmware 01.10.00.00 the printer can take up to ~135 seconds to actually start parsing a freshly uploaded `.3mf` after the MQTT `project_file` command lands; FTP STOR returns 226 cleanly and the upload is intact, but `gcode_state` stays at `IDLE` and `subtask_id` doesn't advance until the printer's slow internal parse completes. Both dispatch watchdogs (`_verify_print_response` in `background_dispatch.py` and `_watchdog_print_start` in `print_scheduler.py`) interpreted the missed transition as a half-broken MQTT session — the original #887/#936 condition where telemetry kept arriving but our publishes were silently swallowed — and called `force_reconnect_stale_session` to wipe paho's QoS-1 queue and reconnect with a fresh client_id. That reconnect mid-parse is precisely what makes the P1P emit `0500_4003`: the new MQTT session interrupts the in-progress parse on the printer side and the printer reports the file as unparseable. The repro: send a print job, wait 15 seconds while the printer is still parsing, watch the watchdog force-reconnect, watch the printer fail with the parse error, retry — same loop. Sending the same file from BambuStudio worked because BambuStudio doesn't reconnect MQTT mid-parse. The fix uses the printer's `gcode_file` field as a definitive discriminator between #1150 (slow parse) and #887/#936 (half-broken session), since both look identical from telemetry alone: in both cases push_status keeps flowing, `state` stays unchanged, and `subtask_id` stays at the pre-dispatch value. The distinguishing signal: when the project_file command actually lands on the printer side, the printer's `gcode_file` field updates in push_status to reflect the newly-uploaded file; if the publish was silently swallowed (#887/#936), the field stays at whatever the printer was previously showing. Both watchdogs now capture `pre_gcode_file` alongside `pre_state` and `pre_subtask_id` from `printer_manager.get_status()` before sending the publish, then compare against the printer's current `gcode_file` after the watchdog times out. If the value changed → command landed → log a `#1150` warning explaining the skip and leave the MQTT session alone. If the value is unchanged → publish was silently swallowed → fall through to the original `force_reconnect_stale_session` call so the #887/#936/#1136 zombie-session recovery is preserved exactly. The user-facing dispatch still fails on timeout (correctly — the print didn't start within the timeout window so the job is marked failed), the queue item still reverts to `pending` so the scheduler can retry, and the next dispatch attempt proceeds against the same intact MQTT session that was about to start the print. Pairs with the 15s → 90s timeout bump that already shipped in commit 9d041868 (the original 15s timeout was a separate v0.2.3.2 limit). Caveat acknowledged in code comments: in a retry-same-file slow-parse scenario the printer's `gcode_file` looks identical before and after the publish lands, so the watchdog falls through to the original reconnect path and the user still sees `0500_4003` on that specific retry — accepted to avoid breaking the half-broken-session recovery, which is the more impactful regression of the two. 4 new unit tests covering both watchdogs: skip reconnect when `gcode_file` changed (the #1150 fix), reconnect when `gcode_file` is unchanged (the #936 protection preserved), skip reconnect when `pre_gcode_file=None` and current is non-None (printer just connected), reconnect when `pre_gcode_file` arg is omitted (backward-compat for callers we haven't updated). All 439 existing dispatch / scheduler / mqtt tests still pass unchanged. - **3MF profile-driven slicing silently produced wrong-printer output (every 3MF slice fell back to the source's embedded printer regardless of the picked profile)** — Two stacked bugs in the slice pipeline. **(1) Pre-forward strip removed too much.** `_strip_3mf_embedded_settings` was scrubbing all four embedded `Metadata/*.config` files before forwarding the 3MF to the sidecar, on the theory that `--load-settings` would then take precedence cleanly. That theory was wrong: `Metadata/model_settings.config` carries the plate definitions the CLI needs to map `--slice N` to a real plate, and `slice_info.config` / `project_settings.config` supply baseline config the CLI's `StaticPrintConfigs` pass needs to even start. Stripping any of them caused the CLI to silently exit immediately after "Initializing StaticPrintConfigs" — exit code 0, no `result.json`, no stderr — which the sidecar treated as failure and Bambuddy then masked by falling back to `slice_without_profiles` using the un-stripped bytes (and the source's embedded printer). Net effect: every 3MF slice with profiles silently produced wrong-printer output. The strip is now gone from the slicer dispatch path entirely; original bytes go to the sidecar so `--load-settings` overrides only the specific fields the user changed (printer/process/filament) while the embedded plate / model definitions remain intact. **(2) Standard-tier preset stubs were missing the `type` field.** `_resolve_standard` in `preset_resolver.py` emitted `{"name": ..., "inherits": ..., "from": "system"}` for the bundled tier, but the CLI's preset parser also requires a `type` discriminator (`machine` / `process` / `filament`) on every loaded settings file — without it the CLI silently rejects with `rc=-5` ("input preset file is invalid"), which the same masking fallback then turned into another wrong-printer slice. New `_SLOT_TO_PROFILE_TYPE` constant maps each slot to its required type, and the stub now emits the right value per slot. **Tests**: integration test renamed from "strip removes all four configs" to `test_3mf_input_forwarded_unmodified_to_sidecar` — asserts every `Metadata/*.config` plus `3D/3dmodel.model` is preserved verbatim in the multipart body the sidecar receives. Preset-resolver test updated for the new stub shape; new `test_standard_emits_correct_type_per_slot` pins each (slot → type) pairing. Pairs with the orca-slicer-api fork's `bambuddy/profile-resolver` branch which now emits `details` on its `AppError` responses and captures CLI stdout/stderr in the failure path so future regressions of this shape produce a real error message instead of a silent fallback. diff --git a/UPDATING.md b/UPDATING.md index 50b049854..3f34ef66a 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -1,9 +1,8 @@ # Updating Bambuddy -> **One-time note for 0.2.2.x → 0.2.3:** the in-app **Update** button does not -> reliably perform this specific migration. Do this one upgrade from the -> command line using the steps below. Once you're on 0.2.3, the in-app Update -> button works normally again for all future releases. +> **0.2.3 note:** the in-app **Update** button is unreliable when upgrading from +> older releases. Use the commands below instead — they cover every supported +> install path and are safe to run repeatedly. Pick the section that matches how Bambuddy was installed. @@ -75,7 +74,10 @@ These installs have no `.git` directory, so neither `update.sh` nor a plain ```bash # 1. Back up your stateful data -Create and download a backup via Bambuddy Settings -> Backup -> Local Backup +sudo systemctl stop bambuddy +sudo tar czf ~/bambuddy-backup.tgz -C /opt/bambuddy \ + data bambuddy.db bambuddy.db-shm bambuddy.db-wal \ + virtual_printer archive projects icons .env 2>/dev/null || true # 2. Remove the old install and reinstall via install.sh sudo rm -rf /opt/bambuddy @@ -83,10 +85,9 @@ curl -fsSL https://raw.githubusercontent.com/maziggy/bambuddy/main/install/insta -o /tmp/install.sh && sudo bash /tmp/install.sh --path /opt/bambuddy # 3. Restore your data -Restore your backup via Bambuddy -> Settings -> Backup -> Local Backup - -# 4. Restart Bambuddy -sudo systemctl restart bambuddy +sudo systemctl stop bambuddy +sudo tar xzf ~/bambuddy-backup.tgz -C /opt/bambuddy +sudo systemctl start bambuddy ``` --- diff --git a/backend/app/api/routes/mfa.py b/backend/app/api/routes/mfa.py index 72e96374c..7b540cdd1 100644 --- a/backend/app/api/routes/mfa.py +++ b/backend/app/api/routes/mfa.py @@ -389,15 +389,14 @@ def _is_valid_email_shaped(value: str | None) -> bool: def _enforce_auto_link_safety(provider: OIDCProvider) -> None: - """Raise HTTP 422 if auto_link_existing_accounts is on without safe email settings. + """Raise HTTP 422 if auto_link_existing_accounts is on with an unsafe combined state. - SEC-1/SEC-6: auto_link is only safe when require_email_verified=True AND - email_claim='email'. Called after ORM construction (create) and after the - setattr loop (update) so partial-update bypasses are also caught. + SEC-1: only Fall B (email_claim='email' + require_email_verified=False) is unsafe — + an attacker-controlled IdP could present an unverified email that matches a local account. + Fall C (custom claim) never performs an email_verified check, so auto_link is safe there. + Called after ORM construction (create) and after the setattr loop (update). """ - if provider.auto_link_existing_accounts and ( - not provider.require_email_verified or provider.email_claim != "email" - ): + if provider.auto_link_existing_accounts and provider.email_claim == "email" and not provider.require_email_verified: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail=AUTO_LINK_REQUIREMENTS_ERROR, diff --git a/backend/app/core/database.py b/backend/app/core/database.py index 5fab67415..7d66b206b 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -276,6 +276,93 @@ async def _migrate_normalize_printer_ids(conn) -> None: await conn.execute(text("UPDATE api_keys SET printer_ids = NULL WHERE printer_ids::text = '[]'")) +async def _migrate_update_auto_link_constraint(conn) -> None: + """Update the auto_link CHECK constraint to allow Fall C (custom email claim). + + Old formula: auto_link = FALSE OR (require_ev = TRUE AND email_claim = 'email') + New formula: auto_link = FALSE OR email_claim != 'email' OR require_ev = TRUE + + Only Fall B (email_claim='email' + require_ev=False) remains blocked. + Fall C (custom claim, e.g. Azure preferred_username/upn) is now allowed. + + PostgreSQL: DROP CONSTRAINT IF EXISTS + ADD new formula via _safe_execute (idempotent). + SQLite: table recreation when old formula is detected in sqlite_master (idempotent). + """ + from sqlalchemy import text + + _NEW_FORMULA = "auto_link_existing_accounts = FALSE OR email_claim != 'email' OR require_email_verified = TRUE" + _CONSTRAINT_NAME = "ck_auto_link_requires_verified_email_claim" + + if not is_sqlite(): + await _safe_execute(conn, f"ALTER TABLE oidc_providers DROP CONSTRAINT IF EXISTS {_CONSTRAINT_NAME}") + await _safe_execute( + conn, + f"ALTER TABLE oidc_providers ADD CONSTRAINT {_CONSTRAINT_NAME} CHECK ({_NEW_FORMULA})", + ) + else: + row = ( + await conn.execute(text("SELECT sql FROM sqlite_master WHERE type='table' AND name='oidc_providers'")) + ).fetchone() + # Only recreate if the old (more restrictive) formula is still present. + # Fresh installs created with the new __table_args__ already have the correct formula. + # Installs without any constraint (pre-SEC-1 upgrades) are skipped — app-level guards suffice. + if row and "require_email_verified = TRUE AND email_claim = 'email'" in row[0]: + try: + async with conn.begin_nested(): + await conn.execute(text("DROP TABLE IF EXISTS oidc_providers_v2")) + await conn.execute( + text( + "CREATE TABLE oidc_providers_v2 (" + "id INTEGER NOT NULL, " + "name VARCHAR(100) NOT NULL, " + "issuer_url VARCHAR(500) NOT NULL, " + "client_id VARCHAR(255) NOT NULL, " + "client_secret VARCHAR(512) NOT NULL, " + "scopes VARCHAR(500), " + "is_enabled BOOLEAN, " + "auto_create_users BOOLEAN, " + "auto_link_existing_accounts BOOLEAN DEFAULT 0, " + "email_claim VARCHAR(64) DEFAULT 'email', " + "require_email_verified BOOLEAN DEFAULT 1, " + "icon_url TEXT, " + "created_at DATETIME DEFAULT CURRENT_TIMESTAMP, " + "updated_at DATETIME DEFAULT CURRENT_TIMESTAMP, " + "PRIMARY KEY (id), " + f"UNIQUE (name), " + f"CONSTRAINT {_CONSTRAINT_NAME} CHECK ({_NEW_FORMULA})" + ")" + ) + ) + await conn.execute( + text( + "INSERT INTO oidc_providers_v2 " + "(id, name, issuer_url, client_id, client_secret, scopes, is_enabled, " + "auto_create_users, auto_link_existing_accounts, email_claim, " + "require_email_verified, icon_url, created_at, updated_at) " + "SELECT id, name, issuer_url, client_id, client_secret, scopes, is_enabled, " + "auto_create_users, auto_link_existing_accounts, email_claim, " + "require_email_verified, icon_url, created_at, updated_at " + "FROM oidc_providers" + ) + ) + original = (await conn.execute(text("SELECT count(*) FROM oidc_providers"))).scalar_one() + copied = (await conn.execute(text("SELECT count(*) FROM oidc_providers_v2"))).scalar_one() + if copied != original: + raise RuntimeError( + f"auto_link constraint migration: row count mismatch after copy " + f"({original} in source, {copied} in copy)" + ) + await conn.execute(text("DROP TABLE oidc_providers")) + await conn.execute(text("ALTER TABLE oidc_providers_v2 RENAME TO oidc_providers")) + except Exception as exc: + logger.error( + "auto_link constraint update (SQLite table recreation) FAILED: %s", + exc, + exc_info=True, + ) + raise + + async def run_migrations(conn): """Run all schema migrations and data backfills on startup. @@ -1569,20 +1656,19 @@ async def run_migrations(conn): await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN require_email_verified BOOLEAN DEFAULT 1") else: await _safe_execute(conn, "ALTER TABLE oidc_providers ADD COLUMN require_email_verified BOOLEAN DEFAULT true") - # SEC-1 backfill: reset auto_link on rows where the combined state is unsafe. - # Runs BEFORE the CHECK constraint below so existing installs that have - # auto_link=TRUE + unsafe email settings self-heal rather than failing when - # PostgreSQL validates ADD CONSTRAINT against existing rows ("check constraint - # is violated by some row"). On fresh installs the column defaults guarantee - # this UPDATE matches zero rows. TRUE/FALSE literals are accepted by both - # SQLite (≥ 3.23) and PostgreSQL — no dialect branch needed. + # SEC-1 backfill: reset auto_link only for Fall B (email_claim='email' + require_email_verified=False). + # Fall C (custom claim) is now allowed to use auto_link — do NOT reset those rows. + # Runs BEFORE the CHECK constraint below so Fall B rows self-heal rather than failing + # PostgreSQL's "check constraint is violated by some row" on ADD CONSTRAINT. + # On fresh installs the column defaults guarantee this UPDATE matches zero rows. + # TRUE/FALSE literals are accepted by both SQLite (≥ 3.23) and PostgreSQL — no dialect branch needed. try: async with conn.begin_nested(): await conn.execute( text( "UPDATE oidc_providers SET auto_link_existing_accounts = FALSE " "WHERE auto_link_existing_accounts = TRUE " - "AND (require_email_verified = FALSE OR email_claim != 'email')" + "AND email_claim = 'email' AND require_email_verified = FALSE" ) ) except Exception as exc: @@ -1594,16 +1680,16 @@ async def run_migrations(conn): ) raise - # SEC-1/SEC-6: Add DB-level CHECK constraint for existing PostgreSQL installs. + # SEC-1: Add DB-level CHECK constraint for existing PostgreSQL installs. # SQLite does not support ALTER TABLE ADD CONSTRAINT — handled by __table_args__ at creation. - # Runs AFTER the backfill so legacy unsafe rows don't fail constraint validation. + # Runs AFTER the backfill so Fall B rows don't fail constraint validation. if not is_sqlite(): try: async with conn.begin_nested(): await conn.execute( text( "ALTER TABLE oidc_providers ADD CONSTRAINT ck_auto_link_requires_verified_email_claim " - "CHECK (auto_link_existing_accounts = FALSE OR (require_email_verified = TRUE AND email_claim = 'email'))" + "CHECK (auto_link_existing_accounts = FALSE OR email_claim != 'email' OR require_email_verified = TRUE)" ) ) except (OperationalError, ProgrammingError) as exc: @@ -1616,6 +1702,13 @@ async def run_migrations(conn): ) raise + # Migration: Update auto_link CHECK constraint formula (existing installs). + # Existing PostgreSQL installs that ran the ADD CONSTRAINT above with the old formula + # (or a previous version of this code) need an explicit DROP + ADD to update it. + # For SQLite, the table is recreated with the new constraint formula if the old formula + # is still present in sqlite_master (SQLite cannot ALTER TABLE DROP/ADD CONSTRAINT). + await _migrate_update_auto_link_constraint(conn) + # Migration: Add password_changed_at to users (M-R7-B) # Tracks the last time a user's password was changed/reset. JWTs whose iat # predates this timestamp are rejected in all six auth validation paths. diff --git a/backend/app/models/oidc_provider.py b/backend/app/models/oidc_provider.py index c84ca5170..140b1c2be 100644 --- a/backend/app/models/oidc_provider.py +++ b/backend/app/models/oidc_provider.py @@ -22,11 +22,11 @@ class OIDCProvider(Base): __tablename__ = "oidc_providers" __table_args__ = ( - # DB-level enforcement of SEC-1/SEC-6: auto_link is only safe when - # require_email_verified=True AND email_claim='email'. Enforced on new - # installations; existing tables get this via the PostgreSQL-only migration. + # DB-level enforcement of SEC-1: blocks only Fall B (email_claim='email' + require_ev=False). + # Fall C (custom claim) is safe — no email_verified gate on that path. + # Enforced on new installations; existing tables updated via _migrate_update_auto_link_constraint. CheckConstraint( - "auto_link_existing_accounts = FALSE OR (require_email_verified = TRUE AND email_claim = 'email')", + "auto_link_existing_accounts = FALSE OR email_claim != 'email' OR require_email_verified = TRUE", name="ck_auto_link_requires_verified_email_claim", ), ) diff --git a/backend/app/schemas/auth.py b/backend/app/schemas/auth.py index 591d4335d..e45067d68 100644 --- a/backend/app/schemas/auth.py +++ b/backend/app/schemas/auth.py @@ -297,7 +297,7 @@ class AdminDisable2FARequest(BaseModel): AUTO_LINK_REQUIREMENTS_ERROR = ( - "auto_link_existing_accounts requires require_email_verified=True and email_claim='email'" + "auto_link_existing_accounts requires require_email_verified=True when email_claim='email'" ) @@ -398,12 +398,12 @@ class OIDCProviderCreate(BaseModel): def validate_icon_url(cls, v: str | None) -> str | None: return _validate_icon_url(v) - # SEC-1 + SEC-6: auto_link requires both require_email_verified=True AND email_claim="email". - # Fall B (require_email_verified=False) accepts absent email_verified → account-takeover risk. - # Fall C (custom claim) skips email_verified entirely → same risk. + # SEC-1: auto_link with email_claim='email' requires require_email_verified=True. + # Fall B (require_email_verified=False + email_claim='email') accepts absent email_verified → account-takeover risk. + # Fall C (custom claim != 'email') is safe: no email_verified gate on that path regardless of require_email_verified. @model_validator(mode="after") def check_auto_link_requires_verified(self) -> "OIDCProviderCreate": - if self.auto_link_existing_accounts and (not self.require_email_verified or self.email_claim != "email"): + if self.auto_link_existing_accounts and self.email_claim == "email" and not self.require_email_verified: raise ValueError(AUTO_LINK_REQUIREMENTS_ERROR) return self @@ -444,13 +444,16 @@ class OIDCProviderUpdate(BaseModel): def validate_icon_url(cls, v: str | None) -> str | None: return _validate_icon_url(v) - # SEC-1 + SEC-6 (schema-level): blocks only when both conflicting fields arrive - # in the same request. Partial updates spanning two requests are caught by the + # SEC-1 (schema-level): blocks only when auto_link=True + email_claim='email' + require_email_verified=False + # arrive in the same request. email_claim=None means the request leaves it unchanged (still 'email' by default), + # so that is also treated as 'email'. Partial updates spanning two requests are caught by the # Combined-State-Guard in the route handler after the setattr loop. @model_validator(mode="after") def check_auto_link_requires_verified(self) -> "OIDCProviderUpdate": - if self.auto_link_existing_accounts is True and ( - self.require_email_verified is False or (self.email_claim is not None and self.email_claim != "email") + if ( + self.auto_link_existing_accounts is True + and self.require_email_verified is False + and (self.email_claim is None or self.email_claim == "email") ): raise ValueError(AUTO_LINK_REQUIREMENTS_ERROR) return self diff --git a/backend/tests/integration/test_mfa_api.py b/backend/tests/integration/test_mfa_api.py index e758cebf7..756c9dbb8 100644 --- a/backend/tests/integration/test_mfa_api.py +++ b/backend/tests/integration/test_mfa_api.py @@ -3522,8 +3522,12 @@ class TestOIDCEmailClaimResolution: @pytest.mark.asyncio @pytest.mark.integration - async def test_auto_link_blocked_with_custom_claim_create(self, async_client: AsyncClient): - """SEC-6: auto_link + email_claim!='email' must be rejected at schema level on CREATE (422).""" + async def test_auto_link_allowed_with_custom_claim_create(self, async_client: AsyncClient): + """Fall C: auto_link + email_claim!='email' must be accepted on CREATE (201). + + Custom claims (e.g. Azure preferred_username/upn) never perform an email_verified + check, so auto_link is safe regardless of require_email_verified. + """ admin_token = await _setup_and_login(async_client, "sec6c_adm", "Sec6CAdm123!") resp = await async_client.post( "/api/v1/auth/oidc/providers", @@ -3538,12 +3542,17 @@ class TestOIDCEmailClaimResolution: }, headers={"Authorization": f"Bearer {admin_token}"}, ) - assert resp.status_code == 422 + assert resp.status_code == 201 + assert resp.json()["auto_link_existing_accounts"] is True + assert resp.json()["email_claim"] == "upn" @pytest.mark.asyncio @pytest.mark.integration - async def test_auto_link_blocked_with_custom_claim_update(self, async_client: AsyncClient): - """SEC-6: auto_link=True + email_claim='upn' in same UPDATE request → 422 (T4).""" + async def test_auto_link_allowed_with_custom_claim_update(self, async_client: AsyncClient): + """Fall C: auto_link=True + email_claim='upn' in same UPDATE request → 200. + + Custom claims never perform an email_verified check, so auto_link is safe. + """ admin_token = await _setup_and_login(async_client, "sec6u_adm", "Sec6UAdm123!") create_resp = await async_client.post( "/api/v1/auth/oidc/providers", @@ -3563,7 +3572,9 @@ class TestOIDCEmailClaimResolution: json={"auto_link_existing_accounts": True, "email_claim": "upn"}, headers={"Authorization": f"Bearer {admin_token}"}, ) - assert resp.status_code == 422 + assert resp.status_code == 200 + assert resp.json()["auto_link_existing_accounts"] is True + assert resp.json()["email_claim"] == "upn" # ── Combined-State-Guard (partial updates across two requests) ───────────── @@ -3602,8 +3613,8 @@ class TestOIDCEmailClaimResolution: @pytest.mark.asyncio @pytest.mark.integration - async def test_partial_update_guard_email_claim(self, async_client: AsyncClient): - """SEC-6 Combined-State-Guard: email_claim='upn' then auto_link=True → 422 (T1 email_claim path).""" + async def test_partial_update_custom_claim_then_auto_link_allowed(self, async_client: AsyncClient): + """Fall C: email_claim='upn' first, then auto_link=True → both 200 (custom claim is safe).""" admin_token = await _setup_and_login(async_client, "pg_ec_adm", "PgEc123!") create_resp = await async_client.post( "/api/v1/auth/oidc/providers", @@ -3631,7 +3642,44 @@ class TestOIDCEmailClaimResolution: json={"auto_link_existing_accounts": True}, headers={"Authorization": f"Bearer {admin_token}"}, ) - assert upd2.status_code == 422 + assert upd2.status_code == 200 + assert upd2.json()["auto_link_existing_accounts"] is True + assert upd2.json()["email_claim"] == "upn" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_partial_update_auto_link_then_custom_claim_allowed(self, async_client: AsyncClient): + """Fall C: auto_link=True first (email_claim='email', safe), then email_claim='upn' → both 200.""" + admin_token = await _setup_and_login(async_client, "pg_al_ec_adm", "PgAlEc123!") + create_resp = await async_client.post( + "/api/v1/auth/oidc/providers", + json={ + "name": "PG-AutoLink-Claim-Test", + "issuer_url": "https://pg-al-ec.test", + "client_id": "pg-al-ec-client", + "client_secret": "sec", + "scopes": "openid email profile", + }, + headers={"Authorization": f"Bearer {admin_token}"}, + ) + assert create_resp.status_code == 201 + provider_id = create_resp.json()["id"] + + upd1 = await async_client.put( + f"/api/v1/auth/oidc/providers/{provider_id}", + json={"auto_link_existing_accounts": True}, + headers={"Authorization": f"Bearer {admin_token}"}, + ) + assert upd1.status_code == 200 + + upd2 = await async_client.put( + f"/api/v1/auth/oidc/providers/{provider_id}", + json={"email_claim": "preferred_username"}, + headers={"Authorization": f"Bearer {admin_token}"}, + ) + assert upd2.status_code == 200 + assert upd2.json()["auto_link_existing_accounts"] is True + assert upd2.json()["email_claim"] == "preferred_username" @pytest.mark.asyncio @pytest.mark.integration @@ -4009,7 +4057,11 @@ class TestOIDCEmailResolutionExtra: @pytest.mark.asyncio @pytest.mark.integration async def test_combined_state_guard_email_claim_inverse_order(self, async_client: AsyncClient): - """T4: Combined-State-Guard — set auto_link=True first, then switch email_claim to custom.""" + """Fall C: auto_link=True first, then switch email_claim to custom → both 200 (now allowed). + + Custom claims never perform an email_verified check, so switching to a custom claim + while auto_link is on transitions from Fall A to Fall C — both are safe. + """ admin_token = await _setup_and_login(async_client, "inv_ec_adm", "InvEc123!") create_resp = await async_client.post( "/api/v1/auth/oidc/providers", @@ -4025,7 +4077,7 @@ class TestOIDCEmailResolutionExtra: assert create_resp.status_code == 201 provider_id = create_resp.json()["id"] - # First: enable auto_link (safe — email_claim="email", require_ev=True) + # First: enable auto_link (Fall A — email_claim='email', require_ev=True) upd1 = await async_client.put( f"/api/v1/auth/oidc/providers/{provider_id}", json={"auto_link_existing_accounts": True}, @@ -4033,10 +4085,158 @@ class TestOIDCEmailResolutionExtra: ) assert upd1.status_code == 200 - # Second: switch email_claim to custom while auto_link is on → unsafe combined state + # Second: switch to custom claim → Fall C, still safe upd2 = await async_client.put( f"/api/v1/auth/oidc/providers/{provider_id}", json={"email_claim": "preferred_username"}, headers={"Authorization": f"Bearer {admin_token}"}, ) - assert upd2.status_code == 422 + assert upd2.status_code == 200 + assert upd2.json()["auto_link_existing_accounts"] is True + assert upd2.json()["email_claim"] == "preferred_username" + + +# =========================================================================== +# E2E: Fall C (custom email claim) auto-link actually links existing user +# =========================================================================== + + +class TestOIDCFallCAutoLinkE2E: + """OIDC callback with email_claim='preferred_username' (Fall C / Azure Entra ID) + must auto-link an existing local user when auto_link_existing_accounts=True. + + This test exercises _resolve_provider_email Fall C and the auto-link path in + oidc_callback — a regression in either would silently drop the link without + being caught by the configuration-layer tests. + """ + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_fall_c_auto_link_links_existing_user_via_callback( + self, async_client: AsyncClient, db_session: AsyncSession + ): + from unittest.mock import AsyncMock, MagicMock, patch + + from sqlalchemy import select as sa_select + + from backend.app.core.auth import get_password_hash + from backend.app.models.oidc_provider import OIDCProvider, UserOIDCLink + + issuer = "https://entra.fallc.example.com" + nonce = secrets.token_urlsafe(32) + code_verifier = secrets.token_urlsafe(48) + + # ── 1. Local user that should be linked ────────────────────────────── + alice = User( + username="fallc_alice", + email="alice.fallc@example.com", + password_hash=get_password_hash(secrets.token_urlsafe(16)), + role="user", + is_active=True, + ) + db_session.add(alice) + await db_session.flush() + + # ── 2. Provider: Fall C config (preferred_username, no email_verified) ─ + provider = OIDCProvider( + name="AzureEntraFallC", + issuer_url=issuer, + client_id="azure-client", + _client_secret_enc="azure-secret", + scopes="openid profile", + is_enabled=True, + auto_link_existing_accounts=True, + auto_create_users=False, + email_claim="preferred_username", + require_email_verified=False, + ) + db_session.add(provider) + await db_session.flush() + + # ── 3. OIDC state token ─────────────────────────────────────────────── + state = secrets.token_urlsafe(32) + db_session.add( + AuthEphemeralToken( + token=state, + token_type="oidc_state", + provider_id=provider.id, + nonce=nonce, + code_verifier=code_verifier, + expires_at=datetime.now(timezone.utc) + timedelta(minutes=10), + ) + ) + await db_session.commit() + + # ── 4. Mock HTTP + JWT ──────────────────────────────────────────────── + fake_discovery = { + "issuer": issuer, + "token_endpoint": f"{issuer}/token", + "jwks_uri": f"{issuer}/.well-known/jwks.json", + } + fake_token = {"access_token": "acc_tok", "id_token": "fake.id.token"} + # Fall C: preferred_username carries the email; no email_verified key at all + fake_claims = { + "sub": "azure-sub-alice", + "preferred_username": "alice.fallc@example.com", + "iss": issuer, + "aud": "azure-client", + "nonce": nonce, + "exp": 9_999_999_999, + } + + disc_resp = AsyncMock() + disc_resp.raise_for_status = MagicMock() + disc_resp.json = MagicMock(return_value=fake_discovery) + + token_resp = AsyncMock() + token_resp.json = MagicMock(return_value=fake_token) + + jwks_resp = AsyncMock() + jwks_resp.raise_for_status = MagicMock() + jwks_resp.json = MagicMock(return_value={}) + + mock_http = AsyncMock() + mock_http.get = AsyncMock(side_effect=[disc_resp, jwks_resp]) + mock_http.post = AsyncMock(return_value=token_resp) + + mock_signing_key = MagicMock() + mock_signing_key.key = "fake_key" + + with ( + patch("backend.app.api.routes.mfa.httpx.AsyncClient") as mock_httpx_cls, + patch("backend.app.api.routes.mfa.jwt.decode", return_value=fake_claims), + patch("backend.app.api.routes.mfa.PyJWKClient") as mock_jwks_cls, + ): + mock_httpx_cls.return_value.__aenter__ = AsyncMock(return_value=mock_http) + mock_httpx_cls.return_value.__aexit__ = AsyncMock(return_value=False) + mock_jwks_cls.return_value.get_signing_key_from_jwt.return_value = mock_signing_key + + callback_resp = await async_client.get( + f"/api/v1/auth/oidc/callback?code=fake_code&state={state}", + follow_redirects=False, + ) + + assert callback_resp.status_code == 302, callback_resp.text + location = callback_resp.headers.get("location", "") + assert "oidc_token=" in location, f"Expected oidc_token in redirect, got: {location}" + + # ── 5. Exchange token → full JWT ────────────────────────────────────── + oidc_exchange_token = location.split("oidc_token=")[1].split("&")[0].split("#")[-1] + exchange_resp = await async_client.post( + "/api/v1/auth/oidc/exchange", + json={"oidc_token": oidc_exchange_token}, + ) + assert exchange_resp.status_code == 200 + assert exchange_resp.json()["user"]["username"] == "fallc_alice" + + # ── 6. Verify UserOIDCLink was created in DB ────────────────────────── + async with db_session as s: + result = await s.execute( + sa_select(UserOIDCLink).where( + UserOIDCLink.user_id == alice.id, + UserOIDCLink.provider_id == provider.id, + ) + ) + link = result.scalar_one_or_none() + assert link is not None, "UserOIDCLink must have been created by auto-link" + assert link.provider_user_id == "azure-sub-alice" diff --git a/backend/tests/unit/test_db_dialect.py b/backend/tests/unit/test_db_dialect.py index cecd60c4d..11aeb194d 100644 --- a/backend/tests/unit/test_db_dialect.py +++ b/backend/tests/unit/test_db_dialect.py @@ -316,7 +316,12 @@ class TestSafeExecutePattern: @pytest.mark.asyncio async def test_check_constraint_false_true_on_sqlite(self): - """CheckConstraint with FALSE/TRUE literals is enforced on SQLite (3.23+).""" + """New constraint formula is enforced on SQLite (3.23+). + + New: auto_link = FALSE OR email_claim != 'email' OR require_ev = TRUE + Blocks Fall B (auto_link=1 + email_claim='email' + require_ev=0). + Allows Fall A (email_claim='email' + require_ev=1) and Fall C (custom claim). + """ from sqlalchemy import text from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import create_async_engine @@ -330,29 +335,32 @@ class TestSafeExecutePattern: auto_link BOOLEAN, require_ev BOOLEAN, email_claim TEXT, - CHECK (auto_link = FALSE OR (require_ev = TRUE AND email_claim = 'email')) + CHECK (auto_link = FALSE OR email_claim != 'email' OR require_ev = TRUE) ) """) ) - # Valid: auto_link=0 (FALSE) + # Valid: auto_link=0 (FALSE) — any combo allowed await conn.execute(text("INSERT INTO ck_test VALUES (1, 0, 0, 'upn')")) - # Valid: auto_link=1, require_ev=1, email_claim='email' + # Valid: Fall A — auto_link=1, require_ev=1, email_claim='email' await conn.execute(text("INSERT INTO ck_test VALUES (2, 1, 1, 'email')")) + # Valid: Fall C — auto_link=1, email_claim='upn' (require_ev irrelevant) + await conn.execute(text("INSERT INTO ck_test VALUES (3, 1, 0, 'upn')")) + await conn.execute(text("INSERT INTO ck_test VALUES (4, 1, 1, 'upn')")) async with engine.begin() as conn: - # Invalid: auto_link=1 but conditions not met + # Invalid: Fall B — auto_link=1 + email_claim='email' + require_ev=0 with pytest.raises(IntegrityError): - await conn.execute(text("INSERT INTO ck_test VALUES (3, 1, 0, 'email')")) + await conn.execute(text("INSERT INTO ck_test VALUES (5, 1, 0, 'email')")) await engine.dispose() @pytest.mark.asyncio async def test_auto_link_sec1_backfill_resets_unsafe_rows(self): - """SEC-1 backfill resets auto_link=TRUE on rows with unsafe combined state. + """SEC-1 backfill resets auto_link=TRUE only for Fall B (email_claim='email' + require_ev=FALSE). Three cases: - 1. auto_link=TRUE + require_ev=FALSE → reset to FALSE (unsafe: permissive mode) - 2. auto_link=TRUE + custom claim → reset to FALSE (unsafe: no email_verified gate) - 3. auto_link=TRUE + require_ev=TRUE + standard claim → unchanged (safe) + 1. auto_link=TRUE + email_claim='email' + require_ev=FALSE → reset to FALSE (Fall B, unsafe) + 2. auto_link=TRUE + custom claim + require_ev=TRUE → unchanged (Fall C, now allowed) + 3. auto_link=TRUE + email_claim='email' + require_ev=TRUE → unchanged (Fall A, safe) """ from sqlalchemy import text from sqlalchemy.ext.asyncio import create_async_engine @@ -369,11 +377,11 @@ class TestSafeExecutePattern: ")" ) ) - # Row 1: unsafe — require_ev=FALSE + # Row 1: Fall B — email_claim='email' + require_ev=FALSE → must be reset await conn.execute(text("INSERT INTO oidc_providers VALUES (1, 1, 0, 'email')")) - # Row 2: unsafe — custom claim + # Row 2: Fall C — custom claim → must NOT be reset (now allowed) await conn.execute(text("INSERT INTO oidc_providers VALUES (2, 1, 1, 'preferred_username')")) - # Row 3: safe — require_ev=TRUE + standard claim + # Row 3: Fall A — email_claim='email' + require_ev=TRUE → must NOT be reset (always safe) await conn.execute(text("INSERT INTO oidc_providers VALUES (3, 1, 1, 'email')")) async with conn.begin_nested(): @@ -381,7 +389,7 @@ class TestSafeExecutePattern: text( "UPDATE oidc_providers SET auto_link_existing_accounts = FALSE " "WHERE auto_link_existing_accounts = TRUE " - "AND (require_email_verified = FALSE OR email_claim != 'email')" + "AND email_claim = 'email' AND require_email_verified = FALSE" ) ) @@ -389,9 +397,9 @@ class TestSafeExecutePattern: rows = {r[0]: r[1] for r in result.fetchall()} await engine.dispose() - assert rows[1] == 0, "unsafe (require_ev=FALSE) row must be reset to FALSE" - assert rows[2] == 0, "unsafe (custom claim) row must be reset to FALSE" - assert rows[3] == 1, "safe row must remain TRUE" + assert rows[1] == 0, "Fall B (require_ev=FALSE) must be reset to FALSE" + assert rows[2] == 1, "Fall C (custom claim) must remain TRUE" + assert rows[3] == 1, "Fall A (require_ev=TRUE) must remain TRUE" @pytest.mark.asyncio async def test_safe_execute_reraises_does_not_exist_without_column(self): @@ -534,3 +542,213 @@ class TestSafeExecutePattern: sql = mock_conn.execute.call_args[0][0].text assert "::text = '[]'" in sql, f"Expected ::text cast in SQL, got: {sql}" assert "printer_ids" in sql + + +class TestAutoLinkConstraintMigration: + """Tests for _migrate_update_auto_link_constraint (Fall C / Azure support).""" + + @pytest.mark.asyncio + async def test_new_constraint_allows_fall_c_sqlite(self): + """New formula allows auto_link=TRUE with a custom claim (Fall C).""" + from sqlalchemy import text + from sqlalchemy.exc import IntegrityError + from sqlalchemy.ext.asyncio import create_async_engine + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.begin() as conn: + await conn.execute( + text( + "CREATE TABLE oidc_providers_ck (" + "id INTEGER PRIMARY KEY, " + "auto_link BOOLEAN, " + "require_ev BOOLEAN, " + "email_claim TEXT, " + "CHECK (auto_link = FALSE OR email_claim != 'email' OR require_ev = TRUE)" + ")" + ) + ) + # Fall C: custom claim + auto_link + require_ev=FALSE must pass + await conn.execute(text("INSERT INTO oidc_providers_ck VALUES (1, 1, 0, 'upn')")) + # Fall C: custom claim + auto_link + require_ev=TRUE must pass + await conn.execute(text("INSERT INTO oidc_providers_ck VALUES (2, 1, 1, 'preferred_username')")) + await engine.dispose() + + @pytest.mark.asyncio + async def test_new_constraint_blocks_fall_b_sqlite(self): + """New formula still blocks Fall B (email_claim='email' + require_ev=FALSE + auto_link=TRUE).""" + from sqlalchemy import text + from sqlalchemy.exc import IntegrityError + from sqlalchemy.ext.asyncio import create_async_engine + + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.begin() as conn: + await conn.execute( + text( + "CREATE TABLE oidc_providers_ck (" + "id INTEGER PRIMARY KEY, " + "auto_link BOOLEAN, " + "require_ev BOOLEAN, " + "email_claim TEXT, " + "CHECK (auto_link = FALSE OR email_claim != 'email' OR require_ev = TRUE)" + ")" + ) + ) + async with engine.begin() as conn: + with pytest.raises(IntegrityError): + await conn.execute(text("INSERT INTO oidc_providers_ck VALUES (1, 1, 0, 'email')")) + await engine.dispose() + + @pytest.mark.asyncio + async def test_constraint_migration_sqlite_recreates_table(self): + """SQLite path recreates oidc_providers with new constraint when old formula is present.""" + from sqlalchemy import text + from sqlalchemy.ext.asyncio import create_async_engine + + from backend.app.core.database import _migrate_update_auto_link_constraint + + # Create table with old constraint formula + engine = create_async_engine("sqlite+aiosqlite:///:memory:") + async with engine.begin() as conn: + await conn.execute( + text( + "CREATE TABLE oidc_providers (" + "id INTEGER NOT NULL PRIMARY KEY, " + "name VARCHAR(100) NOT NULL UNIQUE, " + "issuer_url VARCHAR(500) NOT NULL, " + "client_id VARCHAR(255) NOT NULL, " + "client_secret VARCHAR(512) NOT NULL, " + "scopes VARCHAR(500), " + "is_enabled BOOLEAN, " + "auto_create_users BOOLEAN, " + "auto_link_existing_accounts BOOLEAN DEFAULT 0, " + "email_claim VARCHAR(64) DEFAULT 'email', " + "require_email_verified BOOLEAN DEFAULT 1, " + "icon_url TEXT, " + "created_at DATETIME DEFAULT CURRENT_TIMESTAMP, " + "updated_at DATETIME DEFAULT CURRENT_TIMESTAMP, " + "CONSTRAINT ck_auto_link_requires_verified_email_claim " + "CHECK (auto_link_existing_accounts = FALSE OR " + "(require_email_verified = TRUE AND email_claim = 'email'))" + ")" + ) + ) + await conn.execute( + text( + "INSERT INTO oidc_providers (id, name, issuer_url, client_id, client_secret, " + "scopes, is_enabled, auto_create_users, auto_link_existing_accounts, " + "email_claim, require_email_verified, icon_url, created_at, updated_at) " + "VALUES (1, 'TestIdP', 'https://idp.test', 'cid', 'secret', " + "'openid email', 1, 0, 0, 'email', 1, NULL, " + "CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" + ) + ) + + async with engine.begin() as conn: + with patch("backend.app.core.database.is_sqlite", return_value=True): + await _migrate_update_auto_link_constraint(conn) + + # Verify data survived + result = await conn.execute(text("SELECT id, name FROM oidc_providers")) + rows = result.fetchall() + assert len(rows) == 1 + assert rows[0][0] == 1 + + # Verify new constraint: Fall C (auto_link=TRUE + custom claim) must now be insertable + await conn.execute( + text( + "INSERT INTO oidc_providers (id, name, issuer_url, client_id, client_secret, " + "scopes, is_enabled, auto_create_users, auto_link_existing_accounts, " + "email_claim, require_email_verified, icon_url, created_at, updated_at) " + "VALUES (2, 'AzureIdP', 'https://azure.test', 'cid2', 'secret', " + "'openid', 1, 0, 1, 'upn', 1, NULL, " + "CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" + ) + ) + + # Verify schema has new formula + schema = ( + await conn.execute(text("SELECT sql FROM sqlite_master WHERE type='table' AND name='oidc_providers'")) + ).fetchone()[0] + assert "require_email_verified = TRUE AND email_claim = 'email'" not in schema + assert "email_claim != 'email'" in schema + + await engine.dispose() + + @pytest.mark.asyncio + async def test_constraint_migration_postgres_drops_and_recreates(self): + """PostgreSQL path calls DROP CONSTRAINT IF EXISTS then ADD CONSTRAINT with new formula.""" + from unittest.mock import AsyncMock, MagicMock, call + + from backend.app.core.database import _migrate_update_auto_link_constraint + + # Track all SQL statements passed to _safe_execute by capturing conn.execute calls + executed_sqls: list[str] = [] + + async def fake_safe_execute(conn, sql): + executed_sqls.append(sql) + + nested_cm = MagicMock() + nested_cm.__aenter__ = AsyncMock(return_value=nested_cm) + nested_cm.__aexit__ = AsyncMock(return_value=False) + nested_cm.execute = AsyncMock() + + mock_conn = MagicMock() + mock_conn.begin_nested.return_value = nested_cm + mock_conn.execute = AsyncMock() + + with ( + patch("backend.app.core.database.is_sqlite", return_value=False), + patch("backend.app.core.database._safe_execute", side_effect=fake_safe_execute), + ): + await _migrate_update_auto_link_constraint(mock_conn) + + assert len(executed_sqls) == 2 + drop_sql, add_sql = executed_sqls + assert "DROP CONSTRAINT IF EXISTS" in drop_sql.upper() + assert "ck_auto_link_requires_verified_email_claim" in drop_sql + assert "ADD CONSTRAINT" in add_sql.upper() + assert "email_claim != 'email'" in add_sql + assert "require_email_verified = TRUE AND email_claim = 'email'" not in add_sql + + @pytest.mark.asyncio + async def test_constraint_migration_sqlite_count_guard_raises_on_mismatch(self): + """RuntimeError is raised when the copied row count doesn't match the source.""" + from unittest.mock import AsyncMock, MagicMock, patch + + import pytest + + from backend.app.core.database import _migrate_update_auto_link_constraint + + _OLD_SQL = ( + "CREATE TABLE oidc_providers (id INTEGER NOT NULL, " + "CONSTRAINT ck_auto_link_requires_verified_email_claim " + "CHECK (auto_link_existing_accounts = FALSE OR " + "(require_email_verified = TRUE AND email_claim = 'email')))" + ) + + async def fake_execute(stmt): + sql = str(stmt) + result = MagicMock() + if "sqlite_master" in sql: + result.fetchone.return_value = (_OLD_SQL,) + elif "count(*)" in sql.lower() and "oidc_providers_v2" not in sql: + result.scalar_one.return_value = 2 # source has 2 rows + elif "count(*)" in sql.lower() and "oidc_providers_v2" in sql: + result.scalar_one.return_value = 1 # copy only has 1 — mismatch + else: + result.fetchone.return_value = None + return result + + nested_cm = MagicMock() + nested_cm.__aenter__ = AsyncMock(return_value=None) + nested_cm.__aexit__ = AsyncMock(return_value=False) # don't suppress exceptions + + mock_conn = MagicMock() + mock_conn.execute = AsyncMock(side_effect=fake_execute) + mock_conn.begin_nested.return_value = nested_cm + + with ( + patch("backend.app.core.database.is_sqlite", return_value=True), + pytest.raises(RuntimeError, match="mismatch"), + ): + await _migrate_update_auto_link_constraint(mock_conn) diff --git a/frontend/scripts/check-i18n-parity.mjs b/frontend/scripts/check-i18n-parity.mjs index 73447c93c..4b4c480dd 100644 --- a/frontend/scripts/check-i18n-parity.mjs +++ b/frontend/scripts/check-i18n-parity.mjs @@ -207,20 +207,7 @@ if (isMainModule) { const infoReports = reports.filter((r) => !strictSet.has(codeOf(r.label))); printReports(strictReports, '=== STRICT locales (failures below fail CI) ==='); - // Informational locales: show per-category drift counts only, not the - // full key lists — the leaf-count table below already gives the overall - // picture. Flip VERBOSE_INFO=1 to dump the full missing-key/placeholder - // reports when actually working on translations. - if (infoReports.length) { - if (process.env.VERBOSE_INFO === '1') { - printReports(infoReports, '=== INFORMATIONAL locales (drift shown, does not fail CI) ==='); - } else { - console.error('\n=== INFORMATIONAL locales (drift summary; VERBOSE_INFO=1 for detail) ==='); - for (const { label, items } of infoReports) { - console.error(` ${label}: ${items.length}`); - } - } - } + printReports(infoReports, '=== INFORMATIONAL locales (drift shown, does not fail CI) ==='); console.log('\nLocale leaf counts:'); for (const [code, map] of Object.entries(locales)) { diff --git a/frontend/src/__tests__/components/OIDCProviderSettings.test.tsx b/frontend/src/__tests__/components/OIDCProviderSettings.test.tsx index 05c989fd3..38c508879 100644 --- a/frontend/src/__tests__/components/OIDCProviderSettings.test.tsx +++ b/frontend/src/__tests__/components/OIDCProviderSettings.test.tsx @@ -4,7 +4,7 @@ */ import { describe, it, expect, beforeEach } from 'vitest'; -import { screen, waitFor } from '@testing-library/react'; +import { screen, waitFor, fireEvent } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { render } from '../utils'; import { OIDCProviderSettings } from '../../components/OIDCProviderSettings'; @@ -107,6 +107,33 @@ describe('OIDCProviderSettings', () => { ).toBeInTheDocument(); }); }); + + it('shows security warning when auto_link is enabled with a custom email claim', async () => { + server.use(http.get('/api/v1/auth/oidc/providers/all', () => HttpResponse.json([]))); + const user = userEvent.setup(); + render(); + + await waitFor(() => { + expect(screen.getAllByRole('button', { name: /Add Provider/i })[0]).toBeInTheDocument(); + }); + await user.click(screen.getAllByRole('button', { name: /Add Provider/i })[0]); + + await waitFor(() => { + expect(screen.getByText(/Auto.*Link/i)).toBeInTheDocument(); + }); + + // Enable auto_link (switch index 2) + const autoLinkSwitch = screen.getAllByRole('switch')[2]; + await user.click(autoLinkSwitch); + + // Change email claim to a custom value via fireEvent to bypass the onChange fallback + const emailClaimInput = screen.getByPlaceholderText('email'); + fireEvent.change(emailClaimInput, { target: { value: 'preferred_username' } }); + + await waitFor(() => { + expect(screen.getByText(/tenant-administered/i)).toBeInTheDocument(); + }); + }); }); describe('Provider info view', () => { diff --git a/frontend/src/components/OIDCProviderSettings.tsx b/frontend/src/components/OIDCProviderSettings.tsx index b35c74be5..d4f8e1658 100644 --- a/frontend/src/components/OIDCProviderSettings.tsx +++ b/frontend/src/components/OIDCProviderSettings.tsx @@ -123,14 +123,14 @@ function ProviderForm({

{t('settings.oidc.form.autoCreateDesc')}

-