Stop overwriting global_user display_name on admin REST calls (ARB-515 follow-up) (#386)

crthpl · web-flow · commit 95ff30018a76 · 2026-04-13T15:42:48.000-07:00
diff --git a/backend/global_migrations/005_backfill_placeholder_display_names.sql 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
@@ -44,12 +44,44 @@ pub struct AccessClaims {
 
 #[derive(Debug, Deserialize)]
 struct IdClaims {
-    pub name: String,
+    #[serde(default)]
+    pub name: Option<String>,
+    #[serde(default)]
+    pub given_name: Option<String>,
+    #[serde(default)]
+    pub family_name: Option<String>,
     pub sub: String,
     #[serde(default)]
     pub email: Option<String>,
 }
 
+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<String> {
+        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<AuthConfig> = 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
@@ -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<GlobalUser, sqlx::Error> {
@@ -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,14 +235,41 @@ 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,
             email: email.map(String::from),
         })
     }
 
+    /// 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<Option<GlobalUser>, 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
@@ -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
@@ -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<String> {
 // --- 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
