From 3b86f6c3b65d83311c1a527376b65e55a63a3486 Mon Sep 17 00:00:00 2001 From: Mix Date: Fri, 9 Oct 2026 20:33:37 +0300 Subject: [PATCH] panel-redesign: inbound is delegated per user, separately from outbound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stage 1, step 2 (plan § Who sees what). Store - Reach{AllDomains, DomainIDs, AllInbound, InboundDomainIDs} is what a domain user is assigned. CreateUser takes it; SetUserReach / ClearUserReach replace SetUserDomains / ClearUserDomains and write both directions in one transaction. A domain user with nothing on either list is refused (ErrEmptyReach). With All set the rows of that direction are not stored, so clearing All later brings nothing stale back. - ListDomainsForUser honours all_domains; ListInboundDomainsForUser is its inbound twin. Both filter in the query. Authorization - auth.Principal carries the whole reach: CanAccessDomain (now also true with All), CanAccessInboundDomain, HasOutbound, HasInbound. An id or a name on one list opens nothing on the other. - Inbound handlers in three strengths: requireInbound (the list, filtered), requireInboundDomain (one domain: page, DNS re-check, upstream, recipients), requireInboundGlobal (add, delete confirm, delete). All answer the same 404. - domainIDSet and the DMARC report pages go through the principal instead of looking at p.Domains themselves. Panel (old templates, until stage 2) - User form: two lists with All; inbound is offered only where the feature is on, and saving the form with it off keeps the user's inbound reach. Promotion to global drops the reach. - Menu: a group with nothing assigned is absent; / sends an inbound-only administrator to Inbound. The inbound pages hide add / delete from a domain administrator. Tests: every route of one inbound domain against five kinds of refused request (another tenant's id, a missing id, outbound-only reach, an outbound domain with the same id, one with the same name, no principal) with the 404 bodies compared; add/delete as every non-global user; the filtered list including a domain added later; thirteen outbound routes against another tenant's domain and application id; the user form; the reach in the store; the principal matrix; the menu; the home redirect. Removing the access check or the query filter makes the tests fail (checked by hand). go test ./... green except the two Windows-only failures in internal/domain and internal/logtail that predate this work. Roadmap 12/27. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 + docs/plans/panel-redesign.md | 2 +- docs/roadmap.md | 2 +- internal/store/domains.go | 12 +- internal/store/domains_dmarc_test.go | 2 +- internal/store/inbound.go | 29 ++ internal/store/users.go | 141 ++++-- internal/store/users_test.go | 97 +++- internal/web/auth/principal.go | 41 +- internal/web/auth/principal_test.go | 52 +++ internal/web/handlers/authz.go | 7 +- internal/web/handlers/authz_test.go | 8 +- .../web/handlers/handlers_delegation_test.go | 427 ++++++++++++++++++ .../web/handlers/handlers_dmarc_reports.go | 10 +- internal/web/handlers/handlers_inbound.go | 85 +++- .../web/handlers/handlers_monitor_test.go | 4 +- .../web/handlers/handlers_settings_test.go | 2 +- internal/web/handlers/handlers_users.go | 110 +++-- internal/web/home_test.go | 29 ++ internal/web/view/templates/inbound.html | 6 +- .../web/view/templates/inbound_domain.html | 2 +- .../web/view/templates/layout_legacy.html | 12 +- internal/web/view/templates/user_form.html | 30 +- internal/web/view/templates/users.html | 7 +- internal/web/view/templates_test.go | 43 ++ internal/web/web.go | 6 + 26 files changed, 1043 insertions(+), 133 deletions(-) create mode 100644 internal/web/auth/principal_test.go create mode 100644 internal/web/handlers/handlers_delegation_test.go create mode 100644 internal/web/home_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 944c19f..214199c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,16 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version `user_inbound_domains` and the per-user `all_domains` / `all_inbound_domains` flags are in the schema for the inbound delegation that follows. Details: `docs/schema-migrations.md`. +- panel-redesign (stage 1): inbound domains are delegated to domain + administrators, separately from outbound. The user form has two lists, each + with *All* (which includes domains added later; with it ticked the rows + under it are ignored); a domain administrator needs at least one assignment + on either. Such a user sees the inbound domains assigned to them — the list + is filtered in the query — and can change their upstream and recipients; + adding and deleting an inbound domain stays with the global role. Another + tenant's domain, a missing one and a user with no inbound reach all get the + same 404. A menu group with nothing assigned is absent, and `/` leads an + inbound-only administrator to Inbound. ### Changed diff --git a/docs/plans/panel-redesign.md b/docs/plans/panel-redesign.md index ca209cd..fc35947 100644 --- a/docs/plans/panel-redesign.md +++ b/docs/plans/panel-redesign.md @@ -506,7 +506,7 @@ only when both are done. Every stage-2 step ends with the evidence of **Stage 1 — data and routes** (no template work; parallel with stage 0) - [x] Schema from zero: single `0001_init.sql` baseline with `users.email`, the DMARC default fields, `user_inbound_domains`, the two `all_*` flags; store tests; `schema-migrations.md` rewritten — **Opus** -- [ ] Inbound delegation: `requireInboundDomain`, lists and log filtered in the query, two-list user form data, per-route 404 tests for both roles including another tenant's id — **Opus** +- [x] Inbound delegation: `requireInboundDomain`, lists and log filtered in the query, two-list user form data, per-route 404 tests for both roles including another tenant's id — **Opus** - [ ] Routes renamed per § Routes: new paths, one fragment name, old paths removed, subtree `requireGlobal()` for `/server/`; links in the old templates retargeted so the panel works on every commit — **Opus** - [ ] The application form as one POST; `out-app-created` as the response with `Cache-Control: no-store` — **Opus** - [ ] Server › Health route and handler (tables and *Reload configuration* from `/status`); Overview handler reduced to verdicts — **Opus** diff --git a/docs/roadmap.md b/docs/roadmap.md index 0397dcc..30d57a7 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 | 11/27 | [plans/panel-redesign.md](plans/panel-redesign.md) | +| panel-redesign | Panel redesign (accepted mockups, design contract) | agreed | 12/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/internal/store/domains.go b/internal/store/domains.go index 29c5b8d..0e0821a 100644 --- a/internal/store/domains.go +++ b/internal/store/domains.go @@ -80,16 +80,18 @@ func (s *Store) ListDomains() ([]Domain, error) { return out, rows.Err() } -// ListDomainsForUser returns domains assigned to userID with application counts, -// ordered by name. +// ListDomainsForUser returns the sending domains a domain user reaches, with +// application counts, ordered by name: the assigned ones, or every domain when +// the user's All flag is set — which is how a domain added later is included. +// The filter is in the query, so a caller cannot forget it. func (s *Store) ListDomainsForUser(userID int64) ([]Domain, error) { rows, err := s.db.Query(` SELECT d.id, d.name, d.dkim_selector, d.dmarc_rua, d.dmarc_rua_user_id, d.created_at, (SELECT COUNT(*) FROM applications a WHERE a.domain_id = d.id) FROM domains d - INNER JOIN user_domains ud ON ud.domain_id = d.id - WHERE ud.user_id = ? - ORDER BY d.name`, userID) + WHERE EXISTS (SELECT 1 FROM users u WHERE u.id = ? AND u.all_domains = 1) + OR EXISTS (SELECT 1 FROM user_domains ud WHERE ud.user_id = ? AND ud.domain_id = d.id) + ORDER BY d.name`, userID, userID) if err != nil { return nil, fmt.Errorf("list domains for user: %w", err) } diff --git a/internal/store/domains_dmarc_test.go b/internal/store/domains_dmarc_test.go index 5c173aa..74a35e1 100644 --- a/internal/store/domains_dmarc_test.go +++ b/internal/store/domains_dmarc_test.go @@ -115,7 +115,7 @@ func TestDeletingTheFollowedUserLeavesNoReports(t *testing.T) { t.Fatalf("CreateGlobalUser: %v", err) } d, _ := st.AddDomain("example.com", "sel") - id, err := st.CreateUser("second", "hash", RoleGlobal, nil) + id, err := st.CreateUser("second", "hash", RoleGlobal, Reach{}) if err != nil { t.Fatalf("CreateUser: %v", err) } diff --git a/internal/store/inbound.go b/internal/store/inbound.go index b8ad492..f8ab44f 100644 --- a/internal/store/inbound.go +++ b/internal/store/inbound.go @@ -113,6 +113,35 @@ func (s *Store) ListInboundDomains() ([]InboundDomain, error) { return out, rows.Err() } +// ListInboundDomainsForUser returns the inbound domains a domain user reaches, +// ordered by name: the assigned ones, or every inbound domain when the user's +// All flag is set. Like ListDomainsForUser, the filter is in the query. +func (s *Store) ListInboundDomainsForUser(userID int64) ([]InboundDomain, error) { + rows, err := s.db.Query(` + SELECT d.id, d.name, d.recipient_mode, d.created_at, + t.host, t.port, t.tls_mode, + (SELECT COUNT(*) FROM inbound_recipients r WHERE r.inbound_domain_id = d.id) + FROM inbound_domains d + INNER JOIN inbound_transports t ON t.inbound_domain_id = d.id + WHERE EXISTS (SELECT 1 FROM users u WHERE u.id = ? AND u.all_inbound_domains = 1) + OR EXISTS (SELECT 1 FROM user_inbound_domains ud WHERE ud.user_id = ? AND ud.inbound_domain_id = d.id) + ORDER BY d.name`, userID, userID) + if err != nil { + return nil, fmt.Errorf("list inbound domains for user: %w", err) + } + defer rows.Close() + + var out []InboundDomain + for rows.Next() { + d, err := scanInboundDomain(rows) + if err != nil { + return nil, err + } + out = append(out, d) + } + return out, rows.Err() +} + // GetInboundDomain returns one inbound domain with its recipient list, or // ErrInboundDomainNotFound. func (s *Store) GetInboundDomain(id int64) (InboundDomain, error) { diff --git a/internal/store/users.go b/internal/store/users.go index 7579c94..898a597 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -196,10 +196,39 @@ func (s *Store) ListUsers() ([]User, error) { return users, rows.Err() } -// UserRow is a user plus assigned domain names for the management list. +// Reach is what a domain user is assigned: outbound and inbound domains are +// granted separately, and the same name on both lists is two assignments. All +// means every domain of that direction, including the ones added later; the +// id list beside it is then ignored. Reach widens a list, never the role: +// adding and deleting domains, the queues and Server stay with the global role. +type Reach struct { + AllDomains bool + DomainIDs []int64 + AllInbound bool + InboundDomainIDs []int64 +} + +// Empty reports whether the reach assigns nothing at all. A domain user must +// have something on at least one of the two lists. +func (r Reach) Empty() bool { + return !r.AllDomains && len(r.DomainIDs) == 0 && !r.AllInbound && len(r.InboundDomainIDs) == 0 +} + +// Reach returns what the user is assigned. +func (u User) Reach() Reach { + return Reach{AllDomains: u.AllDomains, DomainIDs: u.DomainIDs, AllInbound: u.AllInboundDomains, InboundDomainIDs: u.InboundDomainIDs} +} + +// ErrEmptyReach is returned when a domain user would be left with nothing +// assigned on either list. +var ErrEmptyReach = errors.New("a domain user needs at least one outbound or inbound domain") + +// UserRow is a user plus the names of the assigned domains for the management +// list. The names are empty where the user's All flag covers the direction. type UserRow struct { - User User - DomainNames []string + User User + DomainNames []string + InboundDomainNames []string } // ListUserRows returns users with assigned domain names for the management UI. @@ -214,11 +243,16 @@ func (s *Store) ListUserRows() ([]UserRow, error) { if u.Role == RoleGlobal { continue } - names, err := s.listUserDomainNames(u.ID) - if err != nil { - return nil, err + if !u.AllDomains { + if rows[i].DomainNames, err = s.listUserDomainNames(u.ID); err != nil { + return nil, err + } + } + if !u.AllInboundDomains { + if rows[i].InboundDomainNames, err = s.listUserInboundDomainNames(u.ID); err != nil { + return nil, err + } } - rows[i].DomainNames = names } return rows, nil } @@ -232,10 +266,12 @@ func (s *Store) CountGlobalUsers() (int, error) { return n, nil } -// CreateUser inserts a panel user and optional domain assignments. -func (s *Store) CreateUser(username, passwordHash string, role Role, domainIDs []int64) (int64, error) { - if role == RoleDomain && len(domainIDs) == 0 { - return 0, fmt.Errorf("create user: the domain role requires domains") +// CreateUser inserts a panel user. A domain user is created with its reach in +// the same call and must be given one (ErrEmptyReach); for a global user the +// reach is ignored. +func (s *Store) CreateUser(username, passwordHash string, role Role, reach Reach) (int64, error) { + if role == RoleDomain && reach.Empty() { + return 0, ErrEmptyReach } now := time.Now().UTC().Format(time.RFC3339) res, err := s.db.Exec( @@ -253,7 +289,7 @@ func (s *Store) CreateUser(username, passwordHash string, role Role, domainIDs [ return 0, fmt.Errorf("create user id: %w", err) } if role == RoleDomain { - if err := s.setUserDomains(id, domainIDs); err != nil { + if err := s.setUserReach(id, reach); err != nil { return 0, err } } @@ -339,28 +375,27 @@ func (s *Store) SetUserRole(userID int64, role Role) error { return nil } -// ClearUserDomains removes all domain assignments for a user. -func (s *Store) ClearUserDomains(userID int64) error { - _, err := s.db.Exec("DELETE FROM user_domains WHERE user_id = ?", userID) - if err != nil { - return fmt.Errorf("clear user domains: %w", err) - } - return nil +// ClearUserReach removes everything a user is assigned — both lists and both +// All flags. A user promoted to the global role keeps none of it, so a later +// demotion does not quietly bring an old assignment back. +func (s *Store) ClearUserReach(userID int64) error { + return s.setUserReach(userID, Reach{}) } -// SetUserDomains replaces the outbound domain assignments of a domain user. -func (s *Store) SetUserDomains(userID int64, domainIDs []int64) error { +// SetUserReach replaces what a domain user is assigned, both directions at +// once. ErrEmptyReach if nothing would be left. +func (s *Store) SetUserReach(userID int64, reach Reach) error { u, err := s.GetUser(userID) if err != nil { return err } if u.Role != RoleDomain { - return fmt.Errorf("set user domains: user does not have the domain role") + return fmt.Errorf("set user reach: user does not have the domain role") } - if len(domainIDs) == 0 { - return fmt.Errorf("set user domains: at least one domain required") + if reach.Empty() { + return ErrEmptyReach } - return s.setUserDomains(userID, domainIDs) + return s.setUserReach(userID, reach) } // DeleteUser removes a panel user. ErrLastGlobal when deleting the only global user. @@ -427,6 +462,26 @@ func (s *Store) listUserDomainIDs(userID int64) ([]int64, error) { return ids, rows.Err() } +func (s *Store) listUserInboundDomainNames(userID int64) ([]string, error) { + rows, err := s.db.Query( + "SELECT d.name FROM user_inbound_domains ud JOIN inbound_domains d ON d.id = ud.inbound_domain_id WHERE ud.user_id = ? ORDER BY d.name", + userID, + ) + if err != nil { + return nil, fmt.Errorf("list user inbound domain names: %w", err) + } + defer rows.Close() + var names []string + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + return nil, fmt.Errorf("list user inbound domain names scan: %w", err) + } + names = append(names, name) + } + return names, rows.Err() +} + func (s *Store) listUserDomainNames(userID int64) ([]string, error) { rows, err := s.db.Query( "SELECT d.name FROM user_domains ud JOIN domains d ON d.id = ud.domain_id WHERE ud.user_id = ? ORDER BY d.name", @@ -447,20 +502,40 @@ func (s *Store) listUserDomainNames(userID int64) ([]string, error) { return names, rows.Err() } -func (s *Store) setUserDomains(userID int64, domainIDs []int64) error { +// setUserReach writes the flags and both assignment tables in one transaction. +// With an All flag set the rows of that direction are dropped rather than +// stored: the flag already covers them, and stale rows would come back to life +// the day the flag is cleared. +func (s *Store) setUserReach(userID int64, reach Reach) error { tx, err := s.db.Begin() if err != nil { - return fmt.Errorf("set user domains begin: %w", err) + return fmt.Errorf("set user reach begin: %w", err) + } + defer tx.Rollback() + if _, err := tx.Exec("UPDATE users SET all_domains = ?, all_inbound_domains = ? WHERE id = ?", + reach.AllDomains, reach.AllInbound, userID); err != nil { + return fmt.Errorf("set user reach flags: %w", err) } if _, err := tx.Exec("DELETE FROM user_domains WHERE user_id = ?", userID); err != nil { - tx.Rollback() - return fmt.Errorf("set user domains clear: %w", err) + return fmt.Errorf("set user reach clear: %w", err) } - for _, did := range domainIDs { - if _, err := tx.Exec("INSERT INTO user_domains (user_id, domain_id) VALUES (?, ?)", userID, did); err != nil { - tx.Rollback() - return fmt.Errorf("set user domains insert: %w", err) + if _, err := tx.Exec("DELETE FROM user_inbound_domains WHERE user_id = ?", userID); err != nil { + return fmt.Errorf("set user reach clear inbound: %w", err) + } + if !reach.AllDomains { + for _, id := range reach.DomainIDs { + if _, err := tx.Exec("INSERT INTO user_domains (user_id, domain_id) VALUES (?, ?)", userID, id); err != nil { + return fmt.Errorf("set user reach insert: %w", err) + } + } + } + if !reach.AllInbound { + for _, id := range reach.InboundDomainIDs { + if _, err := tx.Exec("INSERT INTO user_inbound_domains (user_id, inbound_domain_id) VALUES (?, ?)", userID, id); err != nil { + return fmt.Errorf("set user reach insert inbound: %w", err) + } } } return tx.Commit() + } diff --git a/internal/store/users_test.go b/internal/store/users_test.go index cbadcf8..3e3acbf 100644 --- a/internal/store/users_test.go +++ b/internal/store/users_test.go @@ -74,7 +74,7 @@ func TestCreateDomainAdminUser(t *testing.T) { if err != nil { t.Fatalf("AddDomain: %v", err) } - id, err := st.CreateUser("domainop", "hash2", RoleDomain, []int64{d.ID}) + id, err := st.CreateUser("domainop", "hash2", RoleDomain, Reach{DomainIDs: []int64{d.ID}}) if err != nil { t.Fatalf("CreateUser: %v", err) } @@ -100,3 +100,98 @@ func TestDeleteLastGlobalUser(t *testing.T) { t.Fatalf("DeleteUser = %v, want ErrLastGlobal", err) } } + +// A domain user's reach covers two directions that are granted separately, +// each with an All flag; the store writes and reads all four parts together. +func TestUserReach(t *testing.T) { + st := openTestStore(t) + if err := st.CreateGlobalUser("admin", "hash"); err != nil { + t.Fatalf("CreateGlobalUser: %v", err) + } + out1, _ := st.AddDomain("one.example.com", "s1") + out2, _ := st.AddDomain("two.example.com", "s1") + in1, _ := st.AddInboundDomain("in-one.example.com") + in2, _ := st.AddInboundDomain("in-two.example.com") + + if _, err := st.CreateUser("nothing", "h", RoleDomain, Reach{}); !errors.Is(err, ErrEmptyReach) { + t.Fatalf("a domain user with nothing assigned = %v, want ErrEmptyReach", err) + } + if exists, _ := st.GetUserByUsername("nothing"); exists.ID != 0 { + t.Fatal("the refused user was created") + } + + // Inbound alone is enough: the role needs something on either list. + id, err := st.CreateUser("ops", "h", RoleDomain, Reach{InboundDomainIDs: []int64{in1.ID}}) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + u, _ := st.GetUser(id) + if len(u.DomainIDs) != 0 || len(u.InboundDomainIDs) != 1 || u.InboundDomainIDs[0] != in1.ID || u.AllDomains || u.AllInboundDomains { + t.Fatalf("reach = %+v", u.Reach()) + } + if got, _ := st.ListDomainsForUser(id); len(got) != 0 { + t.Errorf("an inbound assignment gave %d sending domains", len(got)) + } + if got, _ := st.ListInboundDomainsForUser(id); len(got) != 1 || got[0].ID != in1.ID { + t.Errorf("inbound domains for the user = %+v", got) + } + + // All replaces the rows of its direction and leaves the other alone. + if err := st.SetUserReach(id, Reach{AllDomains: true, DomainIDs: []int64{out1.ID}, InboundDomainIDs: []int64{in1.ID, in2.ID}}); err != nil { + t.Fatalf("SetUserReach: %v", err) + } + u, _ = st.GetUser(id) + if !u.AllDomains || len(u.DomainIDs) != 0 || len(u.InboundDomainIDs) != 2 { + t.Fatalf("reach after All = %+v; rows under All must not be stored", u.Reach()) + } + // ...and All includes a domain added afterwards. + out3, _ := st.AddDomain("three.example.com", "s1") + got, err := st.ListDomainsForUser(id) + if err != nil || len(got) != 3 { + t.Fatalf("All lists %d sending domains (%v), want 3 including the one added later", len(got), err) + } + _ = out2 + _ = out3 + + // Clearing All does not bring an old row back. + if err := st.SetUserReach(id, Reach{DomainIDs: []int64{out2.ID}, AllInbound: true}); err != nil { + t.Fatalf("SetUserReach: %v", err) + } + u, _ = st.GetUser(id) + if u.AllDomains || len(u.DomainIDs) != 1 || u.DomainIDs[0] != out2.ID || !u.AllInboundDomains || len(u.InboundDomainIDs) != 0 { + t.Fatalf("reach = %+v", u.Reach()) + } + if got, _ := st.ListDomainsForUser(id); len(got) != 1 || got[0].ID != out2.ID { + t.Errorf("sending domains for the user = %+v", got) + } + if got, _ := st.ListInboundDomainsForUser(id); len(got) != 2 { + t.Errorf("All inbound lists %d domains, want 2", len(got)) + } + + rows, err := st.ListUserRows() + if err != nil { + t.Fatalf("ListUserRows: %v", err) + } + for _, r := range rows { + if r.User.ID != id { + continue + } + if len(r.DomainNames) != 1 || r.DomainNames[0] != "two.example.com" || len(r.InboundDomainNames) != 0 || !r.User.AllInboundDomains { + t.Errorf("list row = %+v", r) + } + } + + if err := st.SetUserReach(id, Reach{}); !errors.Is(err, ErrEmptyReach) { + t.Errorf("emptying a domain user's reach = %v, want ErrEmptyReach", err) + } + admin, _ := st.GetUserByUsername("admin") + if err := st.SetUserReach(admin.ID, Reach{AllDomains: true}); err == nil { + t.Error("a reach was set on a global user") + } + if err := st.ClearUserReach(id); err != nil { + t.Fatalf("ClearUserReach: %v", err) + } + if u, _ = st.GetUser(id); !u.Reach().Empty() { + t.Errorf("reach after clearing = %+v", u.Reach()) + } +} diff --git a/internal/web/auth/principal.go b/internal/web/auth/principal.go index 24989ab..ce4f5e2 100644 --- a/internal/web/auth/principal.go +++ b/internal/web/auth/principal.go @@ -27,7 +27,12 @@ type Principal struct { ID int64 Username string Role Role - Domains []int64 // assigned domain IDs; empty for global (all domains) + // What a domain user reaches (store.Reach). All four are unused for the + // global role, which reaches everything. + Domains []int64 // assigned outbound domain ids + AllDomains bool // every outbound domain, including those added later + InboundDomains []int64 // assigned inbound domain ids + AllInbound bool // every inbound domain, including those added later } // IsGlobal reports whether the principal has full panel access. @@ -35,9 +40,35 @@ func (p Principal) IsGlobal() bool { return p.Role == RoleGlobal } -// CanAccessDomain reports whether the principal may access a domain id. +// HasOutbound reports whether the principal reaches any sending domain. A +// group with nothing in it is absent from that user's menu. +func (p Principal) HasOutbound() bool { + return p.IsGlobal() || p.AllDomains || len(p.Domains) > 0 +} + +// HasInbound reports whether the principal reaches any inbound domain. +func (p Principal) HasInbound() bool { + return p.IsGlobal() || p.AllInbound || len(p.InboundDomains) > 0 +} + +// CanAccessInboundDomain reports whether the principal may access an inbound +// domain id. Inbound is granted separately from outbound: a sending domain of +// the same name gives no access here, and the other way round. +func (p Principal) CanAccessInboundDomain(inboundDomainID int64) bool { + if p.IsGlobal() || p.AllInbound { + return true + } + for _, id := range p.InboundDomains { + if id == inboundDomainID { + return true + } + } + return false +} + +// CanAccessDomain reports whether the principal may access a sending domain id. func (p Principal) CanAccessDomain(domainID int64) bool { - if p.IsGlobal() { + if p.IsGlobal() || p.AllDomains { return true } for _, id := range p.Domains { @@ -59,6 +90,10 @@ func principalFromUser(u store.User) Principal { Username: u.Username, Role: u.Role, Domains: u.DomainIDs, + + AllDomains: u.AllDomains, + InboundDomains: u.InboundDomainIDs, + AllInbound: u.AllInboundDomains, } } diff --git a/internal/web/auth/principal_test.go b/internal/web/auth/principal_test.go new file mode 100644 index 0000000..fab7b1b --- /dev/null +++ b/internal/web/auth/principal_test.go @@ -0,0 +1,52 @@ +package auth + +import ( + "testing" + + "github.com/mixeme/selfpost/internal/store" +) + +// Outbound and inbound are granted separately: an id on one list opens nothing +// on the other, All opens a whole direction, and the global role opens both. +func TestPrincipalReach(t *testing.T) { + for _, tc := range []struct { + name string + p Principal + out7, out8, in7, in8 bool + hasOutbound, hasInbound bool + }{ + {"global", Principal{Role: RoleGlobal}, true, true, true, true, true, true}, + {"outbound 7", Principal{Role: RoleDomain, Domains: []int64{7}}, true, false, false, false, true, false}, + {"inbound 7", Principal{Role: RoleDomain, InboundDomains: []int64{7}}, false, false, true, false, false, true}, + {"all outbound", Principal{Role: RoleDomain, AllDomains: true}, true, true, false, false, true, false}, + {"all inbound, outbound 8", Principal{Role: RoleDomain, AllInbound: true, Domains: []int64{8}}, false, true, true, true, true, true}, + {"nothing", Principal{Role: RoleDomain}, false, false, false, false, false, false}, + } { + got := []bool{ + tc.p.CanAccessDomain(7), tc.p.CanAccessDomain(8), tc.p.CanAccessInboundDomain(7), tc.p.CanAccessInboundDomain(8), + tc.p.HasOutbound(), tc.p.HasInbound(), + } + want := []bool{tc.out7, tc.out8, tc.in7, tc.in8, tc.hasOutbound, tc.hasInbound} + for i := range got { + if got[i] != want[i] { + t.Errorf("%s: [out 7, out 8, in 7, in 8, has out, has in] = %v, want %v", tc.name, got, want) + break + } + } + } + if !(Principal{Role: RoleDomain, Domains: []int64{7}}).CanAccessApp(store.Application{DomainID: 7}) { + t.Error("an application follows its sending domain") + } +} + +// The principal a request carries is the user's whole reach, flags included. +func TestPrincipalFromUser(t *testing.T) { + p := principalFromUser(store.User{ + ID: 3, Username: "ops", Role: store.RoleDomain, + DomainIDs: []int64{1}, AllDomains: true, InboundDomainIDs: []int64{2, 4}, AllInboundDomains: true, + }) + if p.ID != 3 || p.Username != "ops" || p.Role != RoleDomain || !p.AllDomains || !p.AllInbound || + len(p.Domains) != 1 || len(p.InboundDomains) != 2 { + t.Errorf("principal = %+v", p) + } +} diff --git a/internal/web/handlers/authz.go b/internal/web/handlers/authz.go index 6da1b92..4a3bfa6 100644 --- a/internal/web/handlers/authz.go +++ b/internal/web/handlers/authz.go @@ -26,6 +26,9 @@ func (h *Handlers) pageBase(r *http.Request) map[string]any { return map[string]any{ "User": auth.CurrentUser(r), "IsGlobal": p.IsGlobal(), + // Which groups the menu shows this user (plan § Who sees what). + "HasOutbound": p.HasOutbound(), + "HasInbound": p.HasInbound(), } } @@ -60,8 +63,10 @@ func domainNameSet(domains []store.Domain) map[string]bool { return m } +// domainIDSet is the set of sending domains a principal is limited to; nil +// means no limit (the global role, or a domain user with All). func domainIDSet(p auth.Principal) map[int64]bool { - if p.IsGlobal() { + if p.IsGlobal() || p.AllDomains { return nil } m := make(map[int64]bool, len(p.Domains)) diff --git a/internal/web/handlers/authz_test.go b/internal/web/handlers/authz_test.go index 6dfe8d7..7a96172 100644 --- a/internal/web/handlers/authz_test.go +++ b/internal/web/handlers/authz_test.go @@ -52,12 +52,10 @@ var globalOnlyRoutes = []route{ {"POST", "/domains/1/delete", func(h *Handlers) http.HandlerFunc { return h.HandleDeleteDomain }, map[string]string{"id": "1"}}, {"POST", "/reload", func(h *Handlers) http.HandlerFunc { return h.HandleReload }, nil}, - {"GET", "/inbound", func(h *Handlers) http.HandlerFunc { return h.HandleInboundList }, nil}, + // Inbound: only adding and deleting a domain are global. The list and the + // pages of one inbound domain are delegated per domain and have their own + // table in handlers_delegation_test.go. {"POST", "/inbound", func(h *Handlers) http.HandlerFunc { return h.HandleAddInbound }, nil}, - {"GET", "/inbound/1", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDetail }, map[string]string{"id": "1"}}, - {"POST", "/inbound/1/dns-recheck", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDNSRecheck }, map[string]string{"id": "1"}}, - {"POST", "/inbound/1/upstream", func(h *Handlers) http.HandlerFunc { return h.HandleInboundTransport }, map[string]string{"id": "1"}}, - {"POST", "/inbound/1/recipients", func(h *Handlers) http.HandlerFunc { return h.HandleInboundRecipients }, map[string]string{"id": "1"}}, {"GET", "/inbound/1/delete", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDeleteConfirm }, map[string]string{"id": "1"}}, {"POST", "/inbound/1/delete", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDelete }, map[string]string{"id": "1"}}, } diff --git a/internal/web/handlers/handlers_delegation_test.go b/internal/web/handlers/handlers_delegation_test.go new file mode 100644 index 0000000..e25d9b4 --- /dev/null +++ b/internal/web/handlers/handlers_delegation_test.go @@ -0,0 +1,427 @@ +package handlers + +import ( + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "strings" + "testing" + + "github.com/mixeme/selfpost/internal/app" + "github.com/mixeme/selfpost/internal/domain" + "github.com/mixeme/selfpost/internal/store" + "github.com/mixeme/selfpost/internal/web/auth" +) + +// userWith creates a panel user with the given role and reach and returns the +// principal the auth middleware would attach for them — read back from the +// store, so the test exercises the same mapping a real request gets. +func userWith(t *testing.T, st *store.Store, username string, role store.Role, reach store.Reach) auth.Principal { + t.Helper() + id, err := st.CreateUser(username, "test-hash", role, reach) + if err != nil { + t.Fatalf("create user %s: %v", username, err) + } + u, err := st.GetUser(id) + if err != nil { + t.Fatalf("get user %s: %v", username, err) + } + return auth.Principal{ + ID: u.ID, Username: u.Username, Role: u.Role, + Domains: u.DomainIDs, AllDomains: u.AllDomains, + InboundDomains: u.InboundDomainIDs, AllInbound: u.AllInboundDomains, + } +} + +func idStr(id int64) string { return strconv.FormatInt(id, 10) } + +// send runs one request against a handler as p (nil: no principal at all). +func send(h http.HandlerFunc, p *auth.Principal, method, target string, pathValues map[string]string, form url.Values) *httptest.ResponseRecorder { + var req *http.Request + if form != nil { + req = httptest.NewRequest(method, target, strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + } else { + req = httptest.NewRequest(method, target, nil) + } + for k, v := range pathValues { + req.SetPathValue(k, v) + } + if p != nil { + req = auth.RequestWithPrincipal(req, *p) + } + rec := httptest.NewRecorder() + h(rec, req) + return rec +} + +// inboundRoute is one inbound handler taking an {id}. +type inboundRoute struct { + method, suffix string + handler func(*Handlers) http.HandlerFunc + form url.Values +} + +// The pages and actions of ONE inbound domain: open to the global role and to a +// domain administrator this inbound domain is assigned to. +var inboundDomainRoutes = []inboundRoute{ + {"GET", "", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDetail }, nil}, + {"POST", "/upstream", func(h *Handlers) http.HandlerFunc { return h.HandleInboundTransport }, + url.Values{"host": {"mx.internal.example"}, "port": {"25"}, "tls_mode": {"may"}}}, + {"POST", "/recipients", func(h *Handlers) http.HandlerFunc { return h.HandleInboundRecipients }, + url.Values{"recipient_mode": {"any"}}}, + {"POST", "/dns-recheck", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDNSRecheck }, nil}, +} + +// Adding and deleting an inbound domain: the global role only, whatever a +// domain administrator is assigned. +var inboundGlobalRoutes = []inboundRoute{ + {"GET", "/delete", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDeleteConfirm }, nil}, + {"POST", "/delete", func(h *Handlers) http.HandlerFunc { return h.HandleInboundDelete }, nil}, +} + +type inboundStand struct { + h *Handlers + a, b store.InboundDomain + global, ownsA, allIn auth.Principal + outboundOnly, sameNameAs auth.Principal +} + +// newInboundStand is a panel with two inbound domains and one user of each kind +// the delegation has to tell apart. +func newInboundStand(t *testing.T) inboundStand { + t.Helper() + h, st := inboundHandlers(t) + s := inboundStand{h: h} + var err error + if s.a, err = st.AddInboundDomain("a.example.com"); err != nil { + t.Fatal(err) + } + if s.b, err = st.AddInboundDomain("b.example.com"); err != nil { + t.Fatal(err) + } + // A SENDING domain with the same name as inbound domain B, at an id that + // collides with inbound A's: outbound and inbound are separate entities, + // and neither the name nor the number may leak access across. + out, err := st.AddDomain("b.example.com", "mail") + if err != nil { + t.Fatal(err) + } + if out.ID != s.a.ID { + t.Fatalf("the stand expects the outbound domain's id (%d) to collide with inbound A's (%d)", out.ID, s.a.ID) + } + s.global = userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + s.ownsA = userWith(t, st, "owns-a", store.RoleDomain, store.Reach{InboundDomainIDs: []int64{s.a.ID}}) + s.allIn = userWith(t, st, "all-inbound", store.RoleDomain, store.Reach{AllInbound: true}) + s.outboundOnly = userWith(t, st, "outbound-only", store.RoleDomain, store.Reach{AllDomains: true}) + s.sameNameAs = userWith(t, st, "same-name", store.RoleDomain, store.Reach{DomainIDs: []int64{out.ID}}) + return s +} + +func (s inboundStand) call(rt inboundRoute, p *auth.Principal, id int64) *httptest.ResponseRecorder { + return send(rt.handler(s.h), p, rt.method, "/inbound/"+idStr(id)+rt.suffix, map[string]string{"id": idStr(id)}, rt.form) +} + +// Every route of one inbound domain, for every kind of user: 404 for another +// tenant's domain, for a domain that does not exist, for a user with no inbound +// reach, for an outbound assignment of the same id or name, and with no +// principal — and the same answer in each case, so nothing tells them apart. +func TestInboundDomainRoutesAreDelegatedPerDomain(t *testing.T) { + s := newInboundStand(t) + const missing = 9999 + for _, rt := range inboundDomainRoutes { + name := rt.method + " /inbound/{id}" + rt.suffix + denied := []struct { + who string + p *auth.Principal + id int64 + }{ + {"a domain administrator of inbound A, on B", &s.ownsA, s.b.ID}, + {"a domain administrator of inbound A, on a missing id", &s.ownsA, missing}, + {"a domain administrator with outbound reach only", &s.outboundOnly, s.a.ID}, + {"a domain administrator of the outbound domain with the same id", &s.sameNameAs, s.a.ID}, + {"a domain administrator of the outbound domain with the same name", &s.sameNameAs, s.b.ID}, + {"no principal", nil, s.a.ID}, + } + var bodies []string + for _, c := range denied { + rec := s.call(rt, c.p, c.id) + if rec.Code != http.StatusNotFound { + t.Errorf("%s as %s = %d, want 404:\n%s", name, c.who, rec.Code, rec.Body.String()) + } + bodies = append(bodies, rec.Body.String()) + } + for _, b := range bodies[1:] { + if b != bodies[0] { + t.Errorf("%s: the 404 for another tenant's domain differs from the 404 for a missing one — the panel confirms what exists", name) + break + } + } + + // The DNS re-check resolves real names, so its allowed side is not + // run here; its guard is the same call as the three above. + if rt.suffix == "/dns-recheck" { + continue + } + for _, c := range []struct { + who string + p *auth.Principal + id int64 + }{ + {"the global role", &s.global, s.b.ID}, + {"its domain administrator", &s.ownsA, s.a.ID}, + {"a domain administrator with All inbound", &s.allIn, s.b.ID}, + } { + rec := s.call(rt, c.p, c.id) + if rec.Code != http.StatusOK && rec.Code != http.StatusSeeOther { + t.Errorf("%s as %s = %d, want it to go through:\n%s", name, c.who, rec.Code, rec.Body.String()) + } + } + } +} + +func TestAddingAndDeletingInboundDomainsStaysGlobal(t *testing.T) { + s := newInboundStand(t) + for _, p := range []*auth.Principal{&s.ownsA, &s.allIn, &s.outboundOnly, nil} { + for _, rt := range inboundGlobalRoutes { + if rec := s.call(rt, p, s.a.ID); rec.Code != http.StatusNotFound { + t.Errorf("%s /inbound/{id}%s as %+v = %d, want 404", rt.method, rt.suffix, p, rec.Code) + } + } + rec := send(s.h.HandleAddInbound, p, "POST", "/inbound", nil, url.Values{"name": {"new.example.com"}}) + if rec.Code != http.StatusNotFound { + t.Errorf("POST /inbound as %+v = %d, want 404", p, rec.Code) + } + } + list, err := s.h.store.ListInboundDomains() + if err != nil || len(list) != 2 { + t.Fatalf("after the refused requests there are %d inbound domains (%v), want the original 2", len(list), err) + } + + // The global role still can. + if rec := send(s.h.HandleAddInbound, &s.global, "POST", "/inbound", nil, url.Values{"name": {"new.example.com"}}); rec.Code != http.StatusSeeOther { + t.Errorf("POST /inbound as the global role = %d:\n%s", rec.Code, rec.Body.String()) + } + if rec := s.call(inboundGlobalRoutes[1], &s.global, s.a.ID); rec.Code != http.StatusSeeOther { + t.Errorf("POST /inbound/{id}/delete as the global role = %d:\n%s", rec.Code, rec.Body.String()) + } +} + +// The list is filtered in the query: a domain administrator sees the inbound +// domains assigned to them and nothing that lets them add or delete one. +func TestInboundListShowsOnlyTheAssignedDomains(t *testing.T) { + s := newInboundStand(t) + list := func(p *auth.Principal) *httptest.ResponseRecorder { + return send(s.h.HandleInboundList, p, "GET", "/inbound", nil, nil) + } + + rec := list(&s.ownsA) + if rec.Code != http.StatusOK { + t.Fatalf("list as the administrator of A = %d:\n%s", rec.Code, rec.Body.String()) + } + body := rec.Body.String() + if !strings.Contains(body, "a.example.com") || strings.Contains(body, "b.example.com") { + t.Errorf("the administrator of A must see A and not B:\n%s", body) + } + for _, globalOnly := range []string{"Add inbound domain", "/delete"} { + if strings.Contains(body, globalOnly) { + t.Errorf("the list offers a domain administrator %q", globalOnly) + } + } + + // All covers a domain added after the user was given it. + if _, err := s.h.store.AddInboundDomain("later.example.com"); err != nil { + t.Fatal(err) + } + body = list(&s.allIn).Body.String() + for _, want := range []string{"a.example.com", "b.example.com", "later.example.com"} { + if !strings.Contains(body, want) { + t.Errorf("All inbound does not list %s", want) + } + } + if strings.Contains(list(&s.ownsA).Body.String(), "later.example.com") { + t.Error("a domain added later appeared for a user who was assigned A only") + } + + for who, p := range map[string]*auth.Principal{"outbound reach only": &s.outboundOnly, "no principal": nil} { + if rec := list(p); rec.Code != http.StatusNotFound { + t.Errorf("list with %s = %d, want 404", who, rec.Code) + } + } + if body := list(&s.global).Body.String(); !strings.Contains(body, "Add inbound domain") || !strings.Contains(body, "b.example.com") { + t.Errorf("the global role lost the full list or the add form:\n%s", body) + } +} + +// Losing the last assigned inbound domain (it was deleted) takes the group away +// as a whole: the list answers 404, like for a user who never had inbound. +func TestInboundListWithNothingLeftIs404(t *testing.T) { + s := newInboundStand(t) + if err := s.h.store.DeleteInboundDomain(s.a.ID); err != nil { + t.Fatal(err) + } + u, err := s.h.store.GetUser(s.ownsA.ID) + if err != nil { + t.Fatal(err) + } + p := auth.Principal{ID: u.ID, Username: u.Username, Role: u.Role, InboundDomains: u.InboundDomainIDs} + if rec := send(s.h.HandleInboundList, &p, "GET", "/inbound", nil, nil); rec.Code != http.StatusNotFound { + t.Errorf("list after the only assigned inbound domain was deleted = %d, want 404", rec.Code) + } +} + +// The outbound side of the same rule, route by route: a domain administrator +// asking for another tenant's sending domain or application by id gets 404, +// and so does a user whose reach is inbound only. +func TestOutboundRoutesAnswerAnotherTenant404(t *testing.T) { + h, domains := serverWithTwoDomains(t) + h.domains = domain.NewService(h.store, nil, nil, "mail") + h.apps = app.NewService(h.store, nil, nil) + first, second := domains["first.example.ru"], domains["second.example.ru"] + foreignApp, err := h.store.GetApplicationByLogin("second-app") + if err != nil { + t.Fatal(err) + } + in, err := h.store.AddInboundDomain("second.example.ru") + if err != nil { + t.Fatal(err) + } + ownsFirst := userWith(t, h.store, "owns-first", store.RoleDomain, store.Reach{DomainIDs: []int64{first.ID}}) + inboundOnly := userWith(t, h.store, "inbound-only", store.RoleDomain, store.Reach{InboundDomainIDs: []int64{in.ID}}) + + did := map[string]string{"id": idStr(second.ID)} + aid := map[string]string{"aid": idStr(foreignApp.ID)} + routes := []struct { + method, target string + handler http.HandlerFunc + pathValues map[string]string + }{ + {"GET", "/domains/{id}", h.HandleDomainDetail, did}, + {"POST", "/domains/{id}/dns-recheck", h.HandleDomainDNSRecheck, did}, + {"POST", "/domains/{id}/applications", h.HandleAddApplication, did}, + {"POST", "/domains/{id}/ratelimit", h.HandleDomainRateLimit, did}, + {"POST", "/domains/{id}/ratelimit/recalc", h.HandleDomainRateLimitRecalc, did}, + {"POST", "/domains/{id}/dmarc", h.HandleDomainDMARC, did}, + {"POST", "/domains/{id}/export", h.HandleExportDomain, did}, + {"POST", "/applications/{aid}/mode", h.HandleUpdateAppMode, aid}, + {"POST", "/applications/{aid}/authips", h.HandleAppAuthIPs, aid}, + {"POST", "/applications/{aid}/password", h.HandleRegenPassword, aid}, + {"POST", "/applications/{aid}/ratelimit", h.HandleAppRateLimit, aid}, + {"POST", "/applications/{aid}/ratelimit/recalc", h.HandleAppRateLimitRecalc, aid}, + {"POST", "/applications/{aid}/delete", h.HandleDeleteApplication, aid}, + } + for _, rt := range routes { + for who, p := range map[string]*auth.Principal{ + "the administrator of another sending domain": &ownsFirst, + "an inbound-only administrator of the same name": &inboundOnly, + "no principal": nil, + } { + rec := send(rt.handler, p, rt.method, rt.target, rt.pathValues, url.Values{}) + if rec.Code != http.StatusNotFound { + t.Errorf("%s %s on another tenant's id as %s = %d, want 404:\n%s", rt.method, rt.target, who, rec.Code, rec.Body.String()) + } + } + } + + // Nothing was changed by the refused requests. + if _, err := h.store.GetApplicationByLogin("second-app"); err != nil { + t.Errorf("the foreign application did not survive the refused requests: %v", err) + } + apps, err := h.store.ListApplicationsByDomain(second.ID) + if err != nil || len(apps) != 1 { + t.Errorf("the foreign domain has %d applications after the refused requests (%v), want 1", len(apps), err) + } +} + +// The user form carries two lists. A domain administrator needs something on +// at least one of them; All wins over the rows under it; promoting to the +// global role drops the reach. +func TestUserFormAssignsBothDirections(t *testing.T) { + h, st := inboundHandlers(t) + out, _ := st.AddDomain("out.example.com", "mail") + in, _ := st.AddInboundDomain("in.example.com") + root := userWith(t, st, "root", store.RoleGlobal, store.Reach{}) + const password = "a-long-enough-password-1" + + create := func(username string, extra url.Values) (*httptest.ResponseRecorder, store.User) { + t.Helper() + form := url.Values{"username": {username}, "password": {password}, "role": {"domain"}} + for k, v := range extra { + form[k] = v + } + rec := send(h.HandleUserNew, &root, "POST", "/users/new", nil, form) + u, _ := st.GetUserByUsername(username) + return rec, u + } + + rec, u := create("nothing", nil) + if rec.Code != http.StatusBadRequest || u.ID != 0 { + t.Errorf("a domain administrator with nothing assigned = %d, user created: %v", rec.Code, u.ID != 0) + } + + rec, u = create("inbound-only", url.Values{"inbound_domain_ids": {idStr(in.ID)}}) + if rec.Code != http.StatusSeeOther || len(u.InboundDomainIDs) != 1 || len(u.DomainIDs) != 0 || u.AllDomains || u.AllInboundDomains { + t.Errorf("inbound only = %d, %+v:\n%s", rec.Code, u, rec.Body.String()) + } + + // All ticked together with rows: the rows are ignored, not stored. + rec, u = create("all-out", url.Values{"all_domains": {"1"}, "domain_ids": {idStr(out.ID)}}) + if rec.Code != http.StatusSeeOther || !u.AllDomains || len(u.DomainIDs) != 0 { + t.Errorf("All outbound = %d, %+v", rec.Code, u) + } + + update := func(u store.User, form url.Values) store.User { + t.Helper() + form.Set("username", u.Username) + rec := send(h.HandleUserEdit, &root, "POST", "/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)}, form) + if rec.Code != http.StatusSeeOther { + t.Fatalf("update %s = %d:\n%s", u.Username, rec.Code, rec.Body.String()) + } + got, err := st.GetUser(u.ID) + if err != nil { + t.Fatal(err) + } + return got + } + + // Both lists at once; then unticking All brings no stale row back. + u = update(u, url.Values{"role": {"domain"}, "domain_ids": {idStr(out.ID)}, "all_inbound_domains": {"1"}}) + if u.AllDomains || len(u.DomainIDs) != 1 || !u.AllInboundDomains || len(u.InboundDomainIDs) != 0 { + t.Errorf("after the update: %+v", u) + } + + // With inbound switched off the form has no inbound list; saving it must + // not wipe what the user had there. + h.cfg.InboundEnabled = false + u = update(u, url.Values{"role": {"domain"}, "domain_ids": {idStr(out.ID)}}) + if !u.AllInboundDomains { + t.Errorf("saving the form with inbound off dropped the user's inbound reach: %+v", u) + } + h.cfg.InboundEnabled = true + + // Emptying both lists is refused and changes nothing. + rec = send(h.HandleUserEdit, &root, "POST", "/users/"+idStr(u.ID), map[string]string{"uid": idStr(u.ID)}, + url.Values{"username": {u.Username}, "role": {"domain"}}) + if after, _ := st.GetUser(u.ID); rec.Code != http.StatusBadRequest || after.Reach().Empty() { + t.Errorf("emptying both lists = %d, reach now %+v", rec.Code, after.Reach()) + } + + // Promotion drops the reach, so a later demotion starts from nothing. + u = update(u, url.Values{"role": {"global"}}) + if u.Role != store.RoleGlobal || !u.Reach().Empty() { + t.Errorf("after promotion: role %s, reach %+v", u.Role, u.Reach()) + } + + // The form shows both lists with what is ticked. + other, _ := st.GetUserByUsername("inbound-only") + body := send(h.HandleUserEdit, &root, "GET", "/users/"+idStr(other.ID), map[string]string{"uid": idStr(other.ID)}, nil).Body.String() + for _, want := range []string{ + `name="all_domains"`, `name="all_inbound_domains"`, "out.example.com", + `name="inbound_domain_ids" value="` + idStr(in.ID) + `" checked`, + } { + if !strings.Contains(body, want) { + t.Errorf("the user form is missing %q", want) + } + } +} diff --git a/internal/web/handlers/handlers_dmarc_reports.go b/internal/web/handlers/handlers_dmarc_reports.go index 6b35cd7..c57396f 100644 --- a/internal/web/handlers/handlers_dmarc_reports.go +++ b/internal/web/handlers/handlers_dmarc_reports.go @@ -33,15 +33,7 @@ func (h *Handlers) requireDMARC(w http.ResponseWriter, r *http.Request) (auth.Pr } func (h *Handlers) canViewDMARCDomain(p auth.Principal, d store.Domain) bool { - if p.IsGlobal() { - return true - } - for _, id := range p.Domains { - if id == d.ID { - return true - } - } - return false + return p.CanAccessDomain(d.ID) } // HandleDMARCList is the global DMARC reports index. diff --git a/internal/web/handlers/handlers_inbound.go b/internal/web/handlers/handlers_inbound.go index 5a8d873..e3d80d3 100644 --- a/internal/web/handlers/handlers_inbound.go +++ b/internal/web/handlers/handlers_inbound.go @@ -23,7 +23,33 @@ type inboundRow struct { RcptLabel string } +// Inbound is delegated to domain administrators separately from outbound (plan +// § Who sees what), so its handlers come in three strengths: +// +// - requireInbound the list: anyone who reaches at least one inbound +// domain — and the list itself is filtered; +// - requireInboundDomain one domain (its page, DNS re-check, upstream, +// recipients): the global role, or a domain user +// this very inbound domain is assigned to; +// - requireInboundGlobal adding and deleting a domain: the global role only. +// +// All three answer 404 — for a missing domain, for another tenant's, and for a +// user with no inbound reach alike — so the panel never confirms what exists. + func (h *Handlers) requireInbound(w http.ResponseWriter, r *http.Request) (auth.Principal, bool) { + if !h.cfg.InboundEnabled || h.inbound == nil { + http.NotFound(w, r) + return auth.Principal{}, false + } + p, ok := h.principal(r) + if !ok || !p.HasInbound() { + http.NotFound(w, r) + return auth.Principal{}, false + } + return p, true +} + +func (h *Handlers) requireInboundGlobal(w http.ResponseWriter, r *http.Request) (auth.Principal, bool) { if !h.cfg.InboundEnabled || h.inbound == nil { http.NotFound(w, r) return auth.Principal{}, false @@ -31,6 +57,33 @@ func (h *Handlers) requireInbound(w http.ResponseWriter, r *http.Request) (auth. return h.requireGlobal(w, r) } +// requireInboundDomain resolves the {id} of the request to an inbound domain +// the principal may work with. +func (h *Handlers) requireInboundDomain(w http.ResponseWriter, r *http.Request) (store.InboundDomain, bool) { + p, ok := h.requireInbound(w, r) + if !ok { + return store.InboundDomain{}, false + } + d, ok := h.lookupInbound(w, r) + if !ok { + return store.InboundDomain{}, false + } + if !p.CanAccessInboundDomain(d.ID) { + http.NotFound(w, r) + return store.InboundDomain{}, false + } + return d, true +} + +// assignedInboundDomains lists the inbound domains a principal reaches. The +// filter is in the query (ListInboundDomainsForUser), not in the template. +func (h *Handlers) assignedInboundDomains(p auth.Principal) ([]store.InboundDomain, error) { + if p.IsGlobal() { + return h.inbound.List() + } + return h.store.ListInboundDomainsForUser(p.ID) +} + func (h *Handlers) lookupInbound(w http.ResponseWriter, r *http.Request) (store.InboundDomain, bool) { if h.inbound == nil { http.NotFound(w, r) @@ -54,7 +107,8 @@ func (h *Handlers) lookupInbound(w http.ResponseWriter, r *http.Request) (store. return d, true } -// HandleInboundList is the inbound-relay domain list (global administrators only). +// HandleInboundList is the inbound-relay domain list: every inbound domain for +// the global role, the assigned ones for a domain administrator. func (h *Handlers) HandleInboundList(w http.ResponseWriter, r *http.Request) { if _, ok := h.requireInbound(w, r); !ok { return @@ -63,7 +117,8 @@ func (h *Handlers) HandleInboundList(w http.ResponseWriter, r *http.Request) { } func (h *Handlers) renderInboundList(w http.ResponseWriter, r *http.Request, status int, formErr, formName string) { - list, err := h.inbound.List() + p, _ := h.principal(r) + list, err := h.assignedInboundDomains(p) if err != nil { logf("panel: inbound list: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) @@ -143,7 +198,7 @@ func rcptLabel(d store.InboundDomain) string { // HandleAddInbound validates the name, creates the inbound domain, and // redirects to its page. func (h *Handlers) HandleAddInbound(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { + if _, ok := h.requireInboundGlobal(w, r); !ok { return } if err := r.ParseForm(); err != nil { @@ -171,10 +226,7 @@ func (h *Handlers) HandleAddInbound(w http.ResponseWriter, r *http.Request) { } func (h *Handlers) HandleInboundDetail(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { - return - } - d, ok := h.lookupInbound(w, r) + d, ok := h.requireInboundDomain(w, r) if !ok { return } @@ -223,10 +275,7 @@ func inboundFlash(r *http.Request) string { } func (h *Handlers) HandleInboundDNSRecheck(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { - return - } - d, ok := h.lookupInbound(w, r) + d, ok := h.requireInboundDomain(w, r) if !ok { return } @@ -237,10 +286,7 @@ func (h *Handlers) HandleInboundDNSRecheck(w http.ResponseWriter, r *http.Reques } func (h *Handlers) HandleInboundTransport(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { - return - } - d, ok := h.lookupInbound(w, r) + d, ok := h.requireInboundDomain(w, r) if !ok { return } @@ -259,10 +305,7 @@ func (h *Handlers) HandleInboundTransport(w http.ResponseWriter, r *http.Request } func (h *Handlers) HandleInboundRecipients(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { - return - } - d, ok := h.lookupInbound(w, r) + d, ok := h.requireInboundDomain(w, r) if !ok { return } @@ -280,7 +323,7 @@ func (h *Handlers) HandleInboundRecipients(w http.ResponseWriter, r *http.Reques } func (h *Handlers) HandleInboundDeleteConfirm(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { + if _, ok := h.requireInboundGlobal(w, r); !ok { return } d, ok := h.lookupInbound(w, r) @@ -296,7 +339,7 @@ func (h *Handlers) HandleInboundDeleteConfirm(w http.ResponseWriter, r *http.Req } func (h *Handlers) HandleInboundDelete(w http.ResponseWriter, r *http.Request) { - if _, ok := h.requireInbound(w, r); !ok { + if _, ok := h.requireInboundGlobal(w, r); !ok { return } d, ok := h.lookupInbound(w, r) diff --git a/internal/web/handlers/handlers_monitor_test.go b/internal/web/handlers/handlers_monitor_test.go index 6a25b8c..091e777 100644 --- a/internal/web/handlers/handlers_monitor_test.go +++ b/internal/web/handlers/handlers_monitor_test.go @@ -373,7 +373,7 @@ func domainAdmin(t *testing.T, st *store.Store, username string, domainIDs ...in if err != nil { t.Fatalf("add placeholder domain: %v", err) } - id, err := st.CreateUser(username, hash, store.RoleDomain, []int64{placeholder.ID}) + id, err := st.CreateUser(username, hash, store.RoleDomain, store.Reach{DomainIDs: []int64{placeholder.ID}}) if err != nil { t.Fatalf("create domain admin %s: %v", username, err) } @@ -387,7 +387,7 @@ func domainAdmin(t *testing.T, st *store.Store, username string, domainIDs ...in } return auth.Principal{ID: u.ID, Username: u.Username, Role: u.Role, Domains: u.DomainIDs} } - id, err := st.CreateUser(username, hash, store.RoleDomain, domainIDs) + id, err := st.CreateUser(username, hash, store.RoleDomain, store.Reach{DomainIDs: domainIDs}) if err != nil { t.Fatalf("create domain admin %s: %v", username, err) } diff --git a/internal/web/handlers/handlers_settings_test.go b/internal/web/handlers/handlers_settings_test.go index 167a27c..93589f3 100644 --- a/internal/web/handlers/handlers_settings_test.go +++ b/internal/web/handlers/handlers_settings_test.go @@ -95,7 +95,7 @@ func settingsServer(t *testing.T) (*Handlers, string) { if err != nil { t.Fatalf("hash password: %v", err) } - if _, err := st.CreateUser("admin", string(hash), store.RoleGlobal, nil); err != nil { + if _, err := st.CreateUser("admin", string(hash), store.RoleGlobal, store.Reach{}); err != nil { t.Fatalf("create user: %v", err) } diff --git a/internal/web/handlers/handlers_users.go b/internal/web/handlers/handlers_users.go index 6ee5de0..8a2c4e6 100644 --- a/internal/web/handlers/handlers_users.go +++ b/internal/web/handlers/handlers_users.go @@ -15,7 +15,7 @@ type userFormView struct { FormErr string FormUsername string FormRole string - FormDomains map[int64]bool + Reach store.Reach FormPassword string } @@ -88,14 +88,10 @@ func (h *Handlers) HandleUserEdit(w http.ResponseWriter, r *http.Request) { } switch r.Method { case http.MethodGet: - selected := make(map[int64]bool, len(u.DomainIDs)) - for _, id := range u.DomainIDs { - selected[id] = true - } h.renderUserForm(w, r, http.StatusOK, u.ID, userFormView{ FormUsername: u.Username, FormRole: string(u.Role), - FormDomains: selected, + Reach: u.Reach(), }) case http.MethodPost: if err := r.ParseForm(); err != nil { @@ -129,7 +125,21 @@ func (h *Handlers) renderUserForm(w http.ResponseWriter, r *http.Request, status data["FormUsername"] = view.FormUsername data["FormRole"] = view.FormRole data["GlobalRole"] = store.RoleGlobal - data["FormDomains"] = view.FormDomains + // The two lists of the form, each with its All tick. Inbound is offered + // only where the feature is on. + data["FormDomains"] = idSet(view.Reach.DomainIDs) + data["FormAllDomains"] = view.Reach.AllDomains + data["FormInbound"] = idSet(view.Reach.InboundDomainIDs) + data["FormAllInbound"] = view.Reach.AllInbound + if h.cfg.InboundEnabled { + inboundDomains, err := h.store.ListInboundDomains() + if err != nil { + logf("panel: user form: list inbound domains: %v", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + data["InboundDomains"] = inboundDomains + } data["FormPassword"] = view.FormPassword data["IsEdit"] = userID != 0 data["LastGlobalLocked"] = lastGlobalLocked(h, userID, view.FormRole) @@ -152,22 +162,22 @@ func (h *Handlers) submitUserCreate(w http.ResponseWriter, r *http.Request) { username := strings.TrimSpace(r.PostFormValue("username")) password := r.PostFormValue("password") role := store.Role(r.PostFormValue("role")) - domainIDs := parseDomainIDs(r) + reach := h.reachFromForm(r, store.Reach{}) if err := validate.Username(username); err != nil { - h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), FormDomains: domainIDSetFromForm(r)}) + h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), Reach: reach}) return } if err := validate.AdminPassword(password); err != nil { - h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), FormDomains: domainIDSetFromForm(r)}) + h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), Reach: reach}) return } if role != store.RoleGlobal && role != store.RoleDomain { - h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: "Choose a valid role.", FormUsername: username, FormRole: string(role), FormDomains: domainIDSetFromForm(r)}) + h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: "Choose a valid role.", FormUsername: username, FormRole: string(role), Reach: reach}) return } - if role == store.RoleDomain && len(domainIDs) == 0 { - h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: "Select at least one domain for a domain administrator.", FormUsername: username, FormRole: string(role), FormDomains: domainIDSetFromForm(r)}) + if role == store.RoleDomain && reach.Empty() { + h.renderUserForm(w, r, http.StatusBadRequest, 0, userFormView{FormErr: "Assign at least one outbound or inbound domain to a domain administrator.", FormUsername: username, FormRole: string(role), Reach: reach}) return } @@ -177,9 +187,9 @@ func (h *Handlers) submitUserCreate(w http.ResponseWriter, r *http.Request) { h.renderUserForm(w, r, http.StatusInternalServerError, 0, userFormView{FormErr: "Internal error. Please try again."}) return } - if _, err := h.store.CreateUser(username, string(hash), role, domainIDs); err != nil { + if _, err := h.store.CreateUser(username, string(hash), role, reach); err != nil { if errors.Is(err, store.ErrUserExists) { - h.renderUserForm(w, r, http.StatusConflict, 0, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormRole: string(role), FormDomains: domainIDSetFromForm(r)}) + h.renderUserForm(w, r, http.StatusConflict, 0, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormRole: string(role), Reach: reach}) return } logf("panel: create user: %v", err) @@ -203,22 +213,21 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st username := strings.TrimSpace(r.PostFormValue("username")) password := r.PostFormValue("password") role := store.Role(r.PostFormValue("role")) - domainIDs := parseDomainIDs(r) - selected := domainIDSetFromForm(r) + reach := h.reachFromForm(r, u.Reach()) if username == "" { username = u.Username } if err := validate.Username(username); err != nil { - h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), Reach: reach}) return } if role != store.RoleGlobal && role != store.RoleDomain { - h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Choose a valid role.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Choose a valid role.", FormUsername: username, FormRole: string(role), Reach: reach}) return } - if role == store.RoleDomain && len(domainIDs) == 0 { - h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Select at least one domain for a domain administrator.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + if role == store.RoleDomain && reach.Empty() { + h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Assign at least one outbound or inbound domain to a domain administrator.", FormUsername: username, FormRole: string(role), Reach: reach}) return } @@ -229,7 +238,7 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st if u.ID == p.ID { msg = "You cannot demote yourself without another global administrator." } - h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: msg, FormUsername: username, FormRole: string(u.Role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: msg, FormUsername: username, FormRole: string(u.Role), Reach: reach}) return } } @@ -237,13 +246,13 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st hash := u.PasswordHash if password != "" { if err := validate.AdminPassword(password); err != nil { - h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: err.Error(), FormUsername: username, FormRole: string(role), Reach: reach}) return } newHash, err := bcrypt.GenerateFromPassword([]byte(password), bcryptCost) if err != nil { logf("panel: update user hash: %v", err) - h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Internal error. Please try again.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Internal error. Please try again.", FormUsername: username, FormRole: string(role), Reach: reach}) return } hash = string(newHash) @@ -251,31 +260,31 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st if err := h.store.UpdateUser(u.ID, username, hash, u.Email); err != nil { if errors.Is(err, store.ErrUserExists) { - h.renderUserForm(w, r, http.StatusConflict, u.ID, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusConflict, u.ID, userFormView{FormErr: "That username is already in use.", FormUsername: username, FormRole: string(role), Reach: reach}) return } logf("panel: update user: %v", err) - h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save user. Please check the logs.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save user. Please check the logs.", FormUsername: username, FormRole: string(role), Reach: reach}) return } if role != u.Role { if err := h.store.SetUserRole(u.ID, role); err != nil { logf("panel: set user role: %v", err) - h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not update role.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not update role.", FormUsername: username, FormRole: string(role), Reach: reach}) return } if role == store.RoleGlobal { - if err := h.store.ClearUserDomains(u.ID); err != nil { - logf("panel: clear user domains: %v", err) + if err := h.store.ClearUserReach(u.ID); err != nil { + logf("panel: clear user reach: %v", err) } } } if role == store.RoleDomain { - if err := h.store.SetUserDomains(u.ID, domainIDs); err != nil { - logf("panel: set user domains: %v", err) - h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save domain assignments.", FormUsername: username, FormRole: string(role), FormDomains: selected}) + if err := h.store.SetUserReach(u.ID, reach); err != nil { + logf("panel: set user reach: %v", err) + h.renderUserForm(w, r, http.StatusInternalServerError, u.ID, userFormView{FormErr: "Could not save domain assignments.", FormUsername: username, FormRole: string(role), Reach: reach}) return } } @@ -366,20 +375,45 @@ func parseUserID(w http.ResponseWriter, r *http.Request) (int64, bool) { return id, true } -func parseDomainIDs(r *http.Request) []int64 { +// reachFromForm reads the two assignment lists of the user form. With an All +// box ticked the rows under it are ignored — the form works without scripts, +// so both may arrive — and All then also covers domains added later. Where +// inbound is switched off the form has no inbound list, and what the user +// already had there (current) is kept rather than wiped by an unrelated save. +func (h *Handlers) reachFromForm(r *http.Request, current store.Reach) store.Reach { + reach := store.Reach{ + AllDomains: r.PostFormValue("all_domains") != "", + AllInbound: current.AllInbound, InboundDomainIDs: current.InboundDomainIDs, + } + if !reach.AllDomains { + reach.DomainIDs = parseIDs(r, "domain_ids") + } + if h.cfg.InboundEnabled { + reach.AllInbound = r.PostFormValue("all_inbound_domains") != "" + reach.InboundDomainIDs = nil + if !reach.AllInbound { + reach.InboundDomainIDs = parseIDs(r, "inbound_domain_ids") + } + } + return reach +} + +func parseIDs(r *http.Request, field string) []int64 { var ids []int64 - for _, v := range r.PostForm["domain_ids"] { + seen := make(map[int64]bool) + for _, v := range r.PostForm[field] { id, err := strconv.ParseInt(v, 10, 64) - if err == nil && id > 0 { + if err == nil && id > 0 && !seen[id] { + seen[id] = true ids = append(ids, id) } } return ids } -func domainIDSetFromForm(r *http.Request) map[int64]bool { - m := make(map[int64]bool) - for _, id := range parseDomainIDs(r) { +func idSet(ids []int64) map[int64]bool { + m := make(map[int64]bool, len(ids)) + for _, id := range ids { m[id] = true } return m diff --git a/internal/web/home_test.go b/internal/web/home_test.go new file mode 100644 index 0000000..786cece --- /dev/null +++ b/internal/web/home_test.go @@ -0,0 +1,29 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/mixeme/selfpost/internal/web/auth" +) + +// The root sends each user to the first thing they reach. +func TestRedirectHomeFollowsTheReach(t *testing.T) { + for _, tc := range []struct { + name string + p auth.Principal + want string + }{ + {"global", auth.Principal{Role: auth.RoleGlobal}, "/status"}, + {"outbound", auth.Principal{Role: auth.RoleDomain, Domains: []int64{1}}, "/domains"}, + {"both", auth.Principal{Role: auth.RoleDomain, AllDomains: true, AllInbound: true}, "/domains"}, + {"inbound only", auth.Principal{Role: auth.RoleDomain, InboundDomains: []int64{1}}, "/inbound"}, + } { + rec := httptest.NewRecorder() + redirectHome(rec, auth.RequestWithPrincipal(httptest.NewRequest(http.MethodGet, "/", nil), tc.p)) + if got := rec.Header().Get("Location"); rec.Code != http.StatusSeeOther || got != tc.want { + t.Errorf("%s: %d to %q, want 303 to %q", tc.name, rec.Code, got, tc.want) + } + } +} diff --git a/internal/web/view/templates/inbound.html b/internal/web/view/templates/inbound.html index 4011750..9fd3d97 100644 --- a/internal/web/view/templates/inbound.html +++ b/internal/web/view/templates/inbound.html @@ -11,6 +11,9 @@ Off by default in Compose.

