store: users saved in one transaction, the last global kept inside it (audit M7, M9)

M7: DeleteUser counted the global users and then deleted; the handler's demotion and CreateGlobalUser checked and then wrote. MaxOpenConns(1) serialises statements, not pairs: two globals deleting or demoting each other at once could leave none. Now: - DeleteUser: a transaction that deletes first (taking the write lock) and then requires a global left (requireAGlobal) - ErrLastGlobal rolls back. - SaveUser: likewise for a demotion, the count inside the transaction after the write. The handler's early count stays only to answer before hashing a password; SaveUser decides, and ErrLastGlobal maps to the same message. - CreateGlobalUser: INSERT ... SELECT ... WHERE NOT EXISTS (SELECT 1 FROM users), one statement. M9: submitUserUpdate made five writes in a row (UpdateUser, sessions, SetUserRole, ClearUserReach with its error only logged, SetUserReach). store.SaveUser(UserChange) writes name, hash, e-mail, role and reach in one transaction (a global user's reach cleared); sessions end after it commits. The create path had the same flaw - user, reach and then e-mail in three writes - and now goes through AddUser, one transaction; CreateUser stays as a wrapper for its many test callers. SetUserRole is gone; writeUserReach is the reach write inside a transaction. Tests: store - two globals demoting, and deleting, each other from two goroutines: exactly one refused, one global left; a refused SaveUser (taken name, a reach to no domain, nothing assigned) changes nothing; promoted to global loses the reach; AddUser with a bad reach leaves no user; four setups at once create one user. Handlers (the users pages, direct): the whole change saved, back to domain with a reach; the last global not demoted and the typed rename not written; a domain user with nothing assigned refused, nothing changed; create with e-mail and reach. CHANGELOG [Unreleased], audit plan lines ticked (two), roadmap 17/62. Evidence: go vet ./... clean; go test ./internal/store ok; the users tests of ./internal/web/handlers ok; the server run follows before the push. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2026-10-14 15:25:41 +03:00
parent 60c7b85f84
commit f9e75de8d1
7 changed files with 420 additions and 106 deletions
@@ -210,8 +210,9 @@ func (h *Handlers) submitUserCreate(w http.ResponseWriter, r *http.Request) {
 		h.renderUserForm(w, r, http.StatusInternalServerError, 0, userFormView{FormErr: "Internal error. Please try again."})
 		return
 	}
-	id, err := h.store.CreateUser(username, string(hash), role, reach)
-	if err != nil {
+	// The user, the e-mail and the reach in one transaction: a failure leaves
+	// no user rather than half of one (audit 2026-10-12, M9).
+	if _, err := h.store.AddUser(store.UserChange{Username: username, PasswordHash: string(hash), Email: email, Role: role, Reach: reach}); err != nil {
 		if errors.Is(err, store.ErrUserExists) {
 			h.renderUserForm(w, r, http.StatusConflict, 0, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
 			return
@@ -220,15 +221,6 @@ func (h *Handlers) submitUserCreate(w http.ResponseWriter, r *http.Request) {
 		h.renderUserForm(w, r, http.StatusInternalServerError, 0, userFormView{FormErr: "Could not create user. Please check the logs."})
 		return
 	}
-	// No domain follows a user who has just been created, so there is nothing
-	// to resync when their address is first stored.
-	if email != "" {
-		if err := h.store.UpdateUser(id, username, string(hash), email); err != nil {
-			logf("panel: create user %d: store e-mail: %v", id, err)
-			h.renderUserForm(w, r, http.StatusInternalServerError, id, userFormView{FormErr: "The user was created, but the e-mail could not be saved. Set it here.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
-			return
-		}
-	}
 	http.Redirect(w, r, "/server/users?done=created", http.StatusSeeOther)
 }

@@ -274,14 +266,19 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
 		return
 	}

+	lastGlobal := func() {
+		msg := "Cannot demote the last global administrator."
+		if u.ID == p.ID {
+			msg = "You cannot demote yourself without another global administrator."
+		}
+		h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: msg, FormUsername: username, FormEmail: email, FormRole: string(u.Role), Reach: reach})
+	}
+	// Asked here to answer before a password is hashed; SaveUser decides,
+	// inside its transaction.
 	if u.Role == store.RoleGlobal && role == store.RoleDomain {
 		n, err := h.store.CountGlobalUsers()
 		if err != nil || n <= 1 {
-			msg := "Cannot demote the last global administrator."
-			if u.ID == p.ID {
-				msg = "You cannot demote yourself without another global administrator."
-			}
-			h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: msg, FormUsername: username, FormEmail: email, FormRole: string(u.Role), Reach: reach})
+			lastGlobal()
 			return
 		}
 	}
@@ -301,11 +298,17 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
 		hash = string(newHash)
 	}

