diff --git a/src/channels/web/auth.rs b/src/channels/web/auth.rs index 988901f5..7a4afd05 100644 --- a/src/channels/web/auth.rs +++ b/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, - /// LRU cache: token_hash → (identity, inserted_at). - cache: Arc>>, + /// Bounded LRU cache: token_hash → (identity, inserted_at). + cache: Arc>>, } impl DbAuthenticator { @@ -143,7 +147,10 @@ impl DbAuthenticator { pub fn new(store: Arc) -> 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 { 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) diff --git a/src/channels/web/handlers/users.rs b/src/channels/web/handlers/users.rs index 01c92408..eccf2882 100644 --- a/src/channels/web/handlers/users.rs +++ b/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>, - AuthenticatedUser(_user): AuthenticatedUser, + AdminUser(_user): AdminUser, ) -> Result, (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>, - AuthenticatedUser(_user): AuthenticatedUser, + AdminUser(_user): AdminUser, Path(id): Path, ) -> Result, (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>, - AuthenticatedUser(_user): AuthenticatedUser, + AdminUser(_user): AdminUser, Path(id): Path, Json(body): Json, ) -> Result, (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>, - AuthenticatedUser(_user): AuthenticatedUser, + AdminUser(_user): AdminUser, Path(id): Path, ) -> Result, (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>, - AuthenticatedUser(_user): AuthenticatedUser, + AdminUser(_user): AdminUser, Path(id): Path, ) -> Result, (StatusCode, String)> { let store = state.store.as_ref().ok_or(( diff --git a/src/channels/web/tests/multi_tenant.rs b/src/channels/web/tests/multi_tenant.rs index 4f10fb6e..c53436ba 100644 --- a/src/channels/web/tests/multi_tenant.rs +++ b/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"); + } + } +} diff --git a/src/db/libsql/users.rs b/src/db/libsql/users.rs index 85e2a287..9fec6a81 100644 --- a/src/db/libsql/users.rs +++ b/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() + ); + } }