refactor: remove GATEWAY_USER_TOKENS, fix review feedback

GATEWAY_USER_TOKENS never went to production — replaced entirely by DB-backed user management via /api/admin/users and /api/tokens. Removed: - UserTokenConfig struct and GATEWAY_USER_TOKENS env var parsing - user_tokens field from GatewayConfig - GatewayChannel::new_multi_auth() constructor - Env-var user migration block in main.rs (~90 lines) - multi_tenant auto-detection from GATEWAY_USER_TOKENS (now runtime via db.has_any_users() in app.rs) Review fixes (zmanian): - User ID generation: UUID instead of display-name derivation (#1) - Invitation accept moved to public router (no auth needed) (#3) - libSQL get_invitation_by_hash aligned with postgres: filters status='pending' AND expires_at > now (#4) - UUID parse: returns DatabaseError::Serialization instead of unwrap_or_default (#7) - PostgreSQL SELECT * replaced with explicit column lists (#8) - Sort order aligned (both backends use DESC) (#6) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
2026-08-27 16:10:09 +00:00 · 2026-03-24 22:34:05 -07:00
parent 240cee82e1
commit aa35c2e1ce
13 changed files with 74 additions and 255 deletions
@@ -342,17 +342,12 @@ impl AppBuilder {
            ws = ws.with_memory_layers(self.config.workspace.memory_layers.clone());
            let ws = Arc::new(ws);

-            // Detect multi-tenant mode: when GATEWAY_USER_TOKENS is configured,
+            // Detect multi-tenant mode: when the database has registered users,
            // each authenticated user needs their own workspace scope. Use
            // WorkspacePool (which implements WorkspaceResolver) to create
            // per-user workspaces on demand instead of sharing the startup
            // workspace across all users.
-            let is_multi_tenant = self
-                .config
-                .channels
-                .gateway
-                .as_ref()
-                .is_some_and(|gw| gw.user_tokens.is_some());
+            let is_multi_tenant = db.has_any_users().await.unwrap_or(false);

            if is_multi_tenant {
                let pool = Arc::new(crate::channels::web::server::WorkspacePool::new(
@@ -69,6 +69,11 @@ impl MultiAuthState {
    }

    /// Create a multi-user auth state from a map of tokens to identities.
+    ///
+    /// **Test-only** — production multi-user auth is DB-backed via
+    /// `DbAuthenticator`. This constructor is kept public (not `#[cfg(test)]`)
+    /// because integration tests in `tests/` compile the crate as a library
+    /// where `cfg(test)` is not set.
    pub fn multi(tokens: HashMap<String, UserIdentity>) -> Self {
        let hashed_tokens: Vec<([u8; 32], UserIdentity)> = tokens
            .into_iter()
@@ -188,7 +193,7 @@ impl DbAuthenticator {
 /// Combined auth state: tries env-var tokens first, then DB-backed tokens.
 #[derive(Clone)]
 pub struct CombinedAuthState {
-    /// In-memory tokens from GATEWAY_USER_TOKENS or GATEWAY_AUTH_TOKEN.
+    /// In-memory tokens from GATEWAY_AUTH_TOKEN.
    pub env_auth: MultiAuthState,
    /// DB-backed token authenticator (optional — only when a database is available).
    pub db_auth: Option<DbAuthenticator>,
@@ -162,12 +162,7 @@ pub async fn invitations_accept_handler(
        return Err((StatusCode::GONE, "Invitation has expired".to_string()));
    }

-    // Generate a user id from the display name.
-    let new_user_id = display_name
-        .to_ascii_lowercase()
-        .split_whitespace()
-        .collect::<Vec<_>>()
-        .join("-");
+    let new_user_id = Uuid::new_v4().to_string();

    let now = chrono::Utc::now();
    let user_record = UserRecord {
@@ -7,6 +7,7 @@ use axum::{
    extract::{Path, State},
    http::StatusCode,
 };
+use uuid::Uuid;

 use crate::channels::web::auth::AuthenticatedUser;
 use crate::channels::web::server::GatewayState;
@@ -34,16 +35,7 @@ pub async fn users_create_handler(

    let email = body.get("email").and_then(|v| v.as_str()).map(String::from);

-    // Generate user id: prefer email if provided, otherwise derive from display_name.
-    let user_id = if let Some(ref e) = email {
-        e.clone()
-    } else {
-        display_name
-            .to_ascii_lowercase()
-            .split_whitespace()
-            .collect::<Vec<_>>()
-            .join("-")
-    };
+    let user_id = Uuid::new_v4().to_string();

    let now = chrono::Utc::now();
    let user_record = UserRecord {
@@ -124,49 +124,6 @@ impl GatewayChannel {
        }
    }

-    /// Create a gateway channel with a pre-built multi-user auth state.
-    pub fn new_multi_auth(config: GatewayConfig, auth: MultiAuthState) -> Self {
-        let auth = CombinedAuthState {
-            env_auth: auth,
-            db_auth: None,
-        };
-        let state = Arc::new(GatewayState {
-            msg_tx: tokio::sync::RwLock::new(None),
-            sse: Arc::new(SseManager::new()),
-            workspace: None,
-            workspace_pool: None,
-            session_manager: None,
-            log_broadcaster: None,
-            log_level_handle: None,
-            extension_manager: None,
-            tool_registry: None,
-            store: None,
-            job_manager: None,
-            prompt_queue: None,
-            scheduler: None,
-            default_user_id: config.user_id.clone(),
-            shutdown_tx: tokio::sync::RwLock::new(None),
-            ws_tracker: Some(Arc::new(ws::WsConnectionTracker::new())),
-            llm_provider: None,
-            skill_registry: None,
-            skill_catalog: None,
-            chat_rate_limiter: server::PerUserRateLimiter::new(30, 60),
-            oauth_rate_limiter: server::RateLimiter::new(10, 60),
-            registry_entries: Vec::new(),
-            cost_guard: None,
-            routine_engine: Arc::new(tokio::sync::RwLock::new(None)),
-            startup_time: std::time::Instant::now(),
-            webhook_rate_limiter: server::RateLimiter::new(10, 60),
-            active_config: server::ActiveConfigSnapshot::default(),
-        });
-
-        Self {
-            config,
-            state,
-            auth,
-        }
-    }
-
    /// Helper to rebuild state, copying existing fields and applying a mutation.
    fn rebuild_state(&mut self, mutate: impl FnOnce(&mut GatewayState)) {
        let mut new_state = GatewayState {
@@ -417,6 +417,11 @@ pub async fn start_server(
        .route(
            "/api/webhooks/u/{user_id}/{path}",
            post(crate::channels::web::handlers::webhooks::webhook_trigger_user_scoped_handler),
+        )
+        // Invitation accept (public — validated by invite token, not user auth)
+        .route(
+            "/api/invitations/accept",
+            post(super::handlers::invitations::invitations_accept_handler),
        );

    // Protected routes (require auth)
@@ -545,10 +550,6 @@ pub async fn start_server(
            get(super::handlers::invitations::invitations_list_handler)
                .post(super::handlers::invitations::invitations_create_handler),
        )
-        .route(
-            "/api/invitations/accept",
-            post(super::handlers::invitations::invitations_accept_handler),
-        )
        // Gateway control plane
        .route("/api/gateway/status", get(gateway_status_handler))
        // OpenAI-compatible API
@@ -1,6 +1,6 @@
 use std::time::Duration;

-use crate::config::helpers::{optional_env, parse_bool_env, parse_option_env, parse_optional_env};
+use crate::config::helpers::{parse_bool_env, parse_option_env, parse_optional_env};
 use crate::error::ConfigError;
 use crate::settings::Settings;

@@ -34,7 +34,8 @@ pub struct AgentConfig {
    /// Maximum tokens per job (0 = unlimited).
    pub max_tokens_per_job: u64,
    /// Whether the deployment is multi-tenant (multiple users sharing one
-    /// instance). Auto-detected from GATEWAY_USER_TOKENS presence.
+    /// instance). Detected at runtime after DB initialization, not from config.
+    /// See app.rs startup logic.
    pub multi_tenant: bool,
 }

@@ -120,9 +121,9 @@ impl AgentConfig {
                "AGENT_MAX_TOKENS_PER_JOB",
                settings.agent.max_tokens_per_job,
            )?,
-            // Auto-detected from GATEWAY_USER_TOKENS presence. Not a separate
-            // knob — multi-tenant mode is always implied by configuring user tokens.
-            multi_tenant: optional_env("GATEWAY_USER_TOKENS")?.is_some(),
+            // Multi-tenant mode is detected at runtime after DB initialization,
+            // not from config. See app.rs startup logic.
+            multi_tenant: false,
        })
    }
 }
@@ -1,13 +1,11 @@
 use std::collections::HashMap;
 use std::path::PathBuf;

-use secrecy::SecretString;
-use serde::Deserialize;
-
 use crate::bootstrap::ironclaw_base_dir;
 use crate::config::helpers::{optional_env, parse_bool_env, parse_optional_env};
 use crate::error::ConfigError;
 use crate::settings::Settings;
+use secrecy::SecretString;

 /// Channel configurations.
 #[derive(Debug, Clone)]
@@ -54,18 +52,6 @@ pub struct GatewayConfig {
    pub workspace_read_scopes: Vec<String>,
    /// Memory layer definitions (JSON in env var, or from external config).
    pub memory_layers: Vec<crate::workspace::layer::MemoryLayer>,
-    /// Multi-user token map. When set, each token maps to a user identity.
-    /// Parsed from `GATEWAY_USER_TOKENS` (JSON string). When absent, falls back
-    /// to single-user mode via `auth_token` + `user_id`.
-    pub user_tokens: Option<HashMap<String, UserTokenConfig>>,
-}
-
-/// Per-user token configuration for multi-user mode.
-#[derive(Debug, Clone, Deserialize)]
-pub struct UserTokenConfig {
-    pub user_id: String,
-    #[serde(default)]
-    pub workspace_read_scopes: Vec<String>,
 }

 /// Signal channel configuration (signal-cli daemon HTTP/JSON-RPC).
@@ -196,41 +182,6 @@ impl ChannelsConfig {
                }
            }

-            let user_tokens: Option<HashMap<String, UserTokenConfig>> =
-                match optional_env("GATEWAY_USER_TOKENS")? {
-                    Some(json_str) => {
-                        let tokens: HashMap<String, UserTokenConfig> = serde_json::from_str(
-                            &json_str,
-                        )
-                        .map_err(|e| ConfigError::InvalidValue {
-                            key: "GATEWAY_USER_TOKENS".to_string(),
-                            message: format!(
-                                "must be valid JSON object mapping tokens to user configs: {e}"
-                            ),
-                        })?;
-                        if tokens.is_empty() {
-                            return Err(ConfigError::InvalidValue {
-                            key: "GATEWAY_USER_TOKENS".to_string(),
-                            message:
-                                "token map is empty — remove the variable to use single-user mode"
-                                    .to_string(),
-                        });
-                        }
-                        for (tok, cfg) in &tokens {
-                            if cfg.user_id.trim().is_empty() {
-                                return Err(ConfigError::InvalidValue {
-                                    key: "GATEWAY_USER_TOKENS".to_string(),
-                                    message: format!(
-                                        "token '{}...' has an empty user_id",
-                                        &tok[..tok.len().min(8)]
-                                    ),
-                                });
-                            }
-                        }
-                        Some(tokens)
-                    }
-                    None => None,
-                };
            let workspace_read_scopes: Vec<String> = optional_env("WORKSPACE_READ_SCOPES")?
                .map(|s| {
                    s.split(',')
@@ -261,7 +212,6 @@ impl ChannelsConfig {
                user_id,
                workspace_read_scopes,
                memory_layers,
-                user_tokens,
            })
        } else {
            None
@@ -419,7 +369,6 @@ mod tests {
            user_id: "default".to_string(),
            workspace_read_scopes: vec![],
            memory_layers: vec![],
-            user_tokens: None,
        };
        assert_eq!(cfg.host, "127.0.0.1");
        assert_eq!(cfg.port, 3000);
@@ -436,7 +385,6 @@ mod tests {
            user_id: "anon".to_string(),
            workspace_read_scopes: vec![],
            memory_layers: vec![],
-            user_tokens: None,
        };
        assert!(cfg.auth_token.is_none());
    }
@@ -21,8 +21,8 @@ pub struct HeartbeatConfig {
    pub quiet_hours_end: Option<u32>,
    /// Timezone for fire_at and quiet hours evaluation (IANA name).
    pub timezone: Option<String>,
-    /// When true, cycle through all users with routines. Auto-detected from
-    /// GATEWAY_USER_TOKENS or set explicitly via HEARTBEAT_MULTI_TENANT.
+    /// When true, cycle through all users with routines. Set explicitly via
+    /// HEARTBEAT_MULTI_TENANT or detected at runtime after DB initialization.
    pub multi_tenant: bool,
 }

@@ -105,12 +105,7 @@ impl HeartbeatConfig {
                }
                tz
            },
-            // Auto-detect multi-tenant mode from GATEWAY_USER_TOKENS presence,
-            // or allow explicit override via HEARTBEAT_MULTI_TENANT.
-            multi_tenant: parse_bool_env(
-                "HEARTBEAT_MULTI_TENANT",
-                optional_env("GATEWAY_USER_TOKENS")?.is_some(),
-            )?,
+            multi_tenant: parse_bool_env("HEARTBEAT_MULTI_TENANT", false)?,
        })
    }
 }
@@ -26,9 +26,13 @@ fn row_to_user(row: &libsql::Row) -> Result<UserRecord, DatabaseError> {
    })
 }

-fn row_to_api_token(row: &libsql::Row) -> ApiTokenRecord {
-    ApiTokenRecord {
-        id: get_text(row, 0).parse().unwrap_or_default(),
+fn row_to_api_token(row: &libsql::Row) -> Result<ApiTokenRecord, DatabaseError> {
+    let id_str = get_text(row, 0);
+    let id: Uuid = id_str
+        .parse()
+        .map_err(|e| DatabaseError::Serialization(format!("invalid UUID: {e}")))?;
+    Ok(ApiTokenRecord {
+        id,
        user_id: get_text(row, 1),
        name: get_text(row, 2),
        token_prefix: get_text(row, 3),
@@ -36,12 +40,16 @@ fn row_to_api_token(row: &libsql::Row) -> ApiTokenRecord {
        last_used_at: get_opt_ts(row, 5),
        created_at: get_ts(row, 6),
        revoked_at: get_opt_ts(row, 7),
-    }
+    })
 }

-fn row_to_invitation(row: &libsql::Row) -> InvitationRecord {
-    InvitationRecord {
-        id: get_text(row, 0).parse().unwrap_or_default(),
+fn row_to_invitation(row: &libsql::Row) -> Result<InvitationRecord, DatabaseError> {
+    let id_str = get_text(row, 0);
+    let id: Uuid = id_str
+        .parse()
+        .map_err(|e| DatabaseError::Serialization(format!("invalid UUID: {e}")))?;
+    Ok(InvitationRecord {
+        id,
        email: get_opt_text(row, 1),
        invited_by: get_text(row, 2),
        status: get_text(row, 3),
@@ -49,7 +57,7 @@ fn row_to_invitation(row: &libsql::Row) -> InvitationRecord {
        accepted_at: get_opt_ts(row, 5),
        accepted_by: get_opt_text(row, 6),
        created_at: get_ts(row, 7),
-    }
+    })
 }

 #[async_trait]
@@ -274,7 +282,7 @@ impl UserStore for LibSqlBackend {
            .await
            .map_err(|e| DatabaseError::Query(e.to_string()))?
        {
-            tokens.push(row_to_api_token(&row));
+            tokens.push(row_to_api_token(&row)?);
        }
        Ok(tokens)
    }
@@ -328,8 +336,12 @@ impl UserStore for LibSqlBackend {
            .map_err(|e| DatabaseError::Query(e.to_string()))?
        {
            Some(row) => {
+                let id_str = get_text(&row, 0);
+                let token_id: Uuid = id_str
+                    .parse()
+                    .map_err(|e| DatabaseError::Serialization(format!("invalid UUID: {e}")))?;
                let token = ApiTokenRecord {
-                    id: get_text(&row, 0).parse().unwrap_or_default(),
+                    id: token_id,
                    user_id: get_text(&row, 1),
                    name: get_text(&row, 2),
                    token_prefix: get_text(&row, 3),
@@ -410,7 +422,10 @@ impl UserStore for LibSqlBackend {
            .query(
                r#"
                SELECT id, email, invited_by, status, expires_at, accepted_at, accepted_by, created_at
-                FROM invitations WHERE invite_token_hash = ?1
+                FROM invitations
+                WHERE invite_token_hash = ?1
+                  AND status = 'pending'
+                  AND expires_at > strftime('%Y-%m-%dT%H:%M:%S', 'now')
                "#,
                params![libsql::Value::Blob(invite_hash.to_vec())],
            )
@@ -422,7 +437,7 @@ impl UserStore for LibSqlBackend {
            .await
            .map_err(|e| DatabaseError::Query(e.to_string()))?
        {
-            Some(row) => Ok(Some(row_to_invitation(&row))),
+            Some(row) => Ok(Some(row_to_invitation(&row)?)),
            None => Ok(None),
        }
    }
@@ -478,7 +493,7 @@ impl UserStore for LibSqlBackend {
            .await
            .map_err(|e| DatabaseError::Query(e.to_string()))?
        {
-            invitations.push(row_to_invitation(&row));
+            invitations.push(row_to_invitation(&row)?);
        }
        Ok(invitations)
    }
@@ -727,14 +742,21 @@ mod tests {
            .await
            .unwrap();

-        // Verify accepted
-        let accepted = db
-            .get_invitation_by_hash(&invite_hash)
-            .await
-            .unwrap()
-            .unwrap();
-        assert_eq!(accepted.status, "accepted");
-        assert_eq!(accepted.accepted_by, Some("newuser".to_string()));
+        // After acceptance, the invitation should no longer be found via hash
+        // lookup (which filters for status='pending')
+        assert!(
+            db.get_invitation_by_hash(&invite_hash)
+                .await
+                .unwrap()
+                .is_none(),
+            "Accepted invitation should not be returned by pending-only lookup"
+        );
+
+        // Verify via list that it was accepted
+        let all = db.list_invitations(Some("alice")).await.unwrap();
+        assert_eq!(all.len(), 1);
+        assert_eq!(all[0].status, "accepted");
+        assert_eq!(all[0].accepted_by, Some("newuser".to_string()));
    }

    #[tokio::test]
@@ -2314,7 +2314,7 @@ impl Store {
    pub async fn get_user(&self, id: &str) -> Result<Option<UserRecord>, DatabaseError> {
        let conn = self.conn().await?;
        let row = conn
-            .query_opt("SELECT * FROM users WHERE id = $1", &[&id])
+            .query_opt("SELECT id, email, display_name, status, created_at, updated_at, last_login_at, created_by, metadata FROM users WHERE id = $1", &[&id])
            .await?;
        Ok(row.map(|r| row_to_user(&r)))
    }
@@ -2326,7 +2326,7 @@ impl Store {
    ) -> Result<Option<UserRecord>, DatabaseError> {
        let conn = self.conn().await?;
        let row = conn
-            .query_opt("SELECT * FROM users WHERE email = $1", &[&email])
+            .query_opt("SELECT id, email, display_name, status, created_at, updated_at, last_login_at, created_by, metadata FROM users WHERE email = $1", &[&email])
            .await?;
        Ok(row.map(|r| row_to_user(&r)))
    }
@@ -2337,13 +2337,13 @@ impl Store {
        let rows = match status {
            Some(s) => {
                conn.query(
-                    "SELECT * FROM users WHERE status = $1 ORDER BY created_at",
+                    "SELECT id, email, display_name, status, created_at, updated_at, last_login_at, created_by, metadata FROM users WHERE status = $1 ORDER BY created_at DESC",
                    &[&s],
                )
                .await?
            }
            None => {
-                conn.query("SELECT * FROM users ORDER BY created_at", &[])
+                conn.query("SELECT id, email, display_name, status, created_at, updated_at, last_login_at, created_by, metadata FROM users ORDER BY created_at DESC", &[])
                    .await?
            }
        };
@@ -591,97 +591,7 @@ async fn async_main() -> anyhow::Result<()> {
    let mut gateway_url: Option<String> = None;
    let mut sse_manager: Option<std::sync::Arc<ironclaw::channels::web::sse::SseManager>> = None;
    if let Some(ref gw_config) = config.channels.gateway {
-        // Migrate env-var users into DB on first run. If GATEWAY_USER_TOKENS is
-        // set and the users table is empty, insert the env-var users so they
-        // survive a switch to DB-backed auth.
-        if let (Some(user_tokens), Some(db)) = (&gw_config.user_tokens, &components.db) {
-            match db.has_any_users().await {
-                Ok(false) => {
-                    tracing::info!(
-                        "Migrating {} env-var users into database",
-                        user_tokens.len()
-                    );
-                    for (token, cfg) in user_tokens {
-                        use ironclaw::channels::web::auth::hash_token;
-                        let now = chrono::Utc::now();
-                        let user = ironclaw::db::UserRecord {
-                            id: cfg.user_id.clone(),
-                            email: None,
-                            display_name: cfg.user_id.clone(),
-                            status: "active".to_string(),
-                            created_at: now,
-                            updated_at: now,
-                            last_login_at: None,
-                            created_by: None,
-                            metadata: serde_json::json!({"source": "env_migration"}),
-                        };
-                        if let Err(e) = db.create_user(&user).await {
-                            tracing::warn!(
-                                user_id = cfg.user_id,
-                                "Failed to migrate user to DB: {}",
-                                e
-                            );
-                            continue;
-                        }
-                        let token_hash = hash_token(token);
-                        let prefix = if token.len() >= 8 {
-                            &token[..8]
-                        } else {
-                            token.as_str()
-                        };
-                        if let Err(e) = db
-                            .create_api_token(
-                                &cfg.user_id,
-                                "env-migrated",
-                                &token_hash,
-                                prefix,
-                                None,
-                            )
-                            .await
-                        {
-                            tracing::warn!(
-                                user_id = cfg.user_id,
-                                "Failed to migrate token to DB: {}",
-                                e
-                            );
-                        }
-                    }
-                    tracing::info!("Env-var user migration complete");
-                }
-                Ok(true) => {
-                    tracing::info!(
-                        "GATEWAY_USER_TOKENS is set but DB already has users — \
-                         env-var tokens will be checked first, DB tokens as fallback. \
-                         Consider removing GATEWAY_USER_TOKENS and managing users via \
-                         /api/admin/users and /api/tokens endpoints."
-                    );
-                }
-                Err(e) => {
-                    tracing::warn!("Could not check for existing users: {}", e);
-                }
-            }
-        }
-
-        // Build multi-user auth state if user_tokens is configured, else single-user.
-        let mut gw = if let Some(ref user_tokens) = gw_config.user_tokens {
-            use ironclaw::channels::web::auth::{MultiAuthState, UserIdentity};
-            let tokens = user_tokens
-                .iter()
-                .map(|(token, cfg)| {
-                    (
-                        token.clone(),
-                        UserIdentity {
-                            user_id: cfg.user_id.clone(),
-                            workspace_read_scopes: cfg.workspace_read_scopes.clone(),
-                        },
-                    )
-                })
-                .collect();
-            let auth = MultiAuthState::multi(tokens);
-            GatewayChannel::new_multi_auth(gw_config.clone(), auth)
-        } else {
-            GatewayChannel::new(gw_config.clone())
-        };
+        let mut gw = GatewayChannel::new(gw_config.clone());
        gw = gw.with_llm_provider(Arc::clone(&components.llm));
        if let Some(ref ws) = components.workspace {
            gw = gw.with_workspace(Arc::clone(ws));
@@ -431,7 +431,6 @@ mod tests {
            user_id: "test".to_string(),
            workspace_read_scopes: Vec::new(),
            memory_layers: Vec::new(),
-            user_tokens: None,
        });
        c
    }
@@ -445,7 +444,6 @@ mod tests {
            user_id: "test".to_string(),
            workspace_read_scopes: Vec::new(),
            memory_layers: Vec::new(),
-            user_tokens: None,
        });
        c
    }