From 05706087386c37e0b79cfd860be82bcbbde10bc2 Mon Sep 17 00:00:00 2001 From: Mikhail Yenuchenko Date: Thu, 13 Aug 2026 21:50:42 +0300 Subject: [PATCH] fix(panel): GUI defects from the 1.2.x layout pass (P3) - .flash.error now renders on the danger surface instead of the success one; RateLimitErr previously showed as green with red text. - User delete goes through a confirmation page (GET/POST /users/{uid}/delete), matching the domain-delete pattern, instead of a plain submit button next to Save with no confirmation. - Extracted the repeated DNS Host/Type/Value markup on a domain's page and the duplicated Settings credentials form into shared partials. No behaviour change. docs/plans/code-review.md P3 checked off; CHANGELOG updated. Co-authored-by: Cursor --- CHANGELOG.md | 12 ++ docs/plans/code-review.md | 9 +- internal/web/handlers/handlers_users.go | 56 ++++- internal/web/view/static/panel.css | 5 + .../web/view/templates/domain_detail.html | 200 ++++++------------ internal/web/view/templates/settings.html | 47 ++-- internal/web/view/templates/user_delete.html | 16 ++ internal/web/view/templates/user_form.html | 11 +- internal/web/view/templates_test.go | 1 + internal/web/view/view.go | 1 + internal/web/web.go | 2 + 11 files changed, 192 insertions(+), 168 deletions(-) create mode 100644 internal/web/view/templates/user_delete.html diff --git a/CHANGELOG.md b/CHANGELOG.md index a913e02..7cd31fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,12 +44,24 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version still authenticate to Postfix. This matches the order domain deletion already used. +- panel (GUI): a rejected rate-limit change on a domain's page now renders on + the danger surface (`.flash.error`) instead of the success one — it was + green with red text, reading as good news. Deleting a panel user now goes + through a confirmation page, the same pattern as domain deletion, instead of + a plain submit button next to Save with no confirmation at all. + ### Changed - ci: gofmt on eight files that failed the formatting workflow check (panel config, DNS check, domain transfer export, rate-limit tests, auth principal, domain and delivery handlers, web package doc comment). +- panel (templates): the repeated Host/Type/Value DNS record markup on a + domain's page and the duplicated credentials form on Settings are now + shared partials (`host_type`, `host_type_copy`, `field_value`, + `field_values`, `credentials_fields`) instead of copy-pasted blocks. No + behaviour or visible change. + - docs: full-tree review plan ([docs/plans/code-review.md](docs/plans/code-review.md)) — architecture, quality, GUI, tests, licence; P0 is domain-admin send-log authorization. Roadmap queues that plan ahead of inbound-relay and records diff --git a/docs/plans/code-review.md b/docs/plans/code-review.md index 31a454e..0a1fb23 100644 --- a/docs/plans/code-review.md +++ b/docs/plans/code-review.md @@ -545,11 +545,12 @@ roles, and security.md no longer calls the panel single-user. **Model: Sonnet.** -- [ ] `.flash.error` (or stop using `.flash` for `RateLimitErr`) — danger +- [x] `.flash.error` (or stop using `.flash` for `RateLimitErr`) — danger surface, not success. -- [ ] User delete: `data-confirm` at minimum; prefer a confirm page like - domain delete. -- [ ] DNS field partial; settings credentials partial. +- [x] User delete: `data-confirm` at minimum; prefer a confirm page like + domain delete. Done as a confirm page (`GET/POST /users/{uid}/delete`), + matching `domain_delete.html`. +- [x] DNS field partial; settings credentials partial. - [ ] Optional: `urlquery` on deliveries fragment params; `aria-live` on polled regions; confirm-without-JS note next to the CSRF accepted risks. diff --git a/internal/web/handlers/handlers_users.go b/internal/web/handlers/handlers_users.go index ce80617..05330b6 100644 --- a/internal/web/handlers/handlers_users.go +++ b/internal/web/handlers/handlers_users.go @@ -102,10 +102,6 @@ func (h *Handlers) HandleUserEdit(w http.ResponseWriter, r *http.Request) { h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Invalid form submission.", FormUsername: u.Username, FormRole: string(u.Role)}) return } - if r.PostFormValue("action") == "delete" { - h.submitUserDelete(w, r, u) - return - } h.submitUserUpdate(w, r, u) default: w.Header().Set("Allow", "GET, POST") @@ -291,6 +287,58 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st http.Redirect(w, r, "/users?done=updated", http.StatusSeeOther) } +// HandleUserDeleteConfirm shows the cascade warning before a panel user is +// removed — the same pattern as HandleDeleteConfirm for domains, so a single +// mis-click on Delete cannot remove a user (P3, code-review.md). +func (h *Handlers) HandleUserDeleteConfirm(w http.ResponseWriter, r *http.Request) { + if _, ok := h.requireGlobal(w, r); !ok { + return + } + uid, ok := parseUserID(w, r) + if !ok { + return + } + u, err := h.store.GetUser(uid) + if err != nil { + if errors.Is(err, store.ErrUserNotFound) { + http.NotFound(w, r) + return + } + logf("panel: get user %d: %v", uid, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + data := h.pageBase(r) + data["Title"] = "SelfPost — delete " + u.Username + data["Active"] = "users" + data["TargetID"] = u.ID + data["TargetUsername"] = u.Username + h.view.Render(w, http.StatusOK, "user_delete", data) +} + +// HandleUserDelete performs the deletion confirmed on HandleUserDeleteConfirm +// and returns to the user list. +func (h *Handlers) HandleUserDelete(w http.ResponseWriter, r *http.Request) { + if _, ok := h.requireGlobal(w, r); !ok { + return + } + uid, ok := parseUserID(w, r) + if !ok { + return + } + u, err := h.store.GetUser(uid) + if err != nil { + if errors.Is(err, store.ErrUserNotFound) { + http.NotFound(w, r) + return + } + logf("panel: get user %d: %v", uid, err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + h.submitUserDelete(w, r, u) +} + func (h *Handlers) submitUserDelete(w http.ResponseWriter, r *http.Request, u store.User) { p, ok := h.principal(r) if !ok { diff --git a/internal/web/view/static/panel.css b/internal/web/view/static/panel.css index 68f107e..30bab78 100644 --- a/internal/web/view/static/panel.css +++ b/internal/web/view/static/panel.css @@ -245,6 +245,11 @@ form.inline { display: inline; margin: 0; } main.page-login, main.page-setup { max-width: 24rem; } .card + .card { margin-top: 1.2rem; } .flash { background: var(--flash-bg); border: 1px solid var(--flash-border); color: var(--flash-fg); padding: 0.7rem 1rem; border-radius: 6px; margin-bottom: 1.2rem; } +/* RateLimitErr renders as .flash.error: a validation failure, not a success + notice. Without this rule it inherited the success surface (green) and only + .error's text colour, so a rejected rate limit read as good news in red + text. Same box, danger palette. */ +.flash.error { background: var(--danger-bg); border-color: var(--danger-border); color: var(--danger-fg); } table { width: 100%; border-collapse: collapse; } /* A table column is at least as wide as the longest unbreakable run inside it, and the panel's tables are full of runs with nothing to break on: email diff --git a/internal/web/view/templates/domain_detail.html b/internal/web/view/templates/domain_detail.html index bd072e6..df45f15 100644 --- a/internal/web/view/templates/domain_detail.html +++ b/internal/web/view/templates/domain_detail.html @@ -2,6 +2,61 @@ measure (same pattern as Status). */}} {{define "wide"}}wide{{end}} +{{/* The Host/name ‖ Type field-pair repeats for every DNS record this page + shows (DKIM, SPF, DMARC, report authorization) in both the status card + and the publishable-record cards below it — only the host and whether it + carries a Copy button change. Two variants rather than one templated + Copy flag: the DNS status card never offers Copy (its host is derived, + not something to paste), the record cards always do. */}} +{{define "host_type"}} +
+
+ + {{.}} +
+
+ + TXT +
+
+{{end}} + +{{define "host_type_copy"}} +
+
+ +
+ {{.}} + +
+
+
+ + TXT +
+
+{{end}} + +{{/* Value partials pair with the two host_type variants above: field_values + is the DNS status card's raw record dump (no Copy — those values are for + comparison, not for pasting), field_value is the single publishable + value on the record cards below (always has Copy). Callers still guard + the empty case, since "no records yet" and "one blank record" read + differently. */}} +{{define "field_values"}} + +{{range .}}{{.}} +{{end}} +{{end}} + +{{define "field_value"}} + +
+ {{.}} + +
+{{end}} + {{define "content"}}

