Compare commits

..
2 Commits
Author SHA1 Message Date
mixandClaude Opus 5.5 e469034477 import: the export file held to the forms' rules before anything is created (audit M10)
test / test (push) Canceled after 0s
domain.Import trusted the file for everything but the domain name, the
selector and the password: DMARCRua was not validate.Email'ed, client IPs
not parsed, a rate limit's mode, window and count not checked - a window of
0 counted nothing, a garbage IP made the application refuse every client.

handlers.checkDomainImport runs in HandleImportDomain after the domain name
check and before Import, and refuses the file with "Invalid <what> in export
file: <the form's own text>": the report address (validate.Email), each
rate limit (parseRateLimitMode; auto - checkAutoMultiplier, 0 meaning the
default as an empty field does; manual - checkManualLimit, which the form
now uses too: count > 0, <= the level-1 backstop, window > 0; both 0 is an
export without a limit of its own), each application's client addresses
and the legacy ones on its rate limit (parseIPList, written back as the
form stores them; allow-list on with none is refused). Login, sending
addresses and password stay checked in app.ImportApplication.

Tests: a good export passes and its addresses come out normalised; nine
broken fields each refused with their message; a refused file posted to the
handler creates no domain.

CHANGELOG [Unreleased], audit plan line ticked, roadmap 18/62.
Evidence: go vet clean; the import, rate-limit and application 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:28:29 +03:00
mixandClaude Opus 5.5 f9e75de8d1 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
10 changed files with 630 additions and 116 deletions
+20
View File
@@ -58,6 +58,26 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
stored earlier, or imported from another instance, keeps its case, since stored earlier, or imported from another instance, keeps its case, since
its client uses it (audit 2026-10-12, H4). its client uses it (audit 2026-10-12, H4).
- **The last global administrator stays.** Deleting or demoting a global
administrator counted the others first and then wrote, so two
administrators deleting or demoting each other at the same moment could
both succeed and leave nobody able to open Server. The count is now taken
in the same transaction as the write; first-user setup is one statement
(audit 2026-10-12, M7).
- **A user is saved whole or not at all.** The users form wrote the name,
the password, the role and the assignments one after another; a failure
part way left a user renamed with the old role, or a domain administrator
with nothing assigned. Saving and creating a user are now one transaction
each, the e-mail included (audit 2026-10-12, M9).
- **A domain import is checked like the forms.** The import trusted the file
for the report address, the rate limits and the client addresses: a limit
with a window of no seconds counted nothing, and an address that was not
one made the application refuse every client without a word. The file is
now held to the forms' rules before anything is created, and refused with
the form's own message, naming the field and the application (audit
2026-10-12, M10).
### Added ### Added
- **The delivery log measures its own guesses** (migration `0007`). A table - **The delivery log measures its own guesses** (migration `0007`). A table
+3 -3
View File
@@ -663,13 +663,13 @@ has already run is never edited — the next file is added; the published
installation; look). Tests: `Ops`/`ops` refused; the map written lower- installation; look). Tests: `Ops`/`ops` refused; the map written lower-
case. Then verify Postfix's compare on the stand once (`MAIL FROM` as the case. Then verify Postfix's compare on the stand once (`MAIL FROM` as the
other case) so the doc's claim is seen, not assumed. — **Opus** other case) so the doc's claim is seen, not assumed. — **Opus**
- [ ] **Last global in one transaction**: `DeleteUser`/`SetUserRole` take a - [x] **Last global in one transaction**: `DeleteUser`/`SetUserRole` take a
`tx`, count and act inside it; `CreateGlobalUser` likewise. A concurrency `tx`, count and act inside it; `CreateGlobalUser` likewise. A concurrency
test with two goroutines. — **Opus** test with two goroutines. — **Opus**
- [ ] **`submitUserUpdate` in one transaction** (store method that takes the - [x] **`submitUserUpdate` in one transaction** (store method that takes the
whole change). The users pages get direct handler tests while they are whole change). The users pages get direct handler tests while they are
rewired - a row of review-2026-08-followups, placed here 2026-10-14. — **Opus** rewired - a row of review-2026-08-followups, placed here 2026-10-14. — **Opus**
- [ ] **Import validates like the form**: route `DMARCRua`, IPs, limit mode, - [x] **Import validates like the form**: route `DMARCRua`, IPs, limit mode,
window and count through the same parsers; refuse the file with the window and count through the same parsers; refuse the file with the
validator's text. Tests per field. — **Opus** validator's text. Tests per field. — **Opus**
- [ ] **Send-log rows belong to a domain by id** (L12; owner, 2026-10-12). - [ ] **Send-log rows belong to a domain by id** (L12; owner, 2026-10-12).
+1 -1
View File
@@ -42,7 +42,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md).
| from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) | | from-header-binding | Hold a message's `From:` header against the application that sends it | shipped `2.4.0` | 9/9 | [plans/from-header-binding.md](plans/from-header-binding.md) |
| panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | | panel-redesign | Panel redesign (accepted mockups, design contract) | shipped `2.0.0` | 27/27 | [plans/panel-redesign.md](plans/panel-redesign.md) |
| schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) | | schema-squash | Squash SQLite migrations into a 2.x baseline | built in panel-redesign (stage 1) | — | [plans/panel-redesign.md](plans/panel-redesign.md) |
| audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`; next stages 1+2 → `2.5.0`) | 15/62 | [audit-2026-10-12.md](audit-2026-10-12.md) | | audit-2026-10-12 | Follow-ups of the 2026-10-12 full-tree code audit, seven stages | in progress (stage 0 shipped `2.4.2`; next stages 1+2 → `2.5.0`) | 18/62 | [audit-2026-10-12.md](audit-2026-10-12.md) |
**Recommended order** (not binding; owner, 2026-10-09: feature order follows **Recommended order** (not binding; owner, 2026-10-09: feature order follows
development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and development convenience): **panel-redesign** shipped as `2.0.0` (2026-10-11) and
+128 -57
View File
@@ -76,23 +76,22 @@ func (s *Store) UserExists() (bool, error) {
return n > 0, nil return n > 0, nil
} }
// CreateGlobalUser inserts the first global user during setup. // CreateGlobalUser inserts the first global user during setup. The check that
// no user exists yet and the insert are one statement, so two setup requests
// at once cannot both create one (audit 2026-10-12, M7).
func (s *Store) CreateGlobalUser(username, passwordHash string) error { func (s *Store) CreateGlobalUser(username, passwordHash string) error {
exists, err := s.UserExists()
if err != nil {
return err
}
if exists {
return fmt.Errorf("create global user: users already exist")
}
now := time.Now().UTC().Format(time.RFC3339) now := time.Now().UTC().Format(time.RFC3339)
_, err = s.db.Exec( res, err := s.db.Exec(
"INSERT INTO users (username, password_hash, role, created_at) VALUES (?, ?, ?, ?)", `INSERT INTO users (username, password_hash, role, created_at)
SELECT ?, ?, ?, ? WHERE NOT EXISTS (SELECT 1 FROM users)`,
username, passwordHash, RoleGlobal, now, username, passwordHash, RoleGlobal, now,
) )
if err != nil { if err != nil {
return fmt.Errorf("create global user: %w", err) return fmt.Errorf("create global user: %w", err)
} }
if n, _ := res.RowsAffected(); n == 0 {
return fmt.Errorf("create global user: users already exist")
}
return nil return nil
} }
@@ -220,17 +219,37 @@ func (s *Store) CountGlobalUsers() (int, error) {
return n, nil return n, nil
} }
// CreateUser inserts a panel user. A domain user is created with its reach in // CreateUser inserts a panel user without an account e-mail; see AddUser.
// the same call and must be given one (ErrEmptyReach); for a global user the
// reach is ignored.
func (s *Store) CreateUser(username, passwordHash string, role Role, reach Reach) (int64, error) { func (s *Store) CreateUser(username, passwordHash string, role Role, reach Reach) (int64, error) {
if role == RoleDomain && reach.Empty() { return s.AddUser(UserChange{Username: username, PasswordHash: passwordHash, Role: role, Reach: reach})
}
// UserChange is everything the users form sets on a user, written at once by
// AddUser and SaveUser. ID is ignored by AddUser.
type UserChange struct {
ID int64
Username, PasswordHash, Email string
Role Role
Reach Reach // a domain user's; ignored for a global one
}
// AddUser inserts a panel user with its e-mail and, for a domain user, its
// reach, in one transaction: a domain user must be given one (ErrEmptyReach),
// and a failure part way leaves no user rather than one without it (audit
// 2026-10-12, M9).
func (s *Store) AddUser(c UserChange) (int64, error) {
if c.Role == RoleDomain && c.Reach.Empty() {
return 0, ErrEmptyReach return 0, ErrEmptyReach
} }
tx, err := s.db.Begin()
if err != nil {
return 0, fmt.Errorf("create user: %w", err)
}
defer tx.Rollback()
now := time.Now().UTC().Format(time.RFC3339) now := time.Now().UTC().Format(time.RFC3339)
res, err := s.db.Exec( res, err := tx.Exec(
"INSERT INTO users (username, password_hash, role, created_at) VALUES (?, ?, ?, ?)", "INSERT INTO users (username, password_hash, role, email, created_at) VALUES (?, ?, ?, ?, ?)",
username, passwordHash, role, now, c.Username, c.PasswordHash, c.Role, c.Email, now,
) )
if err != nil { if err != nil {
if isUniqueViolation(err) { if isUniqueViolation(err) {
@@ -242,14 +261,79 @@ func (s *Store) CreateUser(username, passwordHash string, role Role, reach Reach
if err != nil { if err != nil {
return 0, fmt.Errorf("create user id: %w", err) return 0, fmt.Errorf("create user id: %w", err)
} }
if role == RoleDomain { if c.Role == RoleDomain {
if err := s.setUserReach(id, reach); err != nil { if err := writeUserReach(tx, id, c.Reach); err != nil {
return 0, err return 0, err
} }
} }
if err := tx.Commit(); err != nil {
return 0, fmt.Errorf("create user: %w", err)
}
return id, nil return id, nil
} }
// SaveUser writes what the users form changed - username, password hash,
// e-mail, role and reach - in one transaction (audit 2026-10-12, M9: five
// writes in a row left, on a failure part way, a user renamed with the old
// role, or a domain user with nothing assigned). A domain user must keep a
// reach (ErrEmptyReach); a user made global loses theirs, so a later demotion
// does not bring an old assignment back. The last global administrator cannot
// be demoted (ErrLastGlobal): the count is taken inside the transaction,
// after the write, so two administrators demoting each other at once cannot
// both succeed (M7).
func (s *Store) SaveUser(c UserChange) error {
if c.Role == RoleDomain && c.Reach.Empty() {
return ErrEmptyReach
}
tx, err := s.db.Begin()
if err != nil {
return fmt.Errorf("save user: %w", err)
}
defer tx.Rollback()
// A write first: it takes the write lock, so what is counted below is
// what is committed.
res, err := tx.Exec(
"UPDATE users SET username = ?, password_hash = ?, email = ?, role = ? WHERE id = ?",
c.Username, c.PasswordHash, c.Email, c.Role, c.ID,
)
if err != nil {
if isUniqueViolation(err) {
return ErrUserExists
}
return fmt.Errorf("save user: %w", err)
}
if n, _ := res.RowsAffected(); n == 0 {
return ErrUserNotFound
}
if err := requireAGlobal(tx); err != nil {
return err
}
reach := c.Reach
if c.Role == RoleGlobal {
reach = Reach{}
}
if err := writeUserReach(tx, c.ID, reach); err != nil {
return err
}
if err := tx.Commit(); err != nil {
return fmt.Errorf("save user: %w", err)
}
return nil
}
// requireAGlobal is ErrLastGlobal when the transaction has left no global
// user.
func requireAGlobal(tx *sql.Tx) error {
var n int
if err := tx.QueryRow("SELECT COUNT(*) FROM users WHERE role = ?", RoleGlobal).Scan(&n); err != nil {
return fmt.Errorf("count global users: %w", err)
}
if n == 0 {
return ErrLastGlobal
}
return nil
}
// UpdateUser replaces a user's username, password hash and account e-mail. // UpdateUser replaces a user's username, password hash and account e-mail.
func (s *Store) UpdateUser(id int64, username, passwordHash, email string) error { func (s *Store) UpdateUser(id int64, username, passwordHash, email string) error {
res, err := s.db.Exec( res, err := s.db.Exec(
@@ -272,22 +356,6 @@ func (s *Store) UpdateUser(id int64, username, passwordHash, email string) error
return nil return nil
} }
// SetUserRole updates a user's role.
func (s *Store) SetUserRole(userID int64, role Role) error {
res, err := s.db.Exec("UPDATE users SET role = ? WHERE id = ?", role, userID)
if err != nil {
return fmt.Errorf("set user role: %w", err)
}
n, err := res.RowsAffected()
if err != nil {
return fmt.Errorf("set user role: %w", err)
}
if n == 0 {
return ErrUserNotFound
}
return nil
}
// ClearUserReach removes everything a user is assigned — both lists and both // ClearUserReach removes everything a user is assigned — both lists and both
// All flags. A user promoted to the global role keeps none of it, so a later // All flags. A user promoted to the global role keeps none of it, so a later
// demotion does not quietly bring an old assignment back. // demotion does not quietly bring an old assignment back.
@@ -311,32 +379,28 @@ func (s *Store) SetUserReach(userID int64, reach Reach) error {
return s.setUserReach(userID, reach) return s.setUserReach(userID, reach)
} }
// DeleteUser removes a panel user. ErrLastGlobal when deleting the only global user. // DeleteUser removes a panel user. ErrLastGlobal when deleting the only global
// user: the delete and the count are one transaction, so two administrators
// deleting each other at once cannot both succeed (audit 2026-10-12, M7).
func (s *Store) DeleteUser(id int64) error { func (s *Store) DeleteUser(id int64) error {
u, err := s.GetUser(id) tx, err := s.db.Begin()
if err != nil {
return err
}
if u.Role == RoleGlobal {
n, err := s.CountGlobalUsers()
if err != nil {
return err
}
if n <= 1 {
return ErrLastGlobal
}
}
res, err := s.db.Exec("DELETE FROM users WHERE id = ?", id)
if err != nil { if err != nil {
return fmt.Errorf("delete user: %w", err) return fmt.Errorf("delete user: %w", err)
} }
n, err := res.RowsAffected() defer tx.Rollback()
res, err := tx.Exec("DELETE FROM users WHERE id = ?", id)
if err != nil { if err != nil {
return fmt.Errorf("delete user: %w", err) return fmt.Errorf("delete user: %w", err)
} }
if n == 0 { if n, _ := res.RowsAffected(); n == 0 {
return ErrUserNotFound return ErrUserNotFound
} }
if err := requireAGlobal(tx); err != nil {
return err
}
if err := tx.Commit(); err != nil {
return fmt.Errorf("delete user: %w", err)
}
return nil return nil
} }
@@ -416,15 +480,23 @@ func (s *Store) listUserDomainNames(userID int64) ([]string, error) {
} }
// setUserReach writes the flags and both assignment tables in one transaction. // setUserReach writes the flags and both assignment tables in one transaction.
// With an All flag set the rows of that direction are dropped rather than
// stored: the flag already covers them, and stale rows would come back to life
// the day the flag is cleared.
func (s *Store) setUserReach(userID int64, reach Reach) error { func (s *Store) setUserReach(userID int64, reach Reach) error {
tx, err := s.db.Begin() tx, err := s.db.Begin()
if err != nil { if err != nil {
return fmt.Errorf("set user reach begin: %w", err) return fmt.Errorf("set user reach begin: %w", err)
} }
defer tx.Rollback() defer tx.Rollback()
if err := writeUserReach(tx, userID, reach); err != nil {
return err
}
return tx.Commit()
}
// writeUserReach writes a user's reach inside tx. With an All flag set the
// rows of that direction are dropped rather than stored: the flag already
// covers them, and stale rows would come back to life the day the flag is
// cleared.
func writeUserReach(tx *sql.Tx, userID int64, reach Reach) error {
if _, err := tx.Exec("UPDATE users SET all_domains = ?, all_inbound_domains = ? WHERE id = ?", if _, err := tx.Exec("UPDATE users SET all_domains = ?, all_inbound_domains = ? WHERE id = ?",
reach.AllDomains, reach.AllInbound, userID); err != nil { reach.AllDomains, reach.AllInbound, userID); err != nil {
return fmt.Errorf("set user reach flags: %w", err) return fmt.Errorf("set user reach flags: %w", err)
@@ -449,6 +521,5 @@ func (s *Store) setUserReach(userID int64, reach Reach) error {
} }
} }
} }
return tx.Commit() return nil
} }
+151
View File
@@ -0,0 +1,151 @@
package store
import (
"errors"
"sync"
"testing"
)
// Two global administrators who demote, or delete, each other at the same
// moment: one of them must stay global. The count is taken inside the
// transaction that writes, so the second sees the first's change (audit
// 2026-10-12, M7).
func TestTheLastGlobalSurvivesTwoAtOnce(t *testing.T) {
for _, how := range []string{"demote", "delete"} {
t.Run(how, func(t *testing.T) {
st := openTestStore(t)
d := addTestDomain(t, st, "example.com")
var ids []int64
for _, name := range []string{"ann", "bob"} {
id, err := st.CreateUser(name, "hash", RoleGlobal, Reach{})
if err != nil {
t.Fatal(err)
}
ids = append(ids, id)
}
var wg sync.WaitGroup
errs := make([]error, len(ids))
for i, id := range ids {
wg.Add(1)
go func(i int, id int64) {
defer wg.Done()
if how == "delete" {
errs[i] = st.DeleteUser(id)
return
}
errs[i] = st.SaveUser(UserChange{ID: id, Username: []string{"ann", "bob"}[i], PasswordHash: "hash",
Role: RoleDomain, Reach: Reach{DomainIDs: []int64{d.ID}}})
}(i, id)
}
wg.Wait()
refused := 0
for _, err := range errs {
switch {
case errors.Is(err, ErrLastGlobal):
refused++
case err != nil:
t.Fatalf("unexpected error: %v", err)
}
}
n, err := st.CountGlobalUsers()
if err != nil {
t.Fatal(err)
}
if refused != 1 || n != 1 {
t.Fatalf("%d refused, %d global users left; want 1 and 1", refused, n)
}
})
}
}
// SaveUser writes all of a change or none of it.
func TestSaveUserIsOneChange(t *testing.T) {
st := openTestStore(t)
d := addTestDomain(t, st, "example.com")
if _, err := st.CreateUser("root", "hash", RoleGlobal, Reach{}); err != nil {
t.Fatal(err)
}
id, err := st.CreateUser("shop", "hash", RoleDomain, Reach{DomainIDs: []int64{d.ID}})
if err != nil {
t.Fatal(err)
}
if _, err := st.CreateUser("taken", "hash", RoleGlobal, Reach{}); err != nil {
t.Fatal(err)
}
// A taken name: nothing else of the change is written either.
err = st.SaveUser(UserChange{ID: id, Username: "taken", PasswordHash: "new", Email: "x@example.com", Role: RoleGlobal})
if !errors.Is(err, ErrUserExists) {
t.Fatalf("SaveUser with a taken name = %v", err)
}
// A reach naming a domain that does not exist fails at the last write.
err = st.SaveUser(UserChange{ID: id, Username: "renamed", PasswordHash: "new", Role: RoleDomain, Reach: Reach{DomainIDs: []int64{9999}}})
if err == nil {
t.Fatal("SaveUser with a reach to no domain succeeded")
}
if err := st.SaveUser(UserChange{ID: id, Username: "renamed", PasswordHash: "new", Role: RoleDomain}); !errors.Is(err, ErrEmptyReach) {
t.Fatalf("SaveUser of a domain user with nothing assigned = %v", err)
}
u, err := st.GetUser(id)
if err != nil {
t.Fatal(err)
}
if u.Username != "shop" || u.PasswordHash != "hash" || u.Email != "" || u.Role != RoleDomain || len(u.DomainIDs) != 1 {
t.Fatalf("a refused change left %+v", u)
}
// Made global, the user loses the reach.
if err := st.SaveUser(UserChange{ID: id, Username: "shop", PasswordHash: "hash", Email: "s@example.com", Role: RoleGlobal, Reach: Reach{DomainIDs: []int64{d.ID}}}); err != nil {
t.Fatal(err)
}
u, _ = st.GetUser(id)
if u.Role != RoleGlobal || u.Email != "s@example.com" || !u.Reach().Empty() {
t.Fatalf("promoted: %+v", u)
}
}
// AddUser writes the user, the e-mail and the reach at once; a reach that
// cannot be written leaves no user behind.
func TestAddUserIsOneChange(t *testing.T) {
st := openTestStore(t)
d := addTestDomain(t, st, "example.com")
id, err := st.AddUser(UserChange{Username: "shop", PasswordHash: "hash", Email: "s@example.com", Role: RoleDomain, Reach: Reach{DomainIDs: []int64{d.ID}}})
if err != nil {
t.Fatal(err)
}
u, _ := st.GetUser(id)
if u.Email != "s@example.com" || len(u.DomainIDs) != 1 {
t.Fatalf("created %+v", u)
}
if _, err := st.AddUser(UserChange{Username: "ghost", PasswordHash: "hash", Role: RoleDomain, Reach: Reach{DomainIDs: []int64{9999}}}); err == nil {
t.Fatal("AddUser with a reach to no domain succeeded")
}
if _, err := st.GetUserByUsername("ghost"); !errors.Is(err, ErrUserNotFound) {
t.Fatalf("the failed create left a user: %v", err)
}
}
// Setup creates the first user once, however many requests race for it.
func TestCreateGlobalUserOnlyOnce(t *testing.T) {
st := openTestStore(t)
var wg sync.WaitGroup
errs := make([]error, 4)
for i := range errs {
wg.Add(1)
go func(i int) {
defer wg.Done()
errs[i] = st.CreateGlobalUser([]string{"a", "b", "c", "d"}[i], "hash")
}(i)
}
wg.Wait()
ok := 0
for _, err := range errs {
if err == nil {
ok++
}
}
n, _ := st.CountGlobalUsers()
if ok != 1 || n != 1 {
t.Fatalf("%d setups succeeded, %d global users; want 1 and 1", ok, n)
}
}
+69
View File
@@ -277,6 +277,11 @@ func (h *Handlers) HandleImportDomain(w http.ResponseWriter, r *http.Request) {
return return
} }
if msg := h.checkDomainImport(&exp); msg != "" {
h.renderBackupPage(w, r, http.StatusBadRequest, msg)
return
}
d, err := h.domains.Import(exp) d, err := h.domains.Import(exp)
if err != nil { if err != nil {
logf("panel: import domain %q: %v", exp.Domain, err) logf("panel: import domain %q: %v", exp.Domain, err)
@@ -365,3 +370,67 @@ func importErrorMessage(err error) (int, string) {
return http.StatusBadRequest, "Could not import the domain: " + err.Error() return http.StatusBadRequest, "Could not import the domain: " + err.Error()
} }
} }
// checkDomainImport holds an export file to the rules the forms hold the same
// settings to - the report address, the rate limits, the client addresses -
// before anything is created, and says what is wrong in the validator's own
// words; "" when nothing is. The file is untrusted input: a window of no
// seconds made a limit that counted nothing, and an address that is not one
// made an application refuse every client without a word at import (audit
// 2026-10-12, M10). Client addresses are written back as the form would store
// them. The login, the addresses an application may send as and the password
// are checked where the application is created (app.ImportApplication).
func (h *Handlers) checkDomainImport(exp *domain.DomainExport) string {
invalid := func(what string, err error) string {
return "Invalid " + what + " in export file: " + err.Error()
}
if exp.DMARCRua != "" {
if err := validate.Email(exp.DMARCRua); err != nil {
return invalid("report address", err)
}
}
limit := func(rl *domain.RateLimitExport) error {
if rl == nil {
return nil
}
mode, err := parseRateLimitMode(rl.Mode)
if err != nil {
return err
}
if mode == store.RateLimitModeAuto {
if rl.AutoMultiplier == 0 {
return nil // as an empty field on the form: the default
}
return checkAutoMultiplier(rl.AutoMultiplier)
}
if rl.MaxMessages <= 0 && rl.WindowSeconds <= 0 {
return nil // no limit of its own (an export of client addresses only)
}
return checkManualLimit(rl.MaxMessages, rl.WindowSeconds, h.l1Messages())
}
if err := limit(exp.RateLimit); err != nil {
return invalid("rate limit for the domain", err)
}
for i := range exp.Applications {
a := &exp.Applications[i]
if err := limit(a.RateLimit); err != nil {
return invalid("rate limit for "+a.Login, err)
}
ips, err := parseIPList(strings.Join(a.AuthAllowedIPs, ","))
if err != nil {
return invalid("client addresses for "+a.Login, err)
}
a.AuthAllowedIPs = ips
if a.RateLimit != nil && len(a.RateLimit.AllowedIPs) > 0 {
legacy, err := parseIPList(strings.Join(a.RateLimit.AllowedIPs, ","))
if err != nil {
return invalid("client addresses for "+a.Login, err)
}
a.RateLimit.AllowedIPs = legacy
}
if a.AuthIPRestrict && len(a.AuthAllowedIPs) == 0 && (a.RateLimit == nil || len(a.RateLimit.AllowedIPs) == 0) {
return invalid("client addresses for "+a.Login, errors.New("enter at least one client IP when the allow-list is enabled"))
}
}
return ""
}
@@ -0,0 +1,102 @@
package handlers
import (
"bytes"
"encoding/json"
"mime/multipart"
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/mixeme/selfpost/internal/domain"
"github.com/mixeme/selfpost/internal/store"
"github.com/mixeme/selfpost/internal/web/auth"
)
// A domain export is held to the rules the forms hold the same settings to,
// field by field, and refused in the validator's words (audit 2026-10-12,
// M10).
func TestDomainImportIsCheckedLikeTheForms(t *testing.T) {
h, _ := inboundHandlers(t)
good := func() domain.DomainExport {
return domain.DomainExport{
Domain: "example.com",
DMARCRua: "reports@example.com",
RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeManual, MaxMessages: 10, WindowSeconds: 3600},
Applications: []domain.AppExport{{
Login: "app1", AddressMode: store.AddressModeWildcard, Password: "imported-password",
AuthIPRestrict: true, AuthAllowedIPs: []string{"192.0.2.1", " 2001:DB8::1 "},
RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeAuto, AutoMultiplier: 2},
}},
}
}
exp := good()
if msg := h.checkDomainImport(&exp); msg != "" {
t.Fatalf("a good export refused: %s", msg)
}
if got := strings.Join(exp.Applications[0].AuthAllowedIPs, " "); got != "192.0.2.1 2001:db8::1" {
t.Errorf("client addresses stored as %q, want as the form stores them", got)
}
for _, c := range []struct {
name string
break_ func(*domain.DomainExport)
want string
}{
{"report address", func(e *domain.DomainExport) { e.DMARCRua = "not an address" },
"Invalid report address in export file: "},
{"no window", func(e *domain.DomainExport) { e.RateLimit.WindowSeconds = 0 },
"Invalid rate limit for the domain in export file: enter a time window greater than zero seconds"},
{"no messages", func(e *domain.DomainExport) { e.RateLimit.MaxMessages = 0 },
"Invalid rate limit for the domain in export file: enter a message limit greater than zero"},
{"over the backstop", func(e *domain.DomainExport) { e.RateLimit.MaxMessages = 1 << 20 },
"Invalid rate limit for the domain in export file: message limit cannot exceed the level-1 backstop"},
{"mode", func(e *domain.DomainExport) { e.RateLimit.Mode = "sometimes" },
"Invalid rate limit for the domain in export file: choose manual or auto mode"},
{"multiplier", func(e *domain.DomainExport) { e.Applications[0].RateLimit.AutoMultiplier = 1000 },
"Invalid rate limit for app1 in export file: multiplier must be between"},
{"client address", func(e *domain.DomainExport) { e.Applications[0].AuthAllowedIPs = []string{"not-an-ip"} },
`Invalid client addresses for app1 in export file: "not-an-ip" is not a valid IP address`},
{"allow-list on, empty", func(e *domain.DomainExport) { e.Applications[0].AuthAllowedIPs = nil },
"Invalid client addresses for app1 in export file: enter at least one client IP when the allow-list is enabled"},
{"legacy client address", func(e *domain.DomainExport) {
e.Applications[0].AuthAllowedIPs = nil
e.Applications[0].RateLimit = &domain.RateLimitExport{AllowedIPs: []string{"300.1.1.1"}}
}, `Invalid client addresses for app1 in export file: "300.1.1.1" is not a valid IP address`},
} {
t.Run(c.name, func(t *testing.T) {
exp := good()
c.break_(&exp)
if msg := h.checkDomainImport(&exp); !strings.HasPrefix(msg, c.want) {
t.Fatalf("got %q, want it to start with %q", msg, c.want)
}
})
}
}
// The check runs before anything is created: a refused file leaves no domain.
func TestDomainImportRefusedCreatesNothing(t *testing.T) {
h, st := inboundHandlers(t)
root := userWith(t, st, "root", store.RoleGlobal, store.Reach{})
raw, _ := json.Marshal(domain.DomainExport{
Format: domain.FormatDomainExport, Domain: "example.com", DKIMSelector: "mail",
RateLimit: &domain.RateLimitExport{Mode: store.RateLimitModeManual, MaxMessages: 10, WindowSeconds: 0},
})
var body bytes.Buffer
mw := multipart.NewWriter(&body)
fw, _ := mw.CreateFormFile("file", "example.com.json")
fw.Write(raw)
mw.Close()
req := httptest.NewRequest("POST", "/server/backup/import", &body)
req.Header.Set("Content-Type", mw.FormDataContentType())
req = auth.RequestWithPrincipal(req, root)
rec := httptest.NewRecorder()
h.HandleImportDomain(rec, req)
if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), "enter a time window greater than zero seconds") {
t.Fatalf("import of a limit with no window = %d:\n%s", rec.Code, rec.Body.String())
}
if domains, _ := st.ListDomains(); len(domains) != 0 {
t.Fatalf("the refused import created %v", domains)
}
}
+31 -10
View File
@@ -45,10 +45,31 @@ func parseAutoMultiplier(raw string) (float64, error) {
if err != nil { if err != nil {
return 0, fmt.Errorf("enter a valid multiplier (%.1f–%.1f)", store.MinAutoMultiplier, store.MaxAutoMultiplier) return 0, fmt.Errorf("enter a valid multiplier (%.1f–%.1f)", store.MinAutoMultiplier, store.MaxAutoMultiplier)
} }
if v < store.MinAutoMultiplier || v > store.MaxAutoMultiplier { return v, checkAutoMultiplier(v)
return 0, fmt.Errorf("multiplier must be between %.1f and %.1f", store.MinAutoMultiplier, store.MaxAutoMultiplier)
} }
return v, nil
// checkAutoMultiplier is the rule for an auto limit's multiplier, shared by
// the form and a domain import.
func checkAutoMultiplier(v float64) error {
if v < store.MinAutoMultiplier || v > store.MaxAutoMultiplier {
return fmt.Errorf("multiplier must be between %.1f and %.1f", store.MinAutoMultiplier, store.MaxAutoMultiplier)
}
return nil
}
// checkManualLimit is the rule for a manual limit, shared by the form and a
// domain import: a window of no seconds would count nothing.
func checkManualLimit(maxMessages, windowSeconds, l1Max int) error {
if maxMessages <= 0 {
return fmt.Errorf("enter a message limit greater than zero")
}
if maxMessages > l1Max {
return fmt.Errorf("message limit cannot exceed the level-1 backstop (%d)", l1Max)
}
if windowSeconds <= 0 {
return fmt.Errorf("enter a time window greater than zero seconds")
}
return nil
} }
func parseRateLimitMode(raw string) (string, error) { func parseRateLimitMode(raw string) (string, error) {
@@ -86,15 +107,15 @@ func parseRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error) {
return rateLimitInput{clear: true}, nil return rateLimitInput{clear: true}, nil
} }
maxMessages, err := parsePositiveInt(rawMax, 0) maxMessages, err := parsePositiveInt(rawMax, 0)
if err != nil || maxMessages <= 0 { if err != nil {
return rateLimitInput{}, fmt.Errorf("enter a message limit greater than zero") maxMessages = 0
}
if maxMessages > l1Max {
return rateLimitInput{}, fmt.Errorf("message limit cannot exceed the level-1 backstop (%d)", l1Max)
} }
windowSeconds, err := parsePositiveInt(r.PostFormValue("window_seconds"), defaultRateLimitWindowSeconds) windowSeconds, err := parsePositiveInt(r.PostFormValue("window_seconds"), defaultRateLimitWindowSeconds)
if err != nil || windowSeconds <= 0 { if err != nil {
return rateLimitInput{}, fmt.Errorf("enter a time window greater than zero seconds") windowSeconds = 0
}
if err := checkManualLimit(maxMessages, windowSeconds, l1Max); err != nil {
return rateLimitInput{}, err
} }
return rateLimitInput{mode: store.RateLimitModeManual, maxMessages: maxMessages, windowSeconds: windowSeconds}, nil return rateLimitInput{mode: store.RateLimitModeManual, maxMessages: maxMessages, windowSeconds: windowSeconds}, nil
} }
+20 -39
View File
@@ -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."}) h.renderUserForm(w, r, http.StatusInternalServerError, 0, userFormView{FormErr: "Internal error. Please try again."})
return return
} }
id, err := h.store.CreateUser(username, string(hash), role, reach) // The user, the e-mail and the reach in one transaction: a failure leaves
if err != nil { // 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) { 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}) 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 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."}) h.renderUserForm(w, r, http.StatusInternalServerError, 0, userFormView{FormErr: "Could not create user. Please check the logs."})
return 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) 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 return
} }
if u.Role == store.RoleGlobal && role == store.RoleDomain { lastGlobal := func() {
n, err := h.store.CountGlobalUsers()
if err != nil || n <= 1 {
msg := "Cannot demote the last global administrator." msg := "Cannot demote the last global administrator."
if u.ID == p.ID { if u.ID == p.ID {
msg = "You cannot demote yourself without another global administrator." 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}) 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 {
lastGlobal()
return return
} }
} }
@@ -301,11 +298,17 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
hash = string(newHash) hash = string(newHash)
} }
if err := h.store.UpdateUser(u.ID, username, hash, email); err != nil { // Name, password, e-mail, role and reach in one transaction (audit
if errors.Is(err, store.ErrUserExists) { // 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}) 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 return
} case errors.Is(err, store.ErrLastGlobal):
lastGlobal()
return
case err != nil:
logf("panel: update user: %v", err) 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}) 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 return
@@ -313,28 +316,6 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
if password != "" { if password != "" {
h.endSessionsAfterPasswordSet(r, p, u) 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) 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)
}
}