You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Multiple reviewers independently got confused about the split between willow-channel (data model) and willow-state (authority). The review surfaced several bugs that trace back to this confusion:
willow-state::materialize::apply_* is the real authority boundary, but only because the client goes through the event pipeline.
Some reviewers assumed willow-channel enforced permissions; others assumed willow-state duplicated willow-channel; neither is correct.
The reality: willow-state is the only authority boundary. willow-channel is a data model used for local rendering and applied during event replay. Direct mutation of a Server is not an enforcement surface.
Without this written down, the same class of bug will keep showing up.
Fix
Add a short spec at docs/specs/authority-model.md (or similar). Target length: one page, ~300-500 words. It should cover:
One-sentence summary: All authority checks live in willow-state::materialize::apply_unchecked and the required_permission() table. Every other crate is untrusted plumbing.
Event flow: user action → ClientMutations → signed event → gossip → peer receives → apply_incremental → permission check → state mutation. Direct Server mutation is not in this path and does not constitute enforcement.
What willow-channel is: a passive data model used by:
Event materialization (to project state)
Local rendering (read-only)
Test fixtures
It is not a permission enforcement surface. Methods on Server are convenience API for replay.
Adding a new permission: step-by-step checklist.
Add variant to Permission in willow-state::event.
Add cases to required_permission() in materialize.rs for every new EventKind variant.
Update tests.
Update willow-channel if needed for display purposes.
Adding a new event kind: explicit checklist item — every new EventKind variant must be considered for required_permission(). The _ => None catch-all in required_permission() is a known footgun; adding a new variant that silently falls into it is the mechanism behind bug [state] RotateChannelKey has no permission or membership check #109.
Companion work
Add a Clippy # [deny(non_exhaustive_omitted_patterns)] or similar hint on the required_permission() match so future variants can't silently fall through. If that's not practical, at minimum add a comment at the _ => None arm listing every variant that's intentionally unrestricted, so a reviewer will notice when the comment gets out of sync with the enum.
Definition of done
docs/specs/authority-model.md exists
It's linked from CLAUDE.md under "Architecture Notes"
A comment at crates/state/src/materialize.rs:220 references the spec
A comment at the top of crates/channel/src/lib.rs references the spec
Parent: #108
Problem
Multiple reviewers independently got confused about the split between
willow-channel(data model) andwillow-state(authority). The review surfaced several bugs that trace back to this confusion:willow-channel::Serverhas methods likecreate_role,toggle_permission,assign_rolethat are not permission-checked (see [channel] Make Server::admins and other public mutable fields private #118).willow-state::materialize::apply_*is the real authority boundary, but only because the client goes through the event pipeline.willow-channelenforced permissions; others assumedwillow-stateduplicatedwillow-channel; neither is correct.The reality:
willow-stateis the only authority boundary.willow-channelis a data model used for local rendering and applied during event replay. Direct mutation of aServeris not an enforcement surface.Without this written down, the same class of bug will keep showing up.
Fix
Add a short spec at
docs/specs/authority-model.md(or similar). Target length: one page, ~300-500 words. It should cover:willow-state::materialize::apply_uncheckedand therequired_permission()table. Every other crate is untrusted plumbing.ClientMutations→ signed event → gossip → peer receives →apply_incremental→ permission check → state mutation. DirectServermutation is not in this path and does not constitute enforcement.willow-channelis: a passive data model used by:It is not a permission enforcement surface. Methods on
Serverare convenience API for replay.Permissioninwillow-state::event.required_permission()inmaterialize.rsfor every newEventKindvariant.willow-channelif needed for display purposes.EventKindvariant must be considered forrequired_permission(). The_ => Nonecatch-all inrequired_permission()is a known footgun; adding a new variant that silently falls into it is the mechanism behind bug [state] RotateChannelKey has no permission or membership check #109.Companion work
Add a Clippy
# [deny(non_exhaustive_omitted_patterns)]or similar hint on therequired_permission()match so future variants can't silently fall through. If that's not practical, at minimum add a comment at the_ => Nonearm listing every variant that's intentionally unrestricted, so a reviewer will notice when the comment gets out of sync with the enum.Definition of done
docs/specs/authority-model.mdexistsCLAUDE.mdunder "Architecture Notes"crates/state/src/materialize.rs:220references the speccrates/channel/src/lib.rsreferences the spec