From a37aba659425d213b0f9d85133b57bcbf317e938 Mon Sep 17 00:00:00 2001 From: Mix Date: Sat, 17 Oct 2026 21:28:18 +0300 Subject: [PATCH] user-reauth: the user form asks for your account password (audit A2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security audit 2026-10-17, finding A2 (high), stage 0; the mockup is 2c27275. A stolen global session could set its own password on the user form - no current password asked, the session kept - and then pass the backup's reauth with it, or add a second global user with a known password. Now the user form calls reauth (the backup's check: bcrypt against the signed-in user's own hash, under auth.AllowPasswordChange) when it adds a user and when a save sets a password, changes the e-mail (reset links go there) or changes the role, the own account included; deleting a user asks too, refused on its own confirmation page (renderUserDelete, a Flash on UserDelete). Name and reach alone are not asked. Missing or wrong: 401 on the same page, "Your account password is incorrect.", nothing saved. Tests: TestUserFormAsksForTheAccountPassword (create, each of password, e-mail, role, own password, reach alone, delete); the user-form tests send the field (withAccountPassword gives root a real hash and h an auth module). go vet, go test ./internal/web/... on Windows. Docs: security.md § Backup and domain export, guide § Users, help word for word with the mockup; CHANGELOG [Unreleased] Security; plan line ticked. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 ++ docs/guide.md | 6 + docs/security-audit-2026-10-17.md | 6 +- docs/security.md | 10 ++ .../web/handlers/handlers_delegation_test.go | 4 +- internal/web/handlers/handlers_dmarc_test.go | 2 +- internal/web/handlers/handlers_links_test.go | 5 +- .../handlers/handlers_server_email_test.go | 7 +- internal/web/handlers/handlers_server_test.go | 4 +- .../web/handlers/handlers_sessions_test.go | 12 +- internal/web/handlers/handlers_users.go | 43 +++++- .../handlers/handlers_users_reauth_test.go | 136 ++++++++++++++++++ internal/web/handlers/handlers_users_test.go | 8 +- internal/web/view/help.go | 2 +- internal/web/view/server.go | 8 +- internal/web/view/templates/user.html | 5 +- internal/web/view/templates/user_delete.html | 3 + 17 files changed, 246 insertions(+), 24 deletions(-) create mode 100644 internal/web/handlers/handlers_users_reauth_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d0be82..6a4c07d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version ## [Unreleased] +### Security + +- **A stolen `global` session no longer takes a full backup** (security + audit 2026-10-17, A2, high): the user form set the signed-in user's own + password without the current one, which then passed the backup's password + check. Adding a user, setting a password, changing an e-mail or a role on + the user form, and deleting a user now ask for your own account password, + as a backup does, under the same limit on wrong guesses. + ## [2.14.3] - 2026-10-17 A tag beside a label. No migration; schema 13. diff --git a/docs/guide.md b/docs/guide.md index a2b39aa..ab7cde1 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -719,6 +719,12 @@ The panel refuses to remove or demote the **last** global user, so it can never end up with none. A user cannot delete the account they are signed in with. The user form also sets the user's e-mail, which the users list shows. +Adding a user, setting a user's password, changing their e-mail or role - +your own account's included - and deleting a user ask for **your account +password**, the one you sign in with, as a backup does and under the same +limit on wrong guesses: a session left open is not enough to hand out an +account. Saving only the name or the domain assignments is not asked. + ### Sessions A login survives a container restart: sessions live in SQLite, not in diff --git a/docs/security-audit-2026-10-17.md b/docs/security-audit-2026-10-17.md index 587d7fe..e484f83 100644 --- a/docs/security-audit-2026-10-17.md +++ b/docs/security-audit-2026-10-17.md @@ -685,12 +685,14 @@ the argument. ### Stage 0 — the high and the cheap guards (~5 h) → patch release -- [ ] **A2.** `reauth` on user create, on a user update that carries +- [x] **A2.** `reauth` on user create, on a user update that carries `password` or changes `role`, and on user delete; the user form gets the `account_password` field the Backup page has; security.md § Backup and domain export says the user form is covered. Done when a test with a session and no password gets 401 on all three and the existing user-form - tests pass with the field. — **Opus**, 2 h. + tests pass with the field. — **Opus**, 2 h. *Done 2026-10-17:* also on a + change of e-mail (where reset links go); a save of the name and the + reach alone is not asked; TestUserFormAsksForTheAccountPassword. - [ ] **I2.** The three DMARC maps in `drop_links`; `[ -L ]` beside `[ -e ]` in postfix-config.sh:538,545; an e2e case plants a link and expects the log line. — **Opus**, 0.5 h. diff --git a/docs/security.md b/docs/security.md index cfe31bc..786a056 100644 --- a/docs/security.md +++ b/docs/security.md @@ -471,6 +471,16 @@ operator can act on it. file's own protection, not this one: whoever downloads it chooses that password. The import and the `selfpost-backup` CLI, which runs inside the container, are not asked. +- **Nor is it enough to make an account that would pass that check** + (security audit 2026-10-17, A2). Until `2.14.4` a `global` session could + set its own password on the user form - no current password asked, the + session kept - and then pass the check above with the password it chose, + or add a second global user with a password it knew. The user form now + asks for the same account password, under the same limiter, when it adds a + user and when a save sets a password, changes the e-mail (where reset + links go) or changes the role - the signed-in user's own account included - + and deleting a user asks for it too. A save of the name and the domain + assignments alone is not asked: neither reaches an account's secret. - Both files are secrets: a full backup carries DKIM keys, `sasldb2`, the panel users' password hashes, `docker-compose.yml`, `.env`, and the TLS private key from `certs/` when present; a domain export carries the DKIM key diff --git a/internal/web/handlers/handlers_delegation_test.go b/internal/web/handlers/handlers_delegation_test.go index 1d999a3..b5d336e 100644 --- a/internal/web/handlers/handlers_delegation_test.go +++ b/internal/web/handlers/handlers_delegation_test.go @@ -353,11 +353,12 @@ func TestUserFormAssignsBothDirections(t *testing.T) { out, _ := st.AddDomain("out.example.com", "mail") in, _ := st.AddInboundDomain("in.example.com") root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + withAccountPassword(t, h, st, root) const password = "a-long-enough-password-1" create := func(username string, extra url.Values) (*httptest.ResponseRecorder, store.User) { t.Helper() - form := url.Values{"username": {username}, "password": {password}, "role": {"domain"}} + form := url.Values{"username": {username}, "password": {password}, "role": {"domain"}, "account_password": {testAccountPassword}} for k, v := range extra { form[k] = v } @@ -385,6 +386,7 @@ func TestUserFormAssignsBothDirections(t *testing.T) { update := func(u store.User, form url.Values) store.User { t.Helper() form.Set("username", u.Username) + form.Set("account_password", testAccountPassword) rec := send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)}, form) if rec.Code != http.StatusSeeOther { t.Fatalf("update %s = %d:\n%s", u.Username, rec.Code, rec.Body.String()) diff --git a/internal/web/handlers/handlers_dmarc_test.go b/internal/web/handlers/handlers_dmarc_test.go index ff8119a..7562ab4 100644 --- a/internal/web/handlers/handlers_dmarc_test.go +++ b/internal/web/handlers/handlers_dmarc_test.go @@ -286,7 +286,7 @@ func TestDeletingAUserLeavesDomainAddressesAndNamesNobody(t *testing.T) { lacks(t, name+" for the co-administrator", body, `boss-private@example.org`, `admin's`, `s default`, `Another user`) } - rec := send(s.h.HandleUserDelete, &globalPrincipal, "POST", "/server/users/"+idStr(ops.ID)+"/delete", map[string]string{"uid": idStr(ops.ID)}, url.Values{}) + rec := send(s.h.HandleUserDelete, &globalPrincipal, "POST", "/server/users/"+idStr(ops.ID)+"/delete", map[string]string{"uid": idStr(ops.ID)}, url.Values{"account_password": {"correct-password-here!"}}) if rec.Code != http.StatusSeeOther { t.Fatalf("deleting a user = %d:\n%s", rec.Code, rec.Body.String()) } diff --git a/internal/web/handlers/handlers_links_test.go b/internal/web/handlers/handlers_links_test.go index e1e246d..ac34693 100644 --- a/internal/web/handlers/handlers_links_test.go +++ b/internal/web/handlers/handlers_links_test.go @@ -18,10 +18,11 @@ func TestUserFormSendsANewAddressItsLink(t *testing.T) { ml := withMail(t, h) out1, _ := st.AddDomain("one.example.com", "mail") root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + withAccountPassword(t, h, st, root) rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, url.Values{ "username": {"ops"}, "email": {"ops@acme.io"}, "password": {"a-long-enough-password-1"}, "role": {"domain"}, - "domain_ids": {idStr(out1.ID)}, + "domain_ids": {idStr(out1.ID)}, "account_password": {testAccountPassword}, }) if rec.Code != http.StatusSeeOther { t.Fatalf("create = %d:\n%s", rec.Code, rec.Body.String()) @@ -42,7 +43,7 @@ func TestUserFormSendsANewAddressItsLink(t *testing.T) { } // The same address saved again keeps its mark and sends nothing; a new // one loses it and is sent its link. - form := url.Values{"username": {"ops"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}, "email": {"ops@acme.io"}} + form := url.Values{"username": {"ops"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}, "email": {"ops@acme.io"}, "account_password": {testAccountPassword}} if rec := send(h.HandleUserEdit, &root, "POST", path, pv, form); rec.Code != http.StatusSeeOther || len(sentBy(h, ml)) != 1 { t.Fatalf("the same address: %d, sent %d", rec.Code, len(sentBy(h, ml))) } diff --git a/internal/web/handlers/handlers_server_email_test.go b/internal/web/handlers/handlers_server_email_test.go index 6ac55c8..a4da4f6 100644 --- a/internal/web/handlers/handlers_server_email_test.go +++ b/internal/web/handlers/handlers_server_email_test.go @@ -16,10 +16,11 @@ func TestUserFormSetsTheAccountEmail(t *testing.T) { h, st := inboundHandlers(t) out1, _ := st.AddDomain("one.example.com", "mail") root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + withAccountPassword(t, h, st, root) rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, url.Values{ "username": {"ops"}, "email": {" ops@acme.io "}, "password": {"a-long-enough-password-1"}, "role": {"domain"}, - "domain_ids": {idStr(out1.ID)}, + "domain_ids": {idStr(out1.ID)}, "account_password": {testAccountPassword}, }) if rec.Code != http.StatusSeeOther { t.Fatalf("create with an e-mail = %d:\n%s", rec.Code, rec.Body.String()) @@ -32,7 +33,7 @@ func TestUserFormSetsTheAccountEmail(t *testing.T) { path, pv := "/server/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)} post := func(extra url.Values) store.User { t.Helper() - form := url.Values{"username": {"ops"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}} + form := url.Values{"username": {"ops"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}, "account_password": {testAccountPassword}} for k, v := range extra { form[k] = v } @@ -54,7 +55,7 @@ func TestUserFormSetsTheAccountEmail(t *testing.T) { // Creating without one is fine. rec = send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, url.Values{ - "username": {"plain"}, "password": {"a-long-enough-password-1"}, "role": {"global"}}) + "username": {"plain"}, "password": {"a-long-enough-password-1"}, "role": {"global"}, "account_password": {testAccountPassword}}) if plain, _ := st.GetUserByUsername("plain"); rec.Code != http.StatusSeeOther || plain.Email != "" { t.Errorf("create without an e-mail = %d, %q", rec.Code, plain.Email) } diff --git a/internal/web/handlers/handlers_server_test.go b/internal/web/handlers/handlers_server_test.go index 79fb6a4..6064e6f 100644 --- a/internal/web/handlers/handlers_server_test.go +++ b/internal/web/handlers/handlers_server_test.go @@ -286,6 +286,7 @@ func TestUserDeletePageNamesTheAssignmentsAndRefusalsStayOnTheForm(t *testing.T) out1, _ := st.AddDomain("out.example.com", "mail") in1, _ := st.AddInboundDomain("in.example.com") root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + withAccountPassword(t, h, st, root) u := userWith(t, st, "shop", store.RoleDomain, store.Reach{DomainIDs: []int64{out1.ID}, InboundDomainIDs: []int64{in1.ID}}) path := "/server/users/" + idStr(u.ID) + "/delete" @@ -294,6 +295,7 @@ func TestUserDeletePageNamesTheAssignmentsAndRefusalsStayOnTheForm(t *testing.T) `

Delete shop

`, `
`, `remove the assignments to out.example.com (outbound) and to in.example.com (inbound).`, `
`, `href="/server/users/` + idStr(u.ID) + `">Keep it`, + `name="account_password" type="password" autocomplete="current-password" required`, } { if !strings.Contains(body, want) { t.Errorf("the delete page is missing %q", want) @@ -309,7 +311,7 @@ func TestUserDeletePageNamesTheAssignmentsAndRefusalsStayOnTheForm(t *testing.T) if _, err := st.GetUser(root.ID); err != nil { t.Errorf("the refused delete removed the user: %v", err) } - if rec := send(h.HandleUserDelete, &root, "POST", path, map[string]string{"uid": idStr(u.ID)}, url.Values{}); rec.Code != http.StatusSeeOther { + if rec := send(h.HandleUserDelete, &root, "POST", path, map[string]string{"uid": idStr(u.ID)}, url.Values{"account_password": {testAccountPassword}}); rec.Code != http.StatusSeeOther { t.Errorf("deleting another user = %d", rec.Code) } if _, err := st.GetUser(u.ID); err == nil { diff --git a/internal/web/handlers/handlers_sessions_test.go b/internal/web/handlers/handlers_sessions_test.go index 2754028..00a1be2 100644 --- a/internal/web/handlers/handlers_sessions_test.go +++ b/internal/web/handlers/handlers_sessions_test.go @@ -102,7 +102,7 @@ func TestAccountPasswordSignsOutOnlyTheUsersOtherSessions(t *testing.T) { // An administrator who sets another user's password on the user form takes // away every login that user had under the old one. func TestUserFormPasswordSignsOutThatUsersSessions(t *testing.T) { - h, _ := settingsServer(t) + h, password := settingsServer(t) admin := globalPrincipal bob := addUser(t, h, "bob", "bobs-long-password-1") adminCurrent := signedIn(t, h, "admin", "admin-current") @@ -110,7 +110,7 @@ func TestUserFormPasswordSignsOutThatUsersSessions(t *testing.T) { bob2 := signedIn(t, h, "bob", "bob-2") rec := postWithSession(h.HandleUserEdit, admin, adminCurrent, "/server/users/"+idStr(bob.ID), map[string]string{"uid": idStr(bob.ID)}, - url.Values{"username": {"bob"}, "role": {"global"}, "password": {"a-brand-new-long-password"}}) + url.Values{"username": {"bob"}, "role": {"global"}, "password": {"a-brand-new-long-password"}, "account_password": {password}}) if rec.Code != http.StatusSeeOther { t.Fatalf("POST user form = %d:\n%s", rec.Code, rec.Body.String()) } @@ -125,12 +125,12 @@ func TestUserFormPasswordSignsOutThatUsersSessions(t *testing.T) { // Saving the form without a new password does not sign anyone out. func TestUserFormWithoutAPasswordKeepsTheSessions(t *testing.T) { - h, _ := settingsServer(t) + h, password := settingsServer(t) bob := addUser(t, h, "bob", "bobs-long-password-1") bob1 := signedIn(t, h, "bob", "bob-1") rec := postWithSession(h.HandleUserEdit, globalPrincipal, "no-session", "/server/users/"+idStr(bob.ID), map[string]string{"uid": idStr(bob.ID)}, - url.Values{"username": {"bob"}, "role": {"global"}, "email": {"bob@example.org"}}) + url.Values{"username": {"bob"}, "role": {"global"}, "email": {"bob@example.org"}, "account_password": {password}}) if rec.Code != http.StatusSeeOther { t.Fatalf("POST user form = %d:\n%s", rec.Code, rec.Body.String()) } @@ -142,12 +142,12 @@ func TestUserFormWithoutAPasswordKeepsTheSessions(t *testing.T) { // An administrator who sets their own password through the user form is signed // out everywhere except in the browser they are using. func TestUserFormOwnPasswordKeepsTheCurrentSession(t *testing.T) { - h, _ := settingsServer(t) + h, password := settingsServer(t) current := signedIn(t, h, "admin", "admin-current") other := signedIn(t, h, "admin", "admin-other") rec := postWithSession(h.HandleUserEdit, globalPrincipal, current, "/server/users/1", map[string]string{"uid": "1"}, - url.Values{"username": {"admin"}, "role": {"global"}, "password": {"a-brand-new-long-password"}}) + url.Values{"username": {"admin"}, "role": {"global"}, "password": {"a-brand-new-long-password"}, "account_password": {password}}) if rec.Code != http.StatusSeeOther { t.Fatalf("POST user form = %d:\n%s", rec.Code, rec.Body.String()) } diff --git a/internal/web/handlers/handlers_users.go b/internal/web/handlers/handlers_users.go index e75e12f..dae20e6 100644 --- a/internal/web/handlers/handlers_users.go +++ b/internal/web/handlers/handlers_users.go @@ -203,6 +203,12 @@ func (h *Handlers) submitUserCreate(w http.ResponseWriter, r *http.Request) { h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: "Assign at least one outbound or inbound domain to a domain administrator.", FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) return } + // A new user is an account whose password the creator knows: a session + // alone must not be enough to make one (audit 2026-10-17, A2). + if status, msg := h.reauth(r, "create user"); status != 0 { + h.renderUserForm(w, r, status, 0, userFormView{FormErr: msg, FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) + return + } hash, err := bcrypt.GenerateFromPassword([]byte(password), bcryptCost) if err != nil { @@ -293,12 +299,25 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st } } - hash := "" // empty keeps the stored hash (SaveUser) if password != "" { if err := validate.AdminPassword(password); err != nil { h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: err.Error(), FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) return } + } + // A password, an e-mail (where reset links go) or a role set here is a way + // to an account, the signed-in one's own included: it asks for the + // signed-in user's password, as a backup does (audit 2026-10-17, A2). + // Name and reach alone do not. + if needsReauth(u, password, email, role) { + if status, msg := h.reauth(r, "user "+strconv.FormatInt(u.ID, 10)); status != 0 { + h.renderUserForm(w, r, status, u.ID, userFormView{FormErr: msg, FormUsername: username, FormEmail: email, FormRole: string(role), Reach: reach}) + return + } + } + + hash := "" // empty keeps the stored hash (SaveUser) + if password != "" { newHash, err := bcrypt.GenerateFromPassword([]byte(password), bcryptCost) if err != nil { logf("panel: update user hash: %v", err) @@ -336,6 +355,12 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st http.Redirect(w, r, "/server/users?done=updated", http.StatusSeeOther) } +// needsReauth is whether a user-form save changes what reaches the account: +// its password, its e-mail (reset links go there) or its role. +func needsReauth(u store.User, password, email string, role store.Role) bool { + return password != "" || email != u.Email || role != u.Role +} + // endSessionsAfterPasswordSet signs out the logins a user had under the old // password when an administrator sets a new one on the user form. An // administrator who sets their own password keeps the browser they are using. @@ -378,6 +403,13 @@ func (h *Handlers) HandleUserDeleteConfirm(w http.ResponseWriter, r *http.Reques http.Error(w, "internal error", http.StatusInternalServerError) return } + h.renderUserDelete(w, r, http.StatusOK, u, "") +} + +// renderUserDelete draws the confirmation, with a refusal above it when +// errMsg is set. +func (h *Handlers) renderUserDelete(w http.ResponseWriter, r *http.Request, status int, u store.User, errMsg string) { + var err error in := view.UserDeleteInput{ ID: u.ID, Username: u.Username, Global: u.Role == store.RoleGlobal, AllOutbound: u.AllDomains, AllInbound: u.AllInboundDomains, @@ -389,7 +421,8 @@ func (h *Handlers) HandleUserDeleteConfirm(w http.ResponseWriter, r *http.Reques return } } - h.view.Render(w, http.StatusOK, "user-delete", view.NewUserDelete(h.shellMeta(r), in)) + in.Error = errMsg + h.view.Render(w, status, "user-delete", view.NewUserDelete(h.shellMeta(r), in)) } // assignedNames names the domains a domain user is assigned one by one, outbound @@ -456,6 +489,12 @@ func (h *Handlers) submitUserDelete(w http.ResponseWriter, r *http.Request, u st h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "You cannot delete your own account while signed in.", FormUsername: u.Username, FormEmail: u.Email, FormRole: string(u.Role)}) return } + // Removing an administrator asks for the signed-in user's password: a + // session alone must not lock the others out (audit 2026-10-17, A2). + if status, msg := h.reauth(r, "delete user "+strconv.FormatInt(u.ID, 10)); status != 0 { + h.renderUserDelete(w, r, status, u, msg) + return + } if err := h.store.DeleteUser(u.ID); err != nil { if errors.Is(err, store.ErrLastGlobal) { h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Cannot delete the last global administrator.", FormUsername: u.Username, FormEmail: u.Email, FormRole: string(u.Role)}) diff --git a/internal/web/handlers/handlers_users_reauth_test.go b/internal/web/handlers/handlers_users_reauth_test.go new file mode 100644 index 0000000..bb7faf1 --- /dev/null +++ b/internal/web/handlers/handlers_users_reauth_test.go @@ -0,0 +1,136 @@ +package handlers + +import ( + "net/http" + "net/url" + "path/filepath" + "strings" + "testing" + + "github.com/mixeme/selfpost/internal/store" + "github.com/mixeme/selfpost/internal/web/auth" + "golang.org/x/crypto/bcrypt" +) + +// testAccountPassword is the signed-in user's own password in the user-form +// tests: what reauth compares account_password with. +const testAccountPassword = "the-signed-in-users-password" + +// withAccountPassword gives p's stored user testAccountPassword and h the +// auth module whose limiter reauth uses (the user form asks for the +// signed-in user's password, audit 2026-10-17 A2). +func withAccountPassword(t *testing.T, h *Handlers, st *store.Store, p auth.Principal) { + t.Helper() + hash, err := bcrypt.GenerateFromPassword([]byte(testAccountPassword), bcrypt.MinCost) + if err != nil { + t.Fatal(err) + } + u, err := st.GetUser(p.ID) + if err != nil { + t.Fatal(err) + } + if err := st.UpdateUser(u.ID, u.Username, string(hash), u.Email); err != nil { + t.Fatal(err) + } + if h.auth == nil { + h.auth = auth.New(st, auth.Config{}, h.view, filepath.Join(t.TempDir(), "setup-token")) + } +} + +// A session alone is not enough to hand out an account (audit 2026-10-17, +// A2): adding a user, and a save that sets a password, an e-mail or a role - +// the signed-in user's own included - ask for the signed-in user's password; +// so does deleting a user. Missing or wrong, it is 401 on the same page and +// nothing changes; a save of the name and the reach alone is not asked. +func TestUserFormAsksForTheAccountPassword(t *testing.T) { + h, st := inboundHandlers(t) + out, _ := st.AddDomain("out.example.com", "mail") + root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + withAccountPassword(t, h, st, root) + const newPassword = "a-long-enough-password-1" + refused := func(what string, rec interface { + Result() *http.Response + }, body string) { + t.Helper() + if code := rec.Result().StatusCode; code != http.StatusUnauthorized || !strings.Contains(body, "Your account password is incorrect.") { + t.Errorf("%s = %d, want 401 with the refusal:\n%.800s", what, code, body) + } + } + + // Create: refused without and with a wrong password, the form kept. + form := url.Values{"username": {"ops"}, "password": {newPassword}, "role": {"domain"}, "domain_ids": {idStr(out.ID)}} + for _, pw := range []string{"", "wrong-password-entirely"} { + form.Set("account_password", pw) + rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, form) + refused("create with account password "+pw, rec, rec.Body.String()) + if !strings.Contains(rec.Body.String(), `value="ops"`) || !strings.Contains(rec.Body.String(), `name="account_password" type="password" autocomplete="current-password" required`) { + t.Errorf("the refused create form lost the name or the field:\n%.800s", rec.Body.String()) + } + if u, _ := st.GetUserByUsername("ops"); u.ID != 0 { + t.Fatalf("a user was created with account password %q", pw) + } + } + form.Set("account_password", testAccountPassword) + if rec := send(h.HandleUserNew, &root, "POST", "/server/users/new", nil, form); rec.Code != http.StatusSeeOther { + t.Fatalf("create with the right password = %d:\n%s", rec.Code, rec.Body.String()) + } + ops, _ := st.GetUserByUsername("ops") + + // Update: each of password, e-mail, role is asked; the own password too. + edit := func(u store.User, f url.Values) (int, string) { + rec := send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)}, f) + return rec.Code, rec.Body.String() + } + base := func() url.Values { + return url.Values{"username": {"ops"}, "role": {"domain"}, "domain_ids": {idStr(out.ID)}, "email": {""}} + } + for name, change := range map[string]func(url.Values){ + "password": func(f url.Values) { f.Set("password", "another-long-password-2") }, + "e-mail": func(f url.Values) { f.Set("email", "thief@example.net") }, + "role": func(f url.Values) { f.Set("role", "global") }, + } { + f := base() + change(f) + code, body := edit(ops, f) + if code != http.StatusUnauthorized || !strings.Contains(body, "Your account password is incorrect.") { + t.Errorf("a %s change without the account password = %d:\n%.600s", name, code, body) + } + if got, _ := st.GetUser(ops.ID); got.PasswordHash != ops.PasswordHash || got.Email != ops.Email || got.Role != ops.Role { + t.Errorf("a refused %s change changed the user: %+v", name, got) + } + } + ownForm := url.Values{"username": {"root"}, "role": {"global"}, "password": {"the-thiefs-own-password"}} + rec := send(h.HandleUserEdit, &root, "POST", "/server/users/"+idStr(root.ID), map[string]string{"uid": idStr(root.ID)}, ownForm) + if rec.Code != http.StatusUnauthorized { + t.Errorf("setting one's own password without the current one = %d", rec.Code) + } + if got, _ := st.GetUser(root.ID); bcrypt.CompareHashAndPassword([]byte(got.PasswordHash), []byte(testAccountPassword)) != nil { + t.Error("a refused own-password change replaced the password") + } + + // Name and reach alone: not asked. + if code, body := edit(ops, base()); code != http.StatusSeeOther { + t.Errorf("a save of the reach alone = %d:\n%.600s", code, body) + } + // With the password: the change goes through. + f := base() + f.Set("password", "another-long-password-2") + f.Set("account_password", testAccountPassword) + if code, body := edit(ops, f); code != http.StatusSeeOther { + t.Errorf("a password change with the account password = %d:\n%.600s", code, body) + } + + // Delete: refused on its own page, then done with the password. + path, pv := "/server/users/"+idStr(ops.ID)+"/delete", map[string]string{"uid": idStr(ops.ID)} + rec = send(h.HandleUserDelete, &root, "POST", path, pv, url.Values{"account_password": {"wrong-password-entirely"}}) + refused("delete with a wrong password", rec, rec.Body.String()) + if !strings.Contains(rec.Body.String(), "Delete ops") || !strings.Contains(rec.Body.String(), `name="account_password"`) { + t.Errorf("the refused delete is not the confirmation page:\n%.800s", rec.Body.String()) + } + if u, _ := st.GetUser(ops.ID); u.ID == 0 { + t.Fatal("a refused delete removed the user") + } + if rec := send(h.HandleUserDelete, &root, "POST", path, pv, url.Values{"account_password": {testAccountPassword}}); rec.Code != http.StatusSeeOther { + t.Fatalf("delete with the right password = %d:\n%s", rec.Code, rec.Body.String()) + } +} diff --git a/internal/web/handlers/handlers_users_test.go b/internal/web/handlers/handlers_users_test.go index 42957fa..dc630ce 100644 --- a/internal/web/handlers/handlers_users_test.go +++ b/internal/web/handlers/handlers_users_test.go @@ -20,9 +20,10 @@ 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{}) + withAccountPassword(t, h, st, root) 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"}}) + url.Values{"username": {"shop2"}, "email": {"shop@example.com"}, "role": {"global"}, "account_password": {testAccountPassword}}) if rec.Code != http.StatusSeeOther { t.Fatalf("POST = %d:\n%s", rec.Code, rec.Body.String()) } @@ -36,7 +37,7 @@ func TestUserFormSavesTheWholeChange(t *testing.T) { // 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)}}) + url.Values{"username": {"shop2"}, "email": {"shop@example.com"}, "role": {"domain"}, "domain_ids": {idStr(out1.ID)}, "account_password": {testAccountPassword}}) if rec.Code != http.StatusSeeOther { t.Fatalf("POST = %d:\n%s", rec.Code, rec.Body.String()) } @@ -85,9 +86,10 @@ 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{}) + withAccountPassword(t, h, st, root) 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)}}) + "role": {"domain"}, "domain_ids": {idStr(out1.ID)}, "account_password": {testAccountPassword}}) if rec.Code != http.StatusSeeOther { t.Fatalf("create = %d:\n%s", rec.Code, rec.Body.String()) } diff --git a/internal/web/view/help.go b/internal/web/view/help.go index 22646c5..bff6c05 100644 --- a/internal/web/view/help.go +++ b/internal/web/view/help.go @@ -284,7 +284,7 @@ var serverTopics = []HelpTopic{ {ID: HelpUsers, Title: "Users and roles", Body: []Text{ Rich("The two roles are a reach, not a rank: every user is an administrator and signs in with a username and password. Global reaches the whole server — Overview, Server, the queue and every domain — and is the only role that adds or deletes domains and users. Domain reaches only the domains assigned to it, outbound and inbound separately (the same name on both lists is two assignments); ", Em("All"), " includes domains added later, and a domain user needs at least one domain or ", Em("All"), - " on either list. The only global user cannot be demoted or deleted, and you cannot delete the account you are signed in with. A new password set here signs that user out everywhere; set for yourself, it keeps the session you set it from."), + " on either list. The only global user cannot be demoted or deleted, and you cannot delete the account you are signed in with. A new password set here signs that user out everywhere; set for yourself, it keeps the session you set it from. Adding a user, changing a user's password, e-mail or role, and deleting a user ask for your own account password, as a backup does, with the same limit on wrong guesses."), }}, {ID: HelpSettings, Title: "Settings", Body: []Text{ Rich("Settings holds what is true of the whole instance. The time zone is the container's, ", Code("TZ"), " in ", Code(".env"), diff --git a/internal/web/view/server.go b/internal/web/view/server.go index 02fa39e..7006d8d 100644 --- a/internal/web/view/server.go +++ b/internal/web/view/server.go @@ -341,7 +341,8 @@ func NewUserForm(m Meta, in UserFormInput) *UserForm { // what goes with them, the one button that does it, and the way out. type UserDelete struct { Meta - Head Head + Head Head + Flash *Flash Danger Box Intro Text @@ -362,6 +363,8 @@ type UserDeleteInput struct { AllInbound bool Outbound []string Inbound []string + // Error is a refusal of the confirmation (a wrong account password). + Error string } // NewUserDelete builds the page. @@ -384,6 +387,9 @@ func NewUserDelete(m Meta, in UserDeleteInput) *UserDelete { Confirm: "Delete " + in.Username, KeepHref: href, } + if in.Error != "" { + p.Flash = &Flash{Error: true, Text: Plain(in.Error)} + } if in.Global { p.Consequences = append(p.Consequences, Plain("remove their global access to the server and to every domain.")) return p diff --git a/internal/web/view/templates/user.html b/internal/web/view/templates/user.html index 27bcf57..16f6e2a 100644 --- a/internal/web/view/templates/user.html +++ b/internal/web/view/templates/user.html @@ -4,7 +4,8 @@ the lists while the role is Global (data-global-role on the select, data-domain-pick on the boxes); without scripts they stay in view and the server ignores them for the global role. A blank password keeps the current - one. */}} + one. Your account password is asked for a new user and for a change of + password, e-mail or role (audit 2026-10-17, A2); the server decides. */}} {{define "content"}} {{template "page_head" .Head}} {{if .Flash}}{{template "flash" .Flash}}{{end}} @@ -25,6 +26,8 @@ {{end}}

Everyone here is an administrator; the role is the reach. Global also sees Server and is the only one who adds or deletes domains. The only global user cannot be changed to domain or deleted.

+
+

Yours, the one you sign in with, not this user's. Asked when you add a user, or change a password, an e-mail or a role here: a session left open must not be enough to hand out an account.

{{template "body_close"}} {{template "box_close"}} diff --git a/internal/web/view/templates/user_delete.html b/internal/web/view/templates/user_delete.html index cae72a9..eb07408 100644 --- a/internal/web/view/templates/user_delete.html +++ b/internal/web/view/templates/user_delete.html @@ -2,6 +2,7 @@ one button that does it, and the way back to the user's form. */}} {{define "content"}} {{template "page_head" .Head}} +{{if .Flash}}{{template "flash" .Flash}}{{end}}
@@ -11,6 +12,8 @@ {{template "confirm_list" .Consequences}}

{{template "inline" .Note}}

+
+

The one you sign in with: a session left open must not be enough to remove an administrator.

Keep it