fix: address PR #1626 review feedback — bounded LRU cache, admin auth, FK cleanup

- Replace HashMap with lru::LruCache in DbAuthenticator so the token
  cache is hard-bounded at 1024 entries (evicts LRU, not just expired)
- Gate admin user endpoints (list/detail/update/suspend/activate) with
  AdminUser extractor so members get 403 instead of full access
- Add api_tokens to libSQL delete_user cleanup list to prevent orphaned
  tokens (libSQL has no FK cascade)
- Add regression tests for all three fixes

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

src/channels/web/auth.rs

@@ -5,6 +5,7 @@
 //! handlers can extract it via `AuthenticatedUser`.
 
 use std::collections::HashMap;
+use std::num::NonZeroUsize;
 
 use axum::{
     extract::{FromRequestParts, Request, State},
@@ -121,17 +122,20 @@ impl MultiAuthState {
     }
 }
 
-/// DB-backed token authenticator with an in-memory LRU cache.
+/// DB-backed token authenticator with a bounded LRU cache.
 ///
 /// Checks an LRU cache first (TTL 60s), then falls back to a DB query.
-/// Cache entries expire naturally — revoking a token or suspending a user
-/// has at most 60s of stale authentication before the cache entry expires.
+/// The cache is bounded to `MAX_CACHE_ENTRIES` — when full, the least
+/// recently used entry is evicted regardless of TTL.
+///
+/// Revoking a token or suspending a user has at most 60s of stale
+/// authentication before the cache entry expires.
 #[derive(Clone)]
 #[allow(clippy::type_complexity)]
 pub struct DbAuthenticator {
     store: Arc<dyn Database>,
-    /// LRU cache: token_hash → (identity, inserted_at).
-    cache: Arc<RwLock<HashMap<[u8; 32], (UserIdentity, Instant)>>>,
+    /// Bounded LRU cache: token_hash → (identity, inserted_at).
+    cache: Arc<RwLock<lru::LruCache<[u8; 32], (UserIdentity, Instant)>>>,
 }
 
 impl DbAuthenticator {
@@ -143,7 +147,10 @@ impl DbAuthenticator {
     pub fn new(store: Arc<dyn Database>) -> Self {
         Self {
             store,
-            cache: Arc::new(RwLock::new(HashMap::new())),
+            cache: Arc::new(RwLock::new(lru::LruCache::new(
+                NonZeroUsize::new(Self::MAX_CACHE_ENTRIES)
+                    .expect("MAX_CACHE_ENTRIES must be non-zero"),
+            ))),
         }
     }
 
@@ -151,13 +158,15 @@ impl DbAuthenticator {
     pub async fn authenticate(&self, candidate: &str) -> Option<UserIdentity> {
         let hash = hash_token(candidate);
 
-        // Check cache first
+        // Check cache first (promotes to most-recent on hit)
         {
-            let cache = self.cache.read().await;
-            if let Some((identity, inserted_at)) = cache.get(&hash)
-                && inserted_at.elapsed().as_secs() < Self::CACHE_TTL_SECS
-            {
-                return Some(identity.clone());
+            let mut cache = self.cache.write().await;
+            if let Some((identity, inserted_at)) = cache.get(&hash) {
+                if inserted_at.elapsed().as_secs() < Self::CACHE_TTL_SECS {
+                    return Some(identity.clone());
+                }
+                // Expired — remove stale entry
+                cache.pop(&hash);
             }
         }
 
@@ -179,15 +188,10 @@ impl DbAuthenticator {
             let _ = store.record_login(&user_id).await;
         });
 
-        // Update cache
+        // Insert into bounded LRU — if full, least-recently-used entry is evicted
         {
             let mut cache = self.cache.write().await;
-            // Evict stale entries if cache is full
-            if cache.len() >= Self::MAX_CACHE_ENTRIES {
-                let now = Instant::now();
-                cache.retain(|_, (_, ts)| now.duration_since(*ts).as_secs() < Self::CACHE_TTL_SECS);
-            }
-            cache.insert(hash, (identity.clone(), Instant::now()));
+            cache.put(hash, (identity.clone(), Instant::now()));
         }
 
         Some(identity)

src/channels/web/handlers/users.rs

@@ -98,7 +98,7 @@ pub async fn users_create_handler(
 /// GET /api/admin/users — list all users.
 pub async fn users_list_handler(
     State(state): State<Arc<GatewayState>>,
-    AuthenticatedUser(_user): AuthenticatedUser,
+    AdminUser(_user): AdminUser,
 ) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
     let store = state.store.as_ref().ok_or((
         StatusCode::SERVICE_UNAVAILABLE,
@@ -133,7 +133,7 @@ pub async fn users_list_handler(
 /// GET /api/admin/users/{id} — get a single user.
 pub async fn users_detail_handler(
     State(state): State<Arc<GatewayState>>,
-    AuthenticatedUser(_user): AuthenticatedUser,
+    AdminUser(_user): AdminUser,
     Path(id): Path<String>,
 ) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
     let store = state.store.as_ref().ok_or((
@@ -164,7 +164,7 @@ pub async fn users_detail_handler(
 /// PATCH /api/admin/users/{id} — update a user's profile.
 pub async fn users_update_handler(
     State(state): State<Arc<GatewayState>>,
-    AuthenticatedUser(_user): AuthenticatedUser,
+    AdminUser(_user): AdminUser,
     Path(id): Path<String>,
     Json(body): Json<serde_json::Value>,
 ) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
@@ -214,7 +214,7 @@ pub async fn users_update_handler(
 /// POST /api/admin/users/{id}/suspend — suspend a user.
 pub async fn users_suspend_handler(
     State(state): State<Arc<GatewayState>>,
-    AuthenticatedUser(_user): AuthenticatedUser,
+    AdminUser(_user): AdminUser,
     Path(id): Path<String>,
 ) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
     let store = state.store.as_ref().ok_or((
@@ -243,7 +243,7 @@ pub async fn users_suspend_handler(
 /// POST /api/admin/users/{id}/activate — activate a user.
 pub async fn users_activate_handler(
     State(state): State<Arc<GatewayState>>,
-    AuthenticatedUser(_user): AuthenticatedUser,
+    AdminUser(_user): AdminUser,
     Path(id): Path<String>,
 ) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
     let store = state.store.as_ref().ok_or((

src/channels/web/tests/multi_tenant.rs

@@ -848,3 +848,153 @@ mod auth_enforcement {
         assert_eq!(resp.status(), StatusCode::UNAUTHORIZED);
     }
 }
+
+// ═══════════════════════════════════════════════════════════════════════
+// Admin Endpoint Role Enforcement Tests
+// ═══════════════════════════════════════════════════════════════════════
+
+mod admin_role_enforcement {
+    use super::*;
+    use crate::channels::web::handlers::users::{
+        users_activate_handler, users_detail_handler, users_list_handler,
+        users_suspend_handler, users_update_handler,
+    };
+    use axum::routing::patch;
+
+    /// Build a router with admin user endpoints behind multi-user auth.
+    /// Uses a member-role token and an admin-role token.
+    fn admin_router() -> Router {
+        let mut tokens = HashMap::new();
+        tokens.insert(
+            "tok-admin".to_string(),
+            UserIdentity {
+                user_id: "admin-user".to_string(),
+                role: "admin".to_string(),
+                workspace_read_scopes: vec![],
+            },
+        );
+        tokens.insert(
+            "tok-member".to_string(),
+            UserIdentity {
+                user_id: "member-user".to_string(),
+                role: "member".to_string(),
+                workspace_read_scopes: vec![],
+            },
+        );
+        let auth = MultiAuthState::multi(tokens);
+        let state = build_state(None, None);
+
+        Router::new()
+            .route("/api/admin/users", get(users_list_handler))
+            .route("/api/admin/users/{id}", get(users_detail_handler))
+            .route("/api/admin/users/{id}", patch(users_update_handler))
+            .route(
+                "/api/admin/users/{id}/suspend",
+                post(users_suspend_handler),
+            )
+            .route(
+                "/api/admin/users/{id}/activate",
+                post(users_activate_handler),
+            )
+            .layer(middleware::from_fn_with_state(
+                crate::channels::web::auth::CombinedAuthState::from(auth),
+                auth_middleware,
+            ))
+            .with_state(state)
+    }
+
+    /// Assert a request returns FORBIDDEN for a member token.
+    async fn assert_forbidden_for_member(app: &Router, method: Method, uri: &str) {
+        let req = Request::builder()
+            .method(method)
+            .uri(uri)
+            .header("Authorization", "Bearer tok-member")
+            .body(Body::empty())
+            .unwrap();
+        let resp = app.clone().oneshot(req).await.unwrap();
+        assert_eq!(
+            resp.status(),
+            StatusCode::FORBIDDEN,
+            "expected 403 for member on {}",
+            uri
+        );
+    }
+
+    #[tokio::test]
+    async fn test_admin_user_endpoints_reject_member_role() {
+        let app = admin_router();
+
+        assert_forbidden_for_member(&app, Method::GET, "/api/admin/users").await;
+        assert_forbidden_for_member(&app, Method::GET, "/api/admin/users/some-id").await;
+        assert_forbidden_for_member(
+            &app,
+            Method::POST,
+            "/api/admin/users/some-id/suspend",
+        )
+        .await;
+        assert_forbidden_for_member(
+            &app,
+            Method::POST,
+            "/api/admin/users/some-id/activate",
+        )
+        .await;
+    }
+
+    #[tokio::test]
+    async fn test_admin_user_endpoints_accept_admin_role() {
+        let app = admin_router();
+
+        // Admin token should pass auth (will get 503 since no DB, but not 403).
+        let req = Request::builder()
+            .uri("/api/admin/users")
+            .header("Authorization", "Bearer tok-admin")
+            .body(Body::empty())
+            .unwrap();
+        let resp = app.clone().oneshot(req).await.unwrap();
+        assert_ne!(
+            resp.status(),
+            StatusCode::FORBIDDEN,
+            "admin should not get 403"
+        );
+    }
+}
+
+// ═══════════════════════════════════════════════════════════════════════
+// DbAuthenticator Cache Bounded Tests
+// ═══════════════════════════════════════════════════════════════════════
+
+mod db_auth_cache {
+    use super::*;
+    use std::time::Instant;
+
+    #[tokio::test]
+    async fn test_cache_bounded_by_max_entries() {
+        // Access the internal cache and verify LRU eviction.
+        // We can't easily test through `authenticate()` since it hits the DB,
+        // so we test the LRU cache directly.
+        let cap = std::num::NonZeroUsize::new(4).unwrap();
+        let cache: lru::LruCache<[u8; 32], (UserIdentity, Instant)> = lru::LruCache::new(cap);
+        let cache = Arc::new(tokio::sync::RwLock::new(cache));
+
+        {
+            let mut c = cache.write().await;
+            for i in 0..10u8 {
+                let mut hash = [0u8; 32];
+                hash[0] = i;
+                c.put(
+                    hash,
+                    (
+                        UserIdentity {
+                            user_id: format!("user-{i}"),
+                            role: "member".to_string(),
+                            workspace_read_scopes: vec![],
+                        },
+                        Instant::now(),
+                    ),
+                );
+            }
+            // Cache must be bounded at capacity, not grown to 10.
+            assert_eq!(c.len(), 4, "cache should be bounded to capacity");
+        }
+    }
+}

src/db/libsql/users.rs

@@ -405,6 +405,7 @@ impl UserStore for LibSqlBackend {
             "memory_documents",
             "agent_jobs",
             "conversations",
+            "api_tokens",
         ] {
             conn.execute(
                 &format!("DELETE FROM {} WHERE user_id = ?1", table),
@@ -420,7 +421,6 @@ impl UserStore for LibSqlBackend {
         )
         .await
         .map_err(|e| DatabaseError::Query(e.to_string()))?;
-        // api_tokens cascade automatically via FK
         let rows = conn
             .execute("DELETE FROM users WHERE id = ?1", params![id])
             .await
@@ -697,4 +697,30 @@ mod tests {
         let tokens = db.list_api_tokens("alice").await.unwrap();
         assert!(tokens[0].last_used_at.is_some());
     }
+
+    #[tokio::test]
+    async fn test_delete_user_removes_api_tokens() {
+        let (db, _dir) = setup().await;
+        db.create_user(&test_user("alice")).await.unwrap();
+
+        let token_hash = hash("alice-tok");
+        db.create_api_token("alice", "primary", &token_hash, "alice-to", None)
+            .await
+            .unwrap();
+
+        // Verify token exists before deletion.
+        let tokens = db.list_api_tokens("alice").await.unwrap();
+        assert_eq!(tokens.len(), 1);
+
+        // Delete user — should also remove their api_tokens.
+        assert!(db.delete_user("alice").await.unwrap());
+
+        // api_tokens must be gone (not orphaned).
+        let tokens = db.list_api_tokens("alice").await.unwrap();
+        assert!(
+            tokens.is_empty(),
+            "expected api_tokens to be deleted with user, found {}",
+            tokens.len()
+        );
+    }
 }