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);