diff --git a/backend/global_migrations/005_backfill_placeholder_display_names.sql b/backend/global_migrations/005_backfill_placeholder_display_names.sql new file mode 100644 index 00000000..f6b0aae7 --- /dev/null +++ b/backend/global_migrations/005_backfill_placeholder_display_names.sql @@ -0,0 +1,13 @@ +-- Backfill display_name for users whose display_name was set to something we now consider +-- sensitive or ugly to show in the UI (the Kinde sub, their email, or the literal "Unknown" +-- fallback from older code paths). Replace each such row with a fresh "Unnamed-XXXX" +-- placeholder, where XXXX is 4 hex characters pulled from SQLite's per-row random blob so +-- two simultaneously-backfilled users remain distinguishable in account lists. +-- +-- `randomblob()` is a volatile function in SQLite and is re-evaluated once per updated row, +-- so each row gets its own suffix. +UPDATE "global_user" +SET "display_name" = 'Unnamed-' || substr(hex(randomblob(2)), 1, 4) +WHERE "display_name" = "kinde_id" + OR "display_name" = "email" + OR "display_name" = 'Unknown'; diff --git a/backend/src/auth.rs b/backend/src/auth.rs index bd8d65a0..9c9c0652 100644 --- a/backend/src/auth.rs +++ b/backend/src/auth.rs @@ -44,12 +44,44 @@ pub struct AccessClaims { #[derive(Debug, Deserialize)] struct IdClaims { - pub name: String, + #[serde(default)] + pub name: Option, + #[serde(default)] + pub given_name: Option, + #[serde(default)] + pub family_name: Option, pub sub: String, #[serde(default)] pub email: Option, } +impl IdClaims { + /// Best-effort display name from `id_token` claims. Prefers `name`, then + /// `given_name` + `family_name` joined, ignoring empty values. Returns + /// `None` when none of these claims yield a non-empty string. + fn resolve_name(&self) -> Option { + if let Some(name) = self.name.as_deref().map(str::trim).filter(|s| !s.is_empty()) { + return Some(name.to_owned()); + } + let given = self + .given_name + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()); + let family = self + .family_name + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()); + match (given, family) { + (Some(g), Some(f)) => Some(format!("{g} {f}")), + (Some(g), None) => Some(g.to_owned()), + (None, Some(f)) => Some(f.to_owned()), + (None, None) => None, + } + } +} + static AUTH_CONFIG: OnceCell = OnceCell::new(); #[async_trait] @@ -166,7 +198,7 @@ pub async fn validate_access_and_id( id: access_claims.sub, roles: access_claims.roles, email: id_claims.as_ref().and_then(|c| c.email.clone()), - name: id_claims.map(|c| c.name), + name: id_claims.as_ref().and_then(IdClaims::resolve_name), }) } @@ -200,7 +232,7 @@ pub async fn validate_id_token_for_sub( anyhow::bail!("sub mismatch"); } Ok(IdTokenInfo { - name: Some(id_claims.name), + name: id_claims.resolve_name(), email: id_claims.email, }) } diff --git a/backend/src/global_db.rs b/backend/src/global_db.rs index b0bd8baa..e8bca494 100644 --- a/backend/src/global_db.rs +++ b/backend/src/global_db.rs @@ -1,11 +1,27 @@ use std::{env, path::Path}; +use rand::{distributions::Alphanumeric, Rng}; use serde::Serialize; use sqlx::{ sqlite::{SqliteConnectOptions, SqliteJournalMode, SqliteSynchronous}, Connection, FromRow, SqliteConnection, SqlitePool, }; +/// Generate a user-facing placeholder display name for users whose real name we don't yet +/// know. The Kinde sub and the user's email are both considered sensitive/ugly to expose +/// in the UI, so we deliberately surface neither — callers should use this helper instead +/// of making something up. The 4-character suffix exists purely so two unnamed users in the +/// same account list are distinguishable at a glance. +#[must_use] +pub fn generate_unnamed_placeholder() -> String { + let suffix: String = rand::thread_rng() + .sample_iter(&Alphanumeric) + .take(4) + .map(char::from) + .collect(); + format!("Unnamed-{suffix}") +} + #[derive(Clone, Debug)] pub struct GlobalDB { pool: SqlitePool, @@ -95,12 +111,16 @@ impl GlobalDB { Ok(Self { pool }) } - /// Create or find a global user by `kinde_id`. Updates `display_name` and email if changed. + /// Create a new global user, or return the existing one. The `name` parameter is only + /// used on creation — once a row exists, its `display_name` is treated as user-owned and + /// is never overwritten by this function. Manual updates made via + /// [`Self::update_user_display_name`] (by the user themselves or an admin) therefore + /// stick, and subsequent Kinde logins won't silently clobber them. `email` and + /// `is_kinde_admin` are still synced on every call so that email changes and Kinde + /// admin-role revocations propagate. /// - /// `is_kinde_admin` is written verbatim — flipping it false on a later auth correctly - /// revokes admin status if the user is no longer granted via the Admin page. The - /// `admin_grant` column is only touched by [`Self::set_user_admin`], so Admin-page - /// grants survive auth. + /// `admin_grant` is never touched here — it is only set via + /// [`Self::set_user_admin`] from the Admin page. /// /// # Errors /// Returns an error on database failure. @@ -121,24 +141,21 @@ impl GlobalDB { .await?; if let Some(mut user) = existing { - let name_changed = user.display_name != name || user.email.as_deref() != email; + let email_changed = email.is_some() && user.email.as_deref() != email; let kinde_admin_changed = user.is_kinde_admin != is_kinde_admin; - if name_changed || kinde_admin_changed { + if email_changed || kinde_admin_changed { sqlx::query( r"UPDATE global_user - SET display_name = ?, - email = COALESCE(?, email), + SET email = COALESCE(?, email), is_kinde_admin = ? WHERE id = ?", ) - .bind(name) .bind(email) .bind(is_kinde_admin) .bind(user.id) .execute(&self.pool) .await?; - user.display_name = name.to_string(); - if email.is_some() { + if email_changed { user.email = email.map(String::from); } if kinde_admin_changed { @@ -172,21 +189,21 @@ impl GlobalDB { }) } - /// Find a global user by `kinde_id`, creating one with a placeholder - /// display name if none exists. Unlike [`Self::ensure_global_user`], this - /// does NOT update the display name or email of an existing user — use it - /// from code paths that don't yet have a trusted name for the user. + /// Find a global user by `kinde_id`, creating one with a generated `"Unnamed-XXXX"` + /// placeholder if none exists. Unlike [`Self::ensure_global_user`], this does NOT + /// update the display name or email of an existing user — use it from code paths that + /// don't yet have a trusted name for the user. /// - /// `is_kinde_admin` is still synced (written verbatim) even on the - /// no-name path, so the Kinde admin role is recognised on the very first - /// REST request even before the WS auth flow has populated a real name. + /// The placeholder is generated by [`generate_unnamed_placeholder`] and is deliberately + /// not derived from the Kinde sub or the user's email: exposing either in the UI is + /// considered a privacy/UX regression. `is_kinde_admin` is still synced verbatim so the + /// Kinde admin role is recognised even on a user's first request. /// /// # Errors /// Returns an error on database failure. pub async fn find_or_create_global_user( &self, kinde_id: &str, - placeholder_name: &str, email: Option<&str>, is_kinde_admin: bool, ) -> Result { @@ -203,12 +220,13 @@ impl GlobalDB { return Ok(user); } + let placeholder_name = generate_unnamed_placeholder(); let id = sqlx::query_scalar::<_, i64>( r"INSERT INTO global_user (kinde_id, display_name, email, is_kinde_admin) VALUES (?, ?, ?, ?) RETURNING id", ) .bind(kinde_id) - .bind(placeholder_name) + .bind(&placeholder_name) .bind(email) .bind(is_kinde_admin) .fetch_one(&self.pool) @@ -217,7 +235,7 @@ impl GlobalDB { Ok(GlobalUser { id, kinde_id: kinde_id.to_string(), - display_name: placeholder_name.to_string(), + display_name: placeholder_name, is_admin: is_kinde_admin, is_kinde_admin, admin_grant: false, @@ -225,6 +243,33 @@ impl GlobalDB { }) } + /// Look up a global user by `kinde_id` and sync their `is_kinde_admin` flag in place if + /// it differs from the passed value. Returns `None` when no row exists — callers that + /// cannot create the user themselves (e.g. `check_admin`, which has no trusted display + /// name) should treat this as "not authorised" rather than creating a placeholder row. + /// + /// # Errors + /// Returns an error on database failure. + pub async fn sync_is_kinde_admin_by_kinde_id( + &self, + kinde_id: &str, + is_kinde_admin: bool, + ) -> Result, sqlx::Error> { + let Some(mut user) = self.get_global_user_by_kinde_id(kinde_id).await? else { + return Ok(None); + }; + if user.is_kinde_admin != is_kinde_admin { + sqlx::query("UPDATE global_user SET is_kinde_admin = ? WHERE id = ?") + .bind(is_kinde_admin) + .bind(user.id) + .execute(&self.pool) + .await?; + user.is_kinde_admin = is_kinde_admin; + user.is_admin = is_kinde_admin || user.admin_grant; + } + Ok(Some(user)) + } + /// Get a global user by `kinde_id`. /// /// # Errors diff --git a/backend/src/handle_socket.rs b/backend/src/handle_socket.rs index 9bc1f16b..4997092e 100644 --- a/backend/src/handle_socket.rs +++ b/backend/src/handle_socket.rs @@ -1240,17 +1240,36 @@ async fn authenticate( // Get or create global user. The Kinde admin role is synced into // `global_user.is_admin` so downstream code can rely on a single source of truth. - let display_name = valid_client.name.as_deref().unwrap_or("Unknown"); + // When the id_token yields a real display name we call `ensure_global_user` + // for its create-with-name path; otherwise we fall back to + // `find_or_create_global_user`, which preserves any previously-populated name + // and, for brand-new rows, generates an `Unnamed-XXXX` placeholder rather than + // exposing the Kinde sub or the user's email in the UI. let is_kinde_admin = valid_client.roles.contains(&Role::Admin); - let global_user = match global_db - .ensure_global_user( - &valid_client.id, - display_name, - valid_client.email.as_deref(), - is_kinde_admin, - ) - .await + let global_user_result = if let Some(display_name) = valid_client + .name + .as_deref() + .map(str::trim) + .filter(|s| !s.is_empty()) { + global_db + .ensure_global_user( + &valid_client.id, + display_name, + valid_client.email.as_deref(), + is_kinde_admin, + ) + .await + } else { + global_db + .find_or_create_global_user( + &valid_client.id, + valid_client.email.as_deref(), + is_kinde_admin, + ) + .await + }; + let global_user = match global_user_result { Ok(user) => user, Err(e) => { tracing::error!("Failed to ensure global user: {e}"); @@ -1338,9 +1357,15 @@ async fn authenticate( } }; - // Create/find user in cohort DB using global_user_id + // Create/find user in cohort DB using global_user_id. Use the already-resolved + // `display_name` from the global user record, which our ensure/find-or-create + // logic above has populated with the best name available. let result = db - .ensure_user_created_by_global_id(global_user.id, display_name, initial_balance) + .ensure_user_created_by_global_id( + global_user.id, + &global_user.display_name, + initial_balance, + ) .await?; let id = match result { diff --git a/backend/src/main.rs b/backend/src/main.rs index c5e9329f..035ed832 100644 --- a/backend/src/main.rs +++ b/backend/src/main.rs @@ -163,21 +163,24 @@ async fn list_cohorts( // the JWT role so the Admin-page toggle and admin checks can rely on the // single effective `is_admin` field. let is_kinde_admin = claims.roles.contains(&backend::auth::Role::Admin); - let global_user = if let Some(name) = id_token_name.as_deref().filter(|n| !n.is_empty()) { + let global_user = if let Some(name) = id_token_name + .as_deref() + .map(str::trim) + .filter(|n| !n.is_empty()) + { state .global_db .ensure_global_user(&claims.sub, name, resolved_email.as_deref(), is_kinde_admin) .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? } else { + // No trusted name from the id_token. Fall back to `find_or_create_global_user`, + // which never clobbers an existing display_name and generates an "Unnamed-XXXX" + // placeholder for brand-new rows — we deliberately avoid exposing either the Kinde + // sub or the user's email in the UI as a display name. state .global_db - .find_or_create_global_user( - &claims.sub, - &claims.sub, - resolved_email.as_deref(), - is_kinde_admin, - ) + .find_or_create_global_user(&claims.sub, resolved_email.as_deref(), is_kinde_admin) .await .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? }; @@ -250,20 +253,18 @@ async fn get_active_auction_cohort_name(state: &AppState) -> Option { // --- Admin Endpoints --- async fn check_admin(state: &AppState, claims: &AccessClaims) -> Result<(), (StatusCode, String)> { - // Sync the Kinde admin role into `global_user.is_admin` and then read the DB field as the - // single source of truth. `claims.sub` is used as a fallback display_name; the WS auth flow - // will update it with the real name on the next connection. + // Look up the caller's global_user row. We deliberately do NOT create the row here: + // this endpoint has no trusted display name to create with, and users always reach the + // admin API via the main app (which hits `/api/cohorts` first and creates the row + // there). Syncing the Kinde admin role is done in-place so that role revocations in + // Kinde take effect on the next admin call rather than waiting for the next login. let is_kinde_admin = claims.roles.contains(&backend::auth::Role::Admin); let global_user = state .global_db - .ensure_global_user( - &claims.sub, - &claims.sub, - claims.email.as_deref(), - is_kinde_admin, - ) + .sync_is_kinde_admin_by_kinde_id(&claims.sub, is_kinde_admin) .await - .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? + .ok_or((StatusCode::FORBIDDEN, "Admin access required".to_string()))?; if global_user.is_admin { Ok(()) @@ -723,17 +724,19 @@ async fn update_my_display_name( "Display name cannot be empty".to_string(), )); } - let is_kinde_admin = claims.roles.contains(&backend::auth::Role::Admin); + // Look up the caller's row instead of creating one here — the main app hits + // `/api/cohorts` before any settings page, and creating a row with `claims.sub` + // as the display name would be a regression in the "name ends up as the Kinde sub" + // bug this patch exists to fix. let global_user = state .global_db - .ensure_global_user( - &claims.sub, - &claims.sub, - claims.email.as_deref(), - is_kinde_admin, - ) + .get_global_user_by_kinde_id(&claims.sub) .await - .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?; + .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))? + .ok_or(( + StatusCode::NOT_FOUND, + "User not found; load the app before updating your display name".to_string(), + ))?; state .global_db