From 7a2a9da339f3f5dbdaa6a7bb114c91de7e2c1fcb Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Apr 2026 07:58:51 +0000 Subject: [PATCH 1/2] channel: make Server id/name/description/admins private Exposing Server::admins, id, name, description as pub fields would let any caller with &mut Server do server.admins.insert(me) and grant itself admin status, bypassing the event-sourced authority model in willow-state. Make the four identity-bearing fields private and provide: - id(), name(), description(), admins() read accessors - Server::with_id(id, name, creator) constructor for the join path - #[doc(hidden)] set_*_for_materializer setters documented as data-shape mutations, not enforcement boundaries The materializer lives in a different crate (willow-state), so pub(crate) setters are not visible to it. The *_for_materializer setters are pub but explicitly named and #[doc(hidden)] to discourage accidental use while remaining reachable from outside the crate. Add a regression test plus a module-level doc clarifying that all authority enforcement happens in willow-state::materialize::apply_*. Closes #118 Progresses #108 --- crates/channel/src/lib.rs | 149 +++++++++++++++++++++++++++++++++++--- 1 file changed, 139 insertions(+), 10 deletions(-) diff --git a/crates/channel/src/lib.rs b/crates/channel/src/lib.rs index fa607b6e..934c3a07 100644 --- a/crates/channel/src/lib.rs +++ b/crates/channel/src/lib.rs @@ -2,6 +2,15 @@ //! //! Servers, channels, roles, and permissions for the Willow P2P network. //! +//! This crate defines data structures for servers, channels, roles, and +//! permissions. **All authority enforcement happens in +//! `willow-state::materialize::apply_*`.** Direct mutation of a [`Server`] +//! value is not an enforcement boundary — it is a data-shape operation +//! used by the materializer and tests. Code that needs to enforce trust +//! decisions (admin promotion, kicks, role assignment, etc.) MUST go +//! through the event-sourced [`willow_state`] machine, not through these +//! types. +//! //! ## Data model //! //! Willow borrows Discord's organisational hierarchy: @@ -326,18 +335,26 @@ impl Invite { /// /// Admins have implicit access to all permissions. Other members' /// access is determined by their roles and direct permission grants. +/// +/// Identity-bearing fields (`id`, `name`, `description`, `admins`) are +/// private — direct mutation of them by external code would bypass the +/// event-sourced authority model in [`willow_state`]. Read them via +/// [`Server::id`], [`Server::name`], [`Server::description`], and +/// [`Server::admins`]. The materializer and integration code use the +/// explicitly-named `*_for_materializer` setters to populate these +/// fields after applying authoritative events. #[derive(Debug, Clone, Serialize, Deserialize)] pub struct Server { /// Unique ID. - pub id: ServerId, + id: ServerId, /// Display name. - pub name: String, + name: String, /// Optional description. - pub description: Option, + description: Option, /// The peer who created the server (genesis author). pub creator: EndpointId, /// The set of peers with admin status. - pub admins: HashSet, + admins: HashSet, /// When the server was created. pub created_at: DateTime, @@ -375,8 +392,83 @@ impl Server { } } + /// Create a new server with a specific [`ServerId`]. + /// + /// Used by the join flow (and by the materializer if it ever needs + /// to seed a [`Server`] mirror) when the ID was decided elsewhere — + /// e.g. parsed from an invite payload or derived from the genesis + /// event hash. The creator is the initial admin and member, just + /// like [`Server::new`]. + pub fn with_id(id: ServerId, name: impl Into, creator: EndpointId) -> Self { + let mut server = Self::new(name, creator); + server.id = id; + server + } + // ── Queries ────────────────────────────────────────────────────────── + /// The server's unique ID. + pub fn id(&self) -> &ServerId { + &self.id + } + + /// The server's display name. + pub fn name(&self) -> &str { + &self.name + } + + /// The server's optional description / topic line. + pub fn description(&self) -> Option<&str> { + self.description.as_deref() + } + + /// The set of peers with admin status. + /// + /// This is a derived view — the authoritative admin set lives in + /// [`willow_state::ServerState::admins`]. Mutations to this set on + /// a `Server` value have no effect on trust decisions: they are a + /// data-shape mirror used by the materializer and tests. + pub fn admins(&self) -> &HashSet { + &self.admins + } + + // ── Materializer-only setters ──────────────────────────────────────── + // + // These exist so `willow_state::materialize` (a different crate) can + // populate the data-shape mirror after applying authoritative events. + // `pub(crate)` would not be visible to `willow_state`, so they are + // `pub` but explicitly named and `#[doc(hidden)]` to discourage + // accidental use. **They are NOT an enforcement boundary** — the + // event-sourced model in `willow_state` is the sole authority. + + /// Internal use only — call from `willow-state::materialize` or test + /// helpers. The event-sourced model is the authority; this is a + /// data-shape mutation, not an enforcement boundary. + #[doc(hidden)] + pub fn set_admin_for_materializer(&mut self, peer: EndpointId, is_admin: bool) { + if is_admin { + self.admins.insert(peer); + } else { + self.admins.remove(&peer); + } + } + + /// Internal use only — call from `willow-state::materialize` or test + /// helpers. The event-sourced model is the authority; this is a + /// data-shape mutation, not an enforcement boundary. + #[doc(hidden)] + pub fn set_name_for_materializer(&mut self, name: impl Into) { + self.name = name.into(); + } + + /// Internal use only — call from `willow-state::materialize` or test + /// helpers. The event-sourced model is the authority; this is a + /// data-shape mutation, not an enforcement boundary. + #[doc(hidden)] + pub fn set_description_for_materializer(&mut self, description: Option) { + self.description = description; + } + /// All channels in this server. pub fn channels(&self) -> Vec<&Channel> { self.channels.values().collect() @@ -718,7 +810,7 @@ mod tests { #[test] fn admin_has_all_permissions() { let (owner, server) = owner_and_server(); - assert!(server.admins.contains(&owner)); + assert!(server.admins().contains(&owner)); assert!(server.has_permission(&owner, Permission::ManageChannels)); assert!(server.has_permission(&owner, Permission::SendMessages)); assert!(server.has_permission(&owner, Permission::SyncProvider)); @@ -802,8 +894,10 @@ mod tests { let bob = Identity::generate().endpoint_id(); server.add_member(bob); - // Add bob to admins set directly. - server.admins.insert(bob); + // Promote bob to admin via the materializer-only setter — this + // is the data-shape mirror of an event the state machine would + // have applied. It is NOT an enforcement boundary. + server.set_admin_for_materializer(bob, true); // Admins have all permissions implicitly. assert!(server.has_permission(&bob, Permission::ManageChannels)); @@ -883,7 +977,7 @@ mod tests { let bytes = willow_transport::pack(&server).unwrap(); let decoded: Server = willow_transport::unpack(&bytes).unwrap(); - assert_eq!(decoded.name, server.name); + assert_eq!(decoded.name(), server.name()); assert_eq!(decoded.channels().len(), 1); } @@ -996,11 +1090,11 @@ mod tests { #[test] fn server_description() { let (_, mut server) = owner_and_server(); - server.description = Some("A cool server".into()); + server.set_description_for_materializer(Some("A cool server".into())); let bytes = willow_transport::pack(&server).unwrap(); let decoded: Server = willow_transport::unpack(&bytes).unwrap(); - assert_eq!(decoded.description.as_deref(), Some("A cool server")); + assert_eq!(decoded.description(), Some("A cool server")); } #[test] @@ -1032,4 +1126,39 @@ mod tests { let decoded: Channel = willow_transport::unpack(&bytes).unwrap(); assert!(decoded.pinned_messages.is_empty()); } + + #[test] + fn server_id_name_description_admins_are_encapsulated() { + // Regression test for issue #118: identity-bearing fields on + // `Server` are private. The only public way to read them is via + // accessor methods, and the only mutation paths are explicit + // `*_for_materializer` setters or the `with_id` constructor. + // This test exists so removing those accessors or re-exposing + // the fields breaks the build. + let (owner, server) = owner_and_server(); + + // Accessors return references / Option references. + let _: &ServerId = server.id(); + let _: &str = server.name(); + let _: Option<&str> = server.description(); + let _: &HashSet = server.admins(); + + // Mutation only via the explicit constructor / setters. + let custom_id = ServerId::new(); + let mut other = Server::with_id(custom_id.clone(), "Other", owner); + assert_eq!(other.id(), &custom_id); + + other.set_name_for_materializer("Renamed"); + assert_eq!(other.name(), "Renamed"); + + other.set_description_for_materializer(Some("desc".into())); + assert_eq!(other.description(), Some("desc")); + + let stranger = Identity::generate().endpoint_id(); + assert!(!other.admins().contains(&stranger)); + other.set_admin_for_materializer(stranger, true); + assert!(other.admins().contains(&stranger)); + other.set_admin_for_materializer(stranger, false); + assert!(!other.admins().contains(&stranger)); + } } From b76b5ca4c26caeb808fea385083c147951cc360d Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Apr 2026 07:58:58 +0000 Subject: [PATCH 2/2] client: route Server field reads through new accessor methods Follow-up to the willow-channel encapsulation of id/name/description/ admins: update all call sites in willow-client to use the new accessor methods (id(), name(), description(), admins()). The one pre-existing mutation site in joining.rs (which set server.id after construction) is rewritten to use the new Server::with_id constructor instead. No behavior change. Progresses #108 --- crates/client/src/invite.rs | 12 ++++++------ crates/client/src/joining.rs | 9 ++++++--- crates/client/src/lib.rs | 10 +++++----- crates/client/src/servers.rs | 2 +- crates/client/src/state.rs | 2 +- crates/client/src/util.rs | 2 +- 6 files changed, 20 insertions(+), 17 deletions(-) diff --git a/crates/client/src/invite.rs b/crates/client/src/invite.rs index ab5c3715..63147501 100644 --- a/crates/client/src/invite.rs +++ b/crates/client/src/invite.rs @@ -83,8 +83,8 @@ pub fn generate_invite( } let payload = InvitePayload { - server_name: server.name.clone(), - server_id: server.id.to_string(), + server_name: server.name().to_string(), + server_id: server.id().to_string(), genesis_author: server.creator, sync_providers: Vec::new(), // populated by caller if known channels, @@ -160,7 +160,7 @@ mod tests { let mut keys = HashMap::new(); let mut topic_map = HashMap::new(); - let topic = format!("{}/general", server.id); + let topic = format!("{}/general", server.id()); if let Some(key) = server.channel_key(&ch_id) { keys.insert(topic.clone(), key.clone()); @@ -198,7 +198,7 @@ mod tests { let mut keys = HashMap::new(); let mut topic_map = HashMap::new(); - let topic = format!("{}/secret", server.id); + let topic = format!("{}/secret", server.id()); if let Some(key) = server.channel_key(&ch_id) { keys.insert(topic.clone(), key.clone()); @@ -249,7 +249,7 @@ mod tests { let mut topic_map = HashMap::new(); for (ch_id, name) in [(ch1, "general"), (ch2, "random"), (ch3, "voice")] { - let topic = format!("{}/{name}", server.id); + let topic = format!("{}/{name}", server.id()); if let Some(key) = server.channel_key(&ch_id) { keys.insert(topic.clone(), key.clone()); } @@ -280,7 +280,7 @@ mod tests { let mut keys = HashMap::new(); let mut topic_map = HashMap::new(); - let topic = format!("{}/general", server.id); + let topic = format!("{}/general", server.id()); if let Some(key) = server.channel_key(&ch_id) { keys.insert(topic.clone(), key.clone()); diff --git a/crates/client/src/joining.rs b/crates/client/src/joining.rs index b97e0484..0f1e5749 100644 --- a/crates/client/src/joining.rs +++ b/crates/client/src/joining.rs @@ -63,11 +63,14 @@ impl ClientHandle { } } } else { - let mut server = - willow_channel::Server::new(&accepted.server_name, accepted.genesis_author); - server.id = willow_channel::ServerId( + let parsed_id = willow_channel::ServerId( uuid::Uuid::parse_str(&server_id).unwrap_or_else(|_| uuid::Uuid::new_v4()), ); + let mut server = willow_channel::Server::with_id( + parsed_id, + &accepted.server_name, + accepted.genesis_author, + ); let mut topic_map = HashMap::new(); let mut keys = HashMap::new(); for (topic, (name, key)) in &accepted.channel_keys { diff --git a/crates/client/src/lib.rs b/crates/client/src/lib.rs index bfb07954..25b39b80 100644 --- a/crates/client/src/lib.rs +++ b/crates/client/src/lib.rs @@ -336,7 +336,7 @@ impl ClientHandle { // Fall back to legacy single-server storage. Do NOT create a default. if state.servers.is_empty() { if let Some((server, keys)) = storage::load_server() { - let sid = server.id.to_string(); + let sid = server.id().to_string(); let mut topic_map = HashMap::new(); for ch in server.channels() { let topic = util::make_topic(&server, &ch.name); @@ -366,7 +366,7 @@ impl ClientHandle { } else { state.event_state = willow_state::ServerState::new( sid.clone(), - ctx.server.name.clone(), + ctx.server.name().to_string(), ctx.server.creator, ); @@ -425,7 +425,7 @@ impl ClientHandle { id.clone(), state_actors::ServerEntry { server: ctx.server.clone(), - name: ctx.server.name.clone(), + name: ctx.server.name().to_string(), topic_map: ctx.topic_map.clone(), keys: ctx.keys.clone(), unread: ctx.unread.clone(), @@ -684,7 +684,7 @@ pub fn test_client() -> ( .unwrap(); let topic = util::make_topic(&server, "general"); - let server_id = server.id.to_string(); + let server_id = server.id().to_string(); let mut topic_map = HashMap::new(); let mut keys = HashMap::new(); @@ -751,7 +751,7 @@ pub fn test_client() -> ( id.clone(), state_actors::ServerEntry { server: ctx.server.clone(), - name: ctx.server.name.clone(), + name: ctx.server.name().to_string(), topic_map: ctx.topic_map.clone(), keys: ctx.keys.clone(), unread: ctx.unread.clone(), diff --git a/crates/client/src/servers.rs b/crates/client/src/servers.rs index a90376a4..6d5b0a6e 100644 --- a/crates/client/src/servers.rs +++ b/crates/client/src/servers.rs @@ -95,7 +95,7 @@ impl ClientHandle { &self.server_registry_addr, move |reg| -> anyhow::Result<(String, String)> { let mut server = willow_channel::Server::new(&name, peer_id); - let server_id = server.id.to_string(); + let server_id = server.id().to_string(); let ch_id = server .create_channel("general", willow_channel::ChannelKind::Text) .map_err(|e| anyhow::anyhow!("{e:?}"))?; diff --git a/crates/client/src/state.rs b/crates/client/src/state.rs index f9f9da9b..c055270b 100644 --- a/crates/client/src/state.rs +++ b/crates/client/src/state.rs @@ -168,7 +168,7 @@ impl ClientState { pub fn server_list(&self) -> Vec<(String, String)> { self.servers .iter() - .map(|(id, ctx)| (id.clone(), ctx.server.name.clone())) + .map(|(id, ctx)| (id.clone(), ctx.server.name().to_string())) .collect() } diff --git a/crates/client/src/util.rs b/crates/client/src/util.rs index c89d4ccb..3ee53941 100644 --- a/crates/client/src/util.rs +++ b/crates/client/src/util.rs @@ -24,7 +24,7 @@ pub fn format_timestamp(ms: u64) -> String { /// Build a gossipsub topic string from a server ID and channel name. pub fn make_topic(server: &willow_channel::Server, channel_name: &str) -> String { - format!("{}/{}", server.id, channel_name) + format!("{}/{}", server.id(), channel_name) } /// Get the current wall-clock time in milliseconds since the Unix epoch.