Forwarding

+ {{/* Adding and deleting a domain stay with the global role; a domain + administrator works with the inbound domains assigned to them. */}} + {{if .IsGlobal}}
@@ -21,6 +24,7 @@ Off by default in Compose.

{{if .Error}}

{{.Error}}

{{end}}
+ {{end}} {{if .Domains}} @@ -34,7 +38,7 @@ Off by default in Compose.

- + {{end}} diff --git a/internal/web/view/templates/inbound_domain.html b/internal/web/view/templates/inbound_domain.html index 1870f03..7c8cdbe 100644 --- a/internal/web/view/templates/inbound_domain.html +++ b/internal/web/view/templates/inbound_domain.html @@ -112,7 +112,7 @@

Danger zone

Stops accepting mail for this domain. Does not touch outbound sending domains.

-

Delete inbound domain

+ {{if .IsGlobal}}

Delete inbound domain

{{end}} {{end}} diff --git a/internal/web/view/templates/layout_legacy.html b/internal/web/view/templates/layout_legacy.html index 5c68606..fb2adf7 100644 --- a/internal/web/view/templates/layout_legacy.html +++ b/internal/web/view/templates/layout_legacy.html @@ -70,20 +70,26 @@ the column: the mark, the panel's pages, and the session last. The mark goes to /status, the page the panel opens on — named outright rather than as "/", which is only a redirect to it. */}} - SelfPost + SelfPost
{{if .IsGlobal}} {{if eq .Active "status"}}{{template "icon-status"}}Status{{else}}{{template "icon-status"}}Status{{end}} {{end}} + {{/* A group with nothing assigned is absent from that user's menu: the + sending pages need outbound reach, Inbound needs inbound reach. */}} + {{if or .IsGlobal .HasOutbound}} {{if eq .Active "domains"}}{{template "icon-domains"}}Domains{{else}}{{template "icon-domains"}}Domains{{end}} - {{if and .IsGlobal .InboundEnabled}} + {{end}} + {{if and .InboundEnabled (or .IsGlobal .HasInbound)}} {{if eq .Active "inbound"}}{{template "icon-inbound"}}Inbound{{else}}{{template "icon-inbound"}}Inbound{{end}} {{end}} - {{if .DMARCEnabled}} + {{if and .DMARCEnabled (or .IsGlobal .HasOutbound)}} {{if eq .Active "dmarc"}}{{template "icon-dmarc"}}DMARC{{else}}{{template "icon-dmarc"}}DMARC{{end}} {{end}} {{if eq .Active "help"}}{{template "icon-help"}}Help{{else}}{{template "icon-help"}}Help{{end}} + {{if or .IsGlobal .HasOutbound}} {{if eq .Active "deliveries"}}{{template "icon-deliveries"}}Deliveries{{else}}{{template "icon-deliveries"}}Deliveries{{end}} + {{end}} {{if .IsGlobal}} {{if eq .Active "mail_queue"}}{{template "icon-mail-queue"}}Mail queue{{else}}{{template "icon-mail-queue"}}Mail queue{{end}} {{if eq .Active "system_log"}}{{template "icon-system-log"}}System log{{else}}{{template "icon-system-log"}}System log{{end}} diff --git a/internal/web/view/templates/user_form.html b/internal/web/view/templates/user_form.html index 0043770..ad7afc1 100644 --- a/internal/web/view/templates/user_form.html +++ b/internal/web/view/templates/user_form.html @@ -21,9 +21,17 @@ {{if .LastGlobalLocked}}

