From d8b3f2e026a1544f6d21617e1b83ddabdf4c9bbd Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Sat, 8 Aug 2026 15:24:06 +0200 Subject: [PATCH] refactor(oidc): migrate provider into issuer this make OIDC compliant with the invariant binding (issuer and subject) admin can now rename their provider without breaking clarifing federation_kind: report the kind of federation wired not the allowed login method hybryd login method are still allowed --- docs/plan/ocm.md | 57 +++++++++++++ examples/bench_round20_micro.rs | 8 +- frontend/src/lib/api/types.ts | 43 +++++++--- .../src/routes/admin/[[tab]]/+page.svelte | 8 +- frontend/src/routes/profile/+page.svelte | 4 +- src/application/dtos/user_dto.rs | 68 +++++++++++++-- src/application/ports/auth_ports.rs | 34 ++++++++ .../services/auth_application_service.rs | 83 +++++++++++++++---- src/domain/repositories/user_repository.rs | 1 + .../repositories/pg/user_pg_repository.rs | 39 ++++++++- src/infrastructure/services/oidc_service.rs | 47 +++++++++++ src/interfaces/api/handlers/auth_handler.rs | 24 ++++++ src/interfaces/nextcloud/ocs_handler.rs | 10 ++- tests/oidc/oidc.hurl | 15 ++-- tools/perf-audit/admin_user_listing_e2e.rs | 10 +-- 15 files changed, 390 insertions(+), 61 deletions(-) diff --git a/docs/plan/ocm.md b/docs/plan/ocm.md index 02b17a85..d4befb1e 100644 --- a/docs/plan/ocm.md +++ b/docs/plan/ocm.md @@ -441,6 +441,63 @@ churn): - OCM notification handler translates peer messages into local state transitions (create/delete grant, update state column). +## Future — multi-federation per user (account linking) + +The current schema is strictly 1:1 — one `auth.users` row carries ONE +`(federation_kind, federation_issuer, federation_subject)` triple. +Scenarios where this breaks: + +- **Multi-IdP link** — Alice logs in via Google today, later wants to + also link her corporate Keycloak (same person, two auth paths). +- **OIDC ↔ OCM identity overlap** — Bob has a local OIDC-provisioned + account AND colleagues share to him via OCM at + `bob@theircloud.example.com`. Different trust chains → two rows. + His "shared with me" view splits across both identities. +- **IdP migration** — Company switches SSO issuer; the old row is + orphaned, new logins mint a fresh row. All grants belong to the + ghost. +- **Genuine multi-account** — some IdPs let one person hold multiple + accounts (different `sub`). Different identities per OIDC spec — + CORRECTLY two users. This case wants NO merging. + +The interesting split: (1)-(3) benefit from merging; (4) must stay +separate. Merging is inherently user-driven, not automatic (auto-link +would misbehave on case 4). Ship OCM WITHOUT this and add it when a +real user asks — every product I've seen shipped account linking as a +distinct feature (Nextcloud, GitHub) precisely so it can have its own +UX design. + +Evolution path (documented so it's not lost, NOT for OCM MVP): + +```sql +CREATE TABLE auth.user_federations ( + id UUID PRIMARY KEY DEFAULT gen_random_uuid(), + user_id UUID NOT NULL REFERENCES auth.users(id) ON DELETE CASCADE, + federation_kind TEXT NOT NULL, + federation_issuer TEXT NOT NULL, + federation_subject TEXT NOT NULL, + linked_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + linked_by UUID REFERENCES auth.users(id), + is_primary BOOLEAN NOT NULL DEFAULT FALSE, + UNIQUE (federation_kind, federation_issuer, federation_subject), + UNIQUE (user_id, is_primary) DEFERRABLE +); +``` + +Migration: one row per existing `auth.users` with `federation_kind +IS NOT NULL`, `is_primary=true`, `linked_by=user_id` (self-owned). +Then drop the three columns from `auth.users` after a backward-compat +window (view unions the two representations). + +Impact on existing code: anti-duplicate lookup, BCL revocation, JIT +provisioning, lazy rebind — all move to JOINs on `user_federations`. +The `get_user_by_federation_subject` repo method is the single +touchpoint; downstream callers don't change. + +Ships when demand appears — probably alongside a "Connect another +account" settings page. Documented here so the OCM 1:1 shape is a +KNOWN LIMITATION, not an oversight. + ## Future — merge with a generic grant-request workflow `ocm.inbound_shares.state` is a purpose-specific state machine for the diff --git a/examples/bench_round20_micro.rs b/examples/bench_round20_micro.rs index 39d8ce22..7485d58b 100644 --- a/examples/bench_round20_micro.rs +++ b/examples/bench_round20_micro.rs @@ -255,7 +255,7 @@ struct BenchUserDto { id: String, username: Option, email: String, - auth_provider: String, + federation_issuer: String, image: Option, can_edit_image: bool, given_name: Option, @@ -271,7 +271,7 @@ fn a2_before(user: &BenchUser) -> BenchUserDto { id: user.id.to_string(), username: user.username.as_deref().map(str::to_string), email: user.email.clone(), - auth_provider: user.oidc_provider.as_deref().unwrap_or("local").to_string(), + federation_issuer: user.oidc_provider.as_deref().unwrap_or("local").to_string(), image: user.image.as_deref().map(|s| s.to_string()), can_edit_image: user.oidc_provider.is_none(), given_name: user.given_name.as_deref().map(str::to_string), @@ -288,7 +288,7 @@ fn a2_after(user: BenchUser) -> BenchUserDto { id: user.id.to_string(), username: user.username, email: user.email, - auth_provider: user.oidc_provider.unwrap_or_else(|| "local".to_string()), + federation_issuer: user.oidc_provider.unwrap_or_else(|| "local".to_string()), image: user.image, can_edit_image, given_name: user.given_name, @@ -321,7 +321,7 @@ fn section_a2() { let a = a2_after(user.clone()); assert_eq!(b.image, a.image, "A2 image differs"); assert_eq!(b.email, a.email, "A2 email differs"); - assert_eq!(b.auth_provider, a.auth_provider, "A2 auth_provider differs"); + assert_eq!(b.federation_issuer, a.federation_issuer, "A2 federation_issuer differs"); assert_eq!( b.can_edit_image, a.can_edit_image, "A2 can_edit_image differs" diff --git a/frontend/src/lib/api/types.ts b/frontend/src/lib/api/types.ts index 041f48de..f8cf8aed 100644 --- a/frontend/src/lib/api/types.ts +++ b/frontend/src/lib/api/types.ts @@ -191,7 +191,24 @@ export interface User { updated_at: string; last_login_at?: string | null; active: boolean; - auth_provider: string; + /** + * Which trust chain minted this user's federation identity. `null` + * (omitted from wire) for local users (password / OPAQUE only). + * `"oidc" | "ocm" | "magic_link"` for federated users. Predicate: + * `!user.federation_kind` = local; `user.federation_kind === 'oidc'` + * = OIDC user. Mirrors `auth.users.federation_kind` verbatim. + */ + federation_kind?: 'oidc' | 'ocm' | 'magic_link'; + /** + * Authority that minted this user's OIDC/OCM identity — issuer URL + * for OIDC (id_token `iss`), peer domain for OCM. `null` (omitted) + * for local users. FE that wants a friendly display label maps this + * against `OidcProviders.issuer → provider_name` when they match; + * shows the raw value otherwise. Renamed from the historical + * `auth_provider` (which held a display label pre-Phase-B and a + * `"local"` sentinel for non-federated users — both are gone). + */ + federation_issuer?: string; image?: string | null; can_edit_image: boolean; is_external: boolean; @@ -229,13 +246,14 @@ export interface User { force_password_change?: boolean; /** * TRUE when the account has a local Argon2id `password_hash` on - * file. Distinct from `auth_provider`: an SSO-linked account can - * ALSO carry a local password (hybrid posture — SSO for daily - * login, local password as fallback). The profile page's - * change-password card gates on this flag rather than on - * `auth_provider === 'local'` so hybrid users can rotate their - * local credential. Optional on the wire for older-backend - * compatibility; missing → `false` (safe default: hide the card). + * file. Distinct from `federation_kind`: an OIDC-linked account + * (`federation_kind === 'oidc'`) can ALSO carry a local password + * (hybrid posture — SSO for daily login, local password as + * fallback). The profile page's change-password card gates on this + * flag rather than on the federation shape so hybrid users can + * rotate their local credential. Optional on the wire for older- + * backend compatibility; missing → `false` (safe default: hide the + * card). */ has_password?: boolean; } @@ -260,15 +278,16 @@ export type AdminUserSummary = Pick< | 'storage_used_bytes' | 'last_login_at' | 'active' - | 'auth_provider' + | 'federation_kind' + | 'federation_issuer' | 'is_external' > & { /** TRUE = user has a server-verifiable password on file (legacy or - * admin-set). Combined with `opaque_registered` and `auth_provider`, + * admin-set). Combined with `opaque_registered` and `federation_kind`, * the admin table derives the full auth capability set — a user with * `has_password=false`, `opaque_registered=false` AND - * `auth_provider === 'local'` is passwordless (magic-link only, - * which is the default for externals). */ + * `federation_kind === undefined` (no federation) is passwordless + * (magic-link only, which is the default for externals). */ has_password?: boolean; /** TRUE = user has an OPAQUE envelope on file (Phase 2 silent migration * succeeded, or the user completed a manual re-registration). */ diff --git a/frontend/src/routes/admin/[[tab]]/+page.svelte b/frontend/src/routes/admin/[[tab]]/+page.svelte index 79b704a3..837c631f 100644 --- a/frontend/src/routes/admin/[[tab]]/+page.svelte +++ b/frontend/src/routes/admin/[[tab]]/+page.svelte @@ -938,7 +938,7 @@ } /** OIDC/SSO-provisioned account (no local password to reset). */ function isOidcUser(u: AdminUserSummary): boolean { - return !!u.auth_provider && u.auth_provider !== 'local'; + return u.federation_kind === 'oidc'; } /** Used-quota percentage (0 when unlimited) for the per-user progress bar. */ function quotaPct(u: AdminUserSummary): number { @@ -2667,7 +2667,7 @@ Auth-capability chip set — ADMIN-ONLY (fields scoped to `AdminUserSummaryDto`; never on `UserDto`). Any user carries ZERO OR MORE of: - * SSO/OIDC — `auth_provider !== 'local'`, + * SSO/OIDC — `federation_kind === 'oidc'`, identity delegated to the IdP; label is the provider name. * password — `has_password` — server has a @@ -2684,9 +2684,9 @@ awaiting their welcome magic-link. --> {#if isOidcUser(u)} - + - {u.auth_provider} + {u.federation_issuer} {/if} {#if u.has_password} diff --git a/frontend/src/routes/profile/+page.svelte b/frontend/src/routes/profile/+page.svelte index 9fb24228..6ca6eb3a 100644 --- a/frontend/src/routes/profile/+page.svelte +++ b/frontend/src/routes/profile/+page.svelte @@ -66,8 +66,8 @@ let creatingPw = $state(false); let autoExpanded = $state(false); - const isOidc = $derived(!!session.user?.auth_provider && session.user.auth_provider !== 'local'); - const isLocal = $derived(!isOidc); + const isOidc = $derived(session.user?.federation_kind === 'oidc'); + const isLocal = $derived(!session.user?.federation_kind); const usernameClaimed = $derived(!!session.user?.username); const isAdmin = $derived(session.user?.role === 'admin'); const canEditImage = $derived(session.user?.can_edit_image === true && isLocal); diff --git a/src/application/dtos/user_dto.rs b/src/application/dtos/user_dto.rs index f121d615..cb975d7d 100644 --- a/src/application/dtos/user_dto.rs +++ b/src/application/dtos/user_dto.rs @@ -25,7 +25,36 @@ pub struct UserDto { pub updated_at: DateTime, pub last_login_at: Option>, pub active: bool, - pub auth_provider: String, + /// Which trust chain minted this user's federation identity — + /// `"oidc" | "ocm" | "magic_link"` — or `None` for pure local + /// users. Load-bearing for "is this user OIDC?"-shape predicates: + /// use `federation_kind == "oidc"` rather than string-scraping + /// `federation_issuer`. Serialized only when populated. + /// + /// Mirrors `auth.users.federation_kind` verbatim — same name at + /// DB, entity, and wire layers so there's no translation to reason + /// about. See docs/plan/ocm.md § Identity & auth model. + #[serde(skip_serializing_if = "Option::is_none")] + pub federation_kind: Option, + /// The authority that mints this user's `federation_subject` — + /// issuer URL for OIDC (id_token `iss` claim), peer domain for + /// OCM, `null` for local users (password / OPAQUE only). + /// + /// Renamed from `auth_provider` (which was a `String` with the + /// sentinel `"local"` for non-federated users, and a human-readable + /// label like `"MockSSO"` before Phase B). This shape mirrors the + /// `auth.users.federation_issuer` column directly: nullable when + /// there's no federation involved. FE predicates for "is this user + /// federated?" should read `federation_kind`, not + /// string-compare this value. + /// + /// When populated, FE code that wants a friendly display label + /// looks this value up against `OidcProviderInfoDto.issuer → + /// provider_name` to render the deployment's configured display + /// name; falls back to the raw issuer for foreign IdPs / legacy + /// rows still holding a pre-Phase-B label. + #[serde(skip_serializing_if = "Option::is_none")] + pub federation_issuer: Option, pub image: Option, pub can_edit_image: bool, /// `true` for grant-only external recipients (magic-link, OIDC-only, @@ -95,8 +124,8 @@ pub struct UserDto { #[serde(default)] pub force_password_change: bool, /// TRUE when the account has a local Argon2id `password_hash` on - /// file. Distinct from `auth_provider`: an SSO-linked account - /// (auth_provider != "local") can ALSO carry a local password if + /// file. Distinct from `federation_kind`: an OIDC-linked account + /// (`federation_kind == "oidc"`) can ALSO carry a local password if /// it was set at signup or later — a hybrid posture. The SPA /// gates the profile page's change-password card on this flag, /// so hybrid users can rotate their local password even though @@ -127,7 +156,12 @@ pub struct AdminUserSummaryDto { pub storage_used_bytes: i64, pub last_login_at: Option>, pub active: bool, - pub auth_provider: String, + /// See `UserDto::federation_kind` — same semantics, same wire spelling. + #[serde(skip_serializing_if = "Option::is_none")] + pub federation_kind: Option, + /// See `UserDto::federation_issuer` — same semantics, same wire spelling. + #[serde(skip_serializing_if = "Option::is_none")] + pub federation_issuer: Option, pub is_external: bool, /// TRUE when the user has a server-verifiable password on file /// (`password_hash IS NOT NULL`). The admin table uses this @@ -171,7 +205,8 @@ impl From for AdminUserSummaryDto { storage_used_bytes: entry.storage_used_bytes, last_login_at: entry.last_login_at, active: entry.active, - auth_provider: entry.federation_issuer.unwrap_or_else(|| "local".to_string()), + federation_kind: entry.federation_kind, + federation_issuer: entry.federation_issuer, is_external: entry.is_external, has_password: entry.has_password, opaque_registered: entry.opaque_registered, @@ -207,8 +242,11 @@ impl From for UserDto { updated_at: p.updated_at, last_login_at: p.last_login_at, active: p.active, - // Some(provider) moves the String; None still allocates "local". - auth_provider: p.federation_issuer.unwrap_or_else(|| "local".to_string()), + // NULL on both fields for local users (no federation wired). + // FE predicates use `!!federation_kind` for "is federated?" — + // no "local" sentinel string; the null tells the whole story. + federation_kind: p.federation_kind.map(|k| k.as_str().to_string()), + federation_issuer: p.federation_issuer, image: p.image, can_edit_image, is_external: p.is_external, @@ -467,6 +505,22 @@ pub struct OidcExchangeDto { #[derive(Debug, Serialize, Deserialize, ToSchema)] pub struct OidcProviderInfoDto { pub enabled: bool, + /// The authoritative issuer URL for THIS deployment's OIDC config — + /// same value that lands on `auth.users.federation_issuer` for + /// users JIT-provisioned via this IdP. + /// + /// Populated so the frontend can resolve display: when + /// `UserDto.federation_issuer` equals this `issuer`, render + /// `provider_name` as the human-friendly label (avoids showing raw + /// issuer URLs like `https://sso.example.com/realms/main` in the + /// admin badge / profile view). Falls back to the raw issuer when + /// there's no match — happens for legacy rows not yet lazy-rebound, + /// or (future) users linked to a different IdP than the currently + /// configured one. + /// + /// Empty string when OIDC is disabled on this deployment. + #[serde(default)] + pub issuer: String, pub provider_name: String, pub authorize_endpoint: String, pub password_login_enabled: bool, diff --git a/src/application/ports/auth_ports.rs b/src/application/ports/auth_ports.rs index dda496ae..4a1dca39 100644 --- a/src/application/ports/auth_ports.rs +++ b/src/application/ports/auth_ports.rs @@ -179,6 +179,20 @@ pub trait UserStoragePort: Send + Sync + 'static { image: Option<&str>, ) -> Result<(), DomainError>; + /// Federation-identity Phase B lazy rebind: overwrite `federation_issuer` + /// on a specific user row. Called when an OIDC login's id_token `iss` + /// claim proves the stored value (typically a legacy display label) is + /// out of sync with the true issuer URL. Guarded (`IS DISTINCT FROM`) + /// so calling with the current value is a zero-write no-op. + /// + /// Audit signal for the rebind lives at the caller (auth service) — + /// this repo method just moves the column value. + async fn rebind_federation_issuer( + &self, + user_id: Uuid, + new_issuer: &str, + ) -> Result<(), DomainError>; + /// Lists users by role (e.g., "admin" or "user") async fn list_users_by_role(&self, role: &str) -> Result, DomainError>; @@ -238,6 +252,15 @@ pub struct OidcTokenSet { #[derive(Debug, Clone)] pub struct OidcIdClaims { pub sub: String, + /// The validated `iss` claim from the id_token. Equal to + /// `discovery.issuer` (the validator enforces `iss == discovery.issuer`, + /// so this is a safe echo of the authoritative issuer URL). + /// + /// Load-bearing for the federation-identity Phase B lazy-rebind: the + /// app service compares this against `user.federation_issuer` and + /// updates the row when the stored value is still a legacy display + /// label (see docs/plan/ocm.md § Rename PR — Phase B). + pub iss: String, pub email: Option, pub email_verified: Option, pub preferred_username: Option, @@ -275,6 +298,17 @@ pub struct OidcLogoutClaims { /// JWT identifier — used by the app service to prevent replay of the /// same logout_token within the token's freshness window. pub jti: Option, + /// The validated `iss` claim from the logout_token — echoed from + /// `discovery.issuer` (the validator enforces `iss == discovery.issuer`, + /// so this is a safe echo of the authoritative issuer URL). + /// + /// Load-bearing for the sub-based revocation path (BCL without sid): + /// the app service passes this to + /// `revoke_user_sessions_by_federation_subject(issuer, sub)`, and the + /// pg impl matches on `auth.users.federation_issuer` — which post + /// Phase B stores the iss URL, NOT the display label. Passing the + /// display label (via `oidc.provider_name()`) misses every row. + pub iss: String, } /// Port for OIDC operations — implemented in infrastructure layer diff --git a/src/application/services/auth_application_service.rs b/src/application/services/auth_application_service.rs index 0af2088d..7b43e707 100644 --- a/src/application/services/auth_application_service.rs +++ b/src/application/services/auth_application_service.rs @@ -1516,16 +1516,19 @@ impl AuthApplicationService { self.backchannel_logout_jti_seen.insert(jti.clone(), ()); } - let provider_name = oidc.provider_name().to_string(); - // Resolve which sessions to revoke. let affected_user_ids: Vec = if let Some(sid) = claims.sid.as_ref() { self.session_storage .revoke_sessions_by_oidc_sid(sid) .await? } else if let Some(sub) = claims.sub.as_ref() { + // Pass claims.iss (the id_token's real issuer URL from the + // logout_token), NOT the OIDC service's provider_name + // display label. Post Phase B of the federation-identity + // rename, `auth.users.federation_issuer` stores the iss + // URL — matching on the display label misses every row. self.session_storage - .revoke_user_sessions_by_federation_subject(&provider_name, sub) + .revoke_user_sessions_by_federation_subject(&claims.iss, sub) .await? .into_iter() .collect() @@ -3396,13 +3399,63 @@ impl AuthApplicationService { .clone() .unwrap_or_else(|| format!("{}@oidc.local", oidc_username)); - // 5. Look up existing user by OIDC subject - let user = match self + // 5. Look up existing user by OIDC subject. + // + // Two-step lookup implements the Phase B lazy-rebind of the + // federation-identity rename (docs/plan/ocm.md § Phase B): + // 1. Canonical lookup keyed on the id_token's real `iss` claim. + // Post-migration this is what every fresh JIT row uses. + // 2. Legacy fallback keyed on the OXICLOUD_OIDC_PROVIDER_NAME + // display label. Fires for rows minted before the rename. + // If the fallback hits, the row is rebound to the real iss + // before this branch returns — first login after upgrade + // self-heals the user; no admin action needed. + // If both miss, JIT provisioning kicks in below and writes the + // canonical value from the start. + let canonical = self .user_storage - .get_user_by_federation_subject(&provider_name, &claims.sub) - .await - { + .get_user_by_federation_subject(&claims.iss, &claims.sub) + .await; + let lookup_result = match canonical { + Ok(u) => Ok(u), + // Only fall through to the legacy lookup if the canonical one + // said "not found" — treat all OTHER errors as fatal to avoid + // masking DB failures with a lookup that would probably fail + // the same way. NotFound is the only benign case here. + Err(e) if e.kind == ErrorKind::NotFound => { + self.user_storage + .get_user_by_federation_subject(&provider_name, &claims.sub) + .await + } + Err(e) => Err(e), + }; + let user = match lookup_result { Ok(mut existing_user) => { + // Lazy-rebind: if the row's stored issuer doesn't match + // the real iss claim, update it now. Covers the legacy- + // label case (fallback hit) AND any drift accumulated + // during Phase A when JIT was still writing labels. + // rebind_federation_issuer is a guarded UPDATE — same-value + // no-op costs nothing. + if existing_user.federation_issuer() != Some(claims.iss.as_str()) { + let old = existing_user + .federation_issuer() + .map(str::to_string) + .unwrap_or_default(); + self.user_storage + .rebind_federation_issuer(existing_user.id(), &claims.iss) + .await?; + tracing::info!( + target: "audit", + event = "federation.issuer_rebound", + reason = "lazy_backfill", + user_id = %existing_user.id(), + federation_kind = "oidc", + old_issuer = %old, + new_issuer = %claims.iss, + "🔗 federation_issuer rebound from legacy label to true iss URL", + ); + } // User exists — dispatch login BEFORE register_login() so // hooks observe `last_login_at = None` on the very first // login (see tip #1 in the trait docstring). @@ -3516,14 +3569,12 @@ impl AuthApplicationService { Some(username.clone()), None, Some(crate::domain::entities::user::FederationKind::Oidc), - // TODO Phase B: `provider_name` still carries the - // OXICLOUD_OIDC_PROVIDER_NAME display label instead - // of the true `iss` URL. Lazy-rebind on subsequent - // logins converts the row (see docs/plan/ocm.md - // § Rename PR — Phase B). First-login value is the - // label for backwards compatibility with existing - // rows. - Some(provider_name.clone()), + // Phase B canonical value: the id_token's real `iss` + // claim (validated to equal discovery.issuer in + // OidcService). No more display-label writes at JIT — + // legacy rows are fixed via lazy rebind in the + // existing-user branch above. + Some(claims.iss.clone()), Some(claims.sub.clone()), role, quota, diff --git a/src/domain/repositories/user_repository.rs b/src/domain/repositories/user_repository.rs index 101ba4fd..0782df59 100644 --- a/src/domain/repositories/user_repository.rs +++ b/src/domain/repositories/user_repository.rs @@ -45,6 +45,7 @@ pub struct UserListEntry { pub storage_used_bytes: i64, pub last_login_at: Option>, pub active: bool, + pub federation_kind: Option, pub federation_issuer: Option, pub is_external: bool, /// TRUE when `auth.users.password_hash IS NOT NULL` — user has a diff --git a/src/infrastructure/repositories/pg/user_pg_repository.rs b/src/infrastructure/repositories/pg/user_pg_repository.rs index 54ba9022..e68e73df 100644 --- a/src/infrastructure/repositories/pg/user_pg_repository.rs +++ b/src/infrastructure/repositories/pg/user_pg_repository.rs @@ -783,6 +783,7 @@ impl UserRepository for UserPgRepository { Option>, bool, Option, + Option, bool, bool, bool, @@ -797,13 +798,15 @@ impl UserRepository for UserPgRepository { // pays. `has_password` on the password_hash column tells // the admin table whether a server-verifiable password is // on file; combined with the two OPAQUE flags and - // federation_issuer, the SPA derives the full "capability - // set" per user (password / OPAQUE / SSO / passwordless). + // federation_kind / federation_issuer, the SPA derives the + // full "capability set" per user (password / OPAQUE / SSO / + // passwordless). r#" SELECT id, username, email, role::text, storage_quota_bytes, storage_used_bytes, - last_login_at, active, federation_issuer, is_external, + last_login_at, active, + federation_kind, federation_issuer, is_external, (password_hash IS NOT NULL) AS has_password, (opaque_envelope IS NOT NULL) AS opaque_registered, (opaque_migrated_at IS NOT NULL) AS opaque_migrated @@ -832,6 +835,7 @@ impl UserRepository for UserPgRepository { storage_used_bytes, last_login_at, active, + federation_kind, federation_issuer, is_external, has_password, @@ -850,6 +854,7 @@ impl UserRepository for UserPgRepository { storage_used_bytes, last_login_at, active, + federation_kind, federation_issuer, is_external, has_password, @@ -1369,6 +1374,34 @@ impl UserStoragePort for UserPgRepository { Ok(()) } + async fn rebind_federation_issuer( + &self, + user_id: Uuid, + new_issuer: &str, + ) -> Result<(), DomainError> { + // Same `IS DISTINCT FROM` guard as sync_oidc_login_profile: this + // fires on every OIDC login, so the common already-migrated case + // must be a zero-write no-op. Only actually flips the column + // when the stored value is stale (legacy display label vs the + // real issuer URL from the id_token's `iss` claim). + sqlx::query( + r#" + UPDATE auth.users + SET federation_issuer = $2, + updated_at = NOW() + WHERE id = $1 + AND federation_issuer IS DISTINCT FROM $2 + "#, + ) + .bind(user_id) + .bind(new_issuer) + .execute(&*self.pool) + .await + .map_err(Self::map_sqlx_error) + .map_err(DomainError::from)?; + Ok(()) + } + async fn list_users_by_role(&self, role: &str) -> Result, DomainError> { UserRepository::list_users_by_role(self, role) .await diff --git a/src/infrastructure/services/oidc_service.rs b/src/infrastructure/services/oidc_service.rs index f6e2da23..b6196065 100644 --- a/src/infrastructure/services/oidc_service.rs +++ b/src/infrastructure/services/oidc_service.rs @@ -180,6 +180,39 @@ impl OidcService { } } + /// Authoritative issuer URL from the IdP's discovery document — + /// **cache-only, non-async, non-blocking**. Returns `Some(issuer)` + /// when discovery has been fetched successfully before AND is not + /// expired; returns `None` otherwise (cold cache OR expired without + /// re-fetch). + /// + /// Deliberately does NOT trigger a network fetch — this is the + /// accessor that public endpoints (`/api/auth/oidc/providers`) + /// use, and driving IdP HTTP off every unauthenticated request is + /// a DoS amplifier. The cache gets warmed as a side-effect of + /// every real OIDC flow (authorize / callback / login / + /// validate_id_token all call `get_discovery`), so within seconds + /// of the first legit login this returns `Some`. + /// + /// Callers that need the definitive answer (validate_id_token, JIT + /// provisioning) should keep going through the async + /// discovery-fetching path. Callers that need a display hint + /// (providers endpoint) MUST use this non-async path and fall back + /// to a config value when it returns `None`. + pub fn cached_issuer(&self) -> Option { + // try_read is non-blocking; if the cache write lock is held + // (extremely rare, only during a discovery refresh), we return + // None rather than block on public traffic. + let cache = self.discovery.try_read().ok()?; + cache.as_ref().and_then(|cached| { + if cached.is_expired() { + None + } else { + Some(cached.value.issuer.clone()) + } + }) + } + /// Fetch and cache the OIDC discovery document (TTL: 1 hour) async fn get_discovery(&self) -> Result { // Check cache first (return cached value only if not expired) @@ -495,6 +528,13 @@ impl OidcServicePort for OidcService { Ok(OidcIdClaims { sub: claims.sub, + // Safe echo: jsonwebtoken::decode with + // `validation.set_issuer(&[&discovery.issuer])` above already + // enforced iss == discovery.issuer, so the discovery value + // IS the validated iss claim. The caller (auth service uses + // it for Phase B lazy-rebind) can trust this without a + // second validation pass. + iss: discovery.issuer.clone(), email: claims.email, email_verified: claims.email_verified, preferred_username: claims.preferred_username, @@ -510,6 +550,11 @@ impl OidcServicePort for OidcService { async fn fetch_user_info(&self, access_token: &str) -> Result { let discovery = self.get_discovery().await?; + // Capture issuer before we move `userinfo_endpoint` out below. + // Same rationale as validate_id_token: discovery.issuer IS the + // authoritative iss for this deployment; fetch_user_info is only + // called after a successful token exchange with this same issuer. + let iss = discovery.issuer.clone(); let userinfo_url = discovery.userinfo_endpoint.ok_or_else(|| { DomainError::new( @@ -551,6 +596,7 @@ impl OidcServicePort for OidcService { Ok(OidcIdClaims { sub: info.sub, + iss, email: info.email, email_verified: info.email_verified, preferred_username: info.preferred_username, @@ -741,6 +787,7 @@ impl OidcServicePort for OidcService { sub: claims.sub, sid: claims.sid, jti: claims.jti, + iss: claims.iss, }) } } diff --git a/src/interfaces/api/handlers/auth_handler.rs b/src/interfaces/api/handlers/auth_handler.rs index cd177dad..06c01c0c 100644 --- a/src/interfaces/api/handlers/auth_handler.rs +++ b/src/interfaces/api/handlers/auth_handler.rs @@ -1294,6 +1294,7 @@ pub async fn oidc_providers( if !auth_app.oidc_enabled() { return Ok(Json(OidcProviderInfoDto { enabled: false, + issuer: String::new(), provider_name: String::new(), authorize_endpoint: String::new(), password_login_enabled, @@ -1305,8 +1306,31 @@ pub async fn oidc_providers( let config = auth_app.oidc_config().unwrap(); + // Prefer the DISCOVERY document's issuer — that's what + // OidcService uses to validate id_tokens AND what lands on + // `auth.users.federation_issuer` at JIT provisioning / lazy + // rebind. The config's `issuer_url` is only what the operator + // typed to point at discovery; the discovery document publishes + // the authoritative value (may differ by trailing slash, host + // casing, etc). **Cache-only lookup on purpose** — this + // endpoint is PUBLIC + UNAUTHENTICATED, so triggering an IdP + // HTTP fetch per request is a DoS amplifier (attacker at N req/s + // → we hit the IdP at N req/s, and the cache only stores on + // success so a degraded IdP means every call retries). On cold + // cache (before the first real OIDC flow warms it), fall back to + // the operator-typed `config.issuer_url`. In practice the cache + // is warm within seconds of the first login; the fallback only + // shows during that window and is only wrong if the IdP + // publishes an issuer that differs from the URL used to fetch + // discovery (rare in normal deployments). + let issuer = auth_app + .oidc_service() + .and_then(|svc| svc.cached_issuer()) + .unwrap_or_else(|| config.issuer_url.clone()); + Ok(Json(OidcProviderInfoDto { enabled: true, + issuer, provider_name: config.provider_name.clone(), authorize_endpoint: "/api/auth/oidc/authorize".to_string(), password_login_enabled, diff --git a/src/interfaces/nextcloud/ocs_handler.rs b/src/interfaces/nextcloud/ocs_handler.rs index d1326ab4..da1c19b1 100644 --- a/src/interfaces/nextcloud/ocs_handler.rs +++ b/src/interfaces/nextcloud/ocs_handler.rs @@ -213,8 +213,14 @@ async fn user_provisioning_response( vec!["users"] }; - // Determine backend based on auth provider - let backend = if user_dto.auth_provider.to_lowercase().contains("oidc") { + // Determine backend based on federation kind. Historically checked + // `auth_provider.to_lowercase().contains("oidc")` which happened to + // work when the DTO field held a display label containing "oidc" + // (e.g. "OIDC-Google") — but broke silently when the label was + // "MockSSO" or, post Phase B of the federation-identity rename, when + // the field became an issuer URL that doesn't contain "oidc". The + // kind field is the load-bearing signal. + let backend = if user_dto.federation_kind.as_deref() == Some("oidc") { "OIDC" } else { "Database" diff --git a/tests/oidc/oidc.hurl b/tests/oidc/oidc.hurl index b6a1ce72..a57d5b79 100644 --- a/tests/oidc/oidc.hurl +++ b/tests/oidc/oidc.hurl @@ -215,12 +215,15 @@ oidc_user_id: jsonpath "$.id" [Asserts] jsonpath "$.username" == "oidc_user" jsonpath "$.email" == "oidc@example.com" -# `auth_provider` stores the OIDC provider's display name (set via -# OXICLOUD_OIDC_PROVIDER_NAME in tests/common/server-with-oidc.env), -# NOT a generic "oidc" tag. A locally registered admin would have -# this field as something like "local". The distinct value here is -# what proves JIT provisioning landed via OIDC, not setup.hurl. -jsonpath "$.auth_provider" == "MockSSO" +# Post the federation-identity rename (docs/plan/ocm.md § Schema +# rename) UserDto exposes federation_kind + federation_issuer as +# separate nullable fields. Local users have both null; OIDC users +# get kind="oidc" and issuer=. For +# the fake IdP (tests/oidc/fake_idp/server.js) that URL is the +# issuer published in its discovery document, which matches +# `oidc_issuer` from test.env. +jsonpath "$.federation_kind" == "oidc" +jsonpath "$.federation_issuer" == "{{oidc_issuer}}" # Full claim round-trip — the fake IdP (tests/oidc/fake_idp/server.js) # pins these values and OxiCloud must persist each one verbatim during # JIT provisioning (see auth_application_service.rs around line 2257). diff --git a/tools/perf-audit/admin_user_listing_e2e.rs b/tools/perf-audit/admin_user_listing_e2e.rs index ffda1de6..93e263a4 100644 --- a/tools/perf-audit/admin_user_listing_e2e.rs +++ b/tools/perf-audit/admin_user_listing_e2e.rs @@ -73,7 +73,7 @@ struct FullUserDto { updated_at: DateTime, last_login_at: Option>, active: bool, - auth_provider: String, + federation_issuer: String, image: Option, can_edit_image: bool, is_external: bool, @@ -100,7 +100,7 @@ impl FullUserDto { storage_used_bytes: self.storage_used_bytes, last_login_at: self.last_login_at, active: self.active, - auth_provider: self.auth_provider.clone(), + federation_issuer: self.federation_issuer.clone(), is_external: self.is_external, } } @@ -117,7 +117,7 @@ struct SummaryUserDto { storage_used_bytes: i64, last_login_at: Option>, active: bool, - auth_provider: String, + federation_issuer: String, is_external: bool, } @@ -270,7 +270,7 @@ async fn load_full(pool: &PgPool) -> Vec { updated_at: row.get("updated_at"), last_login_at: row.get("last_login_at"), active: row.get("active"), - auth_provider: oidc_provider.unwrap_or_else(|| "local".to_owned()), + federation_issuer: oidc_provider.unwrap_or_else(|| "local".to_owned()), image: row.get("image"), can_edit_image, is_external: row.get("is_external"), @@ -312,7 +312,7 @@ async fn load_summary(pool: &PgPool) -> Vec { storage_used_bytes: row.get("storage_used_bytes"), last_login_at: row.get("last_login_at"), active: row.get("active"), - auth_provider: row + federation_issuer: row .get::, _>("oidc_provider") .unwrap_or_else(|| "local".to_owned()), is_external: row.get("is_external"),