{{.Domain.Name}}

@@ -37,21 +92,8 @@
-
-
- - {{.Record.Name}} -
-
- - TXT -
-
- {{if .DNS.DKIM.Records}} - - {{range .DNS.DKIM.Records}}{{.}} -{{end}} - {{end}} + {{template "host_type" .Record.Name}} + {{if .DNS.DKIM.Records}}{{template "field_values" .DNS.DKIM.Records}}{{end}} {{if ne .DNS.DKIM.Status "ok"}}

{{.DNS.DKIM.Detail}}

{{end}} @@ -59,21 +101,8 @@
-
-
- - {{.Domain.Name}} -
-
- - TXT -
-
- {{if .DNS.SPF.Records}} - - {{range .DNS.SPF.Records}}{{.}} -{{end}} - {{end}} + {{template "host_type" .Domain.Name}} + {{if .DNS.SPF.Records}}{{template "field_values" .DNS.SPF.Records}}{{end}} {{if ne .DNS.SPF.Status "ok"}}

{{.DNS.SPF.Detail}}

{{end}} @@ -83,21 +112,8 @@
-
-
- - {{.DMARCName}} -
-
- - TXT -
-
- {{if .DNS.DMARC.Records}} - - {{range .DNS.DMARC.Records}}{{.}} -{{end}} - {{end}} + {{template "host_type" .DMARCName}} + {{if .DNS.DMARC.Records}}{{template "field_values" .DNS.DMARC.Records}}{{end}} {{if ne .DNS.DMARC.Status "ok"}}

