sciagent/docs/auth-implementation-feedback.md

# Evaluation: Auth, Registration, Roles, and User Management

## The most important finding first

The current registration endpoint trusts a client-supplied `role`. This isn't a gap — it's a privilege escalation. Anyone who can hit `/api/v1/auth/register` can `POST` `role: "admin"` and become a Quản trị viên. Bypassing the regex with a `@ump.edu.vn` address is the only constraint. Fix this before anything else; the rest of the cleanup is hygiene by comparison.

## Current implementation

**Email policy is wrong on both ends.** `_normalize_ump_email` and the client `validateUmpEmail` both reject `@umc.edu.vn`. Two places to fix, and the server is authoritative — the client check is UX only.

**The login role picker is conceptually inverted.** The spec says role flows from server to client based on identity. Today, `Login.tsx` makes the user pick `loginRole` and `buildUserWithSelectedRole` then *validates* the pick against DB roles, failing with a Vietnamese error if it doesn't match. That's not "no role picker" — it's a worse version of one, because legitimate users get an unhelpful error when they pick the wrong row in a Select they shouldn't see at all.

**`buildUserWithSelectedRole` after registration is circular.** The client picks a role, the server stores it (it shouldn't), and then the client confirms the role it just sent. The only reason this "works" is that the registration vulnerability lets the client decide. Once registration is fixed to derive the role server-side, this call needs to disappear or become "trust the server's `roles[]`."

**Enum drift in PostgreSQL.** `user_role` defines `applicant`, `council_member`, `editor`, `admin`, `viewer` — five values — but the auth code only ever writes/reads three. The doc itself notes that `applicant` maps to `viewer` "in the app." That's a design smell: either the DB has two pairs of synonyms (in which case drop two), or the Vietnamese taxonomy actually has finer distinctions the code is collapsing. Whichever it is, decide and migrate. Living with both is how you end up with users you can't query consistently.

**Sidebar dead link.** `/dashboard/users` is linked from `DashboardSidebar.tsx` but no route handles it, so it falls through to the 404. Admins clicking this today get nothing.

**JWT staleness.** Roles are baked into the JWT at issue time. When admin promotes a viewer to Hội đồng (once that flow exists), the change won't take effect until refresh. Acceptable, but the new admin UI should make this visible — either auto-refresh affected sessions or document the lag.

## The proposed plan

The five suggested directions are pointed in the right direction but underspecified in places that will matter during implementation.

**(1) Shared allow-list — fine, but the server is the only one that has to be right.** The frontend regex is a UX nicety; treat it as such. Don't overengineer "sharing" — duplicating a small list in two places is cheaper than building a config delivery mechanism for two strings.

**(2) Derive role on register — yes, and also reconcile on login.** The plan says "optionally" reconcile on login. Make it non-optional. If the admin email list changes (someone added, someone removed), you don't want stale `user_roles` rows to outlive the policy. A simple rule: on every login and refresh, if the email is in the admin list and the user lacks `admin`, add it; if the email is *not* in the list and the user has `admin` *granted by the email rule*, remove it. That last clause matters — you don't want to wipe admin grants made through future UI tooling.

**Hardcoding the five emails in source is brittle.** People leave institutions. Put them in environment config or a seeded DB table (`admin_emails`) so ops can change them without a deploy. A DB table also gives you an audit trail.

**(3) Remove the role selectors — yes, but resolve the multi-role question.** The DB supports multiple `user_roles` rows per user; the UI has been pretending one is "active" via `localStorage['auth-active-role']`. Once registration assigns exactly one role and login stops asking, can a user ever have more than one role? If yes (e.g., a Hội đồng member who's also an admin), you need a deterministic rule for `currentRole` — highest-privilege wins is the usual answer. If no, simplify the data model and stop reading `user_roles` as a list.

**(4) Admin API — the plan stops at read.** "List users + roles" satisfies the literal text of the requirement, but the requirement is incoherent without writes. The product says admins should see who is Người nộp đơn vs Hội đồng. How does anyone *become* Hội đồng? Not through registration (everyone defaults to viewer), not through the email list (that's admin only). You need at least:

- `POST /api/v1/admin/users/:id/roles` to grant Hội đồng
- `DELETE /api/v1/admin/users/:id/roles/:role` to revoke it
- Probably `PATCH /api/v1/admin/users/:id` for `is_active` (deactivate without deletion)

Without these, the UserManagement page is a read-only museum and there's no path for a viewer to ever become a council member.

**(5) UserManagement page — same issue.** Plan it as a CRUD-capable surface from day one. Listing without ability to act means admins will ask for "edit" two weeks after launch and you'll rebuild half of it.

## What the plan doesn't mention but should

**Data migration.** Right now there are presumably users in `user_roles` whose `role` was set by the broken registration flow. After fix, you need a one-time migration: for each user, if their email is in the admin list set `admin`; otherwise set `viewer` (and decide what to do with existing `editor` rows — preserve as Hội đồng, or wipe and require re-grant). Document this; don't let it get discovered in production.

**The `applicant` / `council_member` enum values.** Decide their fate as part of this work. If the answer is "they're aliases for viewer/editor," write a migration that consolidates and drops them from the enum. If the answer is "they're the real names and viewer/editor were a mistake," do the inverse. Don't ship the auth refactor while leaving five enum values where three are real.

**Rate limiting on register.** Once role assignment is server-derived, registration becomes lower-risk, but it's still an unauthenticated write endpoint. If you don't already, add basic rate limiting per IP — easy to forget when you're focused on the role logic.

**Tests for the email allow-list and admin derivation.** These are exactly the rules that will silently regress when someone touches the regex six months from now. Worth a small table-driven test suite: each of the five admin emails → admin; a non-admin `@ump.edu.vn` → viewer; a non-admin `@umc.edu.vn` → viewer; a `@gmail.com` → 400; case and whitespace variants → normalized.

## Suggested order of work

1. Fix registration to ignore client role and derive from email. Ship this alone if you have to — it closes the privilege escalation.
2. Add `@umc.edu.vn` to the allow-list (server first, client second).
3. Remove role selectors from Login/Register UI; simplify `buildUserWithSelectedRole`.
4. Build the admin list/grant/revoke API with proper authz.
5. Build the UserManagement page against it.
6. Migration + enum cleanup.

Steps 1–3 are mostly deletion and should be small. Steps 4–5 are where the real new code lives.