The only global administrator cannot be demoted.

{{end}} -
- Assigned domains -

Required for domain administrators.

+ {{/* Outbound and inbound domains are assigned separately; a domain + administrator needs at least one on either list. "All" also covers + domains added later — with it ticked the rows below are ignored. */}} +
+
+ Outbound domains +

A domain administrator needs at least one outbound or inbound domain.

+ {{range .Domains}} {{end}}
+ {{if .InboundEnabled}} +
+ Inbound domains + + {{range .InboundDomains}} + + {{end}} +
+ {{end}} +
diff --git a/internal/web/view/templates/users.html b/internal/web/view/templates/users.html index 5e932b5..374c6c6 100644 --- a/internal/web/view/templates/users.html +++ b/internal/web/view/templates/users.html @@ -8,14 +8,15 @@ {{if .Users}}
{{.Upstream}} {{.RcptLabel}} {{.TLSLabel}}Delete{{if $.IsGlobal}}Delete{{end}}
- + {{if .InboundEnabled}}{{end}} {{range .Users}} - - + + + {{if $.InboundEnabled}}{{end}} {{end}} diff --git a/internal/web/view/templates_test.go b/internal/web/view/templates_test.go index 8d2234b..4e7d949 100644 --- a/internal/web/view/templates_test.go +++ b/internal/web/view/templates_test.go @@ -687,3 +687,46 @@ func TestNavIncludesHelp(t *testing.T) { t.Error("nav is missing Help link") } } + +// A group with nothing assigned is absent from that user's menu: a domain +// administrator who was given inbound domains only sees Inbound and none of the +// sending pages, and the mark takes them to what they have. +func TestLegacyNavFollowsTheUsersReach(t *testing.T) { + engine, err := New("test") + if err != nil { + t.Fatalf("New: %v", err) + } + nav := func(data map[string]any) string { + t.Helper() + data["User"], data["Active"] = "ops", "" + data["InboundEnabled"], data["DMARCEnabled"] = true, true + var buf bytes.Buffer + if err := engine.Page("help").ExecuteTemplate(&buf, "nav", data); err != nil { + t.Fatalf("execute nav: %v", err) + } + return buf.String() + } + has := func(out, href string) bool { return strings.Contains(out, `href="`+href+`"`) } + + out := nav(map[string]any{"HasInbound": true}) + if !has(out, "/inbound") { + t.Errorf("an inbound-only administrator has no Inbound entry:\n%s", out) + } + for _, gone := range []string{"/domains", "/deliveries", "/dmarc", "/status", "/users", "/mail-queue"} { + if has(out, gone) { + t.Errorf("an inbound-only administrator is offered %s", gone) + } + } + + out = nav(map[string]any{"HasOutbound": true}) + if has(out, "/inbound") || !has(out, "/domains") || !has(out, "/deliveries") || !has(out, "/dmarc") { + t.Errorf("an outbound-only administrator's menu is wrong:\n%s", out) + } + + out = nav(map[string]any{"IsGlobal": true}) + for _, want := range []string{"/status", "/domains", "/inbound", "/dmarc", "/deliveries", "/users"} { + if !has(out, want) { + t.Errorf("the global role lost %s", want) + } + } +} diff --git a/internal/web/web.go b/internal/web/web.go index 5a33742..0fa455c 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -251,6 +251,12 @@ func redirectSettings(w http.ResponseWriter, r *http.Request) { func redirectHome(w http.ResponseWriter, r *http.Request) { p, ok := auth.PrincipalFromRequest(r) if ok && !p.IsGlobal() { + // A domain administrator lands on what they reach: their sending + // domains, or the inbound ones when that is all they are assigned. + if !p.HasOutbound() && p.HasInbound() { + http.Redirect(w, r, "/inbound", http.StatusSeeOther) + return + } http.Redirect(w, r, "/domains", http.StatusSeeOther) return }
UsernameRoleDomains
UsernameRoleOutbound domainsInbound domains
{{.User.Username}}{{if eq .User.Role "global"}}Global{{else}}Domain admin{{end}}{{if eq .User.Role "global"}}All{{else}}{{range $i, $n := .DomainNames}}{{if $i}}, {{end}}{{$n}}{{end}}{{end}}{{.User.Role}}{{if or (eq .User.Role "global") .User.AllDomains}}All{{else}}{{range $i, $n := .DomainNames}}{{if $i}}, {{end}}{{$n}}{{end}}{{end}}{{if or (eq .User.Role "global") .User.AllInboundDomains}}All{{else}}{{range $i, $n := .InboundDomainNames}}{{if $i}}, {{end}}{{$n}}{{end}}{{end}}Edit