Skip to content

Commit df40b22

Browse files
ilblackdragonclaude
andcommitted
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>
1 parent 646b63e commit df40b22

File tree

6 files changed

+43
-9
lines changed

6 files changed

+43
-9
lines changed

docs/USER_MANAGEMENT_API.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ All error responses return a plain text body with the error message and the corr
529529

530530
| Column | Type (PG / libSQL) | Notes |
531531
|--------|--------------------|-------|
532-
| `id` | `UUID` / `TEXT` | Primary key, UUID v4 |
532+
| `id` | `TEXT` / `TEXT` | Primary key; values are UUID v4 strings |
533533
| `email` | `TEXT UNIQUE` | Nullable |
534534
| `display_name` | `TEXT NOT NULL` | |
535535
| `status` | `TEXT NOT NULL` | `"active"` or `"suspended"` |

migrations/V14__users.sql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
-- User management tables for multi-tenant deployments.
22
--
33
-- Replaces the static GATEWAY_USER_TOKENS env var with DB-backed
4-
-- user registration, API token management, and invitation flow.
4+
-- user registration and API token management.
55

66
CREATE TABLE users (
7-
id TEXT PRIMARY KEY, -- matches existing user_id pattern (string, not UUID)
7+
id TEXT PRIMARY KEY, -- stored as TEXT; values are UUIDv4 strings
88
email TEXT UNIQUE, -- nullable for token-only users
99
display_name TEXT NOT NULL,
1010
status TEXT NOT NULL DEFAULT 'active', -- active | suspended | deactivated

src/channels/web/handlers/secrets.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,18 @@ pub async fn secrets_put_handler(
2727
Path((user_id, name)): Path<(String, String)>,
2828
Json(body): Json<serde_json::Value>,
2929
) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
30+
let name = name.to_lowercase();
31+
32+
let store = state.store.as_ref().ok_or((
33+
StatusCode::SERVICE_UNAVAILABLE,
34+
"Database not available".to_string(),
35+
))?;
36+
store
37+
.get_user(&user_id)
38+
.await
39+
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?
40+
.ok_or((StatusCode::NOT_FOUND, "User not found".to_string()))?;
41+
3042
let secrets = state.secrets_store.as_ref().ok_or((
3143
StatusCode::SERVICE_UNAVAILABLE,
3244
"Secrets store not available".to_string(),
@@ -67,7 +79,7 @@ pub async fn secrets_put_handler(
6779

6880
Ok(Json(serde_json::json!({
6981
"user_id": user_id,
70-
"name": name.to_lowercase(),
82+
"name": name,
7183
"status": "created",
7284
})))
7385
}
@@ -112,6 +124,18 @@ pub async fn secrets_delete_handler(
112124
AdminUser(_admin): AdminUser,
113125
Path((user_id, name)): Path<(String, String)>,
114126
) -> Result<Json<serde_json::Value>, (StatusCode, String)> {
127+
let name = name.to_lowercase();
128+
129+
let store = state.store.as_ref().ok_or((
130+
StatusCode::SERVICE_UNAVAILABLE,
131+
"Database not available".to_string(),
132+
))?;
133+
store
134+
.get_user(&user_id)
135+
.await
136+
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?
137+
.ok_or((StatusCode::NOT_FOUND, "User not found".to_string()))?;
138+
115139
let secrets = state.secrets_store.as_ref().ok_or((
116140
StatusCode::SERVICE_UNAVAILABLE,
117141
"Secrets store not available".to_string(),

src/channels/web/handlers/users.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,10 @@ pub async fn users_list_handler(
119119
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?;
120120

121121
// Fetch per-user summary stats in a single batch query.
122-
let summary_stats = store.user_summary_stats(None).await.unwrap_or_default();
122+
let summary_stats = store
123+
.user_summary_stats(None)
124+
.await
125+
.map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, e.to_string()))?;
123126

124127
let stats_map: std::collections::HashMap<String, _> = summary_stats
125128
.into_iter()

src/channels/web/static/app.js

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4352,13 +4352,16 @@ function loadUsers() {
43524352
apiFetch('/api/admin/users').then(function(data) {
43534353
renderUsersList(data.users || []);
43544354
}).catch(function(err) {
4355-
// Non-admin users get 403 — show a message instead of an error
43564355
var tbody = document.getElementById('users-tbody');
43574356
var empty = document.getElementById('users-empty');
43584357
if (tbody) tbody.innerHTML = '';
43594358
if (empty) {
43604359
empty.style.display = 'block';
4361-
empty.textContent = 'Admin access required to manage users.';
4360+
if (err.status === 403 || err.status === 401) {
4361+
empty.textContent = 'Admin access required to manage users.';
4362+
} else {
4363+
empty.textContent = 'Failed to load users: ' + err.message;
4364+
}
43624365
}
43634366
});
43644367
}

src/db/libsql/users.rs

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -487,7 +487,9 @@ impl UserStore for LibSqlBackend {
487487
.map_err(|e| DatabaseError::Query(e.to_string()))?
488488
{
489489
let cost_str = get_text(&row, 5);
490-
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).unwrap_or_default();
490+
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).map_err(|e| {
491+
DatabaseError::Query(format!("invalid cost value '{}': {}", cost_str, e))
492+
})?;
491493
stats.push(crate::db::UserUsageStats {
492494
user_id: get_text(&row, 0),
493495
model: get_text(&row, 1),
@@ -555,7 +557,9 @@ impl UserStore for LibSqlBackend {
555557
.map_err(|e| DatabaseError::Query(e.to_string()))?
556558
{
557559
let cost_str = get_text(&row, 2);
558-
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).unwrap_or_default();
560+
let total_cost = rust_decimal::Decimal::from_str_exact(&cost_str).map_err(|e| {
561+
DatabaseError::Query(format!("invalid cost value '{}': {}", cost_str, e))
562+
})?;
559563
stats.push(crate::db::UserSummaryStats {
560564
user_id: get_text(&row, 0),
561565
job_count: row

0 commit comments

Comments
 (0)