From 653e4efa760f06d1f1d134753265ae9dfa72ce4a Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 21 Aug 2026 16:18:29 -0700 Subject: [PATCH] Enforce the full user-management RBAC matrix, add self-service password change api/localauth now enforces every rule of the requested matrix, each checked inside the handler beyond RegisterRoutes' floor: - At least one owner must always exist -- handleDeleteUser and handleSetRole both refuse an operation that would leave zero owners (wouldRemoveLastOwner, backed by new store method CountUsersWithRole), whether the caller is admin or owner. - Owner can create/delete any role, including another owner (subject to the above). Admin can only create/delete viewer or editor -- GET/POST /auth/users and DELETE .../{id} moved from RoleOwner to RoleAdmin floor, with an inner check narrowing what an admin caller specifically may target. - Only a user can change their own password -- new POST /auth/password (RoleViewer floor, i.e. every role) requires the caller's current password (verified via new store method GetPasswordHashByID) and is now the only path to changing your own, including for an owner. The existing admin-reset endpoint (POST /auth/users/{id}/reset-password, also moved to RoleAdmin floor) now refuses id == the caller's own ID, and refuses an owner target unless the caller is themselves an owner -- "admin can change any password except an owner's; owner can change any password, even another owner's." - Role reassignment (PUT .../{id}/role) stays owner-only, unchanged beyond the last-owner guard above. New web/src/routes/account page (linked from NavSidebar next to "Log out", visible to every local-auth role) is the self-service password change UI. /users now mirrors the server's per-row restrictions client-side (disabled role selects/delete/reset buttons with an explanatory title, a restricted role list on the create form) so an admin never sees an action that would just 403 -- the server remains the actual authority. Verified live against real Postgres and in the browser: the full matrix via curl (owner creating a second owner, admin blocked from creating/deleting/resetting admin or owner accounts, last-owner delete and demote both blocked, admin resetting non-owner passwords, self-target reset rejected, self-service change with wrong/right current password), plus the actual /users page rendering correctly restricted for an admin session and a full change-password round trip through the real UI ending in a forced re-login with the new password. --- api/localauth/fake_test.go | 17 ++ api/localauth/handler.go | 196 ++++++++++++++-- api/localauth/handler_test.go | 275 +++++++++++++++++++++++ api/localauth/store.go | 32 +++ api/localauth/store_integration_test.go | 64 ++++++ web/src/lib/api.ts | 14 ++ web/src/lib/components/NavSidebar.svelte | 15 +- web/src/routes/account/+page.svelte | 178 +++++++++++++++ web/src/routes/account/+page.ts | 3 + web/src/routes/users/+page.svelte | 88 +++++++- 10 files changed, 851 insertions(+), 31 deletions(-) create mode 100644 web/src/routes/account/+page.svelte create mode 100644 web/src/routes/account/+page.ts diff --git a/api/localauth/fake_test.go b/api/localauth/fake_test.go index 137ee21..3ad4cc6 100644 --- a/api/localauth/fake_test.go +++ b/api/localauth/fake_test.go @@ -118,6 +118,23 @@ func (f *fakeStore) CountLocalUsers(_ context.Context) (int, error) { return len(f.users), nil } +func (f *fakeStore) CountUsersWithRole(_ context.Context, role authz.Role) (int, error) { + n := 0 + for _, u := range f.users { + if u.Role == role { + n++ + } + } + return n, nil +} + +func (f *fakeStore) GetPasswordHashByID(_ context.Context, id string) (string, error) { + if _, ok := f.users[id]; !ok { + return "", ErrNotFound + } + return f.hashes[id], nil +} + func (f *fakeStore) CreateSession(_ context.Context, userID, tenantID string, role authz.Role, ttl time.Duration) (string, error) { raw, hash, err := newOpaqueToken() if err != nil { diff --git a/api/localauth/handler.go b/api/localauth/handler.go index 6fd0b7a..f89874a 100644 --- a/api/localauth/handler.go +++ b/api/localauth/handler.go @@ -21,9 +21,11 @@ type store interface { ListUsers(ctx context.Context) ([]User, error) GetUserForLogin(ctx context.Context, username string) (*User, string, error) GetUserByID(ctx context.Context, id string) (*User, error) + GetPasswordHashByID(ctx context.Context, id string) (string, error) DeleteUser(ctx context.Context, id string) error SetPasswordHash(ctx context.Context, userID, hash string) error SetRole(ctx context.Context, userID string, role authz.Role) error + CountUsersWithRole(ctx context.Context, role authz.Role) (int, error) CreateSession(ctx context.Context, userID, tenantID string, role authz.Role, ttl time.Duration) (string, error) DeleteSessionByHash(ctx context.Context, tokenHash string) error } @@ -81,19 +83,41 @@ func NewHandler(logger *slog.Logger, store store, authorizer authz.Authorizer, s // cmd/api/main.go) -- a deployment that doesn't enable it simply never // registers these routes at all, so GET /auth/session (etc.) 404s // rather than needing its own "is this feature even on" response -// shape. Login/logout/session are deliberately NOT RequireRole-wrapped -// with anything above RoleViewer's floor: login is how you become -// authenticated in the first place, logout/session must work for any -// already-authenticated user regardless of role. +// shape. Login/logout/session/password are deliberately NOT +// RequireRole-wrapped with anything above RoleViewer's floor: login is +// how you become authenticated in the first place, logout/session/ +// changing your own password must work for any already-authenticated +// user regardless of role. +// +// User management's RBAC matrix (each floor here is the minimum to +// reach the route at all; every handler below applies a further, +// per-target check narrower than its floor): +// - GET/POST /auth/users, DELETE .../{id}, POST .../reset-password: +// RoleAdmin floor. An admin caller is then restricted to +// viewer/editor targets only (handleCreateUser/handleDeleteUser/ +// handleResetPassword); an owner caller has no such restriction. +// - DELETE .../{id} additionally refuses to remove the last owner, +// and POST .../reset-password refuses id == the caller's own ID +// (see POST /auth/password) and refuses an owner target unless the +// caller is themselves an owner. +// - PUT .../{id}/role stays RoleOwner-only (unchanged) but now also +// refuses to demote the last owner. +// - POST /auth/password (self-service, RoleViewer floor) is the only +// way to change your own password, verified against your current +// one -- this applies to every role including owner, since "an +// owner can change any password on behalf of a user" is about +// acting on someone ELSE's account, not a shortcut around +// confirming your own current password. func (h *Handler) RegisterRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /auth/login", h.handleLogin) mux.HandleFunc("POST /auth/logout", h.handleLogout) mux.HandleFunc("GET /auth/session", authz.RequireRole(h.authorizer, authz.RoleViewer, h.handleGetSession)) + mux.HandleFunc("POST /auth/password", authz.RequireRole(h.authorizer, authz.RoleViewer, h.handleChangeOwnPassword)) - mux.HandleFunc("GET /auth/users", authz.RequireRole(h.authorizer, authz.RoleOwner, h.handleListUsers)) - mux.HandleFunc("POST /auth/users", authz.RequireRole(h.authorizer, authz.RoleOwner, h.handleCreateUser)) - mux.HandleFunc("DELETE /auth/users/{id}", authz.RequireRole(h.authorizer, authz.RoleOwner, h.handleDeleteUser)) - mux.HandleFunc("POST /auth/users/{id}/reset-password", authz.RequireRole(h.authorizer, authz.RoleOwner, h.handleResetPassword)) + mux.HandleFunc("GET /auth/users", authz.RequireRole(h.authorizer, authz.RoleAdmin, h.handleListUsers)) + mux.HandleFunc("POST /auth/users", authz.RequireRole(h.authorizer, authz.RoleAdmin, h.handleCreateUser)) + mux.HandleFunc("DELETE /auth/users/{id}", authz.RequireRole(h.authorizer, authz.RoleAdmin, h.handleDeleteUser)) + mux.HandleFunc("POST /auth/users/{id}/reset-password", authz.RequireRole(h.authorizer, authz.RoleAdmin, h.handleResetPassword)) mux.HandleFunc("PUT /auth/users/{id}/role", authz.RequireRole(h.authorizer, authz.RoleOwner, h.handleSetRole)) } @@ -253,6 +277,15 @@ func (h *Handler) handleCreateUser(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, `role must be "viewer", "editor", "admin", or "owner"`) return } + // An admin caller (RegisterRoutes' RoleAdmin floor already excludes + // viewer/editor) may only create viewer/editor accounts -- creating + // an admin or owner account is owner-only. + if identity, ok := authz.IdentityFromContext(r.Context()); ok && !identity.Role.Satisfies(authz.RoleOwner) { + if role == authz.RoleAdmin || role == authz.RoleOwner { + writeError(w, http.StatusForbidden, "admin can only create viewer or editor accounts") + return + } + } hash, err := HashPassword(req.Password) if err != nil { @@ -273,19 +306,60 @@ func (h *Handler) handleCreateUser(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, userResponse{ID: user.ID, Username: user.Username, Role: string(user.Role), CreatedAt: user.CreatedAt}) } -// handleDeleteUser deliberately does not stop an admin from deleting -// their own account -- this package has no separate "you can't remove -// the last admin" guard; a single-operator prototype deployment is -// expected to know what it's doing here, same trust level the rest of -// this codebase's admin-only endpoints assume. +// handleDeleteUser deliberately does not stop a caller from deleting +// their own account (as long as it isn't the last owner, per the check +// below) -- a single-operator prototype deployment is expected to know +// what it's doing here, same trust level the rest of this codebase's +// admin-only endpoints assume. What it does stop, unconditionally: an +// admin caller (RegisterRoutes' RoleAdmin floor already excludes +// viewer/editor) deleting an admin or owner target -- that's +// owner-only -- and anyone at all deleting the last remaining owner. func (h *Handler) handleDeleteUser(w http.ResponseWriter, r *http.Request) { - if err := h.store.DeleteUser(r.Context(), r.PathValue("id")); err != nil { + id := r.PathValue("id") + target, err := h.store.GetUserByID(r.Context(), id) + if err != nil { + h.writeStoreErr(w, err, "deleting user") + return + } + + if identity, ok := authz.IdentityFromContext(r.Context()); ok && !identity.Role.Satisfies(authz.RoleOwner) { + if target.Role == authz.RoleAdmin || target.Role == authz.RoleOwner { + writeError(w, http.StatusForbidden, "admin can only delete viewer or editor accounts") + return + } + } + if target.Role == authz.RoleOwner { + if blocked, err := h.wouldRemoveLastOwner(r.Context(), w); err != nil { + return + } else if blocked { + writeError(w, http.StatusConflict, "cannot delete the last owner") + return + } + } + + if err := h.store.DeleteUser(r.Context(), id); err != nil { h.writeStoreErr(w, err, "deleting user") return } w.WriteHeader(http.StatusNoContent) } +// wouldRemoveLastOwner reports whether the default tenant currently has +// only one owner -- shared by handleDeleteUser (deleting that owner) +// and handleSetRole (demoting them) since both are exactly the same +// invariant violation from two different operations. Writes a 500 and +// returns a non-nil error itself on a store failure, so callers can +// just `return` on a non-nil error without writing their own. +func (h *Handler) wouldRemoveLastOwner(ctx context.Context, w http.ResponseWriter) (bool, error) { + n, err := h.store.CountUsersWithRole(ctx, authz.RoleOwner) + if err != nil { + h.logger.Error("counting owners", "error", err) + writeError(w, http.StatusInternalServerError, "checking owner count failed") + return false, err + } + return n <= 1, nil +} + type setRoleRequest struct { Role string `json:"role"` } @@ -294,6 +368,8 @@ type setRoleRequest struct { // re-promoting their own account -- same "single-operator prototype // deployment knows what it's doing" trust level handleDeleteUser's doc // comment already establishes for this package's owner-only endpoints. +// It does stop demoting the last remaining owner away from RoleOwner, +// the same invariant handleDeleteUser enforces for deletion. func (h *Handler) handleSetRole(w http.ResponseWriter, r *http.Request) { var req setRoleRequest if !decodeJSON(w, r, &req) { @@ -305,12 +381,27 @@ func (h *Handler) handleSetRole(w http.ResponseWriter, r *http.Request) { return } - if err := h.store.SetRole(r.Context(), r.PathValue("id"), role); err != nil { + id := r.PathValue("id") + current, err := h.store.GetUserByID(r.Context(), id) + if err != nil { + h.writeStoreErr(w, err, "updating role") + return + } + if current.Role == authz.RoleOwner && role != authz.RoleOwner { + if blocked, err := h.wouldRemoveLastOwner(r.Context(), w); err != nil { + return + } else if blocked { + writeError(w, http.StatusConflict, "cannot demote the last owner") + return + } + } + + if err := h.store.SetRole(r.Context(), id, role); err != nil { h.writeStoreErr(w, err, "updating role") return } - user, err := h.store.GetUserByID(r.Context(), r.PathValue("id")) + user, err := h.store.GetUserByID(r.Context(), id) if err != nil { h.writeStoreErr(w, err, "fetching updated user") return @@ -332,7 +423,30 @@ type resetPasswordResponse struct { Password string `json:"password,omitempty"` } +// handleResetPassword is exclusively for an owner/admin acting on +// someone ELSE's account -- it refuses id == the caller's own ID +// outright (POST /auth/password is the only path to changing your own +// password, for anyone, including an owner resetting their own), and +// refuses an owner target unless the caller is themselves an owner +// (RegisterRoutes' RoleAdmin floor already means a caller here is +// admin-or-owner, so "not an owner" in the check below means admin). func (h *Handler) handleResetPassword(w http.ResponseWriter, r *http.Request) { + id := r.PathValue("id") + identity, _ := authz.IdentityFromContext(r.Context()) + if id == identity.UserID { + writeError(w, http.StatusBadRequest, "use POST /auth/password to change your own password") + return + } + target, err := h.store.GetUserByID(r.Context(), id) + if err != nil { + h.writeStoreErr(w, err, "resetting password") + return + } + if target.Role == authz.RoleOwner && !identity.Role.Satisfies(authz.RoleOwner) { + writeError(w, http.StatusForbidden, "admin cannot reset an owner's password") + return + } + var req resetPasswordRequest // An empty body is valid here (generate a random password) -- // decodeJSON's json.Decode on an empty io.Reader would error, so @@ -368,7 +482,7 @@ func (h *Handler) handleResetPassword(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusInternalServerError, "resetting password failed") return } - if err := h.store.SetPasswordHash(r.Context(), r.PathValue("id"), hash); err != nil { + if err := h.store.SetPasswordHash(r.Context(), id, hash); err != nil { h.writeStoreErr(w, err, "resetting password") return } @@ -380,6 +494,54 @@ func (h *Handler) handleResetPassword(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, resp) } +type changeOwnPasswordRequest struct { + CurrentPassword string `json:"current_password"` + NewPassword string `json:"new_password"` +} + +// handleChangeOwnPassword is the only path to changing your own +// password, for every role including owner -- distinct from +// handleResetPassword (which acts on someone ELSE's account and never +// asks for a password it could never know) by requiring +// current_password, verified against the caller's own stored hash. +// Like every other password change in this package, it revokes the +// caller's own existing sessions too (SetPasswordHash), including the +// one this very request is authenticated with -- the caller must sign +// in again afterward. +func (h *Handler) handleChangeOwnPassword(w http.ResponseWriter, r *http.Request) { + var req changeOwnPasswordRequest + if !decodeJSON(w, r, &req) { + return + } + if len(req.NewPassword) < 8 { + writeError(w, http.StatusBadRequest, "password must be at least 8 characters") + return + } + + identity, _ := authz.IdentityFromContext(r.Context()) + hash, err := h.store.GetPasswordHashByID(r.Context(), identity.UserID) + if err != nil { + h.writeStoreErr(w, err, "changing password") + return + } + if !ComparePassword(hash, req.CurrentPassword) { + writeError(w, http.StatusBadRequest, "current password is incorrect") + return + } + + newHash, err := HashPassword(req.NewPassword) + if err != nil { + h.logger.Error("hashing password", "error", err) + writeError(w, http.StatusInternalServerError, "changing password failed") + return + } + if err := h.store.SetPasswordHash(r.Context(), identity.UserID, newHash); err != nil { + h.writeStoreErr(w, err, "changing password") + return + } + w.WriteHeader(http.StatusNoContent) +} + func (h *Handler) setCookie(w http.ResponseWriter, raw string, ttl time.Duration) { http.SetCookie(w, &http.Cookie{ Name: sessionCookieName, diff --git a/api/localauth/handler_test.go b/api/localauth/handler_test.go index 3c60e54..b16e347 100644 --- a/api/localauth/handler_test.go +++ b/api/localauth/handler_test.go @@ -373,6 +373,9 @@ func TestOwnerCanReassignEveryRoleTransition(t *testing.T) { func TestOwnerCanReassignOwnRole(t *testing.T) { fs := newFakeStore() admin := mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + // A second owner so demoting "admin" doesn't trip the "at least one + // owner" guard this test isn't exercising. + mustCreateUser(t, fs, "otherowner", "otherpass1", authz.RoleOwner) _, mux := newTestHandler(t, fs) login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin","password":"adminpass1"}`, nil) @@ -427,3 +430,275 @@ func TestNonOwnerCannotReassignRole(t *testing.T) { t.Fatalf("status = %d, want 403 for a non-owner reassigning a role", rec.Code) } } + +// --- RBAC matrix: admin's create/delete scope is viewer/editor only --- + +func TestAdminCanListUsers(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "dave", "davespassword", authz.RoleAdmin) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"dave","password":"davespassword"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodGet, "/auth/users", "", cookie) + if rec.Code != http.StatusOK { + t.Fatalf("admin listing users: status = %d, want 200", rec.Code) + } +} + +func TestAdminCanCreateViewerAndEditorOnly(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "dave", "davespassword", authz.RoleAdmin) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"dave","password":"davespassword"}`, nil) + cookie := sessionCookieFrom(login) + + for _, role := range []string{"viewer", "editor"} { + rec := doRequest(t, mux, http.MethodPost, "/auth/users", + `{"username":"new-`+role+`","password":"somepassword1","role":"`+role+`"}`, cookie) + if rec.Code != http.StatusCreated { + t.Errorf("admin creating %s: status = %d, want 201, body=%s", role, rec.Code, rec.Body.String()) + } + } + for _, role := range []string{"admin", "owner"} { + rec := doRequest(t, mux, http.MethodPost, "/auth/users", + `{"username":"new-`+role+`","password":"somepassword1","role":"`+role+`"}`, cookie) + if rec.Code != http.StatusForbidden { + t.Errorf("admin creating %s: status = %d, want 403", role, rec.Code) + } + } +} + +func TestOwnerCanCreateAnyRole(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + for _, role := range []string{"viewer", "editor", "admin", "owner"} { + rec := doRequest(t, mux, http.MethodPost, "/auth/users", + `{"username":"new-`+role+`","password":"somepassword1","role":"`+role+`"}`, cookie) + if rec.Code != http.StatusCreated { + t.Errorf("owner creating %s: status = %d, want 201, body=%s", role, rec.Code, rec.Body.String()) + } + } +} + +func TestAdminCanDeleteViewerAndEditorOnly(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "dave", "davespassword", authz.RoleAdmin) + viewer := mustCreateUser(t, fs, "vince", "vincepassword", authz.RoleViewer) + editor := mustCreateUser(t, fs, "edith", "edithpassword", authz.RoleEditor) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"dave","password":"davespassword"}`, nil) + cookie := sessionCookieFrom(login) + + for _, target := range []*User{viewer, editor} { + rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+target.ID, "", cookie) + if rec.Code != http.StatusNoContent { + t.Errorf("admin deleting %s: status = %d, want 204, body=%s", target.Role, rec.Code, rec.Body.String()) + } + } +} + +func TestAdminCannotDeleteAdminOrOwner(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "dave", "davespassword", authz.RoleAdmin) + otherAdmin := mustCreateUser(t, fs, "dan", "danspassword", authz.RoleAdmin) + owner := mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"dave","password":"davespassword"}`, nil) + cookie := sessionCookieFrom(login) + + for _, target := range []*User{otherAdmin, owner} { + rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+target.ID, "", cookie) + if rec.Code != http.StatusForbidden { + t.Errorf("admin deleting %s: status = %d, want 403", target.Role, rec.Code) + } + } +} + +// --- RBAC matrix: at least one owner must always remain --- + +func TestCannotDeleteTheLastOwner(t *testing.T) { + fs := newFakeStore() + owner := mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + mustCreateUser(t, fs, "dave", "davespassword", authz.RoleAdmin) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+owner.ID, "", cookie) + if rec.Code != http.StatusConflict { + t.Fatalf("deleting the last owner: status = %d, want 409, body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCanDeleteAnOwnerWhenAnotherRemains(t *testing.T) { + fs := newFakeStore() + owner1 := mustCreateUser(t, fs, "admin1", "adminpass1", authz.RoleOwner) + mustCreateUser(t, fs, "admin2", "adminpass2", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin1","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodDelete, "/auth/users/"+owner1.ID, "", cookie) + if rec.Code != http.StatusNoContent { + t.Fatalf("deleting one of two owners: status = %d, want 204, body=%s", rec.Code, rec.Body.String()) + } +} + +func TestCannotDemoteTheLastOwner(t *testing.T) { + fs := newFakeStore() + owner := mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodPut, "/auth/users/"+owner.ID+"/role", `{"role":"admin"}`, cookie) + if rec.Code != http.StatusConflict { + t.Fatalf("demoting the last owner: status = %d, want 409, body=%s", rec.Code, rec.Body.String()) + } +} + +// --- RBAC matrix: password resets on behalf of another user --- + +func TestAdminCanResetNonOwnerPasswordsButNotOwners(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "dave", "davespassword", authz.RoleAdmin) + viewer := mustCreateUser(t, fs, "vince", "vincepassword", authz.RoleViewer) + otherAdmin := mustCreateUser(t, fs, "dan", "danspassword", authz.RoleAdmin) + owner := mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"dave","password":"davespassword"}`, nil) + cookie := sessionCookieFrom(login) + + for _, target := range []*User{viewer, otherAdmin} { + rec := doRequest(t, mux, http.MethodPost, "/auth/users/"+target.ID+"/reset-password", "", cookie) + if rec.Code != http.StatusOK { + t.Errorf("admin resetting %s's password: status = %d, want 200, body=%s", target.Role, rec.Code, rec.Body.String()) + } + } + + rec := doRequest(t, mux, http.MethodPost, "/auth/users/"+owner.ID+"/reset-password", "", cookie) + if rec.Code != http.StatusForbidden { + t.Fatalf("admin resetting an owner's password: status = %d, want 403", rec.Code) + } +} + +func TestOwnerCanResetAnyPasswordIncludingAnotherOwner(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "admin1", "adminpass1", authz.RoleOwner) + owner2 := mustCreateUser(t, fs, "admin2", "adminpass2", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin1","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodPost, "/auth/users/"+owner2.ID+"/reset-password", "", cookie) + if rec.Code != http.StatusOK { + t.Fatalf("owner resetting another owner's password: status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } +} + +func TestResetPasswordRejectsTargetingSelf(t *testing.T) { + fs := newFakeStore() + owner := mustCreateUser(t, fs, "admin", "adminpass1", authz.RoleOwner) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"admin","password":"adminpass1"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodPost, "/auth/users/"+owner.ID+"/reset-password", "", cookie) + if rec.Code != http.StatusBadRequest { + t.Fatalf("resetting your own password via the admin endpoint: status = %d, want 400, body=%s", rec.Code, rec.Body.String()) + } +} + +// --- self-service password change (POST /auth/password) --- + +func TestChangeOwnPasswordSucceedsWithCorrectCurrentPassword(t *testing.T) { + fs := newFakeStore() + // Deliberately RoleViewer -- self-service must work for every role, + // not just owner/admin. + mustCreateUser(t, fs, "vince", "vincepassword", authz.RoleViewer) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"vince","password":"vincepassword"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodPost, "/auth/password", + `{"current_password":"vincepassword","new_password":"vinces-new-password"}`, cookie) + if rec.Code != http.StatusNoContent { + t.Fatalf("status = %d, want 204, body=%s", rec.Code, rec.Body.String()) + } + + // The old session must be revoked... + stale := doRequest(t, mux, http.MethodGet, "/auth/session", "", cookie) + if stale.Code != http.StatusUnauthorized { + t.Errorf("session after password change: status = %d, want 401", stale.Code) + } + // ...and the new password must actually work. + relogin := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"vince","password":"vinces-new-password"}`, nil) + if relogin.Code != http.StatusOK { + t.Fatalf("login with new password: status = %d, want 200", relogin.Code) + } +} + +func TestChangeOwnPasswordRejectsWrongCurrentPassword(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "vince", "vincepassword", authz.RoleViewer) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"vince","password":"vincepassword"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodPost, "/auth/password", + `{"current_password":"wrongpassword","new_password":"vinces-new-password"}`, cookie) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400, body=%s", rec.Code, rec.Body.String()) + } + + // The session must still be valid -- a rejected attempt is not a + // password change. + still := doRequest(t, mux, http.MethodGet, "/auth/session", "", cookie) + if still.Code != http.StatusOK { + t.Errorf("session after a rejected attempt: status = %d, want 200", still.Code) + } +} + +func TestChangeOwnPasswordRejectsShortNewPassword(t *testing.T) { + fs := newFakeStore() + mustCreateUser(t, fs, "vince", "vincepassword", authz.RoleViewer) + _, mux := newTestHandler(t, fs) + + login := doRequest(t, mux, http.MethodPost, "/auth/login", `{"username":"vince","password":"vincepassword"}`, nil) + cookie := sessionCookieFrom(login) + + rec := doRequest(t, mux, http.MethodPost, "/auth/password", + `{"current_password":"vincepassword","new_password":"short"}`, cookie) + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } +} + +func TestChangeOwnPasswordRequiresAuth(t *testing.T) { + fs := newFakeStore() + _, mux := newTestHandler(t, fs) + + rec := doRequest(t, mux, http.MethodPost, "/auth/password", + `{"current_password":"whatever","new_password":"somenewpassword"}`, nil) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401 with no session", rec.Code) + } +} diff --git a/api/localauth/store.go b/api/localauth/store.go index 3aa6157..64248ec 100644 --- a/api/localauth/store.go +++ b/api/localauth/store.go @@ -258,6 +258,38 @@ func (s *Store) SetRole(ctx context.Context, userID string, role authz.Role) err return tx.Commit(ctx) } +// CountUsersWithRole reports how many tenant_memberships rows in the +// default tenant currently have the given role -- backs the "there +// must always be at least one owner" guard (handleDeleteUser, +// handleSetRole). Counts every membership regardless of whether it +// belongs to a local user or an SSO-provisioned one (unlike ListUsers, +// which is local-only), since the invariant this backs is about the +// tenant having an owner at all, not about this package's own user +// list specifically. +func (s *Store) CountUsersWithRole(ctx context.Context, role authz.Role) (int, error) { + var n int + err := s.pool.QueryRow(ctx, ` + SELECT count(*) FROM tenant_memberships WHERE tenant_id = $1 AND role = $2`, + defaultTenantID, string(role)).Scan(&n) + return n, err +} + +// GetPasswordHashByID is GetUserForLogin's by-ID counterpart, backing +// self-service password changes (handleChangeOwnPassword) -- the only +// other place this package ever reads a password_hash back out. +func (s *Store) GetPasswordHashByID(ctx context.Context, id string) (string, error) { + var hash string + err := s.pool.QueryRow(ctx, ` + SELECT password_hash FROM users WHERE id = $1 AND username IS NOT NULL`, id).Scan(&hash) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return "", ErrNotFound + } + return "", err + } + return hash, nil +} + // CountLocalUsers backs -seed-admin's idempotency check (see // cmd/api/main.go's runSeedAdmin): a deployment that already has at // least one local user never gets a second auto-created admin account. diff --git a/api/localauth/store_integration_test.go b/api/localauth/store_integration_test.go index 388a1d5..08a0635 100644 --- a/api/localauth/store_integration_test.go +++ b/api/localauth/store_integration_test.go @@ -221,3 +221,67 @@ func TestIntegrationSetRoleAcceptsEveryRole(t *testing.T) { } } } + +// TestIntegrationCountUsersWithRole confirms the count reflects real +// inserts/role changes against the actual tenant_memberships table -- +// the "at least one owner" guard this backs (handler.go's +// wouldRemoveLastOwner) is only as trustworthy as this query. +func TestIntegrationCountUsersWithRole(t *testing.T) { + store := integrationStore(t) + ctx := context.Background() + username := testUsername(t) + + hash, _ := HashPassword("password1") + user, err := store.CreateUser(ctx, username, hash, authz.RoleOwner) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + t.Cleanup(func() { _ = store.DeleteUser(ctx, user.ID) }) + + before, err := store.CountUsersWithRole(ctx, authz.RoleOwner) + if err != nil { + t.Fatalf("CountUsersWithRole(owner): %v", err) + } + if before < 1 { + t.Fatalf("owner count = %d, want at least 1 (the user just created)", before) + } + + if err := store.SetRole(ctx, user.ID, authz.RoleAdmin); err != nil { + t.Fatalf("SetRole: %v", err) + } + after, err := store.CountUsersWithRole(ctx, authz.RoleOwner) + if err != nil { + t.Fatalf("CountUsersWithRole(owner) after demotion: %v", err) + } + if after != before-1 { + t.Fatalf("owner count after demoting one = %d, want %d", after, before-1) + } +} + +// TestIntegrationGetPasswordHashByID confirms it reads the same hash +// GetUserForLogin does, by ID instead of username -- the self-service +// password change handler's only way to fetch the caller's own hash. +func TestIntegrationGetPasswordHashByID(t *testing.T) { + store := integrationStore(t) + ctx := context.Background() + username := testUsername(t) + + hash, _ := HashPassword("correct horse battery staple") + user, err := store.CreateUser(ctx, username, hash, authz.RoleViewer) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + t.Cleanup(func() { _ = store.DeleteUser(ctx, user.ID) }) + + got, err := store.GetPasswordHashByID(ctx, user.ID) + if err != nil { + t.Fatalf("GetPasswordHashByID: %v", err) + } + if !ComparePassword(got, "correct horse battery staple") { + t.Errorf("hash from GetPasswordHashByID does not verify against the original password") + } + + if _, err := store.GetPasswordHashByID(ctx, uuid.NewString()); !errors.Is(err, ErrNotFound) { + t.Errorf("GetPasswordHashByID for an unknown ID: err = %v, want ErrNotFound", err) + } +} diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 81859a6..cb103a9 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -482,6 +482,20 @@ export function setUserRole(id: string, role: string): Promise { }); } +// changeOwnPassword is the only way to change your own password (any +// role, including owner) -- distinct from resetPassword above, which +// is exclusively for an owner/admin acting on someone ELSE's account +// and will reject a request that targets the caller's own id. Revokes +// the caller's own session on success (see api/localauth/handler.go's +// handleChangeOwnPassword), so the caller must sign in again afterward. +export function changeOwnPassword(currentPassword: string, newPassword: string): Promise { + return request('/auth/password', { + method: 'POST', + credentials: 'include', + body: JSON.stringify({ current_password: currentPassword, new_password: newPassword }) + }); +} + // --- log retention (owner/admin only, see api/logretention) ----------- // Deletion is scoped to specific (host, service) targets, not wholesale // -- a caller must name which agents' *and* which log types' logs to diff --git a/web/src/lib/components/NavSidebar.svelte b/web/src/lib/components/NavSidebar.svelte index 1aeacc2..6f922e6 100644 --- a/web/src/lib/components/NavSidebar.svelte +++ b/web/src/lib/components/NavSidebar.svelte @@ -46,15 +46,17 @@ }); // The Users nav item only ever makes sense for local-auth mode's - // owner-only user manager (see routes/users/+page.svelte) -- an - // enterprise-SSO deployment or a non-owner local session never sees - // it, same gating that page enforces itself if reached directly. - const isLocalOwner = $derived.by(() => { + // owner/admin user manager (see routes/users/+page.svelte, which + // admin can now partially use too -- viewer/editor accounts only) -- + // an enterprise-SSO deployment or a plain viewer/editor local + // session never sees it, same gating that page enforces itself if + // reached directly. + const canManageUsers = $derived.by(() => { const s = localSession; - return s !== null && s.role === 'owner'; + return s !== null && (s.role === 'owner' || s.role === 'admin'); }); const navItems = $derived( - isLocalOwner ? [...baseNavItems, usersNavItem, settingsNavItem] : [...baseNavItems, settingsNavItem] + canManageUsers ? [...baseNavItems, usersNavItem, settingsNavItem] : [...baseNavItems, settingsNavItem] ); let loggingOut = $state(false); @@ -105,6 +107,7 @@ {localSession.username} {localSession.role} + Change password diff --git a/web/src/routes/account/+page.svelte b/web/src/routes/account/+page.svelte new file mode 100644 index 0000000..1a3ae23 --- /dev/null +++ b/web/src/routes/account/+page.svelte @@ -0,0 +1,178 @@ + + +
+

Change password

+ + {#if !checked} +

Loading…

+ {:else if localSession === 'disabled'} +

This deployment doesn't have local accounts enabled.

+ {:else if localSession === null} +

Sign in to change your password.

+ {:else if done} +

Password changed. Signing you out so you can sign back in…

+ {:else} +

Signed in as {localSession.username} ({localSession.role}).

+ +
+
+ + +
+
+ + +
+
+ + +
+ + {#if error}

{error}

{/if} + + +
+

Changing your password signs you out everywhere -- you'll need to sign back in.

+ {/if} +
+ + diff --git a/web/src/routes/account/+page.ts b/web/src/routes/account/+page.ts new file mode 100644 index 0000000..c902fb0 --- /dev/null +++ b/web/src/routes/account/+page.ts @@ -0,0 +1,3 @@ +// Same shape as settings/+page.ts: no route params, data comes from a +// client-side fetch. +export const prerender = true; diff --git a/web/src/routes/users/+page.svelte b/web/src/routes/users/+page.svelte index 14577e0..c17b4a6 100644 --- a/web/src/routes/users/+page.svelte +++ b/web/src/routes/users/+page.svelte @@ -12,6 +12,11 @@ } from '$lib/api'; const roleOptions = ['viewer', 'editor', 'admin', 'owner'] as const; + // An admin caller can only ever create/pick viewer or editor -- the + // server rejects admin/owner from an admin caller (see + // api/localauth/handler.go's handleCreateUser), so this is here to + // keep the create form from ever offering a choice that would 403. + const adminCreatableRoles = ['viewer', 'editor'] as const; let localSession = $state(null); let checked = $state(false); @@ -22,7 +27,8 @@ async function loadUsers() { localSession = localAuthEnabled ? await getLocalSession() : 'disabled'; checked = true; - if (localSession === 'disabled' || localSession === null || localSession.role !== 'owner') return; + if (localSession === 'disabled' || localSession === null) return; + if (localSession.role !== 'owner' && localSession.role !== 'admin') return; usersLoading = true; usersError = ''; try { @@ -35,6 +41,39 @@ } loadUsers(); + // isOwner gates what this page offers beyond the owner-or-admin floor + // that gets you onto the page at all: role reassignment, and + // creating/deleting/resetting an admin or owner account, all stay + // owner-only (see api/localauth/handler.go's RBAC matrix doc + // comment). $derived.by + a local const alias sidesteps a + // svelte-check narrowing quirk with reading a mutable $state + // directly inside a bare $derived(...) expression. + const isOwner = $derived.by(() => { + const s = localSession; + return s !== null && s !== 'disabled' && s.role === 'owner'; + }); + const ownerCount = $derived(users.filter((u) => u.role === 'owner').length); + + // canDeleteUser/canResetPassword mirror the server's own checks so + // the UI never offers an action that would just come back as a 403 + // or (for the last owner) a 409 -- the server remains the actual + // authority, this is purely a "don't show a doomed button" nicety. + function canDeleteUser(u: LocalUser): boolean { + if ((u.role === 'admin' || u.role === 'owner') && !isOwner) return false; + if (u.role === 'owner' && ownerCount <= 1) return false; + return true; + } + + function deleteDisabledReason(u: LocalUser): string { + if ((u.role === 'admin' || u.role === 'owner') && !isOwner) return 'Only an owner can delete an admin or owner account'; + if (u.role === 'owner' && ownerCount <= 1) return 'There must always be at least one owner'; + return ''; + } + + function canResetPassword(u: LocalUser): boolean { + return !(u.role === 'owner' && !isOwner); + } + // --- create user --- let newUsername = $state(''); let newPassword = $state(''); @@ -159,10 +198,16 @@

{:else if localSession === null}

Sign in to manage users.

- {:else if localSession.role !== 'owner'} -

Only an owner can manage users. Signed in as {localSession.username} ({localSession.role}).

+ {:else if localSession.role !== 'owner' && localSession.role !== 'admin'} +

+ Only an owner or admin can manage users. Signed in as {localSession.username} ({localSession.role}). To + change your own password, use "Change password" in the sidebar. +

{:else} -

Local accounts for this deployment: passwords, and roles.

+

+ Local accounts for this deployment. + {#if !isOwner}As an admin, you can manage viewer and editor accounts.{/if} +

{#if usersError}

{usersError}

{/if} @@ -192,7 +237,8 @@ - {#each roleOptions as r (r)} + {#each (isOwner ? roleOptions : adminCreatableRoles) as r (r)} {/each} @@ -404,6 +469,13 @@ color: var(--color-danger); border-color: var(--color-danger); } + .actions button:disabled { + cursor: default; + opacity: 0.5; + } + .self-note { + font-size: var(--text-xs); + } .password-row td { border-bottom: 1px solid var(--color-border);