channel: encapsulate Server fields and clarify authority boundary#138
Merged
Conversation
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
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
Owner
Author
|
Audit note: this PR and #139 both edit Combined intent (for whoever rebases second): // Validate upfront (from #139)
let parsed_server_uuid = uuid::Uuid::parse_str(&server_id).map_err(|e| {
crate::ClientError::MalformedInvite(format!("invalid server_id `{server_id}`: {e}"))
})?;
// ...
// Use with_id constructor (from #138) with the validated UUID
let mut server = willow_channel::Server::with_id(
willow_channel::ServerId(parsed_server_uuid),
&accepted.server_name,
accepted.genesis_author,
);
// Propagate channel creation errors (from #139)
let ch_id = server
.create_channel(name, willow_channel::ChannelKind::Text)
.map_err(|e| {
crate::ClientError::MalformedInvite(format!(
"could not create channel `{name}` from invite: {e}"
))
})?;Planned merge order: #138 first, then rebase #139 on top of main. No action needed on this PR. Generated by Claude Code |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #118
Progresses #108
Summary
crates/channel/src/lib.rsexposedServer::admins,Server::id,Server::name, andServer::descriptionaspubmutable fields. Anycaller holding
&mut Servercould doserver.admins.insert(me)andgrant itself admin status, quietly bypassing the event-sourced
authority model in
willow-state.This PR makes those four identity-bearing fields private, adds read
accessors (
id(),name(),description(),admins()), adds aServer::with_id(id, name, creator)constructor for the join path,and adds three
#[doc(hidden)]set_*_for_materializersetters whosenames and docs make clear they are a data-shape mirror, not an
enforcement boundary.
A new module-level doc on
willow-channeland an expanded doc onServerspell out that all authority enforcement lives inwillow-state::materialize::apply_*— direct mutation of aServervalue is not a trust decision.
The other fields on
Server(channels,roles,members,invites) were already private and are untouched.Design note on the materializer shim
The
willow-statematerializer lives in a different crate, sopub(crate)would not be visible to it. I evaluated three options forletting it (or any future code) mutate the private fields:
pub(crate)— rejected (not visible cross-crate)#[doc(hidden)] pub fn set_*_for_materializerwith an explicitcontract in the doc comment — chosen
&mut HashSet<EndpointId>admins_mutaccessor — rejected(re-introduces the exact footgun we're trying to fix)
In practice, the current
willow-statematerializer operates on itsown
ServerStatetype and does not actually touchwillow_channel:: Server. The setters are therefore unused by the materializer today,but they are exercised by the new encapsulation regression test and
are ready for any future code that needs them. The
#[doc(hidden)]annotation keeps them out of generated docs, and the
_for_materializersuffix plus the doc comment ("this is a data-shapemutation, not an enforcement boundary") discourages accidental use.
The one pre-existing mutation site —
crates/client/src/joining.rs,which parsed a server ID from an invite payload and then assigned
server.id = ...after callingServer::new— is rewritten to usethe new
Server::with_idconstructor. That is the only structuralchange outside
crates/channel/.Test plan
server_id_name_description_admins_are_encapsulatedregression test in
crates/channel/src/lib.rsthat exercisesthe accessors,
with_id, and eachset_*_for_materializersetter.
admin_has_all_permissionsandadmins_set_grants_everythingtests rewritten to use
server.admins()/set_admin_for_ materializerso they still cover the admin-permission path.server_descriptionandserver_serde_round_triprewrittento use the new accessors / setters; serde round-trip still
covers all four previously-public fields (visibility does not
affect the wire format).
cargo fmt --checkclean.cargo clippy --workspace --all-targets -- -D warningsclean.cargo test -p willow-channel(27 passed).cargo test -p willow-state -p willow-client(155 passed).cargo test -p willow-agent(29 passed).cargo test -p willow-relay -p willow-storage -p willow-replay -p willow-workerall pass.cargo test -p willow-network(integration + unit all pass).cargo test -p willow-actor -p willow-common -p willow-identity -p willow-transport -p willow-messaging -p willow-cryptoallpass.
cargo check --workspaceclean.cargo check --target wasm32-unknown-unknownacross all WASMcrates (the
just check-wasmset) clean.Note on cross-crate touches
Per the task scope, the primary change lives in
crates/channel/.Call-site updates in
crates/client/were unavoidable becauseServeris used pervasively for reads and (in one place) writes:crates/client/src/util.rs—make_topicnow callsserver.id().crates/client/src/invite.rs—generate_inviteand four testsnow read through accessors.
crates/client/src/lib.rs— five read sites rewritten to useserver.id()/server.name().crates/client/src/servers.rs—create_serverusesserver.id().crates/client/src/state.rs—server_listusesserver.name().crates/client/src/joining.rs— one write site rewritten to usethe new
Server::with_idconstructor (overlaps with Agent C'sterritory, but this single-location structural change is the
minimal edit needed after the encapsulation, and an equivalent
read-through-accessor shim would still have required touching the
write site).
No files outside
crates/channel/andcrates/client/were modified.I did not touch
crates/state/,crates/storage/,crates/identity/,crates/crypto/,crates/relay/src/main.rs, orcrates/client/src/listeners.rs.https://claude.ai/code/session_018TTGhL645aTR4RZWrRBLPS