fix: address remaining review comments (round 2)

- Secrets handlers: normalize name to lowercase before store operations,
  validate target user_id exists (returns 404 if not found)
- libSQL: propagate cost parsing errors instead of unwrap_or_default()
  in both user_usage_stats and user_summary_stats
- users_list_handler: propagate user_summary_stats DB errors (was
  silently swallowed with unwrap_or_default)
- loadUsers: distinguish 401/403 (admin required) from other errors
- Docs: fix users.id type (TEXT not UUID), remove "invitation flow"
  from V14 migration comment

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
ilblackdragon@gmail.com
2026-03-26 14:05:33 -07:00
parent 646b63e3eb
commit df40b22f15
6 changed files with 43 additions and 9 deletions

View File

@@ -529,7 +529,7 @@ All error responses return a plain text body with the error message and the corr
| Column | Type (PG / libSQL) | Notes |
|--------|--------------------|-------|
| `id` | `UUID` / `TEXT` | Primary key, UUID v4 |
| `id` | `TEXT` / `TEXT` | Primary key; values are UUID v4 strings |
| `email` | `TEXT UNIQUE` | Nullable |
| `display_name` | `TEXT NOT NULL` | |
| `status` | `TEXT NOT NULL` | `"active"` or `"suspended"` |

View File

@@ -1,10 +1,10 @@
-- User management tables for multi-tenant deployments.
--
-- Replaces the static GATEWAY_USER_TOKENS env var with DB-backed
-- user registration, API token management, and invitation flow.
-- user registration and API token management.
CREATE TABLE users (
id TEXT PRIMARY KEY, -- matches existing user_id pattern (string, not UUID)
id TEXT PRIMARY KEY, -- stored as TEXT; values are UUIDv4 strings
email TEXT UNIQUE, -- nullable for token-only users
display_name TEXT NOT NULL,
status TEXT NOT NULL DEFAULT 'active', -- active | suspended | deactivated

View File

@@ -27,6 +27,18 @@ pub async fn secrets_put_handler(
Path((user_id, name)): Path<(String, String)>,
Json(body): Json<serde_json::Value>,
) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
let name = name.to_lowercase();
let store = state.store.as_ref().ok_or((
StatusCode::SERVICE_UNAVAILABLE,
"Database not available".to_string(),
))?;
store
.get_user(&user_id)
.await
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?
.ok_or((StatusCode::NOT_FOUND, "User not found".to_string()))?;
let secrets = state.secrets_store.as_ref().ok_or((
StatusCode::SERVICE_UNAVAILABLE,
"Secrets store not available".to_string(),
@@ -67,7 +79,7 @@ pub async fn secrets_put_handler(
Ok(Json(serde_json::json!({
"user_id": user_id,
"name": name.to_lowercase(),
"name": name,
"status": "created",
})))
}
@@ -112,6 +124,18 @@ pub async fn secrets_delete_handler(
AdminUser(_admin): AdminUser,
Path((user_id, name)): Path<(String, String)>,
) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
let name = name.to_lowercase();
let store = state.store.as_ref().ok_or((
StatusCode::SERVICE_UNAVAILABLE,
"Database not available".to_string(),
))?;
store
.get_user(&user_id)
.await
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?
.ok_or((StatusCode::NOT_FOUND, "User not found".to_string()))?;
let secrets = state.secrets_store.as_ref().ok_or((
StatusCode::SERVICE_UNAVAILABLE,
"Secrets store not available".to_string(),

View File

@@ -119,7 +119,10 @@ pub async fn users_list_handler(
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?;
// Fetch per-user summary stats in a single batch query.
let summary_stats = store.user_summary_stats(None).await.unwrap_or_default();
let summary_stats = store
.user_summary_stats(None)
.await
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?;
let stats_map: std::collections::HashMap<String, _> = summary_stats
.into_iter()

View File

@@ -4352,13 +4352,16 @@ function loadUsers() {
apiFetch('/api/admin/users').then(function(data) {
renderUsersList(data.users || []);
}).catch(function(err) {
// Non-admin users get 403 — show a message instead of an error
var tbody = document.getElementById('users-tbody');
var empty = document.getElementById('users-empty');
if (tbody) tbody.innerHTML = '';
if (empty) {
empty.style.display = 'block';
empty.textContent = 'Admin access required to manage users.';
if (err.status === 403 || err.status === 401) {
empty.textContent = 'Admin access required to manage users.';
} else {
empty.textContent = 'Failed to load users: ' + err.message;
}
}
});
}

View File

@@ -487,7 +487,9 @@ impl UserStore for LibSqlBackend {
.map_err(|e| DatabaseError::Query(e.to_string()))?
{
let cost_str = get_text(&row, 5);
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).unwrap_or_default();
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).map_err(|e| {
DatabaseError::Query(format!("invalid cost value '{}': {}", cost_str, e))
})?;
stats.push(crate::db::UserUsageStats {
user_id: get_text(&row, 0),
model: get_text(&row, 1),
@@ -555,7 +557,9 @@ impl UserStore for LibSqlBackend {
.map_err(|e| DatabaseError::Query(e.to_string()))?
{
let cost_str = get_text(&row, 2);
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).unwrap_or_default();
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).map_err(|e| {
DatabaseError::Query(format!("invalid cost value '{}': {}", cost_str, e))
})?;
stats.push(crate::db::UserSummaryStats {
user_id: get_text(&row, 0),
job_count: row