diff --git a/CHANGELOG.md b/CHANGELOG.md
index bb96c0b..f60f14f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -144,6 +144,23 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version
- panel-redesign is `agreed` (owner, 2026-10-09); work starts with stage 0,
the component kit.
+### Security
+
+- A panel session belongs to a user by id and ends with them. It used to be
+ keyed by user name: the cookie of a deleted or renamed user stayed valid
+ and became a session of whoever was next created under that name. Signing
+ in again is required after upgrading.
+- Changing one's password ends one's own other sessions only; it used to
+ sign every other user out. A password set by an administrator on the user
+ form ends that user's sessions.
+- Internal errors (database, `saslpasswd2`, Postfix reload) are no longer
+ shown on the application, inbound upstream / recipients and limit forms;
+ they are logged and the page says so.
+- `/static/` no longer answers with a listing of the embedded files.
+- The DMARC reports hub is open to a domain administrator for the domains
+ assigned to them, without the server's ingest statistics; the list of
+ reports no longer treats "no domains" as "all domains".
+
### Fixed
- Saving a domain's report-address form untouched no longer moves a domain
diff --git a/docs/plans/panel-redesign.md b/docs/plans/panel-redesign.md
index 6098cbd..9ebe3f1 100644
--- a/docs/plans/panel-redesign.md
+++ b/docs/plans/panel-redesign.md
@@ -525,7 +525,7 @@ only when both are done. Every stage-2 step ends with the evidence of
**Stage 3 — reviews, docs, 2.0.0**
- [ ] Technical review of the whole against the mockups, evidence pairs complete — **Opus**
-- [ ] Security review of the route rename, subtree authorization and inbound delegation — **Fable**
+- [x] Security review of the route rename, subtree authorization and inbound delegation — **Fable**
- [ ] guide.md screenshots and wording for the new navigation; architecture.md § Panel HTTP surface and § Persistence; security.md roles; *Removed* in the CHANGELOG with one line that 2.0 starts from an empty data directory; strike the three UI rows of review-2026-08-followups that the redesign retires — **Sonnet**
- [ ] `go vet`, `go test ./...`, e2e suite; version cut `2.0.0` — **Haiku**
diff --git a/docs/roadmap.md b/docs/roadmap.md
index d646782..4171060 100644
--- a/docs/roadmap.md
+++ b/docs/roadmap.md
@@ -39,7 +39,7 @@ in `git log` and [CHANGELOG.md](../CHANGELOG.md).
| panel-notifications | E-mail notifications about important events | candidate | — | — |
| delivery-log-storage | Keep a message's `mail.log` lines with its send-log row | candidate | — | — |
| password-reset | Password reset by e-mail | candidate | — | — |
-| panel-redesign | Panel redesign (accepted mockups, design contract) | agreed | 23/27 | [plans/panel-redesign.md](plans/panel-redesign.md) |
+| panel-redesign | Panel redesign (accepted mockups, design contract) | agreed | 24/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) |
**Recommended order** (not binding; owner, 2026-10-09: feature order follows
diff --git a/docs/schema-migrations.md b/docs/schema-migrations.md
index 5abf614..c859f34 100644
--- a/docs/schema-migrations.md
+++ b/docs/schema-migrations.md
@@ -84,7 +84,7 @@ for operators upgrading from older 2.x images.
|-------|------|
| `settings` | Key/value settings of the instance — what is true whoever is signed in (send-log retention). Nothing that belongs to a user |
| `users` | Panel logins. `role` is `global` or `domain`; `email` is the user's own address; `dmarc_default_mode` / `dmarc_default_address` are their default DMARC report address; `all_domains` / `all_inbound_domains` widen a `domain` user to every domain of that direction |
-| `sessions` | Panel login sessions (token hash, sliding idle expiry) |
+| `sessions` | Panel login sessions (token hash, owning user by id and deleted with them, sliding idle expiry) |
| `domains` | Sending (outbound) domains. `dmarc_rua` / `dmarc_rua_user_id` say where aggregate reports go |
| `user_domains` | Outbound domains assigned to a `domain` user |
| `applications` | SASL applications per domain, with the client-IP restriction (`auth_ip_restrict`, `auth_allowed_ips`) |
diff --git a/internal/app/validate.go b/internal/app/validate.go
index 31a42c2..8c72090 100644
--- a/internal/app/validate.go
+++ b/internal/app/validate.go
@@ -12,6 +12,18 @@ const (
maxLoginLen = 64
)
+// ValidationError is a refusal of what was submitted: its text says what the
+// administrator must fix and is written to be shown on the page. Any other
+// error from this package (a failed write, a failed reload) is not, and the
+// caller must not put its text in front of a user.
+type ValidationError struct{ msg string }
+
+func (e *ValidationError) Error() string { return e.msg }
+
+func invalid(format string, args ...any) error {
+ return &ValidationError{msg: fmt.Sprintf(format, args...)}
+}
+
// validateLogin enforces a strict server-side whitelist for the SASL login
// (security.md). It intentionally excludes '@': the login is stored in sasldb2,
// where '@' separates the user from the realm, so allowing it would change the
@@ -22,14 +34,14 @@ const (
// makes that safe.
func validateLogin(login string) error {
if len(login) < minLoginLen || len(login) > maxLoginLen {
- return fmt.Errorf("login must be %d-%d characters", minLoginLen, maxLoginLen)
+ return invalid("login must be %d-%d characters", minLoginLen, maxLoginLen)
}
for _, r := range login {
lower := r >= 'a' && r <= 'z'
upper := r >= 'A' && r <= 'Z'
digit := r >= '0' && r <= '9'
if !lower && !upper && !digit && r != '.' && r != '-' && r != '_' {
- return fmt.Errorf("login may contain only letters, digits, '.', '-' and '_'")
+ return invalid("login may contain only letters, digits, '.', '-' and '_'")
}
}
return nil
@@ -43,14 +55,14 @@ func validateLogin(login string) error {
// (security.md).
func validateImportedPassword(password string) error {
if password == "" {
- return fmt.Errorf("imported application password is empty")
+ return invalid("imported application password is empty")
}
if len(password) > 1024 {
- return fmt.Errorf("imported application password is too long")
+ return invalid("imported application password is too long")
}
for _, r := range password {
if r < 0x20 || r == 0x7f {
- return fmt.Errorf("imported application password contains control characters")
+ return invalid("imported application password contains control characters")
}
}
return nil
@@ -59,7 +71,7 @@ func validateImportedPassword(password string) error {
// validateAddressMode checks the submitted mode is one of the two known values.
func validateAddressMode(mode string) error {
if mode != store.AddressModeWildcard && mode != store.AddressModeList {
- return fmt.Errorf("invalid address mode")
+ return invalid("invalid address mode")
}
return nil
}
@@ -79,11 +91,11 @@ func normalizeAddress(addr string) string {
func validateSenderAddress(addr, domain string) error {
at := strings.LastIndexByte(addr, '@')
if at < 0 {
- return fmt.Errorf("%q is not a valid email address", addr)
+ return invalid("%q is not a valid email address", addr)
}
local, host := addr[:at], addr[at+1:]
if host != domain {
- return fmt.Errorf("%q does not belong to domain %s", addr, domain)
+ return invalid("%q does not belong to domain %s", addr, domain)
}
if err := validateLocalPart(local); err != nil {
return fmt.Errorf("%q: %w", addr, err)
@@ -96,17 +108,17 @@ func validateSenderAddress(addr, domain string) error {
// value is always safe to write verbatim into the Postfix map (security.md).
func validateLocalPart(local string) error {
if local == "" {
- return fmt.Errorf("missing the part before '@'")
+ return invalid("missing the part before '@'")
}
if local[0] == '.' || local[len(local)-1] == '.' {
- return fmt.Errorf("local part must not start or end with '.'")
+ return invalid("local part must not start or end with '.'")
}
for i := 0; i < len(local); i++ {
c := local[i]
lower := c >= 'a' && c <= 'z'
digit := c >= '0' && c <= '9'
if !lower && !digit && c != '.' && c != '-' && c != '_' && c != '+' {
- return fmt.Errorf("local part may contain only lower-case letters, digits, '.', '-', '_' and '+'")
+ return invalid("local part may contain only lower-case letters, digits, '.', '-', '_' and '+'")
}
}
return nil
@@ -134,7 +146,7 @@ func parseAddresses(raw []string, domain string) ([]string, error) {
out = append(out, addr)
}
if len(out) == 0 {
- return nil, fmt.Errorf("list mode requires at least one address")
+ return nil, invalid("list mode requires at least one address")
}
return out, nil
}
diff --git a/internal/dmarc/parse_test.go b/internal/dmarc/parse_test.go
index 0d46993..283583c 100644
--- a/internal/dmarc/parse_test.go
+++ b/internal/dmarc/parse_test.go
@@ -99,7 +99,7 @@ func TestIngestMessageStoresReport(t *testing.T) {
if err := IngestMessage(st, msg, "dmarc-reports@mail.example.com", time.Now().UTC()); err != nil {
t.Fatalf("IngestMessage: %v", err)
}
- list, err := st.ListDMARCReports(nil, 10)
+ list, err := st.ListDMARCReports(store.DMARCReportScope{AllDomains: true}, 10)
if err != nil {
t.Fatal(err)
}
@@ -129,7 +129,7 @@ func TestIngestMessageRejectsUnknownDomain(t *testing.T) {
if err := IngestMessage(st, bytes.NewBufferString(gzipMessage(t, sampleXML)), "dmarc-reports@mail.example.com", time.Now().UTC()); err == nil {
t.Fatal("report for an unconfigured domain was accepted")
}
- list, err := st.ListDMARCReports(nil, 10)
+ list, err := st.ListDMARCReports(store.DMARCReportScope{AllDomains: true}, 10)
if err != nil {
t.Fatal(err)
}
diff --git a/internal/inbound/service.go b/internal/inbound/service.go
index bafc678..a686760 100644
--- a/internal/inbound/service.go
+++ b/internal/inbound/service.go
@@ -5,7 +5,6 @@
package inbound
import (
- "fmt"
"strings"
"github.com/mixeme/selfpost/internal/postfix"
@@ -150,7 +149,7 @@ func parseRecipientAddresses(raw []string, domain string) ([]string, error) {
out = append(out, addr)
}
if len(out) == 0 {
- return nil, fmt.Errorf("listed-recipients mode requires at least one address")
+ return nil, invalid("listed-recipients mode requires at least one address")
}
return out, nil
}
diff --git a/internal/inbound/validate.go b/internal/inbound/validate.go
index eb4139a..9b96036 100644
--- a/internal/inbound/validate.go
+++ b/internal/inbound/validate.go
@@ -7,6 +7,18 @@ import (
"strings"
)
+// ValidationError is a refusal of what was submitted: its text says what the
+// administrator must fix and is written to be shown on the page. Any other
+// error from this package (a failed write, a failed reload) is not, and the
+// caller must not put its text in front of a user.
+type ValidationError struct{ msg string }
+
+func (e *ValidationError) Error() string { return e.msg }
+
+func invalid(format string, args ...any) error {
+ return &ValidationError{msg: fmt.Sprintf(format, args...)}
+}
+
func normalizeDomain(name string) string {
return strings.ToLower(strings.TrimSpace(name))
}
@@ -21,14 +33,14 @@ func normalizeHost(host string) string {
func checkDomain(name string) error {
if name == "" {
- return fmt.Errorf("domain is required")
+ return invalid("domain is required")
}
if len(name) > 253 {
- return fmt.Errorf("domain must be at most 253 characters")
+ return invalid("domain must be at most 253 characters")
}
labels := strings.Split(name, ".")
if len(labels) < 2 {
- return fmt.Errorf("domain must include at least one dot (e.g. example.com)")
+ return invalid("domain must include at least one dot (e.g. example.com)")
}
for _, label := range labels {
if err := checkLabel(label); err != nil {
@@ -40,10 +52,10 @@ func checkDomain(name string) error {
func checkHost(host string) error {
if host == "" {
- return fmt.Errorf("host is required")
+ return invalid("host is required")
}
if len(host) > 253 {
- return fmt.Errorf("host must be at most 253 characters")
+ return invalid("host must be at most 253 characters")
}
if ip := net.ParseIP(host); ip != nil {
return nil
@@ -58,20 +70,20 @@ func checkHost(host string) error {
func checkLabel(label string) error {
if len(label) == 0 {
- return fmt.Errorf("must not contain an empty label")
+ return invalid("must not contain an empty label")
}
if len(label) > 63 {
- return fmt.Errorf("each label must be at most 63 characters")
+ return invalid("each label must be at most 63 characters")
}
if label[0] == '-' || label[len(label)-1] == '-' {
- return fmt.Errorf("labels must not start or end with '-'")
+ return invalid("labels must not start or end with '-'")
}
for i := 0; i < len(label); i++ {
c := label[i]
lower := c >= 'a' && c <= 'z'
digit := c >= '0' && c <= '9'
if !lower && !digit && c != '-' {
- return fmt.Errorf("may contain only lower-case letters, digits, '.' and '-'")
+ return invalid("may contain only lower-case letters, digits, '.' and '-'")
}
}
return nil
@@ -80,11 +92,11 @@ func checkLabel(label string) error {
func parsePort(raw string) (int, error) {
raw = strings.TrimSpace(raw)
if raw == "" {
- return 0, fmt.Errorf("port is required")
+ return 0, invalid("port is required")
}
n, err := strconv.Atoi(raw)
if err != nil || n < 1 || n > 65535 {
- return 0, fmt.Errorf("port must be between 1 and 65535")
+ return 0, invalid("port must be between 1 and 65535")
}
return n, nil
}
@@ -94,7 +106,7 @@ func checkTLSMode(mode string) error {
case "may", "encrypt", "none":
return nil
default:
- return fmt.Errorf("invalid TLS mode")
+ return invalid("invalid TLS mode")
}
}
@@ -103,28 +115,28 @@ func checkRecipientMode(mode string) error {
case "list", "any":
return nil
default:
- return fmt.Errorf("invalid recipient mode")
+ return invalid("invalid recipient mode")
}
}
func checkMailbox(addr, domain string) error {
at := strings.LastIndexByte(addr, '@')
if at <= 0 || at >= len(addr)-1 {
- return fmt.Errorf("%q is not a valid email address", addr)
+ return invalid("%q is not a valid email address", addr)
}
local, host := addr[:at], addr[at+1:]
if host != domain {
- return fmt.Errorf("%q does not belong to domain %s", addr, domain)
+ return invalid("%q does not belong to domain %s", addr, domain)
}
if local == "" || local[0] == '.' || local[len(local)-1] == '.' {
- return fmt.Errorf("%q: invalid local part", addr)
+ return invalid("%q: invalid local part", addr)
}
for i := 0; i < len(local); i++ {
c := local[i]
lower := c >= 'a' && c <= 'z'
digit := c >= '0' && c <= '9'
if !lower && !digit && c != '.' && c != '-' && c != '_' && c != '+' {
- return fmt.Errorf("%q: local part contains invalid characters", addr)
+ return invalid("%q: local part contains invalid characters", addr)
}
}
return nil
diff --git a/internal/store/dmarc.go b/internal/store/dmarc.go
index 310f640..c86fdc4 100644
--- a/internal/store/dmarc.go
+++ b/internal/store/dmarc.go
@@ -4,6 +4,7 @@ import (
"database/sql"
"errors"
"fmt"
+ "strings"
"time"
)
@@ -167,34 +168,37 @@ func (s *Store) IncrDMARCParseFailures() error {
return s.SetSetting(DMARCParseFailuresKey, fmt.Sprintf("%d", n+1))
}
-// ListDMARCReports returns recent summaries, optionally limited to domains.
-func (s *Store) ListDMARCReports(domains []string, limit int) ([]DMARCReportSummary, error) {
+// DMARCReportScope is whose reports a listing may return: every domain's
+// (AllDomains) or those of the named domains. A scope with neither is a viewer
+// who reaches nothing and gets no rows — an empty list is never "all"; see
+// SendLogFilter, which has the same shape.
+type DMARCReportScope struct {
+ AllDomains bool
+ Domains []string
+}
+
+// ListDMARCReports returns recent summaries within the scope.
+func (s *Store) ListDMARCReports(scope DMARCReportScope, limit int) ([]DMARCReportSummary, error) {
if limit <= 0 {
limit = 50
}
- var (
- rows *sql.Rows
- err error
- )
- if len(domains) == 0 {
- rows, err = s.db.Query(`
- SELECT id, domain, reporter, period_begin, period_end, received_at, pass_count, fail_count
- FROM dmarc_reports ORDER BY received_at DESC LIMIT ?`, limit)
- } else {
- placeholders := make([]any, 0, len(domains)+1)
- q := `SELECT id, domain, reporter, period_begin, period_end, received_at, pass_count, fail_count
- FROM dmarc_reports WHERE domain IN (`
- for i, d := range domains {
- if i > 0 {
- q += ","
- }
- q += "?"
- placeholders = append(placeholders, d)
+ q := `SELECT id, domain, reporter, period_begin, period_end, received_at, pass_count, fail_count
+ FROM dmarc_reports`
+ var args []any
+ if !scope.AllDomains {
+ if len(scope.Domains) == 0 {
+ return nil, nil
}
- q += `) ORDER BY received_at DESC LIMIT ?`
- placeholders = append(placeholders, limit)
- rows, err = s.db.Query(q, placeholders...)
+ marks := make([]string, len(scope.Domains))
+ for i, d := range scope.Domains {
+ marks[i] = "?"
+ args = append(args, d)
+ }
+ q += " WHERE domain IN (" + strings.Join(marks, ",") + ")"
}
+ q += " ORDER BY received_at DESC LIMIT ?"
+ args = append(args, limit)
+ rows, err := s.db.Query(q, args...)
if err != nil {
return nil, fmt.Errorf("list dmarc reports: %w", err)
}
@@ -242,7 +246,7 @@ func (s *Store) GetDMARCReport(id int64) (DMARCReport, error) {
// ListDMARCReportsForDomain returns summaries for one sending domain.
func (s *Store) ListDMARCReportsForDomain(domain string, limit int) ([]DMARCReportSummary, error) {
- return s.ListDMARCReports([]string{domain}, limit)
+ return s.ListDMARCReports(DMARCReportScope{Domains: []string{domain}}, limit)
}
// DMARCDomainRollup summarises pass/fail for a domain over the last windowDays.
diff --git a/internal/store/dmarc_test.go b/internal/store/dmarc_test.go
index b69a9cd..c33ef3e 100644
--- a/internal/store/dmarc_test.go
+++ b/internal/store/dmarc_test.go
@@ -29,7 +29,7 @@ func TestDMARCReportRoundTrip(t *testing.T) {
if err != nil {
t.Fatal(err)
}
- list, err := st.ListDMARCReports([]string{"example.com"}, 10)
+ list, err := st.ListDMARCReports(DMARCReportScope{Domains: []string{"example.com"}}, 10)
if err != nil || len(list) != 1 {
t.Fatalf("list: %v err=%v", list, err)
}
@@ -61,8 +61,48 @@ func TestPruneDMARCReports(t *testing.T) {
if err := st.PruneDMARCReports(); err != nil {
t.Fatal(err)
}
- list, _ := st.ListDMARCReports(nil, 100)
+ list, _ := st.ListDMARCReports(DMARCReportScope{AllDomains: true}, 100)
if len(list) != 0 {
t.Fatalf("expected prune by age, got %d", len(list))
}
}
+
+// The domains a list is limited to are the whole answer: an empty list is a
+// viewer who reaches nothing, never a viewer who reaches everything.
+func TestListDMARCReportsEmptyListIsNoRows(t *testing.T) {
+ st := openTestStore(t)
+ now := time.Now().UTC()
+ for _, domain := range []string{"a.example", "b.example"} {
+ if _, err := st.InsertDMARCReport(DMARCReport{
+ Domain: domain, Reporter: "r", ReportID: domain,
+ PeriodBegin: now.Add(-24 * time.Hour), PeriodEnd: now, ReceivedAt: now,
+ }); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ for name, scope := range map[string]DMARCReportScope{
+ "an empty list": {Domains: []string{}},
+ "no scope at all": {},
+ } {
+ list, err := st.ListDMARCReports(scope, 10)
+ if err != nil {
+ t.Fatalf("%s is an error: %v", name, err)
+ }
+ if len(list) != 0 {
+ t.Errorf("%s returned %d report(s), want none", name, len(list))
+ }
+ }
+ list, err := st.ListDMARCReports(DMARCReportScope{Domains: []string{"a.example"}}, 10)
+ if err != nil || len(list) != 1 || list[0].Domain != "a.example" {
+ t.Errorf("one domain = %+v, %v; want only a.example", list, err)
+ }
+ list, err = st.ListDMARCReports(DMARCReportScope{AllDomains: true}, 10)
+ if err != nil || len(list) != 2 {
+ t.Errorf("all domains = %d report(s), %v; want both", len(list), err)
+ }
+ list, err = st.ListDMARCReports(DMARCReportScope{AllDomains: true, Domains: []string{"a.example"}}, 10)
+ if err != nil || len(list) != 2 {
+ t.Errorf("all domains with a list = %d report(s), %v; want both (AllDomains wins)", len(list), err)
+ }
+}
diff --git a/internal/store/migrations/0001_init.sql b/internal/store/migrations/0001_init.sql
index fdbc1b6..e6c5785 100644
--- a/internal/store/migrations/0001_init.sql
+++ b/internal/store/migrations/0001_init.sql
@@ -51,10 +51,12 @@ CREATE TABLE users (
-- redeploy; only the SHA-256 of the session token is stored, never the token
-- itself, so a stolen database file cannot be used to sign in. expires_at
-- implements the sliding idle timeout: it is pushed forward on activity rather
--- than being fixed at creation time.
+-- than being fixed at creation time. A session belongs to a user by id, not by
+-- name: deleting the user ends their sessions, and renaming them leaves the
+-- sessions where they are.
CREATE TABLE sessions (
token_hash TEXT PRIMARY KEY,
- username TEXT NOT NULL,
+ user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
created_at TEXT NOT NULL,
expires_at TEXT NOT NULL
);
diff --git a/internal/store/ratelimit_auto.go b/internal/store/ratelimit_auto.go
index 6066a0a..4235e8b 100644
--- a/internal/store/ratelimit_auto.go
+++ b/internal/store/ratelimit_auto.go
@@ -2,6 +2,7 @@ package store
import (
"database/sql"
+ "errors"
"fmt"
"log"
"math"
@@ -27,15 +28,20 @@ func (s *Store) RecalcAllAutoRateLimits(retentionDays, l1Max, l1Window int) (int
return updated, nil
}
-// RecalcAutoRateLimit recomputes one auto-mode limit. Returns an error when
-// the row is missing or not in auto mode.
+// ErrNotAutoRateLimit is returned by RecalcAutoRateLimit for a limit that is
+// missing or not in auto mode: there is nothing to recalculate. It is the one
+// refusal of that call that is the administrator's to read.
+var ErrNotAutoRateLimit = errors.New("rate limit not in auto mode")
+
+// RecalcAutoRateLimit recomputes one auto-mode limit. Returns
+// ErrNotAutoRateLimit when the row is missing or not in auto mode.
func (s *Store) RecalcAutoRateLimit(scope string, refID int64, retentionDays, l1Max, l1Window int) error {
rl, ok, err := s.GetRateLimit(scope, refID)
if err != nil {
return err
}
if !ok || !rl.IsAuto() {
- return fmt.Errorf("rate limit not in auto mode")
+ return ErrNotAutoRateLimit
}
rl.Scope, rl.RefID = scope, refID
return s.recalcAutoRateLimit(rl, retentionDays, l1Max, l1Window)
diff --git a/internal/store/schema_test.go b/internal/store/schema_test.go
index d3f8673..1d07052 100644
--- a/internal/store/schema_test.go
+++ b/internal/store/schema_test.go
@@ -41,7 +41,7 @@ func TestBaselineSchema(t *testing.T) {
want := map[string]string{
"settings": "key value",
"users": "id username password_hash role email dmarc_default_mode dmarc_default_address all_domains all_inbound_domains created_at",
- "sessions": "token_hash username created_at expires_at",
+ "sessions": "token_hash user_id created_at expires_at",
"domains": "id name dkim_selector dmarc_rua dmarc_rua_user_id created_at",
"user_domains": "user_id domain_id",
"applications": "id domain_id login address_mode auth_ip_restrict auth_allowed_ips created_at",
diff --git a/internal/store/sessions.go b/internal/store/sessions.go
index e254994..6032b57 100644
--- a/internal/store/sessions.go
+++ b/internal/store/sessions.go
@@ -8,18 +8,19 @@ import (
)
// SessionRow is a persisted login session, keyed by the SHA-256 of its token
-// (see internal/web, which owns the token itself).
+// (see internal/web, which owns the token itself). It belongs to a user by id:
+// deleting the user deletes their sessions, and renaming them keeps them.
type SessionRow struct {
- Username string
+ UserID int64
ExpiresAt time.Time
}
-// CreateSession inserts a new session row.
-func (s *Store) CreateSession(tokenHash, username string, expiresAt time.Time) error {
+// CreateSession inserts a new session row for a user.
+func (s *Store) CreateSession(tokenHash string, userID int64, expiresAt time.Time) error {
now := time.Now().UTC().Format(time.RFC3339)
_, err := s.db.Exec(
- `INSERT INTO sessions (token_hash, username, created_at, expires_at) VALUES (?, ?, ?, ?)`,
- tokenHash, username, now, expiresAt.UTC().Format(time.RFC3339),
+ `INSERT INTO sessions (token_hash, user_id, created_at, expires_at) VALUES (?, ?, ?, ?)`,
+ tokenHash, userID, now, expiresAt.UTC().Format(time.RFC3339),
)
if err != nil {
return fmt.Errorf("insert session: %w", err)
@@ -36,9 +37,9 @@ func (s *Store) LookupSession(tokenHash string) (SessionRow, bool, error) {
expiresAt string
)
err := s.db.QueryRow(
- `SELECT username, expires_at FROM sessions WHERE token_hash = ?`,
+ `SELECT user_id, expires_at FROM sessions WHERE token_hash = ?`,
tokenHash,
- ).Scan(&row.Username, &expiresAt)
+ ).Scan(&row.UserID, &expiresAt)
if errors.Is(err, sql.ErrNoRows) {
return SessionRow{}, false, nil
}
@@ -65,20 +66,6 @@ func (s *Store) RenewSession(tokenHash string, expiresAt time.Time) error {
return nil
}
-// RenameSession updates the username carried by a session, keeping its
-// expiry, so a session stays usable after the administrator renames their own
-// account.
-func (s *Store) RenameSession(tokenHash, username string) error {
- _, err := s.db.Exec(
- `UPDATE sessions SET username = ? WHERE token_hash = ?`,
- username, tokenHash,
- )
- if err != nil {
- return fmt.Errorf("rename session: %w", err)
- }
- return nil
-}
-
// DeleteSession removes a session row (logout, or a lookup finding it
// expired).
func (s *Store) DeleteSession(tokenHash string) error {
@@ -88,17 +75,26 @@ func (s *Store) DeleteSession(tokenHash string) error {
return nil
}
-// DeleteOtherSessions removes every session except keepHash. It is called
-// when the administrator changes their password: a stolen cookie issued
-// under the old password must stop working, while the session performing the
-// change stays signed in.
-func (s *Store) DeleteOtherSessions(keepHash string) error {
- if _, err := s.db.Exec(`DELETE FROM sessions WHERE token_hash != ?`, keepHash); err != nil {
+// DeleteOtherSessions removes every session of the user except keepHash. It is
+// called when the user changes their password: a stolen cookie issued under the
+// old password must stop working, while the session performing the change
+// stays signed in. Other users' sessions are not touched.
+func (s *Store) DeleteOtherSessions(userID int64, keepHash string) error {
+ if _, err := s.db.Exec(`DELETE FROM sessions WHERE user_id = ? AND token_hash != ?`, userID, keepHash); err != nil {
return fmt.Errorf("delete other sessions: %w", err)
}
return nil
}
+// DeleteUserSessions removes every session of the user, for when somebody else
+// has set their password.
+func (s *Store) DeleteUserSessions(userID int64) error {
+ if _, err := s.db.Exec(`DELETE FROM sessions WHERE user_id = ?`, userID); err != nil {
+ return fmt.Errorf("delete user sessions: %w", err)
+ }
+ return nil
+}
+
// DeleteExpiredSessions prunes rows whose expiry has already passed, so an
// abandoned session (cookie never presented again) does not sit in the table
// forever. It piggybacks on session creation rather than running as its own
diff --git a/internal/store/sessions_test.go b/internal/store/sessions_test.go
new file mode 100644
index 0000000..1b7c1bb
--- /dev/null
+++ b/internal/store/sessions_test.go
@@ -0,0 +1,127 @@
+package store
+
+import (
+ "testing"
+ "time"
+)
+
+func sessionExpiry() time.Time { return time.Now().Add(time.Hour) }
+
+// A session belongs to a person, not to a name: once the user is deleted the
+// cookie must be dead for good, including when somebody else is later created
+// under the same username.
+func TestSessionDoesNotOutliveItsUserOrPassToANewOneWithTheSameName(t *testing.T) {
+ st := openTestStore(t)
+ if _, err := st.CreateUser("admin", "h", RoleGlobal, Reach{}); err != nil {
+ t.Fatal(err)
+ }
+ a, err := st.CreateUser("alice", "h", RoleGlobal, Reach{})
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := st.CreateSession("old-token", a, sessionExpiry()); err != nil {
+ t.Fatalf("CreateSession: %v", err)
+ }
+
+ if err := st.DeleteUser(a); err != nil {
+ t.Fatalf("DeleteUser: %v", err)
+ }
+ if _, err := st.CreateUser("alice", "h", RoleGlobal, Reach{}); err != nil {
+ t.Fatal(err)
+ }
+
+ if _, found, err := st.LookupSession("old-token"); err != nil || found {
+ t.Fatalf("LookupSession after the user was deleted and the name reused = found %t, err %v; want no session", found, err)
+ }
+}
+
+// Renaming a user must not detach the sessions they already have.
+func TestSessionsFollowTheUserThroughARename(t *testing.T) {
+ st := openTestStore(t)
+ id, err := st.CreateUser("alice", "h", RoleGlobal, Reach{})
+ if err != nil {
+ t.Fatal(err)
+ }
+ if err := st.CreateSession("one", id, sessionExpiry()); err != nil {
+ t.Fatal(err)
+ }
+ if err := st.CreateSession("two", id, sessionExpiry()); err != nil {
+ t.Fatal(err)
+ }
+
+ if err := st.UpdateUser(id, "alicia", "h", ""); err != nil {
+ t.Fatalf("UpdateUser: %v", err)
+ }
+
+ for _, token := range []string{"one", "two"} {
+ row, found, err := st.LookupSession(token)
+ if err != nil || !found {
+ t.Fatalf("session %q after the rename = found %t, err %v; want it kept", token, found, err)
+ }
+ if row.UserID != id {
+ t.Errorf("session %q belongs to user %d after the rename, want %d", token, row.UserID, id)
+ }
+ if u, err := st.GetUser(row.UserID); err != nil || u.Username != "alicia" {
+ t.Errorf("session %q resolves to %q (%v) after the rename, want alicia", token, u.Username, err)
+ }
+ }
+}
+
+// A password change signs out the other sessions of the person who changed it,
+// not of anyone else.
+func TestDeleteOtherSessionsTouchesOnlyThatUsersSessions(t *testing.T) {
+ st := openTestStore(t)
+ ids := map[string]int64{}
+ for _, name := range []string{"alice", "bob"} {
+ id, err := st.CreateUser(name, "h", RoleGlobal, Reach{})
+ if err != nil {
+ t.Fatal(err)
+ }
+ ids[name] = id
+ }
+ for token, name := range map[string]string{"alice-1": "alice", "bob-1": "bob", "bob-2": "bob"} {
+ if err := st.CreateSession(token, ids[name], sessionExpiry()); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ if err := st.DeleteOtherSessions(ids["bob"], "bob-1"); err != nil {
+ t.Fatalf("DeleteOtherSessions: %v", err)
+ }
+
+ for token, want := range map[string]bool{"alice-1": true, "bob-1": true, "bob-2": false} {
+ if _, found, _ := st.LookupSession(token); found != want {
+ t.Errorf("session %q present = %t, want %t", token, found, want)
+ }
+ }
+}
+
+// Setting somebody's password from the user form ends all of their sessions.
+func TestDeleteUserSessionsTouchesOnlyThatUser(t *testing.T) {
+ st := openTestStore(t)
+ alice, _ := st.CreateUser("alice", "h", RoleGlobal, Reach{})
+ bob, _ := st.CreateUser("bob", "h", RoleGlobal, Reach{})
+ for token, id := range map[string]int64{"alice-1": alice, "bob-1": bob, "bob-2": bob} {
+ if err := st.CreateSession(token, id, sessionExpiry()); err != nil {
+ t.Fatal(err)
+ }
+ }
+
+ if err := st.DeleteUserSessions(bob); err != nil {
+ t.Fatalf("DeleteUserSessions: %v", err)
+ }
+
+ for token, want := range map[string]bool{"alice-1": true, "bob-1": false, "bob-2": false} {
+ if _, found, _ := st.LookupSession(token); found != want {
+ t.Errorf("session %q present = %t, want %t", token, found, want)
+ }
+ }
+}
+
+// A session cannot be written for a user that does not exist.
+func TestCreateSessionRefusesAMissingUser(t *testing.T) {
+ st := openTestStore(t)
+ if err := st.CreateSession("orphan", 42, sessionExpiry()); err == nil {
+ t.Fatal("CreateSession for a missing user succeeded")
+ }
+}
diff --git a/internal/web/auth/auth.go b/internal/web/auth/auth.go
index bf4b5a0..7ff51ba 100644
--- a/internal/web/auth/auth.go
+++ b/internal/web/auth/auth.go
@@ -75,14 +75,14 @@ func (m *Module) SessionToken(r *http.Request) (string, bool) {
return m.sessionToken(r)
}
-// RenameSession updates the username carried by a session.
-func (m *Module) RenameSession(token, username string) {
- m.sessions.Rename(token, username)
+// DestroyOtherSessions invalidates every session of the user except keep.
+func (m *Module) DestroyOtherSessions(userID int64, keep string) {
+ m.sessions.DestroyOthers(userID, keep)
}
-// DestroyOtherSessions invalidates every session except keep.
-func (m *Module) DestroyOtherSessions(keep string) {
- m.sessions.DestroyOthers(keep)
+// DestroyUserSessions invalidates every session of the user.
+func (m *Module) DestroyUserSessions(userID int64) {
+ m.sessions.DestroyAll(userID)
}
func logf(format string, args ...any) {
diff --git a/internal/web/auth/auth_test.go b/internal/web/auth/auth_test.go
index 157c87e..f73ef93 100644
--- a/internal/web/auth/auth_test.go
+++ b/internal/web/auth/auth_test.go
@@ -2,6 +2,7 @@ package auth
import (
"database/sql"
+ "errors"
"net/http"
"net/http/httptest"
"net/url"
@@ -26,9 +27,20 @@ func newTestSessionStore(t *testing.T) *sessionStore {
return newSessionStore(st, 7*24*time.Hour)
}
+// mustCreate signs the named user in, creating the user first if there is none
+// by that name.
func mustCreate(t *testing.T, s *sessionStore, username string) string {
t.Helper()
- token, err := s.Create(username)
+ u, err := s.store.GetUserByUsername(username)
+ if errors.Is(err, store.ErrUserNotFound) {
+ var id int64
+ id, err = s.store.CreateUser(username, "hash", store.RoleGlobal, store.Reach{})
+ u.ID = id
+ }
+ if err != nil {
+ t.Fatalf("user %q: %v", username, err)
+ }
+ token, err := s.Create(u.ID)
if err != nil {
t.Fatalf("create session: %v", err)
}
@@ -199,27 +211,17 @@ func dropSessionsTable(t *testing.T, path string) {
}
}
-func TestSessionRename(t *testing.T) {
- s := newTestSessionStore(t)
- token := mustCreate(t, s, "admin")
-
- s.Rename(token, "operator")
-
- name, ok := s.Lookup(token)
- if !ok {
- t.Fatal("session lost after rename")
- }
- if name != "operator" {
- t.Fatalf("session username = %q, want %q", name, "operator")
- }
-}
-
func TestSessionDestroyOthers(t *testing.T) {
s := newTestSessionStore(t)
keep := mustCreate(t, s, "admin")
other := mustCreate(t, s, "admin")
+ elsewhere := mustCreate(t, s, "operator")
+ admin, err := s.store.GetUserByUsername("admin")
+ if err != nil {
+ t.Fatal(err)
+ }
- s.DestroyOthers(keep)
+ s.DestroyOthers(admin.ID, keep)
if _, ok := s.Lookup(keep); !ok {
t.Fatal("current session was destroyed")
@@ -227,6 +229,9 @@ func TestSessionDestroyOthers(t *testing.T) {
if _, ok := s.Lookup(other); ok {
t.Fatal("other session survived")
}
+ if _, ok := s.Lookup(elsewhere); !ok {
+ t.Fatal("another user's session was destroyed")
+ }
}
func TestSessionLookupRejectsExpired(t *testing.T) {
@@ -254,3 +259,103 @@ func TestSessionTouchThrottled(t *testing.T) {
t.Fatal("touch did not renew a session past the throttle window")
}
}
+
+// requireAuthAs runs one request carrying token through RequireAuth and
+// returns the response and the principal the handler saw (nil when it never
+// ran). It is a POST, an activity request of the kind that renews a session.
+func requireAuthAs(t *testing.T, m *Module, token string) (*httptest.ResponseRecorder, *Principal) {
+ t.Helper()
+ var seen *Principal
+ h := m.RequireAuth(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+ if p, ok := CurrentPrincipal(r.Context()); ok {
+ seen = &p
+ }
+ }))
+ r := httptest.NewRequest(http.MethodPost, "http://panel.example.com/domains", nil)
+ r.AddCookie(&http.Cookie{Name: m.sessionCookie(), Value: token})
+ rec := httptest.NewRecorder()
+ h.ServeHTTP(rec, r)
+ return rec, seen
+}
+
+func mustUser(t *testing.T, m *Module, username string) int64 {
+ t.Helper()
+ id, err := m.store.CreateUser(username, "hash", store.RoleGlobal, store.Reach{})
+ if err != nil {
+ t.Fatalf("create user %q: %v", username, err)
+ }
+ return id
+}
+
+// A session is the person's, not the name's: when the user is deleted the
+// cookie is dead, and a new user created under the same name does not inherit
+// it (with that account's role).
+func TestRequireAuthDoesNotRebindASessionToANewUserWithTheSameName(t *testing.T) {
+ m := testModule(t, false)
+ mustUser(t, m, "admin")
+ id := mustUser(t, m, "alice")
+ token := mustCreate(t, m.sessions, "alice")
+
+ if err := m.store.DeleteUser(id); err != nil {
+ t.Fatalf("delete user: %v", err)
+ }
+ mustUser(t, m, "alice")
+
+ rec, seen := requireAuthAs(t, m, token)
+ if seen != nil {
+ t.Fatalf("the old cookie signed in as %q after the user was deleted and the name reused", seen.Username)
+ }
+ if rec.Code != http.StatusSeeOther || rec.Header().Get("Location") != "/login" {
+ t.Fatalf("status = %d, Location = %q; want a redirect to /login", rec.Code, rec.Header().Get("Location"))
+ }
+}
+
+// Renaming a user (the Account page, or an administrator on the user form)
+// leaves every session of theirs signed in, under the new name.
+func TestRenamingAUserKeepsTheirOtherSessions(t *testing.T) {
+ m := testModule(t, false)
+ id := mustUser(t, m, "alice")
+ current := mustCreate(t, m.sessions, "alice")
+ other := mustCreate(t, m.sessions, "alice")
+
+ if err := m.store.UpdateUser(id, "alicia", "hash", ""); err != nil {
+ t.Fatalf("rename: %v", err)
+ }
+
+ for name, token := range map[string]string{"current": current, "other": other} {
+ _, seen := requireAuthAs(t, m, token)
+ if seen == nil {
+ t.Errorf("the %s session was lost by the rename", name)
+ continue
+ }
+ if seen.Username != "alicia" || seen.ID != id {
+ t.Errorf("the %s session is %q (id %d) after the rename, want alicia (id %d)", name, seen.Username, seen.ID, id)
+ }
+ }
+}
+
+// A session whose user is gone is not renewed by being used: the renewal would
+// keep a dead cookie alive for as long as someone presses it.
+func TestRequireAuthDoesNotRenewASessionWhoseUserIsGone(t *testing.T) {
+ m := testModule(t, false)
+ mustUser(t, m, "admin")
+ id := mustUser(t, m, "alice")
+ token := mustCreate(t, m.sessions, "alice")
+ // Past the renewal throttle, so a Touch would write.
+ stale := time.Now().Add(-2 * time.Hour).Add(m.sessions.idle)
+ if err := m.store.RenewSession(hashToken(token), stale); err != nil {
+ t.Fatalf("age session: %v", err)
+ }
+ if err := m.store.DeleteUser(id); err != nil {
+ t.Fatalf("delete user: %v", err)
+ }
+
+ rec, _ := requireAuthAs(t, m, token)
+
+ if got := rec.Header().Values("Set-Cookie"); len(got) != 0 {
+ t.Errorf("a session of a missing user was renewed: %v", got)
+ }
+ if row, found, _ := m.store.LookupSession(hashToken(token)); found && !row.ExpiresAt.Equal(stale.UTC().Truncate(time.Second)) {
+ t.Errorf("expiry of a missing user's session moved to %v", row.ExpiresAt)
+ }
+}
diff --git a/internal/web/auth/handlers.go b/internal/web/auth/handlers.go
index 1379333..37fdb4e 100644
--- a/internal/web/auth/handlers.go
+++ b/internal/web/auth/handlers.go
@@ -120,7 +120,7 @@ func (m *Module) submitLogin(w http.ResponseWriter, r *http.Request) {
return
}
- token, err := m.sessions.Create(user.Username)
+ token, err := m.sessions.Create(user.ID)
if err != nil {
logf("panel: login: create session failed: %v", err)
m.renderLogin(w, http.StatusInternalServerError, "Internal error. Please try again.")
diff --git a/internal/web/auth/handlers_test.go b/internal/web/auth/handlers_test.go
index c8651c0..232aed1 100644
--- a/internal/web/auth/handlers_test.go
+++ b/internal/web/auth/handlers_test.go
@@ -64,9 +64,9 @@ func TestLoginSignsInWithTheRightPassword(t *testing.T) {
if token == "" {
t.Fatal("no session cookie was issued")
}
- name, ok := m.sessions.Lookup(token)
- if !ok || name != "admin" {
- t.Fatalf("the cookie's session resolves to %q, %t; want admin", name, ok)
+ id, ok := m.sessions.Lookup(token)
+ if u, err := m.store.GetUser(id); !ok || err != nil || u.Username != "admin" {
+ t.Fatalf("the cookie's session resolves to user %d (%q), %t, %v; want admin", id, u.Username, ok, err)
}
}
diff --git a/internal/web/auth/middleware.go b/internal/web/auth/middleware.go
index e1d119f..b68464d 100644
--- a/internal/web/auth/middleware.go
+++ b/internal/web/auth/middleware.go
@@ -14,19 +14,20 @@ func (m *Module) RequireAuth(next http.Handler) http.Handler {
http.Redirect(w, r, "/login", http.StatusSeeOther)
return
}
- username, ok := m.sessions.Lookup(token)
+ userID, ok := m.sessions.Lookup(token)
if !ok {
http.Redirect(w, r, "/login", http.StatusSeeOther)
return
}
- if isSessionActivity(r) && m.sessions.Touch(token) {
- m.setSessionCookie(w, token)
- }
- u, err := m.store.GetUserByUsername(username)
+ u, err := m.store.GetUser(userID)
if err != nil {
http.Redirect(w, r, "/login", http.StatusSeeOther)
return
}
+ // Only a session whose user exists is kept alive by being used.
+ if isSessionActivity(r) && m.sessions.Touch(token) {
+ m.setSessionCookie(w, token)
+ }
p := principalFromUser(u)
ctx := withPrincipal(r.Context(), p)
next.ServeHTTP(w, r.WithContext(ctx))
diff --git a/internal/web/auth/session.go b/internal/web/auth/session.go
index dace9ef..d2d0146 100644
--- a/internal/web/auth/session.go
+++ b/internal/web/auth/session.go
@@ -41,14 +41,14 @@ func hashToken(token string) string {
return hex.EncodeToString(sum[:])
}
-// Create issues a new session for username and returns its token. It fails
+// Create issues a new session for the user and returns its token. It fails
// closed: if the row cannot be written the caller gets an error and must not
// hand out a cookie, because a token that is not in the database looks like a
// signed-in browser while every request it makes bounces back to /login.
-func (s *sessionStore) Create(username string) (string, error) {
+func (s *sessionStore) Create(userID int64) (string, error) {
token := randomToken(32)
now := time.Now()
- if err := s.store.CreateSession(hashToken(token), username, now.Add(s.idle)); err != nil {
+ if err := s.store.CreateSession(hashToken(token), userID, now.Add(s.idle)); err != nil {
return "", err
}
// Pruning is housekeeping: the new session is already valid, so a failure
@@ -59,28 +59,28 @@ func (s *sessionStore) Create(username string) (string, error) {
return token, nil
}
-// Lookup returns the session username for a token if it exists and is
-// unexpired.
-func (s *sessionStore) Lookup(token string) (string, bool) {
+// Lookup returns the id of the user a token's session belongs to, if the
+// session exists and is unexpired.
+func (s *sessionStore) Lookup(token string) (int64, bool) {
if token == "" {
- return "", false
+ return 0, false
}
hash := hashToken(token)
row, found, err := s.store.LookupSession(hash)
if err != nil {
logf("panel: session: lookup failed: %v", err)
- return "", false
+ return 0, false
}
if !found {
- return "", false
+ return 0, false
}
if time.Now().After(row.ExpiresAt) {
if err := s.store.DeleteSession(hash); err != nil {
logf("panel: session: delete expired failed: %v", err)
}
- return "", false
+ return 0, false
}
- return row.Username, true
+ return row.UserID, true
}
// Touch extends a session's sliding expiry if it has been at least
@@ -107,17 +107,17 @@ func (s *sessionStore) Touch(token string) bool {
return true
}
-// Rename updates the username carried by a session, keeping its expiry.
-func (s *sessionStore) Rename(token, username string) {
- if err := s.store.RenameSession(hashToken(token), username); err != nil {
- logf("panel: session: rename failed: %v", err)
+// DestroyOthers invalidates every session of the user except keep.
+func (s *sessionStore) DestroyOthers(userID int64, keep string) {
+ if err := s.store.DeleteOtherSessions(userID, hashToken(keep)); err != nil {
+ logf("panel: session: destroy others failed: %v", err)
}
}
-// DestroyOthers invalidates every session except keep.
-func (s *sessionStore) DestroyOthers(keep string) {
- if err := s.store.DeleteOtherSessions(hashToken(keep)); err != nil {
- logf("panel: session: destroy others failed: %v", err)
+// DestroyAll invalidates every session of the user.
+func (s *sessionStore) DestroyAll(userID int64) {
+ if err := s.store.DeleteUserSessions(userID); err != nil {
+ logf("panel: session: destroy all failed: %v", err)
}
}
diff --git a/internal/web/handlers/handlers_account.go b/internal/web/handlers/handlers_account.go
index 22d8795..3fba3e2 100644
--- a/internal/web/handlers/handlers_account.go
+++ b/internal/web/handlers/handlers_account.go
@@ -155,11 +155,6 @@ func (h *Handlers) HandleAccountProfile(w http.ResponseWriter, r *http.Request)
fail(http.StatusInternalServerError, "Could not save the profile. Please check the logs and try again.")
return
}
- if f.Username != u.Username {
- if token, ok := h.auth.SessionToken(r); ok {
- h.auth.RenameSession(token, f.Username)
- }
- }
// The e-mail is the report address of every domain that follows this
// user's "my account e-mail" default.
h.resyncAfterEmailChange(u, f.Email)
@@ -215,7 +210,7 @@ func (h *Handlers) HandleAccountPassword(w http.ResponseWriter, r *http.Request)
return
}
if token, ok := h.auth.SessionToken(r); ok {
- h.auth.DestroyOtherSessions(token)
+ h.auth.DestroyOtherSessions(u.ID, token)
}
logf("panel: user %d changed their password", u.ID)
http.Redirect(w, r, "/account?done=password", http.StatusSeeOther)
diff --git a/internal/web/handlers/handlers_apps.go b/internal/web/handlers/handlers_apps.go
index a5f8741..0937dbb 100644
--- a/internal/web/handlers/handlers_apps.go
+++ b/internal/web/handlers/handlers_apps.go
@@ -737,18 +737,21 @@ func splitAddresses(s string) []string {
})
}
-// applicationErrorMessage turns a service error into a user-facing message,
-// passing through the validation errors (which are safe, fixed strings) and
-// masking anything unexpected.
+// applicationErrorMessage turns a service error into what the form says. A
+// validation error is the administrator's to read — it says what to fix — and
+// is shown as it is. Anything else is the server's own business (a path, a
+// command's output, SQL): it goes to the log and the page gets a fixed line.
func applicationErrorMessage(err error) string {
+ var validation *app.ValidationError
switch {
case errors.Is(err, store.ErrLoginExists):
return "That login is already in use. Choose another."
case errors.Is(err, store.ErrDomainNotFound), errors.Is(err, store.ErrApplicationNotFound):
return "The item no longer exists."
- default:
- // Validation errors from the app service are safe to surface verbatim;
- // they describe what the admin must fix (login/address rules).
+ case errors.As(err, &validation):
return err.Error()
+ default:
+ logf("panel: application: %v", err)
+ return "Could not save the application. Check the logs and try again."
}
}
diff --git a/internal/web/handlers/handlers_apps_test.go b/internal/web/handlers/handlers_apps_test.go
index 45c92c4..5ac94ac 100644
--- a/internal/web/handlers/handlers_apps_test.go
+++ b/internal/web/handlers/handlers_apps_test.go
@@ -3,6 +3,7 @@ package handlers
import (
"net/http"
"net/url"
+ "path/filepath"
"strings"
"testing"
@@ -32,7 +33,13 @@ type appStand struct {
func newAppStand(t *testing.T) appStand {
t.Helper()
- h, _ := settingsServer(t)
+ return newAppStandAt(t, filepath.Join(t.TempDir(), "test.db"))
+}
+
+// newAppStandAt is newAppStand over a database at a path the test knows.
+func newAppStandAt(t *testing.T, dbPath string) appStand {
+ t.Helper()
+ h, _ := settingsServerAt(t, dbPath)
s := appStand{h: h, sasl: map[string]string{}}
sasl := app.NewSASLDB("/data/sasl/sasldb2", "mail.example.org").WithRunner(func(args []string, stdin []byte) error {
login := args[len(args)-1]
diff --git a/internal/web/handlers/handlers_dmarc_pages_test.go b/internal/web/handlers/handlers_dmarc_pages_test.go
index 29bddf6..bb366da 100644
--- a/internal/web/handlers/handlers_dmarc_pages_test.go
+++ b/internal/web/handlers/handlers_dmarc_pages_test.go
@@ -155,8 +155,9 @@ func TestDMARCPagesAreAbsentWithIngestOff(t *testing.T) {
}
// A domain administrator reaches the reports of the domains assigned to them
-// and nothing else: not the hub, not another tenant's domain page, not another
-// tenant's report. A report that does not exist is the same 404 as one that is
+// and nothing else: the hub lists only theirs and does not carry the ingest
+// statistics of the whole instance, and another tenant's domain page and report
+// are not there. A report that does not exist is the same 404 as one that is
// not theirs.
func TestDMARCPagesStayWithinTheViewersDomains(t *testing.T) {
s := newDMARCStand(t)
@@ -174,22 +175,31 @@ func TestDMARCPagesStayWithinTheViewersDomains(t *testing.T) {
}
ops := domainAdmin(t, s.h.store, "ops", s.d.ID)
- // Their own domain and report are theirs; the way back to the hub is not
- // offered, since the hub is not theirs.
+ // The hub is open to them: their own domain's reports, no one else's, and
+ // not the statistics of the instance's ingest box.
+ hub := send(s.h.HandleDMARCList, &ops, "GET", "/outbound/dmarc", nil, nil)
+ if hub.Code != http.StatusOK {
+ t.Fatalf("hub for its administrator = %d, want 200: %s", hub.Code, hub.Body.String())
+ }
+ has(t, "hub for its administrator", hub.Body.String(), `DMARC reports
`, `Recent reports
`,
+ `example.org`, `Outlook.com`, `href="/outbound/dmarc/reports/`+idStr(s.reportID)+`"`)
+ lacks(t, "hub for its administrator", hub.Body.String(), "other.example.net", "google.com",
+ `/outbound/dmarc/reports/`+idStr(otherReport)+`"`, `Ingest
`, `dmarc-reports@mail.example.org`,
+ `parse failure`, `Last report`, `Retention`)
+
+ // Their own domain and report are theirs, and so is the way back to the hub.
own := send(s.h.HandleDMARCDomain, &ops, "GET", "/outbound/dmarc/domains/1", map[string]string{"id": "1"}, nil)
if own.Code != http.StatusOK {
t.Fatalf("own domain = %d", own.Code)
}
- has(t, "domain for its administrator", own.Body.String(), `example.org
`)
- lacks(t, "domain for its administrator", own.Body.String(), ``)
+ has(t, "domain for its administrator", own.Body.String(), `example.org
`, `DMARC reports`)
rep := send(s.h.HandleDMARCReport, &ops, "GET", "/outbound/dmarc/reports/"+idStr(s.reportID), map[string]string{"id": idStr(s.reportID)}, nil)
if rep.Code != http.StatusOK {
t.Fatalf("own report = %d", rep.Code)
}
- lacks(t, "report for its administrator", rep.Body.String(), ``)
+ has(t, "report for its administrator", rep.Body.String(), `DMARC reports`)
for name, code := range map[string]int{
- "hub": send(s.h.HandleDMARCList, &ops, "GET", "/outbound/dmarc", nil, nil).Code,
"another tenant's domain": send(s.h.HandleDMARCDomain, &ops, "GET", "/outbound/dmarc/domains/"+idStr(other.ID),
map[string]string{"id": idStr(other.ID)}, nil).Code,
"another tenant's report": send(s.h.HandleDMARCReport, &ops, "GET", "/outbound/dmarc/reports/"+idStr(otherReport),
@@ -202,3 +212,32 @@ func TestDMARCPagesStayWithinTheViewersDomains(t *testing.T) {
}
}
}
+
+// A domain administrator with no outbound domain is not shown every tenant's
+// reports because their own list is empty: the hub has no rows, and no error.
+func TestDMARCHubForAnAdministratorWithoutOutboundDomains(t *testing.T) {
+ s := newDMARCStand(t)
+ nobody := domainAdmin(t, s.h.store, "nobody")
+
+ rec := send(s.h.HandleDMARCList, &nobody, "GET", "/outbound/dmarc", nil, nil)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("hub = %d, want 200: %s", rec.Code, rec.Body.String())
+ }
+ has(t, "hub", rec.Body.String(), `No reports yet.`)
+ lacks(t, "hub", rec.Body.String(), "example.org", "Outlook.com", `Ingest`)
+}
+
+// The global role sees the ingest box and every report, including those of a
+// domain that has since been removed.
+func TestDMARCHubForTheGlobalRoleSeesEverything(t *testing.T) {
+ s := newDMARCStand(t)
+ if _, err := s.h.store.InsertDMARCReport(store.DMARCReport{
+ Domain: "gone.example.net", Reporter: "yahoo.com", ReportID: "gone", ReceivedAt: time.Now(),
+ PeriodBegin: time.Now().Add(-24 * time.Hour), PeriodEnd: time.Now(), PassCount: 3,
+ }); err != nil {
+ t.Fatal(err)
+ }
+
+ body := getBody(t, s.h.HandleDMARCList, "/outbound/dmarc")
+ has(t, "hub", body, `Ingest
`, `Recent reports
`, `example.org`, `Outlook.com`, `gone.example.net`, `yahoo.com`)
+}
diff --git a/internal/web/handlers/handlers_dmarc_reports.go b/internal/web/handlers/handlers_dmarc_reports.go
index 5323bc9..3abd2bf 100644
--- a/internal/web/handlers/handlers_dmarc_reports.go
+++ b/internal/web/handlers/handlers_dmarc_reports.go
@@ -38,13 +38,13 @@ func (h *Handlers) canViewDMARCDomain(p auth.Principal, d store.Domain) bool {
return p.CanAccessDomain(d.ID)
}
-// HandleDMARCList is the global DMARC reports index.
+// HandleDMARCList is the DMARC reports hub. The global role sees every report
+// and the ingest statistics of the instance; a domain administrator sees the
+// reports of the sending domains assigned to them, and no ingest statistics
+// (the box describes the whole instance: its mailbox, its failures).
func (h *Handlers) HandleDMARCList(w http.ResponseWriter, r *http.Request) {
p, ok := h.requireDMARC(w, r)
- if !ok || !p.IsGlobal() {
- if ok && !p.IsGlobal() {
- http.NotFound(w, r)
- }
+ if !ok {
return
}
assigned, err := h.assignedDomains(p)
@@ -53,24 +53,18 @@ func (h *Handlers) HandleDMARCList(w http.ResponseWriter, r *http.Request) {
http.Error(w, "internal error", http.StatusInternalServerError)
return
}
- names := make([]string, 0, len(assigned))
+ scope := store.DMARCReportScope{AllDomains: p.IsGlobal()}
domainIDs := make(map[string]int64, len(assigned))
for _, d := range assigned {
- names = append(names, d.Name)
+ scope.Domains = append(scope.Domains, d.Name)
domainIDs[d.Name] = d.ID
}
- reports, err := h.store.ListDMARCReports(names, 100)
+ reports, err := h.store.ListDMARCReports(scope, 100)
if err != nil {
logf("panel: dmarc list: %v", err)
http.Error(w, "internal error", http.StatusInternalServerError)
return
}
- stats, err := h.store.DMARCIngestStats()
- if err != nil {
- logf("panel: dmarc ingest stats: %v", err)
- http.Error(w, "internal error", http.StatusInternalServerError)
- return
- }
rows := make([]view.DMARCReportRow, len(reports))
for i, rep := range reports {
rows[i] = dmarcRow(rep)
@@ -78,6 +72,17 @@ func (h *Handlers) HandleDMARCList(w http.ResponseWriter, r *http.Request) {
rows[i].DomainHref = view.DMARCDomainHref(id)
}
}
+ if !p.IsGlobal() {
+ page := view.NewDMARCHub(h.shellMeta(r), view.IngestInput{}).WithoutIngest().WithReports(rows)
+ h.view.Render(w, http.StatusOK, "dmarc", page)
+ return
+ }
+ stats, err := h.store.DMARCIngestStats()
+ if err != nil {
+ logf("panel: dmarc ingest stats: %v", err)
+ http.Error(w, "internal error", http.StatusInternalServerError)
+ return
+ }
in := view.IngestInput{
OK: stats.IngestOK, KeptThisWeek: stats.KeptThisWeek, ParseFailures: stats.ParseFailures,
Hosted: h.dmarc.DefaultHostedSuggestion(), Host: h.cfg.Hostname,
@@ -135,7 +140,7 @@ func (h *Handlers) HandleDMARCDomain(w http.ResponseWriter, r *http.Request) {
Source: s.SourceIP, ThisRelay: relay[s.SourceIP], Pass: s.PassCount, Fail: s.FailCount, Disposition: s.Disposition,
}
}
- page := view.NewDMARCDomain(h.shellMeta(r), d.ID, d.Name, p.IsGlobal(), pass, fail, windowDays,
+ page := view.NewDMARCDomain(h.shellMeta(r), d.ID, d.Name, true, pass, fail, windowDays,
dmarc.TightenPolicyHint(pass, fail, hints)).
WithSources(sourceRows).WithReports(reportRows)
h.view.Render(w, http.StatusOK, "dmarc-domain", page)
@@ -198,7 +203,7 @@ func (h *Handlers) HandleDMARCReport(w http.ResponseWriter, r *http.Request) {
Received: rep.ReceivedAt.UTC().Format("2006-01-02 15:04") + " UTC",
PolicyP: rep.PolicyP, PolicySP: rep.PolicySP, PolicyPct: rep.PolicyPct,
PolicyADKIM: rep.PolicyADKIM, PolicyASPF: rep.PolicyASPF, Recipient: rep.Recipient,
- Hub: p.IsGlobal(),
+ Hub: true,
}).WithRecords(records)
h.view.Render(w, http.StatusOK, "dmarc-report", page)
}
diff --git a/internal/web/handlers/handlers_errors_test.go b/internal/web/handlers/handlers_errors_test.go
new file mode 100644
index 0000000..097fe77
--- /dev/null
+++ b/internal/web/handlers/handlers_errors_test.go
@@ -0,0 +1,208 @@
+package handlers
+
+import (
+ "database/sql"
+ "errors"
+ "net/http"
+ "net/url"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "github.com/mixeme/selfpost/internal/app"
+ "github.com/mixeme/selfpost/internal/inbound"
+ "github.com/mixeme/selfpost/internal/postfix"
+ "github.com/mixeme/selfpost/internal/store"
+)
+
+// A page that a domain administrator sees says what is wrong with what they
+// typed. It never says what went wrong inside the server: paths, SQL, the
+// output of a command. Those go to the log, and the page gets a fixed line.
+
+// leak is a stand-in for internal detail that must never reach a page.
+const leak = "secret-internal-detail"
+
+const savedElsewhere = "Check the logs and try again."
+
+type failingSenderMaps struct{}
+
+func (failingSenderMaps) RebuildSenderLoginMaps([]postfix.Binding) error {
+ return errors.New("write /srv/" + leak + "/sender_login_maps: permission denied")
+}
+
+type failingInboundMaps struct{}
+
+func (failingInboundMaps) RebuildInboundMaps([]postfix.InboundRoute) error {
+ return errors.New("postmap /srv/" + leak + "/transport_maps: exit status 1")
+}
+
+// breakDatabase makes the statements fail inside SQLite with an error that
+// carries the leak, as a real storage failure would carry its own detail.
+func breakDatabase(t *testing.T, dbPath string, triggers ...string) {
+ t.Helper()
+ db, err := sql.Open("sqlite", "file:"+dbPath+"?_pragma=busy_timeout(5000)")
+ if err != nil {
+ t.Fatalf("open database directly: %v", err)
+ }
+ defer db.Close()
+ for _, stmt := range triggers {
+ if _, err := db.Exec(stmt); err != nil {
+ t.Fatalf("%s: %v", stmt, err)
+ }
+ }
+}
+
+// refusal checks a refused request: the status it has today, none of the
+// internal detail, and the fixed line in its place.
+func refusal(t *testing.T, what, body string, wantCode, code int) {
+ t.Helper()
+ shown := noticeOf(body)
+ if code != wantCode {
+ t.Errorf("%s = %d, want %d; page says %q", what, code, wantCode, shown)
+ }
+ if strings.Contains(body, leak) {
+ t.Errorf("%s shows internal detail; page says %q", what, shown)
+ }
+ if !strings.Contains(body, savedElsewhere) {
+ t.Errorf("%s does not give the fixed line %q; page says %q", what, savedElsewhere, shown)
+ }
+}
+
+// noticeOf is the text of the page's refusal notice, for failure messages.
+func noticeOf(body string) string {
+ for _, line := range strings.Split(body, "\n") {
+ if strings.Contains(line, "ti-alert-circle") {
+ return strings.TrimSpace(line)
+ }
+ }
+ return "(no notice)"
+}
+
+func TestApplicationFormDoesNotEchoInternalErrors(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "test.db")
+ s := newAppStandAt(t, path)
+ working := s.h.apps
+ a, _, err := working.CreateWithSettings(s.d.ID, "prod-server", app.Settings{Mode: store.AddressModeWildcard}, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ paths := map[string]string{"id": idStr(s.d.ID), "aid": idStr(a.ID)}
+
+ // The reload of the Postfix maps fails.
+ sasl := app.NewSASLDB("/data/sasl/sasldb2", "mail.example.org").WithRunner(func([]string, []byte) error { return nil })
+ s.h.apps = app.NewService(s.h.store, sasl, failingSenderMaps{})
+
+ rec := postFormAs(s.h.HandleApplicationCreate, globalPrincipal, "/outbound/domains/1/applications/new", s.paths,
+ url.Values{"login": {"new-server"}, "mode": {"wildcard"}})
+ refusal(t, "create when the reload fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+
+ rec = postFormAs(s.h.HandleApplicationSave, globalPrincipal, "/outbound/domains/1/applications/1", paths,
+ url.Values{"mode": {"wildcard"}})
+ refusal(t, "save when the reload fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+
+ // The database fails.
+ s.h.apps = working
+ breakDatabase(t, path, `CREATE TRIGGER boom BEFORE UPDATE ON applications BEGIN SELECT RAISE(ABORT, '`+leak+`'); END`)
+ rec = postFormAs(s.h.HandleApplicationSave, globalPrincipal, "/outbound/domains/1/applications/1", paths,
+ url.Values{"mode": {"wildcard"}})
+ refusal(t, "save when the database fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+}
+
+// What the administrator typed wrong is still said in full.
+func TestApplicationFormStillShowsValidationMessages(t *testing.T) {
+ s := newAppStand(t)
+ a, _, err := s.h.apps.CreateWithSettings(s.d.ID, "prod-server", app.Settings{Mode: store.AddressModeWildcard}, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ paths := map[string]string{"id": idStr(s.d.ID), "aid": idStr(a.ID)}
+
+ for name, tc := range map[string]struct {
+ url.Values
+ want string
+ }{
+ "a bad login": {url.Values{"login": {"has space"}, "mode": {"wildcard"}}, "login may contain only letters, digits"},
+ "another domain's one": {url.Values{"login": {"other-app"}, "mode": {"list"}, "addresses": {"a@evil.example"}}, "does not belong to domain example.org"},
+ "a taken login": {url.Values{"login": {"prod-server"}, "mode": {"wildcard"}}, "That login is already in use"},
+ } {
+ rec := postFormAs(s.h.HandleApplicationCreate, globalPrincipal, "/outbound/domains/1/applications/new", s.paths, tc.Values)
+ if !strings.Contains(rec.Body.String(), tc.want) {
+ t.Errorf("create with %s does not say %q:\n%s", name, tc.want, rec.Body.String())
+ }
+ }
+ rec := postFormAs(s.h.HandleApplicationSave, globalPrincipal, "/outbound/domains/1/applications/1", paths,
+ url.Values{"mode": {"list"}, "addresses": {"a@evil.example"}})
+ if !strings.Contains(rec.Body.String(), "does not belong to domain example.org") {
+ t.Errorf("save with another domain's address does not say so:\n%s", rec.Body.String())
+ }
+}
+
+func TestInboundFormsDoNotEchoInternalErrors(t *testing.T) {
+ h, st := inboundHandlers(t)
+ d, err := st.AddInboundDomain("lists.example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
+ h.inbound = inbound.NewService(st, failingInboundMaps{})
+ id := itoa(d.ID)
+
+ rec := inboundCall(h, http.MethodPost, "/inbound/domains/"+id+"/upstream",
+ url.Values{"host": {"10.0.0.8"}, "port": {"25"}, "tls_mode": {"encrypt"}}, globalPrincipal)
+ refusal(t, "upstream when the reload fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+
+ rec = inboundCall(h, http.MethodPost, "/inbound/domains/"+id+"/recipients",
+ url.Values{"recipient_mode": {"any"}}, globalPrincipal)
+ refusal(t, "recipients when the reload fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+}
+
+func TestInboundFormsStillShowValidationMessages(t *testing.T) {
+ h, st := inboundHandlers(t)
+ d, err := st.AddInboundDomain("lists.example.com")
+ if err != nil {
+ t.Fatal(err)
+ }
+ id := itoa(d.ID)
+
+ for name, tc := range map[string]struct {
+ target string
+ form url.Values
+ want string
+ }{
+ "a bad host": {"/upstream", url.Values{"host": {"bad host!"}, "port": {"25"}, "tls_mode": {"may"}}, "host is invalid"},
+ "a bad port": {"/upstream", url.Values{"host": {"10.0.0.8"}, "port": {"99999"}, "tls_mode": {"may"}}, "port must be between 1 and 65535"},
+ "a bad TLS mode": {"/upstream", url.Values{"host": {"10.0.0.8"}, "port": {"25"}, "tls_mode": {"sometimes"}}, "invalid TLS mode"},
+ "another domain": {"/recipients", url.Values{"recipient_mode": {"list"}, "addresses": {"abuse@other.com"}}, "does not belong to domain lists.example.com"},
+ "an empty list": {"/recipients", url.Values{"recipient_mode": {"list"}}, "requires at least one address"},
+ "an unknown mode": {"/recipients", url.Values{"recipient_mode": {"some"}}, "invalid recipient mode"},
+ } {
+ rec := inboundCall(h, http.MethodPost, "/inbound/domains/"+id+tc.target, tc.form, globalPrincipal)
+ if rec.Code != http.StatusBadRequest || !strings.Contains(rec.Body.String(), tc.want) {
+ t.Errorf("%s = %d, does not say %q:\n%s", name, rec.Code, tc.want, rec.Body.String())
+ }
+ }
+}
+
+func TestRecalculationDoesNotEchoInternalErrors(t *testing.T) {
+ path := filepath.Join(t.TempDir(), "test.db")
+ s := newAppStandAt(t, path)
+ a, _, err := s.h.apps.CreateWithSettings(s.d.ID, "prod-server", app.Settings{Mode: store.AddressModeWildcard}, nil)
+ if err != nil {
+ t.Fatal(err)
+ }
+ for scope, ref := range map[string]int64{store.RateLimitScopeDomain: s.d.ID, store.RateLimitScopeApp: a.ID} {
+ rl := store.RateLimit{Scope: scope, RefID: ref, Mode: store.RateLimitModeAuto, AutoMultiplier: store.DefaultAutoMultiplier, WindowSeconds: 3600}
+ if err := s.h.store.SetRateLimit(rl); err != nil {
+ t.Fatal(err)
+ }
+ }
+ breakDatabase(t, path,
+ `CREATE TRIGGER boom_insert BEFORE INSERT ON rate_limits BEGIN SELECT RAISE(ABORT, '`+leak+`'); END`,
+ `CREATE TRIGGER boom_update BEFORE UPDATE ON rate_limits BEGIN SELECT RAISE(ABORT, '`+leak+`'); END`)
+
+ rec := postFormAs(s.h.HandleDomainRateLimitRecalc, globalPrincipal, "/outbound/domains/1/settings/ratelimit/recalc", s.paths, url.Values{})
+ refusal(t, "domain recalculation when the database fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+
+ paths := map[string]string{"id": idStr(s.d.ID), "aid": idStr(a.ID)}
+ rec = postFormAs(s.h.HandleAppRateLimitRecalc, globalPrincipal, "/outbound/domains/1/applications/1/ratelimit/recalc", paths, url.Values{})
+ refusal(t, "application recalculation when the database fails", rec.Body.String(), http.StatusBadRequest, rec.Code)
+}
diff --git a/internal/web/handlers/handlers_inbound.go b/internal/web/handlers/handlers_inbound.go
index b5179f3..a5b1895 100644
--- a/internal/web/handlers/handlers_inbound.go
+++ b/internal/web/handlers/handlers_inbound.go
@@ -9,6 +9,7 @@ import (
"github.com/mixeme/selfpost/internal/dnscheck"
"github.com/mixeme/selfpost/internal/health"
+ "github.com/mixeme/selfpost/internal/inbound"
"github.com/mixeme/selfpost/internal/store"
"github.com/mixeme/selfpost/internal/web/auth"
"github.com/mixeme/selfpost/internal/web/validate"
@@ -225,6 +226,19 @@ func (h *Handlers) inboundFilter() (on bool, milter, action string) {
return true, h.cfg.InboundAntispamMilter, h.cfg.InboundAntispamAction
}
+// inboundErrorMessage is what a form of the inbound domain page says about a
+// failed save. A validation error says what to fix and is shown as it is;
+// anything else (the reload of the Postfix maps, the database) goes to the log
+// and the page gets a fixed line.
+func inboundErrorMessage(err error, form, what string) string {
+ var validation *inbound.ValidationError
+ if errors.As(err, &validation) {
+ return err.Error()
+ }
+ logf("panel: inbound: save %s: %v", form, err)
+ return "Could not save " + what + ". Check the logs and try again."
+}
+
func firstNonEmpty(ss ...string) string {
for _, s := range ss {
if s != "" {
@@ -271,7 +285,9 @@ func (h *Handlers) HandleInboundTransport(w http.ResponseWriter, r *http.Request
port := r.PostFormValue("port")
tlsMode := r.PostFormValue("tls_mode")
if err := h.inbound.SetTransport(d.ID, host, port, tlsMode); err != nil {
- h.renderInboundDetail(w, r, http.StatusBadRequest, d, inboundDetailView{TransportErr: err.Error()})
+ h.renderInboundDetail(w, r, http.StatusBadRequest, d, inboundDetailView{
+ TransportErr: inboundErrorMessage(err, "upstream", "the upstream"),
+ })
return
}
http.Redirect(w, r, fmt.Sprintf("/inbound/domains/%d?saved=1", d.ID), http.StatusSeeOther)
@@ -289,7 +305,9 @@ func (h *Handlers) HandleInboundRecipients(w http.ResponseWriter, r *http.Reques
mode := r.PostFormValue("recipient_mode")
addrs := splitAddresses(r.PostFormValue("addresses"))
if err := h.inbound.SetRecipients(d.ID, mode, addrs); err != nil {
- h.renderInboundDetail(w, r, http.StatusBadRequest, d, inboundDetailView{RecipientErr: err.Error()})
+ h.renderInboundDetail(w, r, http.StatusBadRequest, d, inboundDetailView{
+ RecipientErr: inboundErrorMessage(err, "recipients", "the recipients"),
+ })
return
}
http.Redirect(w, r, fmt.Sprintf("/inbound/domains/%d?recipients=1", d.ID), http.StatusSeeOther)
diff --git a/internal/web/handlers/handlers_ratelimit.go b/internal/web/handlers/handlers_ratelimit.go
index 322a016..cedf3c4 100644
--- a/internal/web/handlers/handlers_ratelimit.go
+++ b/internal/web/handlers/handlers_ratelimit.go
@@ -1,6 +1,7 @@
package handlers
import (
+ "errors"
"fmt"
"net"
"net/http"
@@ -168,8 +169,7 @@ func (h *Handlers) HandleDomainRateLimitRecalc(w http.ResponseWriter, r *http.Re
return
}
if err := h.recalcRateLimit(store.RateLimitScopeDomain, d.ID); err != nil {
- logf("panel: domain %d: recalc rate limit: %v", d.ID, err)
- h.renderDomainSettings(w, r, http.StatusBadRequest, d, settingsView{Err: err.Error()})
+ h.renderDomainSettings(w, r, http.StatusBadRequest, d, settingsView{Err: recalcErrorMessage(err, "domain", d.ID)})
return
}
http.Redirect(w, r, fmt.Sprintf("/outbound/domains/%d?recalculated=1", d.ID), http.StatusSeeOther)
@@ -181,12 +181,24 @@ func (h *Handlers) HandleAppRateLimitRecalc(w http.ResponseWriter, r *http.Reque
return
}
if err := h.recalcRateLimit(store.RateLimitScopeApp, a.ID); err != nil {
- h.renderApplicationForm(w, r, http.StatusBadRequest, d, &a, nil, fmt.Sprintf("%s: %s", a.Login, err.Error()))
+ h.renderApplicationForm(w, r, http.StatusBadRequest, d, &a, nil,
+ fmt.Sprintf("%s: %s", a.Login, recalcErrorMessage(err, "application", a.ID)))
return
}
http.Redirect(w, r, fmt.Sprintf("/outbound/domains/%d?recalculated=1", a.DomainID), http.StatusSeeOther)
}
+// recalcErrorMessage is what the page says about a recalculation that did not
+// happen. A limit that is not automatic has nothing to recalculate, and the
+// administrator is told so; any other failure goes to the log.
+func recalcErrorMessage(err error, what string, id int64) string {
+ if errors.Is(err, store.ErrNotAutoRateLimit) {
+ return err.Error()
+ }
+ logf("panel: %s %d: recalc rate limit: %v", what, id, err)
+ return "Could not recalculate the limit. Check the logs and try again."
+}
+
func (h *Handlers) recalcRateLimit(scope string, refID int64) error {
return h.store.RecalcAutoRateLimit(scope, refID, h.sendLogRetentionDays(), h.l1Messages(), h.l1Window())
}
diff --git a/internal/web/handlers/handlers_sessions_test.go b/internal/web/handlers/handlers_sessions_test.go
new file mode 100644
index 0000000..2754028
--- /dev/null
+++ b/internal/web/handlers/handlers_sessions_test.go
@@ -0,0 +1,161 @@
+package handlers
+
+import (
+ "crypto/sha256"
+ "encoding/hex"
+ "net/http"
+ "net/http/httptest"
+ "net/url"
+ "strings"
+ "testing"
+ "time"
+
+ "github.com/mixeme/selfpost/internal/store"
+ "github.com/mixeme/selfpost/internal/web/auth"
+ "golang.org/x/crypto/bcrypt"
+)
+
+// sessionCookieName is the panel's cookie without TLS (auth.Config{} here).
+const sessionCookieName = "selfpost_session"
+
+// signedIn stores a session for the user the way a login does and returns the
+// token the browser would hold.
+func signedIn(t *testing.T, h *Handlers, username, token string) string {
+ t.Helper()
+ u, err := h.store.GetUserByUsername(username)
+ if err != nil {
+ t.Fatalf("get %s: %v", username, err)
+ }
+ sum := sha256.Sum256([]byte(token))
+ if err := h.store.CreateSession(hex.EncodeToString(sum[:]), u.ID, time.Now().Add(time.Hour)); err != nil {
+ t.Fatalf("create session of %s: %v", username, err)
+ }
+ return token
+}
+
+func hasSession(t *testing.T, h *Handlers, token string) bool {
+ t.Helper()
+ sum := sha256.Sum256([]byte(token))
+ _, found, err := h.store.LookupSession(hex.EncodeToString(sum[:]))
+ if err != nil {
+ t.Fatalf("lookup session: %v", err)
+ }
+ return found
+}
+
+// postWithSession is postFormAs for a request that also carries the session
+// cookie of the browser it comes from.
+func postWithSession(h http.HandlerFunc, p auth.Principal, token, target string, pathValues map[string]string, values url.Values) *httptest.ResponseRecorder {
+ req := httptest.NewRequest(http.MethodPost, target, strings.NewReader(values.Encode()))
+ req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+ req.AddCookie(&http.Cookie{Name: sessionCookieName, Value: token})
+ for k, v := range pathValues {
+ req.SetPathValue(k, v)
+ }
+ rec := httptest.NewRecorder()
+ h(rec, auth.RequestWithPrincipal(req, p))
+ return rec
+}
+
+// addUser creates a global user with a known password and returns its principal.
+func addUser(t *testing.T, h *Handlers, username, password string) auth.Principal {
+ t.Helper()
+ hash, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.MinCost)
+ if err != nil {
+ t.Fatal(err)
+ }
+ id, err := h.store.CreateUser(username, string(hash), store.RoleGlobal, store.Reach{})
+ if err != nil {
+ t.Fatalf("create %s: %v", username, err)
+ }
+ return auth.Principal{ID: id, Username: username, Role: auth.RoleGlobal}
+}
+
+// Changing your own password signs out your other sessions, and only yours:
+// the Account page is open to every role, so it cannot reach into anybody
+// else's login.
+func TestAccountPasswordSignsOutOnlyTheUsersOtherSessions(t *testing.T) {
+ h, _ := settingsServer(t)
+ bob := addUser(t, h, "bob", "bobs-long-password-1")
+ aliceCurrent := signedIn(t, h, "admin", "admin-current")
+ bobCurrent := signedIn(t, h, "bob", "bob-current")
+ bobOther := signedIn(t, h, "bob", "bob-other")
+
+ const next = "bobs-new-long-password-2"
+ rec := postWithSession(h.HandleAccountPassword, bob, bobCurrent, "/account/password", nil,
+ url.Values{"current_password": {"bobs-long-password-1"}, "new_password": {next}, "new_password_confirm": {next}})
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST /account/password = %d:\n%s", rec.Code, rec.Body.String())
+ }
+
+ if hasSession(t, h, bobOther) {
+ t.Error("the user's other session survived their password change")
+ }
+ if !hasSession(t, h, bobCurrent) {
+ t.Error("the session that changed the password was signed out")
+ }
+ if !hasSession(t, h, aliceCurrent) {
+ t.Error("another user's session was signed out by someone else's password change")
+ }
+}
+
+// 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)
+ admin := globalPrincipal
+ bob := addUser(t, h, "bob", "bobs-long-password-1")
+ adminCurrent := signedIn(t, h, "admin", "admin-current")
+ bob1 := signedIn(t, h, "bob", "bob-1")
+ 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"}})
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST user form = %d:\n%s", rec.Code, rec.Body.String())
+ }
+
+ if hasSession(t, h, bob1) || hasSession(t, h, bob2) {
+ t.Error("the user's sessions survived an administrator setting their password")
+ }
+ if !hasSession(t, h, adminCurrent) {
+ t.Error("the administrator's own session was signed out by editing someone else")
+ }
+}
+
+// Saving the form without a new password does not sign anyone out.
+func TestUserFormWithoutAPasswordKeepsTheSessions(t *testing.T) {
+ h, _ := 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"}})
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST user form = %d:\n%s", rec.Code, rec.Body.String())
+ }
+ if !hasSession(t, h, bob1) {
+ t.Error("an edit without a new password signed the user out")
+ }
+}
+
+// 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)
+ 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"}})
+ if rec.Code != http.StatusSeeOther {
+ t.Fatalf("POST user form = %d:\n%s", rec.Code, rec.Body.String())
+ }
+
+ if !hasSession(t, h, current) {
+ t.Error("the administrator's current session was signed out")
+ }
+ if hasSession(t, h, other) {
+ t.Error("the administrator's other session survived their own password change")
+ }
+}
diff --git a/internal/web/handlers/handlers_settings_test.go b/internal/web/handlers/handlers_settings_test.go
index 553d22c..591dcf9 100644
--- a/internal/web/handlers/handlers_settings_test.go
+++ b/internal/web/handlers/handlers_settings_test.go
@@ -91,7 +91,14 @@ func TestServerSettingsIsGlobalOnly(t *testing.T) {
func settingsServer(t *testing.T) (*Handlers, string) {
t.Helper()
- st, err := store.Open(filepath.Join(t.TempDir(), "test.db"))
+ return settingsServerAt(t, filepath.Join(t.TempDir(), "test.db"))
+}
+
+// settingsServerAt is settingsServer over a database at a path the test knows,
+// for the tests that break the database from outside.
+func settingsServerAt(t *testing.T, dbPath string) (*Handlers, string) {
+ t.Helper()
+ st, err := store.Open(dbPath)
if err != nil {
t.Fatalf("open store: %v", err)
}
diff --git a/internal/web/handlers/handlers_users.go b/internal/web/handlers/handlers_users.go
index 2e08b45..e61f694 100644
--- a/internal/web/handlers/handlers_users.go
+++ b/internal/web/handlers/handlers_users.go
@@ -7,6 +7,7 @@ import (
"strings"
"github.com/mixeme/selfpost/internal/store"
+ "github.com/mixeme/selfpost/internal/web/auth"
"github.com/mixeme/selfpost/internal/web/validate"
"github.com/mixeme/selfpost/internal/web/view"
"golang.org/x/crypto/bcrypt"
@@ -310,6 +311,9 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
return
}
h.resyncAfterEmailChange(u, email)
+ if password != "" {
+ h.endSessionsAfterPasswordSet(r, p, u)
+ }
if role != u.Role {
if err := h.store.SetUserRole(u.ID, role); err != nil {
@@ -335,6 +339,19 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
http.Redirect(w, r, "/server/users?done=updated", http.StatusSeeOther)
}
+// 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.
+func (h *Handlers) endSessionsAfterPasswordSet(r *http.Request, by auth.Principal, u store.User) {
+ if u.ID == by.ID {
+ if token, ok := h.auth.SessionToken(r); ok {
+ h.auth.DestroyOtherSessions(u.ID, token)
+ return
+ }
+ }
+ h.auth.DestroyUserSessions(u.ID)
+}
+
// 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).
diff --git a/internal/web/view/dmarc.go b/internal/web/view/dmarc.go
index b16eecf..fabcb96 100644
--- a/internal/web/view/dmarc.go
+++ b/internal/web/view/dmarc.go
@@ -30,11 +30,14 @@ func FormatReceived(t time.Time) string { return t.UTC().Format("01-02 15:04") }
// ---- The hub
// DMARCHub is the data of the DMARC reports hub: whether reports are arriving
-// and the latest of them.
+// and the latest of them. The ingest box describes the whole instance (the
+// hosted mailbox, the reports kept across every domain), so it is drawn only
+// for the viewer it belongs to — see WithoutIngest.
type DMARCHub struct {
Meta
Head Head
+ IngestOn bool
Ingest Box
IngestFacts []Fact
@@ -104,7 +107,8 @@ func NewDMARCHub(m Meta, in IngestInput) *DMARCHub {
Lead: Rich("How receivers saw mail claiming to be from your domains. Aggregate (", Code("rua="),
") only; open a report for the parsed XML."),
},
- Ingest: Box{No: "01", Title: "Ingest", End: Rich(TagOf(status, label))},
+ IngestOn: true,
+ Ingest: Box{No: "01", Title: "Ingest", End: Rich(TagOf(status, label))},
IngestFacts: []Fact{
last,
{Label: "This week", Value: Plain(FormatCount(int64(in.KeptThisWeek)) + " kept · " + plural(int64(in.ParseFailures), "parse failure", "parse failures"))},
@@ -116,6 +120,15 @@ func NewDMARCHub(m Meta, in IngestInput) *DMARCHub {
}
}
+// WithoutIngest leaves the ingest box out, for a viewer who reaches some of the
+// domains and not the instance: the box is about all of them, so what it says
+// is not theirs to read. The reports box is then the page's first and only one.
+func (p *DMARCHub) WithoutIngest() *DMARCHub {
+ p.IngestOn = false
+ p.Recent.No = "01"
+ return p
+}
+
// WithReports sets the table of the latest reports.
func (p *DMARCHub) WithReports(rows []DMARCReportRow) *DMARCHub {
p.Rows = rows
diff --git a/internal/web/view/static.go b/internal/web/view/static.go
index ba798b8..f919983 100644
--- a/internal/web/view/static.go
+++ b/internal/web/view/static.go
@@ -40,6 +40,9 @@ func buildStaticETags() map[string]string {
}
// StaticHandler serves the embedded assets under /static/ with a content ETag.
+// A path that is not one of those assets — the folder itself included — is a
+// 404: the file server would otherwise answer the folder with a listing of
+// everything the binary embeds.
//
// Cache-Control is no-cache rather than a max-age: it lets the browser keep the
// copy but requires it to revalidate, so an asset that changed is picked up on
@@ -50,10 +53,13 @@ func buildStaticETags() map[string]string {
func StaticHandler() http.Handler {
files := http.FileServer(http.FS(assetsFS))
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
- if etag, ok := staticETags[path.Clean(r.URL.Path)]; ok {
- w.Header().Set("ETag", etag)
- w.Header().Set("Cache-Control", "no-cache")
+ etag, ok := staticETags[path.Clean(r.URL.Path)]
+ if !ok {
+ http.NotFound(w, r)
+ return
}
+ w.Header().Set("ETag", etag)
+ w.Header().Set("Cache-Control", "no-cache")
files.ServeHTTP(w, r)
})
}
diff --git a/internal/web/view/static_test.go b/internal/web/view/static_test.go
index ed3cd8d..97fd8ec 100644
--- a/internal/web/view/static_test.go
+++ b/internal/web/view/static_test.go
@@ -106,3 +106,26 @@ func TestOFLTravelsWithFonts(t *testing.T) {
t.Error("GET /static/OFL.txt did not serve the OFL text")
}
}
+
+// /static/ is a set of known assets, not a directory to browse: the folder, and
+// a path that is not an asset, answer 404 rather than a listing of what the
+// binary embeds.
+func TestStaticDoesNotListOrServeWhatIsNotAnAsset(t *testing.T) {
+ for _, target := range []string{"/static/", "/static", "/static/nope", "/static/../static/", "/static/panel.css/../"} {
+ rec := serveStatic(target, nil)
+ if rec.Code != http.StatusNotFound {
+ t.Errorf("GET %s = %d, want 404:\n%s", target, rec.Code, rec.Body.String())
+ }
+ if strings.Contains(rec.Body.String(), "panel.css") {
+ t.Errorf("GET %s lists the assets:\n%s", target, rec.Body.String())
+ }
+ }
+
+ rec := serveStatic("/static/panel.css", nil)
+ if rec.Code != http.StatusOK {
+ t.Fatalf("GET /static/panel.css = %d, want 200", rec.Code)
+ }
+ if got, want := rec.Header().Get("ETag"), staticETags["/static/panel.css"]; got == "" || got != want {
+ t.Errorf("ETag of panel.css = %q, want %q", got, want)
+ }
+}
diff --git a/internal/web/view/templates/dmarc.html b/internal/web/view/templates/dmarc.html
index 52a5e9c..32d066d 100644
--- a/internal/web/view/templates/dmarc.html
+++ b/internal/web/view/templates/dmarc.html
@@ -1,13 +1,17 @@
{{/* DMARC reports (mockup dmarc.html): whether aggregate reports are arriving,
- and the latest of them across the domains the viewer reaches. */}}
+ and the latest of them across the domains the viewer reaches. The ingest box
+ is the instance's, so a viewer who reaches only some domains gets the
+ reports alone. */}}
{{define "content"}}
{{template "page_head" .Head}}
+{{if .IngestOn}}
{{template "box_open" .Ingest}}
{{template "body_open"}}
{{template "facts" .IngestFacts}}
{{template "body_close"}}
{{template "box_close"}}
+{{end}}
{{template "box_open" .Recent}}
{{if .Rows}}