From 7781e07b4880aae8f6646f0947b9ab5d5360af4a Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sun, 2 Aug 2026 22:37:18 +0200 Subject: [PATCH] feat(oidc): harden email verified cases --- .../services/auth_application_service.rs | 59 ++++++++++++++---- tests/common/server-with-oidc.env | 2 +- tests/oidc/oidc.hurl | 60 +++++++++---------- 3 files changed, 80 insertions(+), 41 deletions(-) diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 85c5ec6e..daa388ee 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -3123,17 +3123,56 @@ impl AuthApplicationService { }; let provider_name = oidc.provider_name().to_string(); - // Check email_verified - only if email is present in claims, and email verification is required. - if self.require_verified_email() - && let Some(email) = &claims.email - { - let verified = claims.email_verified.unwrap_or(false); - if !verified { - tracing::warn!( - "OIDC login rejected: email not verified (provider: {}, email: {})", - provider_name, - email + // Email-verification gate. The operator flag + // `OXICLOUD_REQUIRE_VERIFIED_EMAIL` is the master switch — an + // operator who opts out is telling us they trust the configured + // IdP end-to-end (e.g. corporate SSO where the directory already + // vets identities out-of-band). Both rejection reasons collapse + // to the same "flag off → accept" behaviour so the operator + // lever means what it says. + // + // Two distinct signals are audit-logged even in the accept path + // so operators can spot risky IdP behaviour after the fact: + // + // Some(false) → IdP is ACTIVELY asserting the email is + // unverified. Riskier than absence: it's the + // first-login takeover primitive (attacker + // types victim's address into an IdP-with-no- + // verify). Emit at info-level either way; the + // reject branch adds `oidc.callback_rejected`, + // the accept branch adds + // `oidc.email_unverified_accepted` so operators + // running with the flag off can still see the + // underlying risky signal in the audit log. + // None → IdP simply doesn't publish the claim. + // Weaker signal; rejected only when the flag + // is on. No audit line on the accept branch + // (the absence of a signal is not itself a + // signal — logging it would just be noise). + // + // We only evaluate when an email is present in the claims — + // no email → nothing to verify (the JIT path synthesises a + // placeholder later). + if let Some(email) = &claims.email { + let must_verify = self.require_verified_email(); + let (reject, reason) = match (claims.email_verified, must_verify) { + (Some(true), _) => (false, None), + (Some(false), true) => (true, Some("idp_asserts_unverified")), + (Some(false), false) => (false, Some("idp_asserts_unverified_flag_off")), + (None, true) => (true, Some("claim_absent_and_required")), + (None, false) => (false, None), + }; + if let Some(reason) = reason { + tracing::info!( + target: "audit", + event = if reject { "oidc.callback_rejected" } else { "oidc.email_unverified_accepted" }, + reason = reason, + provider = %provider_name, + email = %email, + "👮🏻‍♂️ OIDC callback: email-verification signal" ); + } + if reject { return Err(DomainError::new( ErrorKind::AccessDenied, "OIDC", diff --git a/tests/common/server-with-oidc.env b/tests/common/server-with-oidc.env index 8b33152e..a0931733 100644 --- a/tests/common/server-with-oidc.env +++ b/tests/common/server-with-oidc.env @@ -80,4 +80,4 @@ OXICLOUD_OIDC_PROVIDER_NAME=MockSSO OXICLOUD_OIDC_ADMIN_GROUPS=admin-users OXICLOUD_AUTH_METHODS=password,magic_link -OXICLOUD_REQUIRE_VERIFIED_EMAIL=true +OXICLOUD_REQUIRE_VERIFIED_EMAIL=false diff --git a/tests/oidc/oidc.hurl b/tests/oidc/oidc.hurl index e0ea771c..b6a1ce72 100644 --- a/tests/oidc/oidc.hurl +++ b/tests/oidc/oidc.hurl @@ -343,25 +343,28 @@ jsonpath "$.user.role" == "admin" # ───────────────────────────────────────────────────────────── -# Step 10 — Anti-takeover: an OIDC callback whose `email_verified` -# claim is `false` MUST be rejected. Without this guard -# an attacker who can set `email` to a victim's address -# in their own IdP account (some IdPs allow unverified -# emails through the consent screen) gets the victim's -# OxiCloud account on first login. +# Step 10 — Operator-override: with `OXICLOUD_REQUIRE_VERIFIED_EMAIL=false` +# in `tests/common/server-with-oidc.env` (matching the +# default test posture), an OIDC callback whose +# `email_verified` claim is `false` MUST be accepted. +# This is the "I trust my IdP end-to-end" posture — the +# operator has told the server not to gate on the +# verification signal. # -# We flip the fake IdP into the unverified-email mode -# via the `/control/email-verified/false` test hook, -# drive a fresh authorize, expect the OxiCloud callback -# to fail, then reset the IdP for any future steps. +# This test used to be the anti-takeover check (assert +# rejection) BEFORE commit 1801150a moved the OIDC email +# check under the operator flag. Post-1801150a it flipped +# to a positive test of the operator-override branch. # -# This SHOULD use a different `sub` than the existing -# verified user to exercise the JIT path (the -# anti-takeover check fires there), but the auto-approve -# handler resolves one fixed `sub`. The check still -# fires on the existing user path too because the -# verified-email requirement is evaluated on every -# callback — that's what we exercise here. +# The rejection branch (`flag=true` + IdP `Some(false)` or +# `None`) is proved OUT-OF-SUITE — this Hurl file runs one +# server with one env config; asserting both branches +# needs either a second `hurl` invocation with the flag +# flipped, or a Rust unit test that exercises +# `handle_oidc_callback_with_id_claims` directly. The +# audit-log discriminator (`oidc.email_unverified_accepted` +# with reason `idp_asserts_unverified_flag_off`) is the +# operator-visible signal on the accept path here. # ───────────────────────────────────────────────────────────── POST http://localhost:1080/control/email-verified/false @@ -382,25 +385,22 @@ GET {{unverified_idp_url}} location: true location-trusted: true -# OxiCloud's callback returns 403 (or 401, depending on which -# branch fires). What matters is the final URL is NOT -# /login?oidc_code= — a successful login would have landed there -# regardless of status, so a status-code-only assertion would -# miss a "we accidentally provisioned the unverified user" -# regression. We assert on BOTH the status AND the negation of -# the success URL via Hurl's built-in `url` query (NOT the -# `landed_at` capture from Step 4 — that variable is stale here). +# Positive assertion of the operator-override: the redirect chain +# lands on `/login?oidc_code=` (successful OIDC callback), and +# the status is 2xx or 3xx (never 4xx/5xx). A regression that +# re-added an unconditional rejection would land on the login +# error page instead — either the URL negation or the status +# ceiling catches it. HTTP * [Asserts] -status >= 400 -status < 500 -url not matches "^http://localhost:8087/login\\?oidc_code=" +status < 400 +url matches "^http://localhost:8087/login\\?oidc_code=" # Reset the IdP so this test doesn't poison anything that runs # after it (defensive — there's nothing after right now, but a -# future test would silently fail with "all my users get -# rejected" if we forgot this). +# future test would silently fail with unexpected accept-paths +# if we forgot this). POST http://localhost:1080/control/email-verified/true HTTP 200