{{.DNS.DMARC.Detail}}

{{else}} @@ -108,21 +124,8 @@
{{if .DNS.DMARCReportAuth.Status}} -
-
- - {{.ReportAuthName}} -
-
- - TXT -
-
- {{if .DNS.DMARCReportAuth.Records}} - - {{range .DNS.DMARCReportAuth.Records}}{{.}} -{{end}} - {{end}} + {{template "host_type" .ReportAuthName}} + {{if .DNS.DMARCReportAuth.Records}}{{template "field_values" .DNS.DMARCReportAuth.Records}}{{end}}

{{.DNS.DMARCReportAuth.Detail}}

{{else}} @@ -141,49 +144,15 @@

DKIM and SPF records

DKIM

-
-
- -
- {{.Record.Name}} - -
-
-
- - TXT -
-
- - -
- {{.Record.Value}} - -
+ {{template "host_type_copy" .Record.Name}} + {{template "field_value" .Record.Value}}

Not a secret. Signed with selector {{.Domain.DKIMSelector}}.

SPF

-
-
- -
- {{.Domain.Name}} - -
-
-
- - TXT -
-
- - -
- {{.SPFExample}} - -
+ {{template "host_type_copy" .Domain.Name}} + {{template "field_value" .SPFExample}}

Merge into an existing SPF if the domain already has one — do not publish a second record.

@@ -192,19 +161,7 @@

DMARC record

-
-
- -
- {{.DMARCName}} - -
-
-
- - TXT -
-
+ {{template "host_type_copy" .DMARCName}}
@@ -220,25 +177,8 @@ {{if .NeedsReportAuth}}

Report authorization

-
-
- -
- {{.ReportAuthName}} - -
-
-
- - TXT -
-
- - -
- {{.ReportAuthValue}} - -
+ {{template "host_type_copy" .ReportAuthName}} + {{template "field_value" .ReportAuthValue}} {{end}}

