From 3192d89edfcb0ca0e602b283f17ff77a71fcb050 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 27 Oct 2026 22:33:53 +0100 Subject: [PATCH] mesh-grants: refuse a take-over over an uncompared deny; ask words only for added rights Review round 1 of issue 1945's fix found two ways the guard could let a loss through or hide one from the operator. A publish deny was never compared: grants.User carried no deny, so a widened allow that brings a new deny (a lost publish right) passed as an added right. The user now carries its deny; a deny the shadow adds is a loss, and while nats_users answers no deny for the loaded list, a user whose grants differ and whose shadow grant has a deny refuses, named as a deny the loaded list cannot be compared on. None of today's 41 differing users carries a deny (only the four node-tools users do, and they are the same in both lists). The ask said "takes none away" whenever rights were added, whatever else differed. Its words now name the added rights only when nothing else refuses (Refusals: unread list, Removing, a missing hash), and otherwise say what the yes would be refused for, naming the users; Decide uses the same Refusals. issue 1945 --- modules/mesh-grants/grants_test.go | 51 +++++++++- modules/mesh-grants/internal/grants/part.go | 2 +- .../mesh-grants/internal/grants/takeover.go | 46 +++------ .../mesh-grants/internal/grants/userlist.go | 99 +++++++++++++++++-- 4 files changed, 156 insertions(+), 42 deletions(-) diff --git a/modules/mesh-grants/grants_test.go b/modules/mesh-grants/grants_test.go index 44a5a05..2ea008b 100644 --- a/modules/mesh-grants/grants_test.go +++ b/modules/mesh-grants/grants_test.go @@ -749,6 +749,14 @@ func TestGrants(t *testing.T) { // Only added rights (ADR 0562): the loaded list lacks a publish of a.mod-x the shadow grants. fewer := slices.Clone(shadow) fewer[x].Publish = slices.Clone(fewer[x].Publish[1:]) + // A deny the shadow adds where the loaded list says its own (empty): a publish right lost. And the same deny where + // the loaded list says none, as nats_users answers today, on a user whose grants differ: it cannot be compared. + denied := slices.Clone(shadow) + denied[x].PublishDeny = []string{"mesh.seat.node-launcher.tool.secret"} + readable := slices.Clone(fewer) + for i := range readable { + readable[i].PublishDeny = []string{} + } // The live comparison the operator's yes was refused on, 2026-10-27 21:19 UTC (issue 1945): 41 users differ, every // difference an added right. Read whole; a row's mutate makes one of the cases that must still refuse. var live struct { @@ -791,6 +799,8 @@ func TestGrants(t *testing.T) { {"one loaded user placement no longer names", append(slices.Clone(shadow), grants.User{User: "b.gone"}), nil, []string{"b.gone"}, []string{"b.gone"}, nil, true, nil, nil, 0}, {"grants differ", changed, nil, nil, nil, []string{"a.mod-x"}, false, nil, []string{"a.mod-x"}, 1}, {"only a right added", fewer, nil, nil, nil, []string{"a.mod-x"}, false, nil, nil, 1}, + {"a deny added refuses", readable, nil, nil, nil, []string{"a.mod-x"}, false, nil, []string{"a.mod-x"}, 1}, + {"a deny the loaded list cannot say refuses a user whose grants differ", fewer, nil, nil, nil, []string{"a.mod-x"}, false, nil, []string{"a.mod-x"}, 1}, {"captured 21:19: only added rights", nil, nil, nil, nil, nil, false, captured(nil), nil, 41}, {"captured, a right removed", nil, nil, nil, nil, nil, false, captured(func(c *grants.Comparison) { nodeAce(c).SubscribeDropped = []string{"mesh.mod.mesh-vault.event.secret-changed"} @@ -804,13 +814,18 @@ func TestGrants(t *testing.T) { {"captured, a user added", nil, nil, nil, nil, nil, false, captured(func(c *grants.Comparison) { c.Added = []string{"shanks.new-module"} }), []string{"shanks.new-module"}, 41}, + {"captured, a login lost", nil, nil, nil, nil, nil, false, captured(nil), []string{"novox.telegram"}, 41}, } { t.Run(row.name, func(t *testing.T) { var c grants.Comparison if row.live != nil { c = *row.live } else { - c = grants.Compare(shadow, row.loaded, grants.Placed(in)) + from := shadow + if strings.Contains(row.name, "deny") { + from = denied + } + c = grants.Compare(from, row.loaded, grants.Placed(in)) var differ []string for _, d := range c.Differ { differ = append(differ, d.User) @@ -820,17 +835,45 @@ func TestGrants(t *testing.T) { t.Errorf("compare = %+v", c) } } + var cr grants.Credentials + if row.live != nil { + cr = live.Credentials + } + if strings.Contains(row.name, "login lost") { + cr.LosesLogin = []string{"novox.telegram"} + } + // Removing, and the users with no hash in the vault, are what the yes is refused for. var removing []string for _, r := range c.Removing() { removing = append(removing, strings.Fields(r)[0]) } + removing = append(removing, cr.LosesLogin...) if !slices.Equal(removing, row.removing) || len(c.AddedRights()) != row.adds { t.Errorf("take-over refused for %v, want %v; added rights of %d users, want %d", c.Removing(), row.removing, len(c.AddedRights()), row.adds) } - // The ask's words for the added rights pass the operator channel's content rule, read at a glance and fit one - // message, and name every user whose rights are added. - summary, points, list := c.AddedWords() + // The ask's words pass the operator channel's content rule, read at a glance and fit one message; where only + // rights are added they name every user whose rights are added, and where a loss stands they never say none is + // taken away and name the users the yes would be refused for. + summary, points, list := grants.TakeOverWords(c, cr) + if row.removing != nil { + if strings.Contains(summary, "takes none away") { + t.Errorf("the ask says none is taken away with a loss: %q", summary) + } + for _, u := range row.removing { + if !strings.Contains(list, u) { + t.Errorf("the ask does not name %s, refused for: %s", u, list) + } + } + } + if row.live != nil && row.removing == nil && c.Equal() { + t.Error("the captured comparison is equal: the old guard would not have refused it") + } if row.adds > 0 { + if r, ok := outward.Check(summary + "\n" + strings.Join(points, "\n") + "\n" + list); !ok { + t.Errorf("the ask's words break the content rule: %s", r) + } + } + if row.adds > 0 && row.removing == nil { expl := summary + "\n" + strings.Join(points, "\n") for _, text := range []string{expl, list} { if r, ok := outward.Check(text); !ok { diff --git a/modules/mesh-grants/internal/grants/part.go b/modules/mesh-grants/internal/grants/part.go index cef6d3b..16d177f 100644 --- a/modules/mesh-grants/internal/grants/part.go +++ b/modules/mesh-grants/internal/grants/part.go @@ -299,7 +299,7 @@ func (p *Part) Compose() Result { } } if !p.deciding.Load() { - if err := p.RaiseRequested(res.UserList.Compare); err != nil { + if err := p.RaiseRequested(res.UserList); err != nil { res.Errors = append(res.Errors, err.Error()) } if err := p.Reconcile(); err != nil { diff --git a/modules/mesh-grants/internal/grants/takeover.go b/modules/mesh-grants/internal/grants/takeover.go index eeaa087..bd5e678 100644 --- a/modules/mesh-grants/internal/grants/takeover.go +++ b/modules/mesh-grants/internal/grants/takeover.go @@ -5,7 +5,6 @@ import ( "encoding/hex" "encoding/json" "fmt" - "sort" "strings" "time" @@ -179,7 +178,7 @@ func (p *Part) request(act, caller, why string) (any, error) { // RaiseRequested is the process's: it asks the operator the request a tool left, once, naming the rights the pass's // comparison c adds. Where the part has no asker (the tools) it does nothing. -func (p *Part) RaiseRequested(c Comparison) error { +func (p *Part) RaiseRequested(st UserListState) error { if p.Asker == nil { return nil } @@ -197,12 +196,12 @@ func (p *Part) RaiseRequested(c Comparison) error { if s, err := p.readSwitch(); err != nil || s.Open != r.ID { return nil // an ask no longer open is not asked } - ask, err := p.AskOf(r, c) + ask, err := p.AskOf(r, st) if err != nil { return err } if r.Act == ActTakeOver { - r.Adds = c.AddedDigest() + r.Adds = st.Compare.AddedDigest() } if err := p.Asker.Ask(ask); err != nil { // Closed as refused, and the half-made record the asker kept before publishing is taken away, so nothing stays open. @@ -302,8 +301,10 @@ func (p *Part) Following() string { } // AskOf is the ask of a request, in the words the operator reads: what changes, why, what it risks; for a take-over every -// right the grants part's list adds by the comparison c, by user (ADR 0562 §3). Its yes binds the act. -func (p *Part) AskOf(r Request, c Comparison) (asks.Ask, error) { +// right the grants part's list adds by the user list st compares now, by user, or what a yes would be refused for +// (ADR 0562 §3, TakeOverWords). Its yes binds the act. +func (p *Part) AskOf(r Request, st UserListState) (asks.Ask, error) { + c := st.Compare verb, headline, does, stays, expl := "grants_take_over", "Switch the bus's user list to the grants part?", "The switch is recorded; it takes effect when the bus node loads the grants part's list.", "The controller's list stays in force.", "The grants part has composed the user list the bus holds today. Your yes records the switch; it takes effect when the bus node loads the part's list.\n"+ "If the lists differ other than by an added right, or a user has no hash in the vault, the switch is refused and you are told which." @@ -313,9 +314,14 @@ func (p *Part) AskOf(r Request, c Comparison) (asks.Ask, error) { } details := "Asked by " + r.By + ": " + r.Why act := asks.Act{"verb": verb, "module": "mesh-grants", "ask": r.ID} - if summary, points, list := c.AddedWords(); r.Act == ActTakeOver && summary != "" { - expl = summary + " Your yes records the switch; it takes effect when the bus node loads the part's list.\n" + strings.Join(points, "\n") - details = "Added, by user:\n" + list + "\nA user added or dropped, a right taken away or a missing hash refuses the switch, naming them.\n" + details + if summary, points, list := TakeOverWords(c, st.Credentials); r.Act == ActTakeOver && summary != "" { + if len(Refusals(c, st.Credentials)) > 0 { + expl = summary + "\n" + strings.Join(points, "\n") + details = "Refused for:\n" + list + "\n" + details + } else { + expl = summary + " Your yes records the switch; it takes effect when the bus node loads the part's list.\n" + strings.Join(points, "\n") + details = "Added, by user:\n" + list + "\nA user added or dropped, a right taken away or a missing hash refuses the switch, naming them.\n" + details + } act["adds"] = c.AddedDigest() } bind, err := asks.ActDigest(act) @@ -418,31 +424,11 @@ func (p *Part) decide(ask string) []string { p.deciding.Store(true) res := p.Compose() p.deciding.Store(false) - var reasons []string - if c := res.UserList.Compare; !c.Equal() { - if c.Unread != "" { - reasons = append(reasons, "the loaded list could not be read: "+c.Unread) - } - if d := c.Removing(); len(d) > 0 { - reasons = append(reasons, fmt.Sprintf("%d users differ between the shadow list and the loaded list other than by an added right: %s", len(d), strings.Join(d, ", "))) - } - } + reasons := Refusals(res.UserList.Compare, res.UserList.Credentials) if ask != "" && p.askedAdds(ask) != res.UserList.Compare.AddedDigest() { reasons = append(reasons, "the rights the grants part's list adds changed since you were asked, so the yes does not cover them: ask the take-over again") } reasons = append(reasons, p.followReasons(res, ReadInputs(p.Read, &p.cache))...) - cr, gone := res.UserList.Credentials, res.UserList.Compare.Gone - var unhashed []string - for _, u := range append(append([]string(nil), cr.LeftOut...), cr.LosesLogin...) { - if !contains(gone, u) { - unhashed = append(unhashed, u) - } - } - if len(unhashed) > 0 { - sort.Strings(unhashed) - reasons = append(reasons, fmt.Sprintf("the vault holds no hash for %d users (ADR 0547 clause 9: handed or adopted before take-over): %s", - len(unhashed), strings.Join(unhashed, ", "))) - } if len(reasons) == 0 { if s, err := p.readSwitch(); err == nil && len(s.Refusing) > 0 { p.clearRefused(&s) diff --git a/modules/mesh-grants/internal/grants/userlist.go b/modules/mesh-grants/internal/grants/userlist.go index 86eec88..601236c 100644 --- a/modules/mesh-grants/internal/grants/userlist.go +++ b/modules/mesh-grants/internal/grants/userlist.go @@ -21,6 +21,9 @@ type User struct { Publish []string `json:"publish"` Subscribe []string `json:"subscribe"` AllowResponses bool `json:"allow_responses"` + // PublishDeny is what the user may not publish though an allow reaches it (buscompose's ControllerOnly). The bus + // module's nats_users answers none today, so a loaded user's is nil, read as unknown; empty and not nil once it says. + PublishDeny []string `json:"publish_deny,omitempty"` // Hash is the bcrypt hash the vault holds for the user (WithHashes): in memory alone, never in a digest, a compare // or the part's state, and empty for a loaded user. Hash string `json:"-"` @@ -126,7 +129,7 @@ func ComposeUsers(r buscompose.Records) ([]User, error) { return nil, fmt.Errorf("%s: %w", p.Username(), err) } out = append(out, User{User: p.Username(), Publish: set(perms.Publish), Subscribe: set(perms.Subscribe), - AllowResponses: perms.AllowResponses}) + AllowResponses: perms.AllowResponses, PublishDeny: set(perms.PublishDeny)}) } sort.Slice(out, func(i, j int) bool { return out[i].User < out[j].User }) return out, nil @@ -224,6 +227,16 @@ type UserDiff struct { SubscribeAdded []string `json:"subscribe_added,omitempty"` SubscribeDropped []string `json:"subscribe_dropped,omitempty"` Responses string `json:"responses,omitempty"` + // DenyAdded is what the shadow list denies the user to publish and the loaded list does not: a right lost. DenyUnread + // is the shadow's deny of a user whose grants differ, where the loaded list's deny cannot be read (nats_users answers + // none): it cannot be compared, so it is no added right either (ADR 0562 §2). + DenyAdded []string `json:"deny_added,omitempty"` + DenyUnread []string `json:"deny_unread,omitempty"` +} + +// same says the user's grants agree in both lists. +func (d UserDiff) same() bool { + return len(d.PublishAdded)+len(d.PublishDropped)+len(d.SubscribeAdded)+len(d.SubscribeDropped)+len(d.DenyAdded) == 0 && d.Responses == "" } // Comparison is the shadow list against the loaded list. @@ -292,16 +305,23 @@ func (c Comparison) Removing() []string { if d.responsesLost() { lost = append(lost, "its allowance of responses") } - if len(lost) > 0 { + if n := len(d.DenyAdded); n > 0 { + lost = append(lost, fmt.Sprintf("%d publish to a deny added", n)) + } + switch { + case len(lost) > 0: out = append(out, d.User+" (would lose "+strings.Join(lost, " and ")+")") + case len(d.DenyUnread) > 0: + out = append(out, d.User+" (a deny the loaded list cannot be compared on)") } } sort.Strings(out) return out } -// AddedRights are the users whose grants the shadow list widens and nothing else, with what it adds: the differences of a -// comparison whose Removing is empty (ADR 0562 §1). +// AddedRights are the rights the shadow list adds, by user: every user's added publish and subscribe rights and an +// allowance of responses gained, whatever else the comparison holds. They are the whole difference only where Removing is +// empty, and only then is a take-over allowed for them (ADR 0562 §1, §2). func (c Comparison) AddedRights() []UserDiff { var out []UserDiff for _, d := range c.Differ { @@ -395,7 +415,7 @@ const AddedWordsBudget = 2200 // a name that would not pass written with spaces) and AddedWordsBudget, past which the last lines are counted, not named. func (c Comparison) AddedWords() (summary string, points []string, list string) { adds := c.AddedRights() - if len(adds) == 0 { + if len(adds) == 0 || c.Unread != "" || len(c.Removing()) > 0 { return "", nil, "" } total, byKind, usersOf := 0, map[string]int{}, map[string]int{} @@ -467,6 +487,64 @@ func count(n int, thing string) string { return fmt.Sprintf("%d %ss", n, thing) } +// Refusals are why a take-over is refused for the lists themselves, each naming its users: the loaded list unread, any user +// the lists keep apart other than by an added right (Removing), and any user with no hash in the vault but those both +// lists agree are gone. Empty when the lists differ at most by added rights and every login is kept. +func Refusals(c Comparison, cr Credentials) []string { + var reasons []string + if c.Unread != "" { + reasons = append(reasons, "the loaded list could not be read: "+c.Unread) + } + if d := c.Removing(); len(d) > 0 { + reasons = append(reasons, fmt.Sprintf("%d users differ between the shadow list and the loaded list other than by an added right: %s", len(d), strings.Join(d, ", "))) + } + var unhashed []string + for _, u := range append(append([]string(nil), cr.LeftOut...), cr.LosesLogin...) { + if !slices.Contains(c.Gone, u) { + unhashed = append(unhashed, u) + } + } + if len(unhashed) > 0 { + sort.Strings(unhashed) + reasons = append(reasons, fmt.Sprintf("the vault holds no hash for %d users (ADR 0547 clause 9: handed or adopted before take-over): %s", + len(unhashed), strings.Join(unhashed, ", "))) + } + return reasons +} + +// TakeOverWords are the take-over ask's words for the lists as compared now: the rights the grants part's list adds +// (AddedWords) when nothing else keeps them apart, or else what a yes would be refused for, the users named in its list +// (ADR 0562 §2, §3). Empty when the lists agree. Held to the content rule: the loaded list's read error is not said. +func TakeOverWords(c Comparison, cr Credentials) (summary string, points []string, list string) { + refusals := Refusals(c, cr) + if len(refusals) == 0 { + return c.AddedWords() + } + summary = "Today a yes would be refused: the lists differ other than by an added right, the loaded list is unread, or a user has no hash." + var lines []string + if c.Unread != "" { + lines = append(lines, "The loaded list could not be read.") + refusals = refusals[1:] + } + used := 0 + for _, r := range refusals { + if used+len([]rune(r)) > AddedWordsBudget-80 { + r = string([]rune(r)[:max(0, AddedWordsBudget-80-used)]) + "… grants_take_over_state says the rest." + } + lines = append(lines, r) + used += len([]rune(r)) + 1 + if used >= AddedWordsBudget-80 { + break + } + } + for i := range lines { + if _, ok := outward.Check(lines[i]); !ok { + lines[i] = strings.ReplaceAll(lines[i], ".", " ") + } + } + return summary, []string{"Your yes now is refused and said, naming them; ask again once they agree."}, strings.Join(lines, "\n") +} + // plainName is a user's name as the operator channel's content rule lets it pass: as it is, or with spaces for its dots // where it reads as a host name. func plainName(user string) string { @@ -497,10 +575,17 @@ func Compare(shadow, loaded []User, placed func(user string) bool) Comparison { if s.AllowResponses != l.AllowResponses { d.Responses = fmt.Sprintf("shadow %v, loaded %v", s.AllowResponses, l.AllowResponses) } - if len(d.PublishAdded)+len(d.PublishDropped)+len(d.SubscribeAdded)+len(d.SubscribeDropped) == 0 && d.Responses == "" { + if l.PublishDeny != nil { + d.DenyAdded = minus(s.PublishDeny, l.PublishDeny) + } + if d.same() { + // The same allows compose the same deny: one composer derives it from them. c.Same++ continue } + if l.PublishDeny == nil { + d.DenyUnread = s.PublishDeny + } c.Differ = append(c.Differ, d) } for _, l := range loaded { @@ -567,7 +652,7 @@ func (c Comparison) Excusing(f func(user, subject string, added, subscribe bool) } d.PublishAdded, d.PublishDropped = keep(d.PublishAdded, true, false), keep(d.PublishDropped, false, false) d.SubscribeAdded, d.SubscribeDropped = keep(d.SubscribeAdded, true, true), keep(d.SubscribeDropped, false, true) - if len(d.PublishAdded)+len(d.PublishDropped)+len(d.SubscribeAdded)+len(d.SubscribeDropped) == 0 && d.Responses == "" { + if d.same() { c.Same++ continue }