From 15255a090797f85874921120003c645962fefaed Mon Sep 17 00:00:00 2001 From: tornquist Date: Thu, 3 Sep 2026 15:58:01 +0000 Subject: [PATCH 1/3] refactor(relay): extract NIP-29 membership authority into pure channel_authz MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validate_admin_event` carried the kind:9000/9001/9022 authorization policy inline in a 452-line `async fn` over `&Arc`, so none of it could be tested without Postgres and Redis. Last-owner protection alone was restated five times across three shapes, in two of which it had drifted to a different error string. Add `handlers::channel_authz`: data-only inputs, typed decisions, no state handle. `validate_admin_event` keeps every database read and calls the pure policy — the same shell/pure split `handlers::moderation_authz` already uses. The five last-owner restatements collapse to one `is_sole_owner` predicate. Both historical phrasings are preserved as distinct `ChannelAuthzError` variants, so no client-visible message changes: the rule is defined once, the wording stays per call site. kind:9001 self-removal and kind:9022 leave were character-identical and now share `decide_self_departure`. The buzz-db last-owner guards stay as independent defence in depth. Widen the `just test-unit` nextest filter to select the relay's pure authorization-decision tests. They ran in no lane before: `test(/^api::admin::/)` never matched them and the PostgreSQL lane pairs `--run-ignored ignored-only` with a `postgres_tests::` default-filter, so 14 existing tests in `handlers::side_effects` and `handlers::moderation_authz` could go red and still ship green. Scoped to the three decision modules rather than all of `handlers::`, which is mostly Postgres-backed. Behaviour is unchanged: the full `e2e_relay` suite against a live relay is identical before and after (45 pass; `test_unarchive_emits_member_added_ notification` fails on both, a pre-existing kind:9002 failure). Refs: https://github.com/TheSentinel454/buzz/issues/24 Co-Authored-By: Claude Opus 5 Signed-off-by: tornquist --- Justfile | 15 +- .../buzz-relay/src/handlers/channel_authz.rs | 694 ++++++++++++++++++ crates/buzz-relay/src/handlers/mod.rs | 2 + .../buzz-relay/src/handlers/side_effects.rs | 175 ++--- 4 files changed, 752 insertions(+), 134 deletions(-) create mode 100644 crates/buzz-relay/src/handlers/channel_authz.rs diff --git a/Justfile b/Justfile index c81adb2381b..6df2aff7976 100644 --- a/Justfile +++ b/Justfile @@ -429,8 +429,21 @@ test-unit: # disabled_mode_regression_pin_unauthenticated_request_is_served on the # DB-free /probe route, and its Host/Origin gating is covered here by # disabled_mode_still_requires_the_correct_host / _a_matching_origin. + # The second clause adds the relay's pure authorization-decision tests: + # the NIP-29 channel membership grid (handlers::channel_authz), the + # moderation capability grid (handlers::moderation_authz), and the pure + # helpers in handlers::side_effects. They ran in NO lane before — + # `test(/^api::admin::/)` never matched them, and the PostgreSQL lane + # pairs `--run-ignored ignored-only` with a `postgres_tests::` + # default-filter — so a red one shipped green, exactly the gap the + # api::admin clause above was added to close. + # Deliberately scoped to these three modules instead of all of + # `handlers::`: the wider set is mostly Postgres-backed, and five of its + # non-postgres_tests cases only "pass" without a database by waiting out + # the ~30s sqlx acquire timeout, so they do not belong in the infra-free + # unit job either. cargo nextest run -p buzz-relay --lib \ - -E 'test(/^api::admin::/) - test(=api::admin::tests::disabled_mode_allows_unauthenticated_requests_on_the_admin_host) - test(=api::admin::tests::nip98_mode_unrostered_signer_does_not_consume_a_replay_slot)' + -E '(test(/^api::admin::/) - test(=api::admin::tests::disabled_mode_allows_unauthenticated_requests_on_the_admin_host) - test(=api::admin::tests::nip98_mode_unrostered_signer_does_not_consume_a_replay_slot)) + test(/^handlers::channel_authz::/) + test(/^handlers::moderation_authz::/) + test(/^handlers::side_effects::tests::/)' # ACP author-gate and queue tests protect the trust boundary between # relay events and agent prompts. They are infra-free; ignored lifecycle # tests remain excluded and run in their dedicated integration lanes. diff --git a/crates/buzz-relay/src/handlers/channel_authz.rs b/crates/buzz-relay/src/handlers/channel_authz.rs new file mode 100644 index 00000000000..08de2debc9f --- /dev/null +++ b/crates/buzz-relay/src/handlers/channel_authz.rs @@ -0,0 +1,694 @@ +//! Pure NIP-29 channel membership-authority decisions (kinds 9000/9001/9022). +//! +//! `validate_admin_event` in [`super::side_effects`] keeps every database read; +//! this module holds only the policy those reads feed. Each function takes +//! already-resolved data and returns a typed decision, so the authorization +//! rules are exhaustively unit-testable without Postgres or Redis — the same +//! shell/pure split [`super::moderation_authz`] uses for the moderation +//! capability grid. +//! +//! ## Error strings are the wire contract +//! +//! Every [`ChannelAuthzError`] message is returned to clients verbatim in the +//! NIP-29 `OK` frame. The variants deliberately preserve the two historically +//! distinct last-owner phrasings — [`ChannelAuthzError::LastOwnerRemoval`] for +//! the pre-storage validator and +//! [`ChannelAuthzError::LastOwnerRemovalTransferFirst`] for the side-effect +//! appliers. The *rule* is defined once in [`is_sole_owner`]; only the wording +//! differs per call site. + +use buzz_db::channel::{MemberRecord, MemberRole}; + +/// A membership-authority denial. `Display` is the client-visible reason. +#[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] +pub enum ChannelAuthzError { + /// Actor holds no authority for this action. + #[error("actor not authorized")] + ActorNotAuthorized, + /// Actor tried to grant `owner`/`admin` without holding it. + #[error("only owners/admins may grant elevated roles")] + ElevatedRoleGrantDenied, + /// Actor tried to change an active member's role without being elevated. + #[error("only owners/admins may change an active member's role")] + RoleChangeDenied, + /// Demoting the channel's only owner would orphan it. + #[error("cannot demote the last owner — transfer ownership first")] + LastOwnerDemotion, + /// Actor is not an active member of the channel. + #[error("actor is not an active member")] + NotActiveMember, + /// Removing the channel's only owner would orphan it (validator wording). + #[error("cannot remove the last owner")] + LastOwnerRemoval, + /// Removing the channel's only owner would orphan it (applier wording). + #[error("cannot remove the last owner — transfer ownership first")] + LastOwnerRemovalTransferFirst, + /// `owner_only` policy on an agent with no owner recorded. + #[error("policy:owner_only — agent has no owner set")] + PolicyOwnerOnlyNoOwner, + /// `owner_only` policy and the actor is not the agent's owner. + #[error("policy:owner_only — only the agent owner can add this agent")] + PolicyOwnerOnlyDenied, + /// `nobody` policy — the agent has opted out of third-party adds. + #[error("policy:nobody — this agent has disabled external channel additions")] + PolicyNobody, +} + +/// Whether `pubkey` is the channel's only remaining `owner`. +/// +/// This is the single definition of last-owner protection. Every call site +/// that once restated it — kind:9000 demotion, kind:9001 self-removal, +/// kind:9022 leave, and the `handle_remove_user` / `handle_leave_request` +/// appliers — asks this one question and supplies its own wording. +/// +/// `members` must already be filtered to *active* membership; both +/// `get_members` and `get_members_for_event_write` are, so a soft-removed +/// owner row never counts toward the roster. +pub fn is_sole_owner(members: &[MemberRecord], pubkey: &[u8]) -> bool { + let mut owners = members.iter().filter(|m| m.role == "owner"); + let sole = matches!(owners.next(), Some(first) if first.pubkey == pubkey); + sole && owners.next().is_none() +} + +/// Decide whether `actor` may remove themselves from the channel. +/// +/// Shared by kind:9001 self-removal and kind:9022 leave — they enforced +/// character-identical rules and messages before this seam existed. +pub fn decide_self_departure( + members: &[MemberRecord], + actor: &[u8], +) -> Result<(), ChannelAuthzError> { + if !members.iter().any(|m| m.pubkey == actor) { + return Err(ChannelAuthzError::NotActiveMember); + } + if is_sole_owner(members, actor) { + return Err(ChannelAuthzError::LastOwnerRemoval); + } + Ok(()) +} + +/// The outcome of a kind:9000 membership-authority check. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PutUserDecision { + /// Authorized outright. A self-add bypasses the target's agent + /// `channel_add_policy` — you may always add yourself. + Allow, + /// Authorized so far; the caller must still load and evaluate the target's + /// agent `channel_add_policy` via [`decide_channel_add_policy`]. + CheckAddPolicy, +} + +/// Decide whether `actor` may add `target` to the channel, or change the role +/// `target` already holds (NIP-29 kind:9000 PUT_USER). +/// +/// `actor_role` and `members` must come from an *active* membership read; +/// `requested_role` is `None` when the event carries no `role` tag, which +/// means "no role change requested" rather than "demote to member". +/// +/// The database read for the target's agent channel-add policy stays with the +/// caller: this returns [`PutUserDecision::CheckAddPolicy`] when that read is +/// still required. +pub fn decide_put_user( + visibility: &str, + actor_role: Option, + requested_role: Option, + members: &[MemberRecord], + target: &[u8], + actor: &[u8], +) -> Result { + // Open channels allow any authenticated user; private channels require the + // actor to be an existing active member. Any active member may add an + // ordinary member, guest, or bot, but only owners/admins may grant an + // elevated role. + if visibility == "private" { + if actor_role.is_none() { + return Err(ChannelAuthzError::ActorNotAuthorized); + } + + if requested_role.is_some_and(|role| role.is_elevated()) + && !actor_role.is_some_and(|role| role.is_elevated()) + { + return Err(ChannelAuthzError::ElevatedRoleGrantDenied); + } + } + + // Changing an ACTIVE existing member's role is privileged in both + // directions, on every visibility. `members` comes from a `removed_at IS + // NULL` read, so a soft-removed row is deliberately not an "existing + // member" here: its stored role is history, not live authority, and + // reactivation is governed by the elevated-granter check above rather than + // by the role the row remembers. + // + // `add_member` is the authority (it also covers the desktop/admin callers + // that skip this validator); rejecting here too means the client gets a + // real error instead of an OK for an event whose side effect then fails. + // Re-adding at the same role stays idempotent — the huddle bot-add path + // relies on that. + if let Some((existing, role)) = members + .iter() + .find(|m| m.pubkey == target) + .zip(requested_role) + .filter(|(m, role)| m.role != role.as_str()) + { + if !actor_role.is_some_and(|r| r.is_elevated()) { + return Err(ChannelAuthzError::RoleChangeDenied); + } + if existing.role == "owner" && role != MemberRole::Owner && is_sole_owner(members, target) { + return Err(ChannelAuthzError::LastOwnerDemotion); + } + } + + // Self-add: always allowed regardless of the target's agent policy. + if target == actor { + return Ok(PutUserDecision::Allow); + } + + Ok(PutUserDecision::CheckAddPolicy) +} + +/// Evaluate a target agent's `channel_add_policy` for a third-party add. +/// +/// `policy` and `owner` come from `get_agent_channel_policy`; callers skip +/// this entirely when the target has no policy row, or when the add is a +/// self-add ([`PutUserDecision::Allow`]). +/// +/// Unknown policy values allow. The database enum prevents them from being +/// stored, so this is defence in depth rather than reachable behaviour — if a +/// new value is added to the enum, extend this match. +pub fn decide_channel_add_policy( + policy: &str, + owner: Option<&[u8]>, + actor: &[u8], +) -> Result<(), ChannelAuthzError> { + match policy { + "owner_only" => { + let owner = owner.ok_or(ChannelAuthzError::PolicyOwnerOnlyNoOwner)?; + if actor != owner { + return Err(ChannelAuthzError::PolicyOwnerOnlyDenied); + } + Ok(()) + } + "nobody" => Err(ChannelAuthzError::PolicyNobody), + _ => Ok(()), + } +} + +/// What authority `actor` holds to remove *somebody else* (kind:9001). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RemoveOtherDecision { + /// Channel owner or admin — authorized for any target. + Allow, + /// An active non-elevated member. Authorized only if they own the target + /// agent, which the caller must confirm with a database read. + CheckAgentOwner, + /// Not an active member. Denied without any further read — you must be in + /// the channel to remove anyone, even your own bot. + Deny, +} + +/// Classify `actor`'s authority to remove another member (NIP-29 kind:9001). +pub fn classify_remove_other(members: &[MemberRecord], actor: &[u8]) -> RemoveOtherDecision { + match members.iter().find(|m| m.pubkey == actor) { + Some(m) if m.role == "owner" || m.role == "admin" => RemoveOtherDecision::Allow, + Some(_) => RemoveOtherDecision::CheckAgentOwner, + None => RemoveOtherDecision::Deny, + } +} + +#[cfg(test)] +mod tests { + use super::*; + use chrono::Utc; + use uuid::Uuid; + + /// Build a member roster from `(pubkey_byte, role)` pairs. + fn roster(entries: &[(u8, &str)]) -> Vec { + let channel_id = Uuid::new_v4(); + entries + .iter() + .map(|(tag, role)| MemberRecord { + channel_id, + pubkey: vec![*tag; 32], + role: (*role).to_string(), + joined_at: Utc::now(), + invited_by: None, + removed_at: None, + }) + .collect() + } + + fn pk(tag: u8) -> Vec { + vec![tag; 32] + } + + /// A roster literal: `(pubkey_tag, role)` pairs. + type Roster<'a> = &'a [(u8, &'a str)]; + /// `(roster, subject, expected)`. + type SoleOwnerCase<'a> = (Roster<'a>, u8, bool); + /// `(roster, actor, expected_error)`. + type DepartureCase<'a> = (Roster<'a>, u8, Option); + /// `(roster, actor, expected_decision)`. + type RemoveOtherCase<'a> = (Roster<'a>, u8, RemoveOtherDecision); + /// `(policy, owner_tag, actor, expected_error)`. + type AddPolicyCase<'a> = (&'a str, Option, u8, Option); + + /// The single definition of last-owner protection, over the full shape + /// space the five former call sites covered. + #[test] + fn sole_owner_table() { + let cases: &[SoleOwnerCase] = &[ + // Only owner, and it is the subject. + (&[(1, "owner")], 1, true), + (&[(1, "owner"), (2, "member")], 1, true), + (&[(1, "owner"), (2, "admin"), (3, "bot")], 1, true), + // Only owner, but the subject is somebody else. + (&[(1, "owner"), (2, "member")], 2, false), + (&[(1, "owner")], 2, false), + // Two owners: neither is sole. + (&[(1, "owner"), (2, "owner")], 1, false), + (&[(1, "owner"), (2, "owner")], 2, false), + (&[(1, "owner"), (2, "owner"), (3, "member")], 3, false), + // No owners at all. + (&[(1, "member"), (2, "admin")], 1, false), + (&[], 1, false), + // An admin is not an owner. + (&[(1, "admin")], 1, false), + ]; + + for (entries, subject, expected) in cases { + let members = roster(entries); + assert_eq!( + is_sole_owner(&members, &pk(*subject)), + *expected, + "roster {entries:?} subject {subject}" + ); + } + } + + /// kind:9001 self-removal and kind:9022 leave share one rule: the actor + /// must be an active member, and must not be the channel's only owner. + #[test] + fn self_departure_table() { + let cases: &[DepartureCase] = &[ + // Non-member cannot leave. + (&[(1, "owner")], 9, Some(ChannelAuthzError::NotActiveMember)), + (&[], 1, Some(ChannelAuthzError::NotActiveMember)), + // Sole owner is pinned. + ( + &[(1, "owner"), (2, "member")], + 1, + Some(ChannelAuthzError::LastOwnerRemoval), + ), + ( + &[(1, "owner")], + 1, + Some(ChannelAuthzError::LastOwnerRemoval), + ), + // Co-owner may leave. + (&[(1, "owner"), (2, "owner")], 1, None), + // Non-owner roles may always leave. + (&[(1, "owner"), (2, "member")], 2, None), + (&[(1, "owner"), (2, "admin")], 2, None), + (&[(1, "owner"), (2, "bot")], 2, None), + (&[(1, "owner"), (2, "guest")], 2, None), + ]; + + for (entries, actor, expected) in cases { + let members = roster(entries); + assert_eq!( + decide_self_departure(&members, &pk(*actor)).err(), + *expected, + "roster {entries:?} actor {actor}" + ); + } + } + + /// kind:9000 authorization, over visibility × actor role × requested role + /// × existing-target shape. Ordering matters: the private-channel gates + /// run before the role-change gates, which run before the self-add + /// shortcut. + #[test] + fn put_user_table() { + use ChannelAuthzError as E; + use MemberRole::{Admin, Member, Owner}; + use PutUserDecision::{Allow, CheckAddPolicy}; + + // (visibility, roster, actor, actor_role, target, requested_role, expected) + type Case<'a> = ( + &'a str, + &'a [(u8, &'a str)], + u8, + Option, + u8, + Option, + Result, + ); + let cases: &[Case] = &[ + // ── Private channels require the actor to be an active member ── + ( + "private", + &[(1, "owner")], + 9, + None, + 5, + Some(Member), + Err(E::ActorNotAuthorized), + ), + // Even a self-add cannot bootstrap membership into a private channel. + ( + "private", + &[(1, "owner")], + 9, + None, + 9, + None, + Err(E::ActorNotAuthorized), + ), + // ── Private: only elevated actors may grant elevated roles ── + ( + "private", + &[(1, "owner"), (2, "member")], + 2, + Some(Member), + 5, + Some(Admin), + Err(E::ElevatedRoleGrantDenied), + ), + ( + "private", + &[(1, "owner"), (2, "member")], + 2, + Some(Member), + 5, + Some(Owner), + Err(E::ElevatedRoleGrantDenied), + ), + // An elevated actor may grant an elevated role. + ( + "private", + &[(1, "owner")], + 1, + Some(Owner), + 5, + Some(Admin), + Ok(CheckAddPolicy), + ), + ( + "private", + &[(1, "owner"), (2, "admin")], + 2, + Some(Admin), + 5, + Some(Admin), + Ok(CheckAddPolicy), + ), + // A plain member may still add an ordinary member to a private channel. + ( + "private", + &[(1, "owner"), (2, "member")], + 2, + Some(Member), + 5, + Some(Member), + Ok(CheckAddPolicy), + ), + // ── Open channels skip both private gates entirely ── + ( + "open", + &[(1, "owner")], + 9, + None, + 5, + Some(Member), + Ok(CheckAddPolicy), + ), + ( + "open", + &[(1, "owner")], + 9, + None, + 5, + Some(Admin), + Ok(CheckAddPolicy), + ), + // ── Changing an ACTIVE member's role is privileged on every visibility ── + ( + "open", + &[(1, "owner"), (2, "member")], + 9, + None, + 2, + Some(Admin), + Err(E::RoleChangeDenied), + ), + ( + "open", + &[(1, "owner"), (2, "member"), (3, "member")], + 3, + Some(Member), + 2, + Some(Admin), + Err(E::RoleChangeDenied), + ), + // Demotion is privileged in the same way as promotion. + ( + "open", + &[(1, "owner"), (2, "admin"), (3, "member")], + 3, + Some(Member), + 2, + Some(Member), + Err(E::RoleChangeDenied), + ), + // An elevated actor may change roles. + ( + "open", + &[(1, "owner"), (2, "member")], + 1, + Some(Owner), + 2, + Some(Admin), + Ok(CheckAddPolicy), + ), + // ── Last-owner demotion guard ── + // The sole owner demoting themselves. + ( + "open", + &[(1, "owner"), (2, "member")], + 1, + Some(Owner), + 1, + Some(Member), + Err(E::LastOwnerDemotion), + ), + // Another owner demoting the sole owner is impossible (they'd be an + // owner too), but an admin demoting the sole owner is not. + ( + "open", + &[(1, "owner"), (2, "admin")], + 2, + Some(Admin), + 1, + Some(Member), + Err(E::LastOwnerDemotion), + ), + // With a co-owner present the demotion is allowed. + ( + "open", + &[(1, "owner"), (2, "owner")], + 1, + Some(Owner), + 2, + Some(Member), + Ok(CheckAddPolicy), + ), + // Owner → Owner is not a demotion, so the guard does not fire; it is + // also not a role change, so it short-circuits as idempotent. + ( + "open", + &[(1, "owner")], + 1, + Some(Owner), + 1, + Some(Owner), + Ok(Allow), + ), + // ── Re-adding at the same role stays idempotent (huddle bot path) ── + ( + "open", + &[(1, "owner"), (2, "bot")], + 9, + None, + 2, + Some(MemberRole::Bot), + Ok(CheckAddPolicy), + ), + // An absent role tag requests no change, so the role-change gate + // never fires even for an unprivileged actor. + ( + "open", + &[(1, "owner"), (2, "member")], + 9, + None, + 2, + None, + Ok(CheckAddPolicy), + ), + // ── Self-add short-circuits the agent channel-add policy ── + ("open", &[(1, "owner")], 9, None, 9, Some(Member), Ok(Allow)), + ( + "open", + &[(1, "owner"), (2, "member")], + 2, + Some(Member), + 2, + Some(Member), + Ok(Allow), + ), + ( + "private", + &[(1, "owner"), (2, "member")], + 2, + Some(Member), + 2, + None, + Ok(Allow), + ), + ]; + + for (visibility, entries, actor, actor_role, target, requested_role, expected) in cases { + let members = roster(entries); + assert_eq!( + decide_put_user( + visibility, + *actor_role, + *requested_role, + &members, + &pk(*target), + &pk(*actor), + ), + *expected, + "visibility {visibility} roster {entries:?} actor {actor} target {target} requested {requested_role:?}" + ); + } + } + + /// The target's agent `channel_add_policy`, evaluated for a third-party + /// add. Self-adds never reach here. + #[test] + fn channel_add_policy_table() { + use ChannelAuthzError as E; + + // (policy, owner, actor, expected) + let cases: &[AddPolicyCase] = &[ + // "anyone" allows any actor. + ("anyone", None, 7, None), + ("anyone", Some(1), 7, None), + // "nobody" blocks every actor, including the agent's own owner. + ("nobody", Some(7), 7, Some(E::PolicyNobody)), + ("nobody", None, 7, Some(E::PolicyNobody)), + // "owner_only" admits exactly the configured owner. + ("owner_only", Some(7), 7, None), + ("owner_only", Some(1), 7, Some(E::PolicyOwnerOnlyDenied)), + // "owner_only" with no owner recorded is a misconfiguration, and + // fails closed with its own distinct message. + ("owner_only", None, 7, Some(E::PolicyOwnerOnlyNoOwner)), + // Unknown values fall through to allow; the DB enum prevents them + // from being stored, so this is defence in depth. + ("something_new", None, 7, None), + ("", None, 7, None), + ]; + + for (policy, owner, actor, expected) in cases { + let owner_bytes = owner.map(pk); + assert_eq!( + decide_channel_add_policy(policy, owner_bytes.as_deref(), &pk(*actor)).err(), + *expected, + "policy {policy} owner {owner:?} actor {actor}" + ); + } + } + + /// kind:9001 removal of somebody else. A plain member is not denied + /// outright — they may still own the target agent, which requires a + /// database read the caller performs. + #[test] + fn remove_other_table() { + use RemoveOtherDecision::{Allow, CheckAgentOwner, Deny}; + + let cases: &[RemoveOtherCase] = &[ + // Owners and admins may remove anyone. + (&[(1, "owner"), (2, "member")], 1, Allow), + (&[(1, "owner"), (2, "admin")], 2, Allow), + // A plain member, guest, or bot may only remove an agent they own. + (&[(1, "owner"), (2, "member")], 2, CheckAgentOwner), + (&[(1, "owner"), (2, "guest")], 2, CheckAgentOwner), + (&[(1, "owner"), (2, "bot")], 2, CheckAgentOwner), + // Non-members are denied without an agent-owner read: you must be + // in the channel to remove anyone, even your own bot. + (&[(1, "owner")], 9, Deny), + (&[], 9, Deny), + ]; + + for (entries, actor, expected) in cases { + let members = roster(entries); + assert_eq!( + classify_remove_other(&members, &pk(*actor)), + *expected, + "roster {entries:?} actor {actor}" + ); + } + } + + /// The wire contract: these strings reach NIP-29 clients verbatim, and the + /// two last-owner phrasings are deliberately distinct. + #[test] + fn error_strings_are_the_wire_contract() { + let cases = [ + ( + ChannelAuthzError::ActorNotAuthorized, + "actor not authorized", + ), + ( + ChannelAuthzError::ElevatedRoleGrantDenied, + "only owners/admins may grant elevated roles", + ), + ( + ChannelAuthzError::RoleChangeDenied, + "only owners/admins may change an active member's role", + ), + ( + ChannelAuthzError::LastOwnerDemotion, + "cannot demote the last owner — transfer ownership first", + ), + ( + ChannelAuthzError::NotActiveMember, + "actor is not an active member", + ), + ( + ChannelAuthzError::LastOwnerRemoval, + "cannot remove the last owner", + ), + ( + ChannelAuthzError::LastOwnerRemovalTransferFirst, + "cannot remove the last owner — transfer ownership first", + ), + ( + ChannelAuthzError::PolicyOwnerOnlyNoOwner, + "policy:owner_only — agent has no owner set", + ), + ( + ChannelAuthzError::PolicyOwnerOnlyDenied, + "policy:owner_only — only the agent owner can add this agent", + ), + ( + ChannelAuthzError::PolicyNobody, + "policy:nobody — this agent has disabled external channel additions", + ), + ]; + + for (error, expected) in cases { + assert_eq!(error.to_string(), expected); + } + } +} diff --git a/crates/buzz-relay/src/handlers/mod.rs b/crates/buzz-relay/src/handlers/mod.rs index d1c56a2b48f..2f4aa00b595 100644 --- a/crates/buzz-relay/src/handlers/mod.rs +++ b/crates/buzz-relay/src/handlers/mod.rs @@ -2,6 +2,8 @@ pub mod admin_action_worker; pub mod admin_outbox_worker; pub mod auth; +/// Pure NIP-29 channel membership-authority decisions (kinds 9000/9001/9022). +pub mod channel_authz; /// Subscription close (CLOSE) handler. pub mod close; /// Command executor — transactional processing for command kinds. diff --git a/crates/buzz-relay/src/handlers/side_effects.rs b/crates/buzz-relay/src/handlers/side_effects.rs index 8183fe98d80..7282c913423 100644 --- a/crates/buzz-relay/src/handlers/side_effects.rs +++ b/crates/buzz-relay/src/handlers/side_effects.rs @@ -16,6 +16,7 @@ use buzz_core::kind::{ use buzz_core::StoredEvent; use buzz_db::channel::{MemberRecord, MemberRole}; +use super::channel_authz::{self, ChannelAuthzError, PutUserDecision, RemoveOtherDecision}; use super::event::dispatch_persistent_event; use crate::protocol::RelayMessage; use crate::state::AppState; @@ -365,120 +366,51 @@ pub async fn validate_admin_event( let target_pubkey = extract_p_tag(event).ok_or_else(|| anyhow::anyhow!("missing p tag"))?; - // PUT_USER: open channels allow any authenticated user; private channels - // require the actor to be an existing active member. Any active member may - // add an ordinary member, guest, or bot, but only owners/admins may grant - // an elevated role. - if channel.visibility == "private" { - if actor_role.is_none() { - return Err(anyhow::anyhow!("actor not authorized")); - } - - if requested_role.is_some_and(|role| role.is_elevated()) - && !actor_role.is_some_and(|role| role.is_elevated()) - { - return Err(anyhow::anyhow!( - "only owners/admins may grant elevated roles" - )); - } - } - - // Changing an ACTIVE existing member's role is privileged in both - // directions, on every visibility. `get_members` filters - // `removed_at IS NULL`, so a soft-removed row is deliberately not an - // "existing member" here: its stored role is history, not live - // authority, and reactivation is governed by the elevated-granter - // check above rather than by the role the row remembers. - // - // `add_member` is the authority (it also covers the desktop/admin - // callers that skip this validator); rejecting here too means the - // client gets a real error instead of an OK for an event whose side - // effect then fails. Re-adding at the same role stays idempotent — - // the huddle bot-add path relies on that. - if let Some((target, role)) = members - .iter() - .find(|m| m.pubkey == target_pubkey) - .zip(requested_role) - .filter(|(m, role)| m.role != role.as_str()) - { - if !actor_role.is_some_and(|r| r.is_elevated()) { - return Err(anyhow::anyhow!( - "only owners/admins may change an active member's role" - )); - } - if target.role == "owner" - && role != buzz_db::channel::MemberRole::Owner - && members.iter().filter(|m| m.role == "owner").count() <= 1 - { - return Err(anyhow::anyhow!( - "cannot demote the last owner — transfer ownership first" - )); - } - } - - // Self-add: always allowed regardless of policy. - if target_pubkey == actor_bytes { - return Ok(()); - } - - // Third-party add: check channel_add_policy on the target. - if let Some((policy, owner)) = state - .db - .get_agent_channel_policy(tenant.community(), &target_pubkey) - .await? - { - match policy.as_str() { - "owner_only" => { - let owner_bytes = owner.ok_or_else(|| { - anyhow::anyhow!("policy:owner_only — agent has no owner set") - })?; - if actor_bytes != owner_bytes { - return Err(anyhow::anyhow!( - "policy:owner_only — only the agent owner can add this agent" - )); - } - } - "nobody" => { - return Err(anyhow::anyhow!( - "policy:nobody — this agent has disabled external channel additions" - )); + // Authorization policy — visibility gate, elevated-grant gate, + // active-member role-change gate, and last-owner demotion — lives in + // `channel_authz`, which is pure and table-tested. The database reads + // it depends on stay here. + match channel_authz::decide_put_user( + &channel.visibility, + actor_role, + requested_role, + &members, + &target_pubkey, + &actor_bytes, + )? { + // Self-add: always allowed regardless of policy. + PutUserDecision::Allow => Ok(()), + // Third-party add: check channel_add_policy on the target. + PutUserDecision::CheckAddPolicy => { + if let Some((policy, owner)) = state + .db + .get_agent_channel_policy(tenant.community(), &target_pubkey) + .await? + { + channel_authz::decide_channel_add_policy( + &policy, + owner.as_deref(), + &actor_bytes, + )?; } - // "anyone" or any unknown value → allow. - // NOTE: DB ENUM constraint prevents unknown values from being stored. - // If a new policy value is added to the ENUM, update this match. - _ => {} + + Ok(()) } } - - Ok(()) } 9001 => { // REMOVE_USER: self-remove allowed unless actor is the last owner; removing others requires owner/admin let target_pubkey = extract_p_tag(event).ok_or_else(|| anyhow::anyhow!("missing p tag"))?; + let members = state.db.get_members(tenant.community(), channel_id).await?; if target_pubkey == actor_bytes { // Self-removal: must be an active member, and cannot be the last owner. - let members = state.db.get_members(tenant.community(), channel_id).await?; - let actor_member = members.iter().find(|m| m.pubkey == actor_bytes); - match actor_member { - None => { - return Err(anyhow::anyhow!("actor is not an active member")); - } - Some(m) if m.role == "owner" => { - let owner_count = members.iter().filter(|m| m.role == "owner").count(); - if owner_count <= 1 { - return Err(anyhow::anyhow!("cannot remove the last owner")); - } - } - _ => {} - } + channel_authz::decide_self_departure(&members, &actor_bytes)?; Ok(()) } else { - let members = state.db.get_members(tenant.community(), channel_id).await?; - let actor_member = members.iter().find(|m| m.pubkey == actor_bytes); - match actor_member { - Some(m) if m.role == "owner" || m.role == "admin" => Ok(()), - Some(_) => { + match channel_authz::classify_remove_other(&members, &actor_bytes) { + RemoveOtherDecision::Allow => Ok(()), + RemoveOtherDecision::CheckAgentOwner => { if state .db .is_agent_owner(tenant.community(), &target_pubkey, &actor_bytes) @@ -486,13 +418,13 @@ pub async fn validate_admin_event( { Ok(()) } else { - Err(anyhow::anyhow!("actor not authorized")) + Err(ChannelAuthzError::ActorNotAuthorized.into()) } } // Non-members fall here. We intentionally do NOT check // is_agent_owner for non-members — you must be in the channel // to remove anyone, even your own bot. - _ => Err(anyhow::anyhow!("actor not authorized")), + RemoveOtherDecision::Deny => Err(ChannelAuthzError::ActorNotAuthorized.into()), } } } @@ -742,20 +674,9 @@ pub async fn validate_admin_event( } 9022 => { // LEAVE_REQUEST: must be an active member, and cannot be the last owner. + // Identical rule to kind:9001 self-removal, including its wording. let members = state.db.get_members(tenant.community(), channel_id).await?; - let actor_member = members.iter().find(|m| m.pubkey == actor_bytes); - match actor_member { - None => { - return Err(anyhow::anyhow!("actor is not an active member")); - } - Some(m) if m.role == "owner" => { - let owner_count = members.iter().filter(|m| m.role == "owner").count(); - if owner_count <= 1 { - return Err(anyhow::anyhow!("cannot remove the last owner")); - } - } - _ => {} - } + channel_authz::decide_self_departure(&members, &actor_bytes)?; Ok(()) } _ => Ok(()), @@ -1459,14 +1380,8 @@ async fn handle_remove_user( .db .get_members_for_event_write(tenant.community(), channel_id) .await?; - let owner_count = members.iter().filter(|m| m.role == "owner").count(); - let actor_is_owner = members - .iter() - .any(|m| m.pubkey == actor_bytes && m.role == "owner"); - if actor_is_owner && owner_count <= 1 { - return Err(anyhow::anyhow!( - "cannot remove the last owner — transfer ownership first" - )); + if channel_authz::is_sole_owner(&members, &actor_bytes) { + return Err(ChannelAuthzError::LastOwnerRemovalTransferFirst.into()); } } @@ -2126,14 +2041,8 @@ async fn handle_leave_request( .db .get_members_for_event_write(tenant.community(), channel_id) .await?; - let owner_count = members.iter().filter(|m| m.role == "owner").count(); - let actor_is_owner = members - .iter() - .any(|m| m.pubkey == actor_bytes && m.role == "owner"); - if actor_is_owner && owner_count <= 1 { - return Err(anyhow::anyhow!( - "cannot remove the last owner — transfer ownership first" - )); + if channel_authz::is_sole_owner(&members, &actor_bytes) { + return Err(ChannelAuthzError::LastOwnerRemovalTransferFirst.into()); } state From 6494ebbdb354d7aedc445b17a5f8dfef8b20ceb4 Mon Sep 17 00:00:00 2001 From: tornquist Date: Thu, 3 Sep 2026 17:38:43 +0000 Subject: [PATCH 2/3] test(relay): bind departure guards to the wire path Exercise kind 9001 and 9022 self-departure through the live relay path, pinning the exact accepted status and OK-frame message for sole owners, nonmembers, and co-owner departures. Mirror the three infra-free relay handler modules from the nextest unit expression in the cargo-test fallback so either runner covers the same authorization decisions. Signed-off-by: tornquist Co-authored-by: Codex --- crates/buzz-test-client/tests/e2e_relay.rs | 148 +++++++++++++++++++++ scripts/run-tests.sh | 12 ++ 2 files changed, 160 insertions(+) diff --git a/crates/buzz-test-client/tests/e2e_relay.rs b/crates/buzz-test-client/tests/e2e_relay.rs index b119d267740..fa0ba21037c 100644 --- a/crates/buzz-test-client/tests/e2e_relay.rs +++ b/crates/buzz-test-client/tests/e2e_relay.rs @@ -2345,6 +2345,154 @@ async fn add_member_with_role_ws( (ok.accepted, ok.message) } +/// Submit a self-targeted NIP-29 departure and return the relay's exact OK +/// frame payload. kind:9001 carries a self `p` tag; kind:9022 does not. +async fn self_departure_ws(url: &str, channel_id: &str, actor: &Keys, kind: u16) -> (bool, String) { + let h_tag = Tag::parse(["h", channel_id]).unwrap(); + let event = match kind { + 9001 => EventBuilder::new(Kind::Custom(kind), "") + .allow_self_tagging() + .tags([ + h_tag, + Tag::parse(["p", &actor.public_key().to_hex()]).unwrap(), + ]) + .sign_with_keys(actor) + .expect("sign kind:9001 self-removal"), + 9022 => EventBuilder::new(Kind::Custom(kind), "") + .tags([h_tag]) + .sign_with_keys(actor) + .expect("sign kind:9022 leave request"), + _ => panic!("unsupported self-departure kind: {kind}"), + }; + + let mut client = BuzzTestClient::connect(url, actor) + .await + .expect("connect departure actor"); + let ok = client.send_event(event).await.expect("send self-departure"); + client.disconnect().await.ok(); + (ok.accepted, ok.message) +} + +async fn promote_co_owner(url: &str, channel_id: &str, owner: &Keys, co_owner: &Keys) { + let mut client = BuzzTestClient::connect(url, owner) + .await + .expect("connect channel owner"); + let result = add_member_with_role_ws( + &mut client, + channel_id, + &co_owner.public_key().to_hex(), + "owner", + owner, + ) + .await; + client.disconnect().await.ok(); + assert_eq!(result, (true, String::new()), "promote co-owner OK frame"); + assert_eq!( + member_role(url, owner, channel_id, &co_owner.public_key().to_hex()) + .await + .as_deref(), + Some("owner"), + "the setup must leave a second active owner" + ); +} + +/// Binds kind:9001's production `validate_admin_event` call to the WebSocket +/// OK frame. The DB applier has a different rejection message, so this exact +/// historical result can only come from the pre-storage relay validator. +#[tokio::test] +#[ignore] +async fn test_nip29_kind_9001_sole_owner_departure_rejected_at_wire() { + let url = relay_url(); + let owner = Keys::generate(); + let channel_id = create_test_channel(&owner).await; + + let result = self_departure_ws(&url, &channel_id, &owner, 9001).await; + + assert_eq!( + result, + (false, "invalid: cannot remove the last owner".to_string()) + ); +} + +/// An open channel lets the nonmember event reach the per-kind validator; a +/// private channel would be rejected earlier by the generic membership gate. +#[tokio::test] +#[ignore] +async fn test_nip29_kind_9001_nonmember_departure_rejected_at_wire() { + let url = relay_url(); + let owner = Keys::generate(); + let nonmember = Keys::generate(); + let channel_id = create_test_channel(&owner).await; + + let result = self_departure_ws(&url, &channel_id, &nonmember, 9001).await; + + assert_eq!( + result, + (false, "invalid: actor is not an active member".to_string()) + ); +} + +#[tokio::test] +#[ignore] +async fn test_nip29_kind_9001_co_owner_departure_allowed_at_wire() { + let url = relay_url(); + let owner = Keys::generate(); + let co_owner = Keys::generate(); + let channel_id = create_test_channel(&owner).await; + promote_co_owner(&url, &channel_id, &owner, &co_owner).await; + + let result = self_departure_ws(&url, &channel_id, &co_owner, 9001).await; + + assert_eq!(result, (true, String::new())); +} + +/// Binds kind:9022's distinct production `validate_admin_event` call to the +/// same historical WebSocket rejection contract as self-removal. +#[tokio::test] +#[ignore] +async fn test_nip29_kind_9022_sole_owner_departure_rejected_at_wire() { + let url = relay_url(); + let owner = Keys::generate(); + let channel_id = create_test_channel(&owner).await; + + let result = self_departure_ws(&url, &channel_id, &owner, 9022).await; + + assert_eq!( + result, + (false, "invalid: cannot remove the last owner".to_string()) + ); +} + +#[tokio::test] +#[ignore] +async fn test_nip29_kind_9022_nonmember_departure_rejected_at_wire() { + let url = relay_url(); + let owner = Keys::generate(); + let nonmember = Keys::generate(); + let channel_id = create_test_channel(&owner).await; + + let result = self_departure_ws(&url, &channel_id, &nonmember, 9022).await; + + assert_eq!( + result, + (false, "invalid: actor is not an active member".to_string()) + ); +} + +#[tokio::test] +#[ignore] +async fn test_nip29_kind_9022_co_owner_departure_allowed_at_wire() { + let url = relay_url(); + let owner = Keys::generate(); + let co_owner = Keys::generate(); + let channel_id = create_test_channel(&owner).await; + promote_co_owner(&url, &channel_id, &owner, &co_owner).await; + + let result = self_departure_ws(&url, &channel_id, &co_owner, 9022).await; + + assert_eq!(result, (true, String::new())); +} + /// Any active member can add any ordinary role to a private channel. #[tokio::test] #[ignore] diff --git a/scripts/run-tests.sh b/scripts/run-tests.sh index 6f7093084d7..4d460acd0ad 100755 --- a/scripts/run-tests.sh +++ b/scripts/run-tests.sh @@ -125,6 +125,18 @@ run_unit_tests() { # step with `just test-unit`; ignored lifecycle tests run elsewhere. run_test_step "buzz-acp unit tests" \ cargo test -p buzz-acp --lib -- --nocapture + + # Mirror the three infra-free relay handler modules in `just test-unit`'s + # nextest expression. Keep the side-effects filter pinned to `::tests::` so + # it does not select the sibling Postgres-backed test module. + run_test_step "buzz-relay channel authorization tests" \ + cargo test -p buzz-relay --lib handlers::channel_authz:: -- --nocapture + + run_test_step "buzz-relay moderation authorization tests" \ + cargo test -p buzz-relay --lib handlers::moderation_authz:: -- --nocapture + + run_test_step "buzz-relay side-effects helper tests" \ + cargo test -p buzz-relay --lib handlers::side_effects::tests:: -- --nocapture } # ---- DB / integration tests (infra required) -------------------------------- From b9dec5cc7593008e3217521edcd69862caeb2f39 Mon Sep 17 00:00:00 2001 From: tornquist Date: Thu, 3 Sep 2026 19:42:59 +0000 Subject: [PATCH 3/3] test(relay): run departure wire tests in CI Give the six kind 9001 and 9022 departure tests one shared filter and invoke it from the required Relay E2E job. Pin the workflow selection in the existing required-context contract test. Signed-off-by: tornquist Co-authored-by: Codex --- .github/workflows/_ci-relay.yml | 1 + crates/buzz-test-client/tests/e2e_relay.rs | 12 ++++++------ scripts/test-ci-required-context-isolation.sh | 3 +++ 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/workflows/_ci-relay.yml b/.github/workflows/_ci-relay.yml index c0fc3d5333f..97d32f912c7 100644 --- a/.github/workflows/_ci-relay.yml +++ b/.github/workflows/_ci-relay.yml @@ -569,6 +569,7 @@ jobs: cargo test -p buzz-test-client --test e2e_persona --test e2e_team_catalog --test e2e_nostr_interop --test e2e_project -- --ignored --nocapture cargo test -p buzz-test-client --test e2e_relay invite -- --ignored --nocapture cargo test -p buzz-test-client --test e2e_relay nip43_membership_snapshots_are_rejected -- --ignored --nocapture + cargo test -p buzz-test-client --test e2e_relay nip29_departure_wire -- --ignored --nocapture env: RELAY_URL: ws://localhost:3000 GIT_CREDENTIAL_NOSTR_BIN: ${{ github.workspace }}/target/ci/git-credential-nostr diff --git a/crates/buzz-test-client/tests/e2e_relay.rs b/crates/buzz-test-client/tests/e2e_relay.rs index fa0ba21037c..a801c1aaae6 100644 --- a/crates/buzz-test-client/tests/e2e_relay.rs +++ b/crates/buzz-test-client/tests/e2e_relay.rs @@ -2401,7 +2401,7 @@ async fn promote_co_owner(url: &str, channel_id: &str, owner: &Keys, co_owner: & /// historical result can only come from the pre-storage relay validator. #[tokio::test] #[ignore] -async fn test_nip29_kind_9001_sole_owner_departure_rejected_at_wire() { +async fn test_nip29_departure_wire_kind_9001_sole_owner_rejected() { let url = relay_url(); let owner = Keys::generate(); let channel_id = create_test_channel(&owner).await; @@ -2418,7 +2418,7 @@ async fn test_nip29_kind_9001_sole_owner_departure_rejected_at_wire() { /// private channel would be rejected earlier by the generic membership gate. #[tokio::test] #[ignore] -async fn test_nip29_kind_9001_nonmember_departure_rejected_at_wire() { +async fn test_nip29_departure_wire_kind_9001_nonmember_rejected() { let url = relay_url(); let owner = Keys::generate(); let nonmember = Keys::generate(); @@ -2434,7 +2434,7 @@ async fn test_nip29_kind_9001_nonmember_departure_rejected_at_wire() { #[tokio::test] #[ignore] -async fn test_nip29_kind_9001_co_owner_departure_allowed_at_wire() { +async fn test_nip29_departure_wire_kind_9001_co_owner_allowed() { let url = relay_url(); let owner = Keys::generate(); let co_owner = Keys::generate(); @@ -2450,7 +2450,7 @@ async fn test_nip29_kind_9001_co_owner_departure_allowed_at_wire() { /// same historical WebSocket rejection contract as self-removal. #[tokio::test] #[ignore] -async fn test_nip29_kind_9022_sole_owner_departure_rejected_at_wire() { +async fn test_nip29_departure_wire_kind_9022_sole_owner_rejected() { let url = relay_url(); let owner = Keys::generate(); let channel_id = create_test_channel(&owner).await; @@ -2465,7 +2465,7 @@ async fn test_nip29_kind_9022_sole_owner_departure_rejected_at_wire() { #[tokio::test] #[ignore] -async fn test_nip29_kind_9022_nonmember_departure_rejected_at_wire() { +async fn test_nip29_departure_wire_kind_9022_nonmember_rejected() { let url = relay_url(); let owner = Keys::generate(); let nonmember = Keys::generate(); @@ -2481,7 +2481,7 @@ async fn test_nip29_kind_9022_nonmember_departure_rejected_at_wire() { #[tokio::test] #[ignore] -async fn test_nip29_kind_9022_co_owner_departure_allowed_at_wire() { +async fn test_nip29_departure_wire_kind_9022_co_owner_allowed() { let url = relay_url(); let owner = Keys::generate(); let co_owner = Keys::generate(); diff --git a/scripts/test-ci-required-context-isolation.sh b/scripts/test-ci-required-context-isolation.sh index dc5c11708ee..77effa10c65 100755 --- a/scripts/test-ci-required-context-isolation.sh +++ b/scripts/test-ci-required-context-isolation.sh @@ -37,6 +37,7 @@ clients_call=$(extract_job clients "$orchestrator") mobile_swift_call=$(extract_job mobile-swift-domain "$orchestrator") desktop_gate=$(extract_job desktop "$orchestrator") macos_gate=$(extract_job desktop-build-macos "$orchestrator") +relay_e2e_job=$(extract_job relay-e2e "$relay_workflow") [[ "$desktop_call" == *'uses: ./.github/workflows/_ci-desktop.yml'* ]] || fail "Desktop Domain must call _ci-desktop.yml" @@ -68,6 +69,8 @@ grep -Fq "inputs.lane == 'postgres'" "$relay_workflow" || fail "PostgreSQL tests must be selected by their isolated lane" grep -Fq "inputs.lane == 'artifacts'" "$relay_workflow" || fail "relay artifacts must be selected by their isolated lane" +[[ "$relay_e2e_job" == *'cargo test -p buzz-test-client --test e2e_relay nip29_departure_wire -- --ignored --nocapture'* ]] || + fail "Relay E2E must select the NIP-29 departure wire tests" grep -Fq "inputs.lane == 'mobile-swift'" "$clients_workflow" || fail "Mobile Swift must be selected by its isolated lane"