From 02afa0fa80eefced52ac1f21444b6f67d1a1ba42 Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Fri, 14 Aug 2026 14:48:43 +0300 Subject: [PATCH] fix(panel): code-review P6 optional cleanup Close the remaining low-risk items from the full-tree review: rename the settings handler, query assigned domains in SQL, bound the login limiter map, collapse panel.js show/hide helpers, and soften DMARC copy that promised a future in-panel receiver. Co-authored-by: Cursor --- CHANGELOG.md | 11 + docs/architecture.md | 2 +- docs/plans/code-review.md | 16 +- internal/store/domains.go | 26 +++ internal/web/auth/auth.go | 2 + internal/web/auth/middleware.go | 12 -- internal/web/auth/ratelimit.go | 53 ++++- internal/web/auth/ratelimit_test.go | 25 +++ internal/web/handlers/authz.go | 12 +- internal/web/handlers/authz_test.go | 2 +- .../web/handlers/handlers_monitor_test.go | 59 ++++-- ...ndlers_account.go => handlers_settings.go} | 48 ++--- internal/web/view/static/panel.js | 192 ++++++------------ .../web/view/templates/domain_detail.html | 3 +- internal/web/view/templates/settings.html | 4 +- internal/web/web.go | 2 +- 16 files changed, 260 insertions(+), 209 deletions(-) rename internal/web/handlers/{handlers_account.go => handlers_settings.go} (73%) diff --git a/CHANGELOG.md b/CHANGELOG.md index f987dd7..26eade3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -154,6 +154,17 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version [docs/plans/dmarc-reports.md](docs/plans/dmarc-reports.md) to the setting's actual home after migration `0005`. No behaviour change. +- panel: code-review P6 cleanup — the unused `auth.RequireGlobal` middleware is + gone (handlers already call `requireGlobal`); the settings route handler is + named `HandleSettings` in `handlers_settings.go`; domain lists for a + domain-admin now come from `ListDomainsForUser` in SQL instead of loading + every domain and filtering in Go; the login and setup rate limiters sweep + expired buckets on a timer and cap the map at 4096 keys; the five + show/hide field helpers in `panel.js` are one rule table; DMARC copy no + longer promises in-panel report reception in a future release — SelfPost + does not receive inbound mail. No optional post-restore map `Resync` (the + backup package comment fix in P2 is enough). + ## [1.2.5] - 2026-08-13 Rate-limit form polish after 1.2.4. Upgrading is a tag bump; no migration. diff --git a/docs/architecture.md b/docs/architecture.md index cffd813..731545e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -196,7 +196,7 @@ holds the cookie works after process restart, redeploy, or full backup restore. (`renewThreshold` in [internal/web/auth/session.go](../internal/web/auth/session.go)). - **Password change** — all other sessions are deleted; the current session stays active ([internal/store/sessions.go](../internal/store/sessions.go), - [handlers_account.go](../internal/web/handlers/handlers_account.go)). + [handlers_settings.go](../internal/web/handlers/handlers_settings.go)). Restoring an **older** backup also restores session rows: a session invalidated after that backup was taken can become valid again if the browser still has the diff --git a/docs/plans/code-review.md b/docs/plans/code-review.md index bb095dd..cbabfbd 100644 --- a/docs/plans/code-review.md +++ b/docs/plans/code-review.md @@ -614,14 +614,14 @@ for SPDX consistency and deleting the done logrotate plan. **Model: Sonnet** unless noted. -- [ ] Use `auth.RequireGlobal` or delete it (**Haiku** if delete). -- [ ] Rename `handlers_account.go` / `HandleAccount` to settings (**Haiku**). -- [ ] `ListDomainsForUser` instead of load-all-and-filter. -- [ ] Cap or periodically sweep the login limiter map (**Opus**, small). -- [ ] Collapse `panel.js` field-sync helpers. -- [ ] Soften or keep DMARC “future release” UI copy (product call). -- [ ] Optional startup `Resync` after restore (**Opus**). Only if P2’s comment - fix is judged insufficient. +- [x] Use `auth.RequireGlobal` or delete it (**Haiku** if delete). Deleted. +- [x] Rename `handlers_account.go` / `HandleAccount` to settings (**Haiku**). +- [x] `ListDomainsForUser` instead of load-all-and-filter. +- [x] Cap or periodically sweep the login limiter map (**Opus**, small). +- [x] Collapse `panel.js` field-sync helpers. +- [x] Soften or keep DMARC “future release” UI copy (product call). Softened. +- [ ] Optional startup `Resync` after restore (**Opus**). Dropped — P2 backup + comment fix is sufficient. ### P7 — Security review of the P0–P1 diff diff --git a/internal/store/domains.go b/internal/store/domains.go index 2fd9872..0c28e2a 100644 --- a/internal/store/domains.go +++ b/internal/store/domains.go @@ -75,6 +75,32 @@ func (s *Store) ListDomains() ([]Domain, error) { return out, rows.Err() } +// ListDomainsForUser returns domains assigned to userID with application counts, +// ordered by name. +func (s *Store) ListDomainsForUser(userID int64) ([]Domain, error) { + rows, err := s.db.Query(` + SELECT d.id, d.name, d.dkim_selector, d.dmarc_rua, d.created_at, + (SELECT COUNT(*) FROM applications a WHERE a.domain_id = d.id) + FROM domains d + INNER JOIN user_domains ud ON ud.domain_id = d.id + WHERE ud.user_id = ? + ORDER BY d.name`, userID) + if err != nil { + return nil, fmt.Errorf("list domains for user: %w", err) + } + defer rows.Close() + + var out []Domain + for rows.Next() { + d, err := scanDomain(rows) + if err != nil { + return nil, err + } + out = append(out, d) + } + return out, rows.Err() +} + // GetDomain returns a single domain (with its application count) by id, or // ErrDomainNotFound. func (s *Store) GetDomain(id int64) (Domain, error) { diff --git a/internal/web/auth/auth.go b/internal/web/auth/auth.go index cc4bff3..bf4b5a0 100644 --- a/internal/web/auth/auth.go +++ b/internal/web/auth/auth.go @@ -51,6 +51,8 @@ func New(st *store.Store, cfg Config, v *view.Engine, setupTokenPath string) *Mo loginLimiter: newRateLimiter(10, 15*time.Minute), trustedProxies: cfg.TrustedProxyCIDRs, } + m.setupLimiter.startSweeper() + m.loginLimiter.startSweeper() m.setup = newSetupManager(st, cfg.Hostname, setupTokenPath) return m } diff --git a/internal/web/auth/middleware.go b/internal/web/auth/middleware.go index 127373c..e1d119f 100644 --- a/internal/web/auth/middleware.go +++ b/internal/web/auth/middleware.go @@ -44,15 +44,3 @@ func CurrentUser(r *http.Request) string { } return "" } - -// RequireGlobal wraps a handler that only global administrators may reach. -func RequireGlobal(next http.Handler) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - p, ok := CurrentPrincipal(r.Context()) - if !ok || !p.IsGlobal() { - http.NotFound(w, r) - return - } - next.ServeHTTP(w, r) - }) -} diff --git a/internal/web/auth/ratelimit.go b/internal/web/auth/ratelimit.go index 5658fc8..f8b09f4 100644 --- a/internal/web/auth/ratelimit.go +++ b/internal/web/auth/ratelimit.go @@ -5,11 +5,14 @@ import ( "time" ) +const defaultMaxBuckets = 4096 + // rateLimiter is a simple fixed-window per-key counter used to throttle the // setup and login routes (security.md). Keys are client IPs. type rateLimiter struct { - max int - window time.Duration + max int + window time.Duration + maxBuckets int mu sync.Mutex buckets map[string]*rlBucket @@ -22,12 +25,25 @@ type rlBucket struct { func newRateLimiter(max int, window time.Duration) *rateLimiter { return &rateLimiter{ - max: max, - window: window, - buckets: make(map[string]*rlBucket), + max: max, + window: window, + maxBuckets: defaultMaxBuckets, + buckets: make(map[string]*rlBucket), } } +func (r *rateLimiter) startSweeper() { + go func() { + ticker := time.NewTicker(r.window) + defer ticker.Stop() + for range ticker.C { + r.mu.Lock() + r.sweep(time.Now()) + r.mu.Unlock() + } + }() +} + func (r *rateLimiter) Allow(key string) bool { now := time.Now() r.mu.Lock() @@ -35,6 +51,7 @@ func (r *rateLimiter) Allow(key string) bool { b := r.buckets[key] if b == nil || now.After(b.windowEnds) { + r.makeRoom(now) r.buckets[key] = &rlBucket{count: 1, windowEnds: now.Add(r.window)} r.sweep(now) return true @@ -46,6 +63,32 @@ func (r *rateLimiter) Allow(key string) bool { return true } +func (r *rateLimiter) makeRoom(now time.Time) { + if r.maxBuckets <= 0 || len(r.buckets) < r.maxBuckets { + return + } + r.sweep(now) + for len(r.buckets) >= r.maxBuckets { + r.evictOldest() + } +} + +func (r *rateLimiter) evictOldest() { + var oldestKey string + var oldestEnds time.Time + first := true + for k, b := range r.buckets { + if first || b.windowEnds.Before(oldestEnds) { + oldestKey = k + oldestEnds = b.windowEnds + first = false + } + } + if oldestKey != "" { + delete(r.buckets, oldestKey) + } +} + func (r *rateLimiter) sweep(now time.Time) { for k, b := range r.buckets { if now.After(b.windowEnds) { diff --git a/internal/web/auth/ratelimit_test.go b/internal/web/auth/ratelimit_test.go index 86a7f32..696bff9 100644 --- a/internal/web/auth/ratelimit_test.go +++ b/internal/web/auth/ratelimit_test.go @@ -98,3 +98,28 @@ func expire(r *rateLimiter, key string) { b.windowEnds = time.Now().Add(-time.Second) } } + +// A long-running panel can see many unique client addresses. Finished buckets +// are swept on every new window, and a hard cap evicts the oldest when the map +// would otherwise grow without bound. +func TestRateLimiterCapsBucketCount(t *testing.T) { + r := newRateLimiter(1, time.Minute) + r.maxBuckets = 3 + + for i, key := range []string{"203.0.113.7", "198.51.100.9", "192.0.2.5"} { + if !r.Allow(key) { + t.Fatalf("attempt %d for %s was refused under the cap", i+1, key) + } + expire(r, key) + } + + if !r.Allow("203.0.113.8") { + t.Fatal("a fourth address was refused even though room was made") + } + + r.mu.Lock() + defer r.mu.Unlock() + if len(r.buckets) > 3 { + t.Fatalf("bucket count = %d, want at most 3", len(r.buckets)) + } +} diff --git a/internal/web/handlers/authz.go b/internal/web/handlers/authz.go index 5d8690b..3c5eabd 100644 --- a/internal/web/handlers/authz.go +++ b/internal/web/handlers/authz.go @@ -32,17 +32,7 @@ func (h *Handlers) assignedDomains(p auth.Principal) ([]store.Domain, error) { if p.IsGlobal() { return h.store.ListDomains() } - all, err := h.store.ListDomains() - if err != nil { - return nil, err - } - var out []store.Domain - for _, d := range all { - if p.CanAccessDomain(d.ID) { - out = append(out, d) - } - } - return out, nil + return h.store.ListDomainsForUser(p.ID) } func domainNameSet(domains []store.Domain) map[string]bool { diff --git a/internal/web/handlers/authz_test.go b/internal/web/handlers/authz_test.go index d1404f2..017f771 100644 --- a/internal/web/handlers/authz_test.go +++ b/internal/web/handlers/authz_test.go @@ -57,7 +57,7 @@ var globalOnlyRoutes = []route{ // case the send-log leak (P0, code-review.md) showed is easy to get wrong. func TestGlobalOnlyRoutesAnswerADomainAdmin404(t *testing.T) { h, domains := serverWithTwoDomains(t) - p := domainAdmin(domains["first.example.ru"].ID) + p := domainAdmin(t, h.store, "global-only", domains["first.example.ru"].ID) for _, rt := range globalOnlyRoutes { rec := call(h, rt, p) diff --git a/internal/web/handlers/handlers_monitor_test.go b/internal/web/handlers/handlers_monitor_test.go index 748bbbe..b70db28 100644 --- a/internal/web/handlers/handlers_monitor_test.go +++ b/internal/web/handlers/handlers_monitor_test.go @@ -222,34 +222,41 @@ func TestSendLogScopedToAssignedDomains(t *testing.T) { h, domains := serverWithTwoDomains(t) for name, tc := range map[string]struct { - principal auth.Principal + username string + domainIDs []int64 want []string unwanted []string }{ "global sees both": { - globalPrincipal, []string{"First message", "Second message"}, nil, + "", nil, []string{"First message", "Second message"}, nil, }, "one assigned domain": { - domainAdmin(domains["first.example.ru"].ID), + "one-domain", []int64{domains["first.example.ru"].ID}, []string{"First message"}, []string{"Second message", "second-app"}, }, "two assigned domains": { - domainAdmin(domains["first.example.ru"].ID, domains["second.example.ru"].ID), + "two-domains", []int64{domains["first.example.ru"].ID, domains["second.example.ru"].ID}, []string{"First message", "Second message"}, nil, }, // Every assigned domain deleted cascades the assignments away. That // leaves a principal entitled to nothing, which is an empty log — the // case that used to hand over the whole journal. "no assigned domains": { - domainAdmin(), []string{"No messages logged yet."}, + "no-domains", nil, []string{"No messages logged yet."}, []string{"First message", "Second message"}, }, } { + var p auth.Principal + if tc.username == "" { + p = globalPrincipal + } else { + p = domainAdmin(t, h.store, tc.username, tc.domainIDs...) + } for view, handler := range map[string]http.HandlerFunc{ "page": h.HandleDeliveries, "fragment": h.HandleDeliveriesRows, } { - out := getBodyAs(t, handler, "/deliveries", tc.principal) + out := getBodyAs(t, handler, "/deliveries", p) for _, want := range tc.want { if !strings.Contains(out, want) { t.Errorf("%s (%s): missing %q:\n%s", name, view, want, out) @@ -270,7 +277,7 @@ func TestSendLogScopedToAssignedDomains(t *testing.T) { // trusted for having been rendered by us. func TestSendLogIgnoresForgedFilters(t *testing.T) { h, domains := serverWithTwoDomains(t) - p := domainAdmin(domains["first.example.ru"].ID) + p := domainAdmin(t, h.store, "forged-filter", domains["first.example.ru"].ID) for _, target := range []string{ "/deliveries?domain=second.example.ru", @@ -299,7 +306,7 @@ func TestDeliveryPageForeignDomainNotFound(t *testing.T) { rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodGet, "/deliveries/"+itoa(rows[0].ID), nil) req.SetPathValue("id", itoa(rows[0].ID)) - req = auth.RequestWithPrincipal(req, domainAdmin(domains["first.example.ru"].ID)) + req = auth.RequestWithPrincipal(req, domainAdmin(t, h.store, "foreign-detail", domains["first.example.ru"].ID)) h.HandleDelivery(rec, req) if rec.Code != http.StatusNotFound { t.Errorf("delivery page for a foreign domain = %d, want 404", rec.Code) @@ -343,13 +350,37 @@ func serverWithTwoDomains(t *testing.T) (*Handlers, map[string]store.Domain) { var globalPrincipal = auth.Principal{ID: 1, Username: "admin", Role: auth.RoleGlobal} -func domainAdmin(domainIDs ...int64) auth.Principal { - return auth.Principal{ - ID: 2, - Username: "domain-admin", - Role: auth.RoleDomainAdmin, - Domains: domainIDs, +func domainAdmin(t *testing.T, st *store.Store, username string, domainIDs ...int64) auth.Principal { + t.Helper() + const hash = "test-hash" + if len(domainIDs) == 0 { + placeholder, err := st.AddDomain(username+".placeholder.invalid", "mail") + if err != nil { + t.Fatalf("add placeholder domain: %v", err) + } + id, err := st.CreateUser(username, hash, store.RoleDomainAdmin, []int64{placeholder.ID}) + if err != nil { + t.Fatalf("create domain admin %s: %v", username, err) + } + if err := st.DeleteDomain(placeholder.ID); err != nil { + t.Fatalf("delete placeholder domain: %v", err) + } + domainIDs = nil + u, err := st.GetUser(id) + if err != nil { + t.Fatalf("get domain admin %s: %v", username, err) + } + return auth.Principal{ID: u.ID, Username: u.Username, Role: u.Role, Domains: u.DomainIDs} } + id, err := st.CreateUser(username, hash, store.RoleDomainAdmin, domainIDs) + if err != nil { + t.Fatalf("create domain admin %s: %v", username, err) + } + u, err := st.GetUser(id) + if err != nil { + t.Fatalf("get domain admin %s: %v", username, err) + } + return auth.Principal{ID: u.ID, Username: u.Username, Role: u.Role, Domains: u.DomainIDs} } // serverWithDelivery builds a panel over a store holding one delivery, written diff --git a/internal/web/handlers/handlers_account.go b/internal/web/handlers/handlers_settings.go similarity index 73% rename from internal/web/handlers/handlers_account.go rename to internal/web/handlers/handlers_settings.go index dbacd70..210cb8e 100644 --- a/internal/web/handlers/handlers_account.go +++ b/internal/web/handlers/handlers_settings.go @@ -14,8 +14,8 @@ import ( "golang.org/x/crypto/bcrypt" ) -// HandleAccount serves the signed-in user's account settings. -func (h *Handlers) HandleAccount(w http.ResponseWriter, r *http.Request) { +// HandleSettings serves the signed-in user's panel settings. +func (h *Handlers) HandleSettings(w http.ResponseWriter, r *http.Request) { switch r.Method { case http.MethodGet: p, ok := h.principal(r) @@ -25,20 +25,20 @@ func (h *Handlers) HandleAccount(w http.ResponseWriter, r *http.Request) { } u, err := h.store.GetUser(p.ID) if err != nil { - logf("panel: account: get user failed: %v", err) + logf("panel: settings: get user failed: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) return } - h.renderAccount(w, r, http.StatusOK, "", u.Username, u.DMARCReportEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusOK, "", u.Username, u.DMARCReportEmail, p.IsGlobal()) case http.MethodPost: - h.submitAccount(w, r) + h.submitSettings(w, r) default: w.Header().Set("Allow", "GET, POST") http.Error(w, "method not allowed", http.StatusMethodNotAllowed) } } -func (h *Handlers) renderAccount(w http.ResponseWriter, r *http.Request, status int, formErr, formUsername, formDMARCEmail string, showDMARC bool) { +func (h *Handlers) renderSettings(w http.ResponseWriter, r *http.Request, status int, formErr, formUsername, formDMARCEmail string, showDMARC bool) { var reportAuth dnscheck.Result if showDMARC && formDMARCEmail != "" { if hub := dnscheck.EmailDomain(formDMARCEmail); hub != "" { @@ -58,13 +58,13 @@ func (h *Handlers) renderAccount(w http.ResponseWriter, r *http.Request, status data["ReportAuthDNS"] = reportAuth data["ReportAuthHub"] = dnscheck.EmailDomain(formDMARCEmail) data["Error"] = formErr - data["Flash"] = accountFlash(r) + data["Flash"] = settingsFlash(r) data["L1Messages"] = h.l1Messages() data["L1Window"] = h.l1Window() h.view.Render(w, status, "settings", data) } -func accountFlash(r *http.Request) string { +func settingsFlash(r *http.Request) string { switch r.URL.Query().Get("updated") { case "username": return "Username changed." @@ -85,16 +85,16 @@ func accountFlash(r *http.Request) string { } } -func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { +func (h *Handlers) submitSettings(w http.ResponseWriter, r *http.Request) { if !h.auth.AllowLoginAttempt(r) { p, _ := h.principal(r) - h.renderAccount(w, r, http.StatusTooManyRequests, + h.renderSettings(w, r, http.StatusTooManyRequests, "Too many attempts. Please wait and try again.", auth.CurrentUser(r), "", p.IsGlobal()) return } if err := r.ParseForm(); err != nil { p, _ := h.principal(r) - h.renderAccount(w, r, http.StatusBadRequest, "Invalid form submission.", auth.CurrentUser(r), "", p.IsGlobal()) + h.renderSettings(w, r, http.StatusBadRequest, "Invalid form submission.", auth.CurrentUser(r), "", p.IsGlobal()) return } @@ -105,7 +105,7 @@ func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { } user, err := h.store.GetUser(p.ID) if err != nil { - logf("panel: account: get user failed: %v", err) + logf("panel: settings: get user failed: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) return } @@ -123,21 +123,21 @@ func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { } if err := bcrypt.CompareHashAndPassword([]byte(user.PasswordHash), []byte(current)); err != nil { - h.renderAccount(w, r, http.StatusUnauthorized, "Current password is incorrect.", username, dmarcEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusUnauthorized, "Current password is incorrect.", username, dmarcEmail, p.IsGlobal()) return } renaming := username != user.Username if renaming { if err := validate.Username(username); err != nil { - h.renderAccount(w, r, http.StatusBadRequest, err.Error(), username, dmarcEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusBadRequest, err.Error(), username, dmarcEmail, p.IsGlobal()) return } } if p.IsGlobal() { if err := validate.Email(dmarcEmail); err != nil { - h.renderAccount(w, r, http.StatusBadRequest, err.Error(), username, dmarcEmail, true) + h.renderSettings(w, r, http.StatusBadRequest, err.Error(), username, dmarcEmail, true) return } } @@ -147,16 +147,16 @@ func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { repassword := password != "" || confirm != "" if repassword { if password != confirm { - h.renderAccount(w, r, http.StatusBadRequest, "New passwords do not match.", username, dmarcEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusBadRequest, "New passwords do not match.", username, dmarcEmail, p.IsGlobal()) return } if err := validate.AdminPassword(password); err != nil { - h.renderAccount(w, r, http.StatusBadRequest, err.Error(), username, dmarcEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusBadRequest, err.Error(), username, dmarcEmail, p.IsGlobal()) return } } if !renaming && !repassword && !emailChanging { - h.renderAccount(w, r, http.StatusBadRequest, + h.renderSettings(w, r, http.StatusBadRequest, "Nothing to change: enter a new username, password, or DMARC report address.", username, dmarcEmail, p.IsGlobal()) return } @@ -165,8 +165,8 @@ func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { if repassword { newHash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) if err != nil { - logf("panel: account: hashing password failed: %v", err) - h.renderAccount(w, r, http.StatusInternalServerError, + logf("panel: settings: hashing password failed: %v", err) + h.renderSettings(w, r, http.StatusInternalServerError, "Internal error. Please try again.", username, dmarcEmail, p.IsGlobal()) return } @@ -174,17 +174,17 @@ func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { } if err := h.store.UpdateUser(user.ID, username, hash, dmarcEmail); err != nil { - logf("panel: account: update user failed: %v", err) + logf("panel: settings: update user failed: %v", err) msg := "Could not save the changes. Please check the logs and try again." if errors.Is(err, store.ErrUserNotFound) { msg = "There is no user account to update." } if errors.Is(err, store.ErrUserExists) { msg = "That username is already in use." - h.renderAccount(w, r, http.StatusConflict, msg, username, dmarcEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusConflict, msg, username, dmarcEmail, p.IsGlobal()) return } - h.renderAccount(w, r, http.StatusInternalServerError, msg, username, dmarcEmail, p.IsGlobal()) + h.renderSettings(w, r, http.StatusInternalServerError, msg, username, dmarcEmail, p.IsGlobal()) return } @@ -197,7 +197,7 @@ func (h *Handlers) submitAccount(w http.ResponseWriter, r *http.Request) { } } - logf("panel: user %d account updated (username: %t, password: %t, dmarc email: %t)", user.ID, renaming, repassword, emailChanging) + logf("panel: user %d settings updated (username: %t, password: %t, dmarc email: %t)", user.ID, renaming, repassword, emailChanging) http.Redirect(w, r, "/settings?updated="+updatedFlag(renaming, repassword, emailChanging), http.StatusSeeOther) } diff --git a/internal/web/view/static/panel.js b/internal/web/view/static/panel.js index 2e8993c..98147f9 100644 --- a/internal/web/view/static/panel.js +++ b/internal/web/view/static/panel.js @@ -46,145 +46,81 @@ } }); - // --- Address list shown only in list mode ----------------------------- - // The "Addresses" field applies to list mode only; in wildcard mode the - // server ignores it, so hiding it removes a field that does nothing. The - // toggle runs on load too, because the edit form of an existing application - // may already be set to list mode. - function syncAddressField(select) { - var form = select.closest("form"); - var field = form && form.querySelector("[data-addresses]"); - if (!field) { + // --- Conditional field visibility ------------------------------------ + // Several forms hide a block until a select, checkbox or file input says it + // applies. One rule table drives them all so the five near-identical helpers + // do not drift. + var showWhenRules = [ + { + match: "select[data-list-mode]", + target: "[data-addresses]", + visible: function (el) { return el.value === el.dataset.listMode; } + }, + { + match: "select[data-custom-mode]", + target: "[data-custom-address]", + visible: function (el) { return el.value === el.dataset.customMode; } + }, + { + match: "select[data-global-role]", + target: "[data-domain-pick]", + visible: function (el) { return el.value !== el.dataset.globalRole; } + }, + { + match: "input[data-encrypt-toggle]", + target: "[data-encrypt-fields]", + visible: function (el) { return el.checked; }, + clearWhenHidden: true + }, + { + match: "input[data-import-file]", + target: "[data-import-password-fields]", + visible: function (el) { + var name = (el.files && el.files[0] && el.files[0].name || "").toLowerCase(); + return name !== "" && !/\.json$/.test(name); + }, + clearWhenHidden: true + } + ]; + + function syncShowWhen(control) { + var form = control.closest("form"); + if (!form) { return; } - // The mode values come from the server (store.AddressModeList), so the - // select carries the one that means "list" rather than this script - // hard-coding it. - field.hidden = select.value !== select.dataset.listMode; - } - - function initAddressFields(root) { - root.querySelectorAll("select[data-list-mode]").forEach(function (select) { - syncAddressField(select); - select.addEventListener("change", function () { - syncAddressField(select); - }); - }); - } - - // --- Custom DMARC rua address shown only for "custom" mode ------------ - // Same idea as the address list: the email field only applies when the - // operator picks Custom address. With JavaScript blocked the field stays - // visible and the server still ignores it for inherit/none. - function syncCustomAddressField(select) { - var form = select.closest("form"); - var field = form && form.querySelector("[data-custom-address]"); - if (!field) { + for (var i = 0; i < showWhenRules.length; i++) { + var rule = showWhenRules[i]; + if (!control.matches(rule.match)) { + continue; + } + var target = form.querySelector(rule.target); + if (!target) { + return; + } + var show = rule.visible(control); + target.hidden = !show; + if (!show && rule.clearWhenHidden) { + target.querySelectorAll("input").forEach(function (input) { + input.value = ""; + }); + } return; } - field.hidden = select.value !== select.dataset.customMode; } - function initCustomAddressFields(root) { - root.querySelectorAll("select[data-custom-mode]").forEach(function (select) { - syncCustomAddressField(select); - select.addEventListener("change", function () { - syncCustomAddressField(select); - }); - }); - } - - // --- Domain pick shown only for domain administrators ------------------ - // Global administrators manage every domain, so the assignment checkboxes - // are irrelevant for that role. The toggle runs on load too, because the - // edit form of an existing global user should not flash the fieldset. - function syncDomainPickField(select) { - var form = select.closest("form"); - var field = form && form.querySelector("[data-domain-pick]"); - if (!field) { - return; - } - field.hidden = select.value === select.dataset.globalRole; - } - - function initDomainPickFields(root) { - root.querySelectorAll("select[data-global-role]").forEach(function (select) { - syncDomainPickField(select); - select.addEventListener("change", function () { - syncDomainPickField(select); - }); - }); - } - - // --- Encryption password fields shown only when asked for -------------- - // The backup, export and import forms carry an optional password block. It - // is hidden until the checkbox next to it is ticked, and cleared when it is - // unticked, so a password typed and then abandoned is never submitted. With - // JavaScript blocked the block stays visible and the forms behave exactly as - // the server reads them: the checkbox alone decides whether encryption - // happens. - function syncEncryptFields(box) { - var form = box.closest("form"); - var fields = form && form.querySelector("[data-encrypt-fields]"); - if (!fields) { - return; - } - fields.hidden = !box.checked; - if (!box.checked) { - fields.querySelectorAll("input").forEach(function (input) { - input.value = ""; - }); - } - } - - function initEncryptFields(root) { - root.querySelectorAll("input[data-encrypt-toggle]").forEach(function (box) { - syncEncryptFields(box); - box.addEventListener("change", function () { - syncEncryptFields(box); - }); - }); - } - - // --- Import password field shown based on the chosen file's extension --- - // The domain-import file decides for itself whether it is encrypted (the - // server checks the envelope magic, not a checkbox), so the panel offers - // the password field the same way: reveal it for a .spde file, hide and - // clear it for a plain .json one. With no file chosen yet there is nothing - // to ask a password for, so the field stays hidden until a file names it. - // An unrecognised name leaves the field visible rather than guessing wrong - // and hiding a password the file needs. - function syncImportPasswordField(input) { - var form = input.closest("form"); - var fields = form && form.querySelector("[data-import-password-fields]"); - if (!fields) { - return; - } - var name = (input.files && input.files[0] && input.files[0].name || "").toLowerCase(); - var hide = name === "" || /\.json$/.test(name); - fields.hidden = hide; - if (hide) { - fields.querySelectorAll("input").forEach(function (pw) { - pw.value = ""; - }); - } - } - - function initImportPasswordField(root) { - root.querySelectorAll("input[data-import-file]").forEach(function (input) { - syncImportPasswordField(input); - input.addEventListener("change", function () { - syncImportPasswordField(input); + function initShowWhen(root) { + showWhenRules.forEach(function (rule) { + root.querySelectorAll(rule.match).forEach(function (control) { + syncShowWhen(control); + control.addEventListener("change", function () { + syncShowWhen(control); + }); }); }); } document.addEventListener("DOMContentLoaded", function () { - initAddressFields(document); - initCustomAddressFields(document); - initDomainPickFields(document); - initEncryptFields(document); - initImportPasswordField(document); + initShowWhen(document); }); // --- Adaptive monitoring polling --------------------------------------- diff --git a/internal/web/view/templates/domain_detail.html b/internal/web/view/templates/domain_detail.html index df45f15..a32b250 100644 --- a/internal/web/view/templates/domain_detail.html +++ b/internal/web/view/templates/domain_detail.html @@ -171,8 +171,7 @@ {{if .SameDomainRUA}}

The report address is on this sending domain. SelfPost does - not receive inbound mail — use a mailbox elsewhere or wait for in-panel report - reception in a future release.

+ not receive inbound mail — use a mailbox on another domain.

{{end}} {{if .NeedsReportAuth}} diff --git a/internal/web/view/templates/settings.html b/internal/web/view/templates/settings.html index 4d5a37f..c83310a 100644 --- a/internal/web/view/templates/settings.html +++ b/internal/web/view/templates/settings.html @@ -42,8 +42,8 @@

DMARC aggregate reports

Optional default rua= address for every sending domain (can be overridden per domain). Use a mailbox on a domain that - receives inbound mail. SelfPost is send-only today; a future release will - be able to receive reports in the panel itself.

+ receives inbound mail. SelfPost does not receive inbound mail — point + rua= at a mailbox elsewhere.