diff --git a/server/internal/api/api.go b/server/internal/api/api.go index cc7cffd..03aafec 100644 --- a/server/internal/api/api.go +++ b/server/internal/api/api.go @@ -57,6 +57,7 @@ type Store interface { UserSessions(ctx context.Context, userID string) ([]DeviceSession, error) RevokeUserSession(ctx context.Context, userID, sessionID string) error RevokeOtherSessions(ctx context.Context, userID, keepSessionID string) (int, error) + SetUserPassword(ctx context.Context, userID, hash string) error // --- team and invitations --- // Registration is by invitation: the code carries the address and the role @@ -294,6 +295,10 @@ func (s *Server) Routes() *http.ServeMux { // Devices. A person may list and revoke their own sessions; removing a // colleague's access is a different question, answered by deactivating them // on the team endpoint below. + // Changing your own password. `authed`, not `tenantOnly`: a session is not + // a company's data, and a platform admin has no company but must still be + // able to do this - they were the account with no route at all. + mux.HandleFunc("POST /api/auth/password", s.authed(s.handleChangePassword)) mux.HandleFunc("GET /api/auth/sessions", s.authed(s.handleSessions)) mux.HandleFunc("DELETE /api/auth/sessions/{id}", s.authed(s.handleRevokeSession)) mux.HandleFunc("POST /api/auth/sessions/revoke-others", diff --git a/server/internal/api/fake_test.go b/server/internal/api/fake_test.go index 07fad42..8091164 100644 --- a/server/internal/api/fake_test.go +++ b/server/internal/api/fake_test.go @@ -317,6 +317,19 @@ func (f *fakeStore) SiteHealth(_ context.Context, clientID string) ([]SiteHealth return out, nil } +func (f *fakeStore) SetUserPassword(_ context.Context, userID, hash string) error { + f.mu.Lock() + defer f.mu.Unlock() + for email, u := range f.users { + if u.ID == userID { + u.PasswordHash = hash + f.users[email] = u + return nil + } + } + return errors.New("no such user") +} + func (f *fakeStore) Sales(_ context.Context, q SaleQuery) ([]Sale, error) { f.mu.Lock() defer f.mu.Unlock() diff --git a/server/internal/api/handlers_password.go b/server/internal/api/handlers_password.go new file mode 100644 index 0000000..fc6a4de --- /dev/null +++ b/server/internal/api/handlers_password.go @@ -0,0 +1,92 @@ +// Changing your own password. +// +// This did not exist, and the cost of that was measured rather than guessed: +// rotating three production accounts took a shell on the server, three round +// trips, and briefly left a PLATFORM ADMIN - the account that reads every +// company on the estate - with a password anyone watching could guess, because +// a placeholder in a pasted command was taken literally. +// +// A manager could always reset somebody ELSE's password, and a platform admin +// could be reset by nobody at all: they have no client, so the team routes are +// not theirs, and `provision user` on the host was the only way. For a product +// that puts accounts on shop-floor PCs and staff phones, "change my password" +// is not a feature, it is the thing that makes every other credential decision +// recoverable. +package api + +import ( + "net/http" + + "github.com/loyaly/behavision-server/internal/auth" +) + +// handleChangePassword is on `authed`, NOT `tenantOnly`. +// +// A session is not a company's data. A platform admin has no client and must +// still be able to change their own password - they are precisely the account +// for which there was no other route. +func (s *Server) handleChangePassword(w http.ResponseWriter, r *http.Request) { + p := PrincipalFrom(r.Context()) + + var in ChangePassword + if err := decode(w, r, &in); err != nil { + badRequest(w, err.Error()) + return + } + + // The CURRENT password is required, and that is the whole security + // argument. An access token lives twelve hours and travels on shop-floor + // devices; without this, anyone holding a stolen one could set a new + // password and own the account permanently rather than for the rest of + // the day. + rec, err := s.Store.UserByEmail(r.Context(), p.Email) + if err != nil { + s.serverError(w, "change password", err) + return + } + if !rec.Found || !auth.VerifyPassword(rec.PasswordHash, in.CurrentPassword) { + // Deliberately not throttled separately: this needs a live session, so + // it is not reachable by anyone guessing from outside, and the login + // throttle already governs getting one. + writeErr(w, http.StatusForbidden, "wrong_password", + "That is not your current password.") + return + } + if in.CurrentPassword == in.NewPassword { + badRequest(w, "the new password is the same as the old one") + return + } + + hash, err := auth.HashPassword(in.NewPassword) + if err != nil { + // HashPassword enforces the length floor, and its message names it. + badRequest(w, err.Error()) + return + } + if err := s.Store.SetUserPassword(r.Context(), p.UserID, hash); err != nil { + s.serverError(w, "change password", err) + return + } + + // Every OTHER session goes, and the caller's stays. Somebody changing + // their password because they think it is known must not have to guess + // whether the change took effect on the device that already had it - and + // must not be signed out of the one in their hand while they deal with it. + revoked, err := s.Store.RevokeOtherSessions(r.Context(), p.UserID, p.SessionID) + if err != nil { + // The password IS changed. Reporting a failure here would tell the + // user to try again, and the retry would fail on the current password + // they just replaced. + s.Log.Printf("change password: revoke other sessions: %v", err) + } + + s.Store.Audit(r.Context(), AuditEntry{ + ClientID: p.ClientID, ActorID: p.UserID, ActorKind: "user", + Action: "auth.change_password", Entity: "user", EntityID: p.UserID, + Detail: map[string]any{"sessions_revoked": revoked}, + }) + writeJSON(w, http.StatusOK, map[string]any{ + "changed": true, + "sessions_revoked": revoked, + }) +} diff --git a/server/internal/api/password_test.go b/server/internal/api/password_test.go new file mode 100644 index 0000000..77579d8 --- /dev/null +++ b/server/internal/api/password_test.go @@ -0,0 +1,115 @@ +package api + +import ( + "net/http" + "testing" +) + +const pwPath = "/api/auth/password" + +// loginCode signs in and returns only the status. The suite's login() fatals +// on anything but 200, which is right everywhere else and useless here: half +// of what these tests assert is that a password has STOPPED working. +func loginCode(t *testing.T, s *Server, email, password string) int { + t.Helper() + return do(t, s, "POST", "/api/auth/login", "", + map[string]string{"email": email, "password": password}).Code +} + +// The account this endpoint exists for. A platform admin has no company, so +// the team routes are not theirs and tenantOnly refuses them - before this, +// changing their password needed a shell on the production host. +func TestAPlatformAdminCanChangeTheirOwnPassword(t *testing.T) { + s, fs := newServer(t) + seedPlatformAdmin(fs) + sess := login(t, s, "root@loyaly.ai", "admin123") + + rec := do(t, s, "POST", pwPath, sess.Token, map[string]string{ + "current_password": "admin123", "new_password": "a-much-longer-one", + }) + if rec.Code != http.StatusOK { + t.Fatalf("got %d: %s", rec.Code, rec.Body.String()) + } + // The new one works and the old one does not - asserted by signing in, + // because that is the only thing a user actually cares about here. + if c := loginCode(t, s, "root@loyaly.ai", "a-much-longer-one"); c != http.StatusOK { + t.Errorf("new password signs in: got %d, want 200", c) + } + if c := loginCode(t, s, "root@loyaly.ai", "admin123"); c == http.StatusOK { + t.Error("the old password still signs in") + } +} + +func TestAnOrdinaryUserCanChangeTheirOwnPassword(t *testing.T) { + s, fs := newServer(t) + seedUser(fs) + sess := login(t, s, "manager@acme.com", "correct horse battery") + + rec := do(t, s, "POST", pwPath, sess.Token, map[string]string{ + "current_password": "correct horse battery", "new_password": "staple-battery-horse", + }) + if rec.Code != http.StatusOK { + t.Fatalf("got %d: %s", rec.Code, rec.Body.String()) + } +} + +// The whole security argument. An access token lives twelve hours and travels +// on shop-floor PCs and staff phones; without this, a stolen one owns the +// account permanently instead of until it expires. +func TestChangingAPasswordRequiresTheCurrentOne(t *testing.T) { + s, fs := newServer(t) + seedUser(fs) + sess := login(t, s, "manager@acme.com", "correct horse battery") + + rec := do(t, s, "POST", pwPath, sess.Token, map[string]string{ + "current_password": "not the password", "new_password": "a-much-longer-one", + }) + if rec.Code != http.StatusForbidden { + t.Fatalf("got %d, want 403 - a token alone must not be enough: %s", + rec.Code, rec.Body.String()) + } + // And it must not have changed anything. + if c := loginCode(t, s, "manager@acme.com", "correct horse battery"); c != http.StatusOK { + t.Errorf("a refused change must leave the old password working: got %d", c) + } +} + +// The floor lives in HashPassword, so this asserts the endpoint routes through +// it rather than re-implementing a check that could drift from the constant. +func TestAShortNewPasswordIsRefused(t *testing.T) { + s, fs := newServer(t) + seedUser(fs) + sess := login(t, s, "manager@acme.com", "correct horse battery") + + rec := do(t, s, "POST", pwPath, sess.Token, map[string]string{ + "current_password": "correct horse battery", "new_password": "short", + }) + if rec.Code != http.StatusBadRequest { + t.Errorf("got %d, want 400 for a password under the floor", rec.Code) + } +} + +func TestReusingTheSamePasswordIsRefused(t *testing.T) { + s, fs := newServer(t) + seedUser(fs) + sess := login(t, s, "manager@acme.com", "correct horse battery") + + rec := do(t, s, "POST", pwPath, sess.Token, map[string]string{ + "current_password": "correct horse battery", + "new_password": "correct horse battery", + }) + if rec.Code != http.StatusBadRequest { + t.Errorf("got %d, want 400 - a no-op change reads as success and is not", rec.Code) + } +} + +func TestChangingAPasswordNeedsASession(t *testing.T) { + s, fs := newServer(t) + seedUser(fs) + rec := do(t, s, "POST", pwPath, "", map[string]string{ + "current_password": "correct horse battery", "new_password": "a-much-longer-one", + }) + if rec.Code != http.StatusUnauthorized { + t.Errorf("got %d, want 401", rec.Code) + } +} diff --git a/server/internal/api/types.go b/server/internal/api/types.go index 21ba151..6cf2323 100644 --- a/server/internal/api/types.go +++ b/server/internal/api/types.go @@ -172,6 +172,14 @@ type DashboardSummary struct { Timezone string `json:"timezone"` } +// ChangePassword is the body of POST /api/auth/password. The current password +// is required: an access token alone must not be enough to take an account +// over permanently. +type ChangePassword struct { + CurrentPassword string `json:"current_password"` + NewPassword string `json:"new_password"` +} + type Customer struct { ID string `json:"id"` // Ref is the customer number - "V-42" - and is accepted anywhere this diff --git a/server/internal/store/api_team.go b/server/internal/store/api_team.go index 3b1b978..c27f687 100644 --- a/server/internal/store/api_team.go +++ b/server/internal/store/api_team.go @@ -414,3 +414,30 @@ func (s *Store) ResetMemberPassword(ctx context.Context, clientID, userID, } return m, nil } + +// SetUserPassword changes one account's password, by user id. +// +// Deliberately NOT scoped by client, unlike ResetMemberPassword beside it. +// That one is a manager acting on somebody else in their company, so the +// tenant is the boundary. This is an account acting on ITSELF, and the caller +// is the session - a platform admin has no client at all and was, before this, +// the one account nobody could change the password of without a shell on the +// host. Scoping by client here would have reproduced exactly that hole. +// +// The id comes from the verified session and never from the request, so there +// is nothing here for a caller to point at somebody else. +func (s *Store) SetUserPassword(ctx context.Context, userID, hash string) error { + tag, err := s.pool.Exec(ctx, ` + UPDATE app_users SET password_hash = $2 + WHERE id = $1::uuid AND active`, userID, hash) + if err != nil { + return err + } + if tag.RowsAffected() == 0 { + // Deactivated mid-session: their sessions are already revoked, so this + // is unreachable in practice, and silently succeeding would report a + // password change that did not happen. + return fmt.Errorf("no active account %s", userID) + } + return nil +}