-	if err := h.store.UpdateUser(u.ID, username, hash, email); err != nil {
-		if errors.Is(err, store.ErrUserExists) {
-			h.renderUserForm(w, r, http.StatusConflict, u.ID, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
-			return
-		}
+	// Name, password, e-mail, role and reach in one transaction (audit
+	// 2026-10-12, M9). Sessions end only once it has committed.
+	err := h.store.SaveUser(store.UserChange{ID: u.ID, Username: username, PasswordHash: hash, Email: email, Role: role, Reach: reach})
+	switch {
+	case errors.Is(err, store.ErrUserExists):
+		h.renderUserForm(w, r, http.StatusConflict, u.ID, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
+		return
+	case errors.Is(err, store.ErrLastGlobal):
+		lastGlobal()
+		return
+	case err != nil:
 		logf("panel: update user: %v", err)
 		h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save user. Please check the logs.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
 		return
@@ -313,28 +316,6 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
 	if password != "" {
 		h.endSessionsAfterPasswordSet(r, p, u)
 	}
-
-	if role != u.Role {
-		if err := h.store.SetUserRole(u.ID, role); err != nil {
-			logf("panel: set user role: %v", err)
-			h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not update role.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
-			return
-		}
-		if role == store.RoleGlobal {
-			if err := h.store.ClearUserReach(u.ID); err != nil {
-				logf("panel: clear user reach: %v", err)
-			}
-		}
-	}
-
-	if role == store.RoleDomain {
-		if err := h.store.SetUserReach(u.ID, reach); err != nil {
-			logf("panel: set user reach: %v", err)
-			h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save domain assignments.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach})
-			return
-		}
-	}
-
 	http.Redirect(w, r, "/server/users?done=updated", http.StatusSeeOther)
 }

@@ -0,0 +1,99 @@
+package handlers
+
+import (
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+
+	"github.com/mixeme/selfpost/internal/store"
+)
+
+// The users form's writes, driven through the handlers (audit 2026-10-12, M7
+// and M9: the save is one store call now, and these pin what it does).
+
+// A save that changes name, e-mail and role lands whole; promoted to global
+// the user keeps no assignment.
+func TestUserFormSavesTheWholeChange(t *testing.T) {
+	h, st := inboundHandlers(t)
+	out1, _ := st.AddDomain("out.example.com", "mail")
+	root := userWith(t, st, "root", store.RoleGlobal, store.Reach{})
+	u := userWith(t, st, "shop", store.RoleDomain, store.Reach{DomainIDs: []int64{out1.ID}})
+	rec := send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)},
+		url.Values{"username": {"shop2"}, "email": {"shop@example.com"}, "role": {"global"}})
+	if rec.Code != http.StatusSeeOther {
+		t.Fatalf("POST = %d:\n%s", rec.Code, rec.Body.String())
+	}
+	after, err := st.GetUser(u.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if after.Username != "shop2" || after.Email != "shop@example.com" || after.Role != store.RoleGlobal || !after.Reach().Empty() {
+		t.Fatalf("after the save: %+v", after)
+	}
+
+	// And back to a domain user, with an assignment.
+	rec = send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)},
+		url.Values{"username": {"shop2"}, "email": {"shop@example.com"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}})
+	if rec.Code != http.StatusSeeOther {
+		t.Fatalf("POST = %d:\n%s", rec.Code, rec.Body.String())
+	}
+	after, _ = st.GetUser(u.ID)
+	if after.Role != store.RoleDomain || len(after.DomainIDs) != 1 || after.DomainIDs[0] != out1.ID {
+		t.Fatalf("demoted: %+v", after)
+	}
+}
+
+// The last global administrator is not demoted, and a refused save changes
+// nothing - not the name typed in the same form either.
+func TestUserFormKeepsTheLastGlobal(t *testing.T) {
+	h, st := inboundHandlers(t)
+	out1, _ := st.AddDomain("out.example.com", "mail")
+	root := userWith(t, st, "root", store.RoleGlobal, store.Reach{})
+	rec := send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(root.ID), map[string]string{"uid": idStr(root.ID)},
+		url.Values{"username": {"renamed"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}})
+	if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "You cannot demote yourself without another global administrator.") {
+		t.Fatalf("demoting the last global = %d:\n%s", rec.Code, rec.Body.String())
+	}
+	after, _ := st.GetUser(root.ID)
+	if after.Username != "root" || after.Role != store.RoleGlobal {
+		t.Fatalf("the refused save changed the user: %+v", after)
+	}
+}
+
+// A domain user must keep something assigned; the refusal changes nothing.
+func TestUserFormRefusesADomainUserWithNothingAssigned(t *testing.T) {
+	h, st := inboundHandlers(t)
+	out1, _ := st.AddDomain("out.example.com", "mail")
+	root := userWith(t, st, "root", store.RoleGlobal, store.Reach{})
+	u := userWith(t, st, "shop", store.RoleDomain, store.Reach{DomainIDs: []int64{out1.ID}})
+	rec := send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)},
+		url.Values{"username": {"renamed"}, "role": {"domain"}})
+	if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "Assign at least one outbound or inbound domain") {
+		t.Fatalf("nothing assigned = %d:\n%s", rec.Code, rec.Body.String())
+	}
+	after, _ := st.GetUser(u.ID)
+	if after.Username != "shop" || len(after.DomainIDs) != 1 {
+		t.Fatalf("the refused save changed the user: %+v", after)
+	}
+}
+
+// A user is created with the e-mail and the assignment in the one save.
+func TestUserFormCreatesWithEmailAndReach(t *testing.T) {
+	h, st := inboundHandlers(t)
+	out1, _ := st.AddDomain("out.example.com", "mail")
+	root := userWith(t, st, "root", store.RoleGlobal, store.Reach{})
+	rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil,
+		url.Values{"username": {"fresh"}, "password": {"a-long-enough-password"}, "email": {"fresh@example.com"},
+			"role": {"domain"}, "domain_ids": {idStr(out1.ID)}})
+	if rec.Code != http.StatusSeeOther {
+		t.Fatalf("create = %d:\n%s", rec.Code, rec.Body.String())
+	}
+	u, err := st.GetUserByUsername("fresh")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if u.Email != "fresh@example.com" || u.Role != store.RoleDomain || len(u.DomainIDs) != 1 {
+		t.Fatalf("created %+v", u)
+	}
+}