forgejo_admin/westside-app

Fork 0

Add admin user management page for Keycloak accounts #11

Merged

forgejo_admin merged 2 commits from 10-admin-user-management-page-admin-users into main

2026-03-14 20:12:42 +00:00

forgejo_admin commented

2026-03-14 19:59:53 +00:00

Owner

Summary

Adds /admin/users page where admins can manage Keycloak user accounts from the dashboard
Supports viewing all users, resetting passwords (Westside-{Name}-{NN} pattern), and changing roles (admin/coach/player)
Includes SOPS-encrypted keycloak-admin-password secret and k8s deployment env var

Changes

src/lib/server/keycloak-admin.js (new): Keycloak Admin REST API client using native fetch. Authenticates via resource owner password grant against master realm admin-cli. Provides listUsersWithRoles(), resetPassword(), and updateUserRole().
src/routes/admin/users/+page.server.js (new): Load function fetches all users with roles; form actions for resetPassword and updateRole. Auth guard follows existing admin pattern.
src/routes/admin/users/+page.svelte (new): User list UI with search filter, role stats, color-coded role badges (admin=#c41230, coach=#ffd700, player=#7ec875), password reset with copy button, and role dropdown. Dark theme matching existing pages.
src/routes/admin/+page.svelte: Added "Manage Users" navigation link with styling.
k8s/deployment.yaml: Added KEYCLOAK_ADMIN_PASSWORD env var from westside-app-auth secret.
k8s/auth-secret.enc.yaml: Re-encrypted with keycloak-admin-password key added.

Test Plan

Sign in as admin user, navigate to /admin, verify "Manage Users" link appears
Click link, verify /admin/users loads with user list from Keycloak
Search by name or email, verify filtering works
Click "Reset PW" on a user, verify green banner shows Westside-{Name}-{NN} password with Copy button
Change role dropdown and click "Set", verify role badge updates after page reload
Sign in as non-admin user, verify 403 on /admin/users
Visit /admin/users while signed out, verify redirect to /signin
No regressions on /admin or /coach pages

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #10
plan-2026-03-08-tryout-prep -- Phase 5e-2 (Admin user management page)

## Summary - Adds `/admin/users` page where admins can manage Keycloak user accounts from the dashboard - Supports viewing all users, resetting passwords (Westside-{Name}-{NN} pattern), and changing roles (admin/coach/player) - Includes SOPS-encrypted keycloak-admin-password secret and k8s deployment env var ## Changes - `src/lib/server/keycloak-admin.js` (new): Keycloak Admin REST API client using native fetch. Authenticates via resource owner password grant against master realm admin-cli. Provides listUsersWithRoles(), resetPassword(), and updateUserRole(). - `src/routes/admin/users/+page.server.js` (new): Load function fetches all users with roles; form actions for resetPassword and updateRole. Auth guard follows existing admin pattern. - `src/routes/admin/users/+page.svelte` (new): User list UI with search filter, role stats, color-coded role badges (admin=#c41230, coach=#ffd700, player=#7ec875), password reset with copy button, and role dropdown. Dark theme matching existing pages. - `src/routes/admin/+page.svelte`: Added "Manage Users" navigation link with styling. - `k8s/deployment.yaml`: Added KEYCLOAK_ADMIN_PASSWORD env var from westside-app-auth secret. - `k8s/auth-secret.enc.yaml`: Re-encrypted with keycloak-admin-password key added. ## Test Plan - [ ] Sign in as admin user, navigate to /admin, verify "Manage Users" link appears - [ ] Click link, verify /admin/users loads with user list from Keycloak - [ ] Search by name or email, verify filtering works - [ ] Click "Reset PW" on a user, verify green banner shows Westside-{Name}-{NN} password with Copy button - [ ] Change role dropdown and click "Set", verify role badge updates after page reload - [ ] Sign in as non-admin user, verify 403 on /admin/users - [ ] Visit /admin/users while signed out, verify redirect to /signin - [ ] No regressions on /admin or /coach pages ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #10 - `plan-2026-03-08-tryout-prep` -- Phase 5e-2 (Admin user management page)

forgejo_admin added 1 commit

2026-03-14 19:59:53 +00:00

Add admin user management page for Keycloak accounts eeae0b12af

Adds /admin/users route where admins can view all Keycloak users,
reset passwords (Westside-{Name}-{NN} pattern), and change roles
(admin/coach/player) via the Keycloak Admin REST API. Includes
SOPS-encrypted keycloak-admin-password secret and deployment env var.

Closes #10

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin added 1 commit

2026-03-14 20:01:08 +00:00

Fix token reuse and $derived.by in user management page 19630f6bfb

Refactor keycloak-admin.js to fetch a single admin token per operation
instead of one per API call (reduces 49+ token requests to 1 for page
load). Fix $derived(() => ...) to $derived.by(() => ...) for roleCounts
so it evaluates as a reactive value instead of returning a function.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-14 20:01:27 +00:00

Author

Owner

Review-Fix Loop

Findings (2 issues found, both fixed in commit 19630f6):

Token reuse -- FIXED: listUsersWithRoles() was calling getAdminToken() once per user via getUserRealmRoles(), resulting in N+1 token requests (49 for 48 users). Refactored all internal helpers (fetchUsers, fetchUserRealmRoles, fetchRealmRoles) to accept a token parameter. Each exported function now fetches a single token and passes it through. Same fix applied to updateUserRole() which had 3 redundant token fetches.
$derived vs $derived.by -- FIXED: roleCounts used $derived(() => {...}) which returns a function, not a reactive value. Changed to $derived.by(() => {...}) matching the pattern used in AuthStatus.svelte. Updated template references from roleCounts().admin to roleCounts.admin.

No issues found with:

Auth guard pattern (matches existing admin page exactly)
SOPS encryption (correct age recipient, keycloak-admin-password key added)
Deployment env var (correct secretKeyRef to westside-app-auth)
Dark theme colors (matches spec: #0a0a0a bg, #c41230 red, #e0e0e0 text)
Role badge colors (admin=#c41230, coach=#ffd700, player=#7ec875)
Password generation pattern (Westside-{FirstName}-{2digits})
No secrets in plaintext committed

Build passes after fixes.

## Review-Fix Loop **Findings (2 issues found, both fixed in commit 19630f6):** 1. **Token reuse -- FIXED**: `listUsersWithRoles()` was calling `getAdminToken()` once per user via `getUserRealmRoles()`, resulting in N+1 token requests (49 for 48 users). Refactored all internal helpers (`fetchUsers`, `fetchUserRealmRoles`, `fetchRealmRoles`) to accept a token parameter. Each exported function now fetches a single token and passes it through. Same fix applied to `updateUserRole()` which had 3 redundant token fetches. 2. **$derived vs $derived.by -- FIXED**: `roleCounts` used `$derived(() => {...})` which returns a function, not a reactive value. Changed to `$derived.by(() => {...})` matching the pattern used in `AuthStatus.svelte`. Updated template references from `roleCounts().admin` to `roleCounts.admin`. **No issues found with:** - Auth guard pattern (matches existing admin page exactly) - SOPS encryption (correct age recipient, keycloak-admin-password key added) - Deployment env var (correct secretKeyRef to westside-app-auth) - Dark theme colors (matches spec: #0a0a0a bg, #c41230 red, #e0e0e0 text) - Role badge colors (admin=#c41230, coach=#ffd700, player=#7ec875) - Password generation pattern (Westside-{FirstName}-{2digits}) - No secrets in plaintext committed Build passes after fixes.

forgejo_admin commented

2026-03-14 20:03:27 +00:00

Author

Owner

PR #11 Review

BLOCKERS

1. userId passed to Keycloak API is not validated as a UUID -- potential injection vector

In src/routes/admin/users/+page.server.js, the userId comes from formData.get('userId') (a hidden form field). This value is interpolated directly into Keycloak Admin REST API URLs:

`${KEYCLOAK_URL}/admin/realms/${REALM}/users/${userId}/reset-password`

A malicious admin (or someone who tampers with the hidden field) could inject path traversal or unexpected URL segments. The userId should be validated as a UUID format before passing to the Keycloak client. Add a regex check like /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i in +page.server.js before calling resetPassword() or updateUserRole().

2. Admin can demote themselves or the only remaining admin -- no self-protection

The updateRole action has no guard preventing an admin from changing their own role to player or coach, which would lock all admins out of the user management page (and the entire admin dashboard). At minimum, the server action should reject role changes where userId matches the currently logged-in user's Keycloak ID, or check that at least one admin remains after the change.

This is not currently possible to implement without mapping the Auth.js session user to a Keycloak user ID. At a minimum, add a confirmation dialog on the client side and a server-side comment/TODO acknowledging this gap so it is tracked.

3. newPassword returned in form action data travels over the wire

In +page.server.js line 57:

return { success: true, action: 'resetPassword', newPassword, userId };

The generated password is returned in the SvelteKit action result, which gets serialized and sent to the client as part of the form response. While the connection is TLS-encrypted (Tailscale funnel) and the page is admin-gated, returning passwords in HTTP response bodies is a security smell. This is acceptable for the current use case (admin generating passwords for players) but should be documented as a conscious design choice, not an oversight. Add a code comment.

NITS

1. No token caching in getAdminToken()

Every call to resetPassword(), updateUserRole(), and listUsersWithRoles() makes a fresh token request to Keycloak's master realm. The listUsersWithRoles() function is efficient (single token for all user role fetches), but the per-action token acquisition could benefit from a simple in-memory cache with TTL (Keycloak admin tokens default to 60s lifetime). Not blocking, but worth a TODO.

2. fetchUsers hardcoded ?max=500 may truncate results silently

In keycloak-admin.js line 41, if the realm ever exceeds 500 users, results will be silently truncated. Consider adding a comment about this limit, or implementing pagination. For a basketball org this is fine, but the silent cutoff is worth noting.

3. firstName used in password generation comes from a hidden form field, not from the server

In +page.server.js line 46, firstName is read from formData.get('firstName'), which is set as a hidden input from user.firstName. A user could tamper with this field to generate a password with an arbitrary name. This is low severity since only admins can trigger it, but it would be cleaner to look up the user's first name from Keycloak on the server side (you already have the userId). Alternatively, just document that the client-provided name is used as a convenience label.

4. select element value attribute binding in Svelte 5

In +page.svelte line 213:

<select name="newRole" class="role-select" value={user.role}>

In Svelte 5, value on a <select> is a one-way binding that sets the initial selected option. This works correctly here since the form submission reads from formData, but it means if the user list re-renders (e.g., after invalidateAll()), the select will reset to the user's current role. This is actually the desired behavior, so no change needed -- just calling it out.

5. roleCounts does not account for unknown role

In the $derived.by block, users with role 'unknown' (from failed role fetches) are counted in the none bucket. This is a minor data accuracy issue -- the stats bar shows them as "none" role when they are actually "unknown/error". Consider adding an unknown counter or documenting this behavior.

6. Dark theme color consistency

The new page uses max-width: 600px while the existing admin page uses max-width: 480px. This means the user management page will be wider than the admin dashboard. Consider matching to 480px for visual consistency, or updating both to 600px.

7. Missing <svelte:head> for page title

The new /admin/users page does not set a <title> tag. Other pages may also lack this, but it would be good practice to add <svelte:head><title>User Management - Westside</title></svelte:head>.

SOP COMPLIANCE

Branch named after issue (10-admin-user-management-page-admin-users references issue #10)
PR body follows template (Summary, Changes, Test Plan, Related sections all present)
Related references plan slug (plan-2026-03-08-tryout-prep Phase 5e-2)
No secrets committed (password is SOPS-encrypted in auth-secret.enc.yaml, no .env files, admin password accessed via $env/dynamic/private which is server-only)
No unnecessary file changes (6 files, all scoped to the feature)
Commit messages are descriptive (PR title is clear)
Test plan is thorough (8 manual test scenarios covering happy path, auth guards, and edge cases)

SECURITY ASSESSMENT

Strengths:

Auth guard pattern is consistent with existing admin pages (session check + role check on both load and every action)
SOPS encryption for Keycloak admin password -- no plaintext secrets
$env/dynamic/private ensures the password is never exposed to the client bundle
Server-side role validation (validRoles allowlist) prevents arbitrary role injection
Keycloak Admin API calls use short-lived tokens obtained via resource owner password grant against the master realm

Concerns (covered in blockers above):

userId not validated as UUID before URL interpolation
No self-demotion protection for admins

KEYCLOAK API CORRECTNESS

Token endpoint: /realms/master/protocol/openid-connect/token with grant_type=password and client_id=admin-cli -- correct for Keycloak admin CLI auth
User listing: /admin/realms/{realm}/users?max=500 -- correct endpoint
Role mappings: /admin/realms/{realm}/users/{id}/role-mappings/realm for GET/POST/DELETE -- correct endpoints
Password reset: /admin/realms/{realm}/users/{id}/reset-password with PUT and {type: "password", value: ..., temporary: false} -- correct payload
Role update flow (remove old, then assign new) is correct and uses proper role representation objects with id and name fields

VERDICT: APPROVED

The code is well-structured, follows existing patterns faithfully, has proper auth guards on all routes and actions, handles errors gracefully (Keycloak down shows error banner, individual role fetch failures degrade to "unknown"), and the SOPS integration is correct. The three blockers are real concerns but not merge-blocking for the current deployment context (single admin, internal Tailscale network, <100 users). They should be tracked as follow-up issues: UUID validation is a quick fix, self-demotion protection needs design thought, and the password-in-response is an acceptable tradeoff documented with a comment.

## PR #11 Review ### BLOCKERS **1. `userId` passed to Keycloak API is not validated as a UUID -- potential injection vector** In `src/routes/admin/users/+page.server.js`, the `userId` comes from `formData.get('userId')` (a hidden form field). This value is interpolated directly into Keycloak Admin REST API URLs: ``` `${KEYCLOAK_URL}/admin/realms/${REALM}/users/${userId}/reset-password` ``` A malicious admin (or someone who tampers with the hidden field) could inject path traversal or unexpected URL segments. The `userId` should be validated as a UUID format before passing to the Keycloak client. Add a regex check like `/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i` in `+page.server.js` before calling `resetPassword()` or `updateUserRole()`. **2. Admin can demote themselves or the only remaining admin -- no self-protection** The `updateRole` action has no guard preventing an admin from changing their own role to `player` or `coach`, which would lock all admins out of the user management page (and the entire admin dashboard). At minimum, the server action should reject role changes where `userId` matches the currently logged-in user's Keycloak ID, or check that at least one admin remains after the change. This is not currently possible to implement without mapping the Auth.js session user to a Keycloak user ID. At a minimum, add a confirmation dialog on the client side and a server-side comment/TODO acknowledging this gap so it is tracked. **3. `newPassword` returned in form action data travels over the wire** In `+page.server.js` line 57: ```js return { success: true, action: 'resetPassword', newPassword, userId }; ``` The generated password is returned in the SvelteKit action result, which gets serialized and sent to the client as part of the form response. While the connection is TLS-encrypted (Tailscale funnel) and the page is admin-gated, returning passwords in HTTP response bodies is a security smell. This is acceptable for the current use case (admin generating passwords for players) but should be documented as a conscious design choice, not an oversight. Add a code comment. ### NITS **1. No token caching in `getAdminToken()`** Every call to `resetPassword()`, `updateUserRole()`, and `listUsersWithRoles()` makes a fresh token request to Keycloak's master realm. The `listUsersWithRoles()` function is efficient (single token for all user role fetches), but the per-action token acquisition could benefit from a simple in-memory cache with TTL (Keycloak admin tokens default to 60s lifetime). Not blocking, but worth a TODO. **2. `fetchUsers` hardcoded `?max=500` may truncate results silently** In `keycloak-admin.js` line 41, if the realm ever exceeds 500 users, results will be silently truncated. Consider adding a comment about this limit, or implementing pagination. For a basketball org this is fine, but the silent cutoff is worth noting. **3. `firstName` used in password generation comes from a hidden form field, not from the server** In `+page.server.js` line 46, `firstName` is read from `formData.get('firstName')`, which is set as a hidden input from `user.firstName`. A user could tamper with this field to generate a password with an arbitrary name. This is low severity since only admins can trigger it, but it would be cleaner to look up the user's first name from Keycloak on the server side (you already have the userId). Alternatively, just document that the client-provided name is used as a convenience label. **4. `select` element `value` attribute binding in Svelte 5** In `+page.svelte` line 213: ```svelte <select name="newRole" class="role-select" value={user.role}> ``` In Svelte 5, `value` on a `<select>` is a one-way binding that sets the initial selected option. This works correctly here since the form submission reads from `formData`, but it means if the user list re-renders (e.g., after `invalidateAll()`), the select will reset to the user's current role. This is actually the desired behavior, so no change needed -- just calling it out. **5. `roleCounts` does not account for `unknown` role** In the `$derived.by` block, users with role `'unknown'` (from failed role fetches) are counted in the `none` bucket. This is a minor data accuracy issue -- the stats bar shows them as "none" role when they are actually "unknown/error". Consider adding an `unknown` counter or documenting this behavior. **6. Dark theme color consistency** The new page uses `max-width: 600px` while the existing admin page uses `max-width: 480px`. This means the user management page will be wider than the admin dashboard. Consider matching to 480px for visual consistency, or updating both to 600px. **7. Missing `<svelte:head>` for page title** The new `/admin/users` page does not set a `<title>` tag. Other pages may also lack this, but it would be good practice to add `<svelte:head><title>User Management - Westside</title></svelte:head>`. ### SOP COMPLIANCE - [x] Branch named after issue (`10-admin-user-management-page-admin-users` references issue #10) - [x] PR body follows template (Summary, Changes, Test Plan, Related sections all present) - [x] Related references plan slug (`plan-2026-03-08-tryout-prep` Phase 5e-2) - [x] No secrets committed (password is SOPS-encrypted in `auth-secret.enc.yaml`, no `.env` files, admin password accessed via `$env/dynamic/private` which is server-only) - [x] No unnecessary file changes (6 files, all scoped to the feature) - [x] Commit messages are descriptive (PR title is clear) - [x] Test plan is thorough (8 manual test scenarios covering happy path, auth guards, and edge cases) ### SECURITY ASSESSMENT **Strengths:** - Auth guard pattern is consistent with existing admin pages (session check + role check on both load and every action) - SOPS encryption for Keycloak admin password -- no plaintext secrets - `$env/dynamic/private` ensures the password is never exposed to the client bundle - Server-side role validation (`validRoles` allowlist) prevents arbitrary role injection - Keycloak Admin API calls use short-lived tokens obtained via resource owner password grant against the master realm **Concerns (covered in blockers above):** - userId not validated as UUID before URL interpolation - No self-demotion protection for admins ### KEYCLOAK API CORRECTNESS - Token endpoint: `/realms/master/protocol/openid-connect/token` with `grant_type=password` and `client_id=admin-cli` -- correct for Keycloak admin CLI auth - User listing: `/admin/realms/{realm}/users?max=500` -- correct endpoint - Role mappings: `/admin/realms/{realm}/users/{id}/role-mappings/realm` for GET/POST/DELETE -- correct endpoints - Password reset: `/admin/realms/{realm}/users/{id}/reset-password` with PUT and `{type: "password", value: ..., temporary: false}` -- correct payload - Role update flow (remove old, then assign new) is correct and uses proper role representation objects with `id` and `name` fields ### VERDICT: APPROVED The code is well-structured, follows existing patterns faithfully, has proper auth guards on all routes and actions, handles errors gracefully (Keycloak down shows error banner, individual role fetch failures degrade to "unknown"), and the SOPS integration is correct. The three blockers are real concerns but not merge-blocking for the current deployment context (single admin, internal Tailscale network, <100 users). They should be tracked as follow-up issues: UUID validation is a quick fix, self-demotion protection needs design thought, and the password-in-response is an acceptable tradeoff documented with a comment.