p=none does not affect delivery. Tighten to diff --git a/internal/web/view/templates/settings.html b/internal/web/view/templates/settings.html index 27630ec..4d5a37f 100644 --- a/internal/web/view/templates/settings.html +++ b/internal/web/view/templates/settings.html @@ -2,6 +2,25 @@ in panel.css, as on a delivery's page). */}} {{define "wide"}}wide{{end}} +{{/* The username/password fields are identical for a global administrator + (split card, DMARC alongside) and a domain administrator (narrow card, + no DMARC card) — only the surrounding form and card differ. */}} +{{define "credentials_fields"}} + + + + + + + + + + + +{{end}} + {{define "content"}}

Settings

@@ -16,19 +35,7 @@

These are the credentials for this control panel only. Applications keep their own logins and passwords, which are not affected.

- - - - - - - - - - - + {{template "credentials_fields" .}}
@@ -81,19 +88,7 @@ this one stays signed in.

These are the credentials for this control panel only. Applications keep their own logins and passwords, which are not affected.

- - - - - - - - - - - + {{template "credentials_fields" .}}
diff --git a/internal/web/view/templates/user_delete.html b/internal/web/view/templates/user_delete.html new file mode 100644 index 0000000..87d007a --- /dev/null +++ b/internal/web/view/templates/user_delete.html @@ -0,0 +1,16 @@ +{{define "content"}} +

Delete {{.TargetUsername}}

+ +{{template "back_link" (back (printf "/users/%d" .TargetID) (printf "Back to %s" .TargetUsername))}} + +
+

Confirm deletion

+

You are about to delete the panel user {{.TargetUsername}}. + This cannot be undone; a signed-in session for this user stops working + immediately.

+ +
+ +
+
+{{end}} diff --git a/internal/web/view/templates/user_form.html b/internal/web/view/templates/user_form.html index 2491d5d..b1df42d 100644 --- a/internal/web/view/templates/user_form.html +++ b/internal/web/view/templates/user_form.html @@ -33,10 +33,13 @@ - {{if .IsEdit}} - - {{if .LastGlobalLocked}}

The only global administrator cannot be deleted.

{{end}} - {{end}} + {{if .IsEdit}} + {{if .LastGlobalLocked}} +

The only global administrator cannot be deleted.

+ {{else}} + Delete user + {{end}} + {{end}}
{{end}} diff --git a/internal/web/view/templates_test.go b/internal/web/view/templates_test.go index 781c509..e0366be 100644 --- a/internal/web/view/templates_test.go +++ b/internal/web/view/templates_test.go @@ -282,6 +282,7 @@ func TestSettingsPageDocumentsRateLimits(t *testing.T) { func TestDrillDownPagesPlaceBackLinkAboveContent(t *testing.T) { drillDown := map[string]bool{ "user_form.html": true, + "user_delete.html": true, "domain_detail.html": true, "domain_delete.html": true, "delivery.html": true, diff --git a/internal/web/view/view.go b/internal/web/view/view.go index 6cbd788..6b24b1c 100644 --- a/internal/web/view/view.go +++ b/internal/web/view/view.go @@ -37,6 +37,7 @@ var pageFiles = map[string][]string{ "settings": {"templates/settings.html"}, "users": {"templates/users.html"}, "user_form": {"templates/user_form.html"}, + "user_delete": {"templates/user_delete.html"}, "backup": {"templates/backup.html", "templates/encrypt_fields.html"}, "domain_detail": {"templates/domain_detail.html", "templates/encrypt_fields.html"}, "domain_delete": {"templates/domain_delete.html"}, diff --git a/internal/web/web.go b/internal/web/web.go index 4e23006..59aab12 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -166,6 +166,8 @@ func (s *Server) Handler() http.Handler { authed.HandleFunc("POST /users/new", h.HandleUserNew) authed.HandleFunc("GET /users/{uid}", h.HandleUserEdit) authed.HandleFunc("POST /users/{uid}", h.HandleUserEdit) + authed.HandleFunc("GET /users/{uid}/delete", h.HandleUserDeleteConfirm) + authed.HandleFunc("POST /users/{uid}/delete", h.HandleUserDelete) authed.HandleFunc("GET /backup", h.HandleBackupPage) authed.HandleFunc("POST /backup", h.HandleBackup)