From 177fb1c7d146346c94d1d7a2fae845c2d2e29e84 Mon Sep 17 00:00:00 2001 From: Nik Weidenbacher Date: Thu, 30 Jul 2026 15:20:08 +0000 Subject: [PATCH 1/6] cli: unsubscribe orphaned users before rotating or deleting a feed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An EdgeSeat user's multicast groups must stay a subset of the groups carried by the feeds on their access pass — serviceability relies on that when deciding whether an unsubscribe releases a feed's seat. `UpdateFeed` and `DeleteFeed` can break it and the program cannot notice: there is no reverse index from a Feed to its subscribers. The resulting illegal state lets a later unsubscribe release a seat the user still needs. `doublezero feed update` and `feed delete` now scan for the users a group drop would orphan and fail closed, listing them, unless `--force-unsubscribe` is passed — in which case the memberships are removed first, re-verified against a fresh scan, and only then is the feed change submitted. Unsubscribe-then-rotate only ever passes through legal states. Ref malbeclabs/infra#2113 --- CHANGELOG.md | 3 + smartcontract/cli/src/feed/delete.rs | 180 +++++- smartcontract/cli/src/feed/guard.rs | 553 ++++++++++++++++++ smartcontract/cli/src/feed/mod.rs | 1 + smartcontract/cli/src/feed/update.rs | 292 ++++++++- .../cli/src/user/create_subscribe.rs | 15 +- 6 files changed, 1034 insertions(+), 10 deletions(-) create mode 100644 smartcontract/cli/src/feed/guard.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 365320a01d..1e529b4889 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ All notable changes to this project will be documented in this file. ### Breaking +- CLI + - `doublezero feed update` (when `--group` drops a group) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user subscribed to a multicast group no feed on their access pass carries, printing the affected users and groups and submitting nothing. Pass `--force-unsubscribe` to unsubscribe them first and then apply the change. A rename, a purely additive `--group` set, and an update without `--group` are unaffected. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113) + ### Changes - Serviceability diff --git a/smartcontract/cli/src/feed/delete.rs b/smartcontract/cli/src/feed/delete.rs index 805c7bef09..263540c2ec 100644 --- a/smartcontract/cli/src/feed/delete.rs +++ b/smartcontract/cli/src/feed/delete.rs @@ -1,6 +1,6 @@ use crate::{ - doublezerocommand::CliCommand, helpers::parse_or_resolve_exchange, - validators::validate_pubkey_or_code, + doublezerocommand::CliCommand, feed::guard::unsubscribe_orphans, + helpers::parse_or_resolve_exchange, validators::validate_pubkey_or_code, }; use clap::Args; use doublezero_cli_core::{print_signature, require, CliContext, RequirementCheck}; @@ -15,6 +15,11 @@ pub struct DeleteFeedCliCommand { /// Metro (exchange) pubkey or code to disambiguate a code that exists in multiple metros #[arg(long, value_parser = validate_pubkey_or_code)] pub exchange: Option, + /// Unsubscribe EdgeSeat users from the deleted feed's groups, instead of refusing the delete. + /// Without it, a delete that would leave a user subscribed to a group outside their access + /// pass's feeds fails and changes nothing. + #[arg(long, default_value_t = false)] + pub force_unsubscribe: bool, } impl DeleteFeedCliCommand { @@ -34,11 +39,21 @@ impl DeleteFeedCliCommand { .as_deref() .map(|e| parse_or_resolve_exchange(client, e)) .transpose()?; - let (pubkey, _feed) = client.get_feed(GetFeedCommand { + let (pubkey, feed) = client.get_feed(GetFeedCommand { pubkey_or_code: self.pubkey, exchange, })?; + // Deleting the feed drops every group it carried. + unsubscribe_orphans( + client, + out, + &pubkey, + &feed.code, + &feed.groups, + self.force_unsubscribe, + )?; + let signature = client.delete_feed(DeleteFeedCommand { pubkey })?; print_signature(out, &signature) } @@ -46,17 +61,173 @@ impl DeleteFeedCliCommand { #[cfg(test)] mod tests { - use crate::{feed::delete::DeleteFeedCliCommand, tests::utils::create_test_client}; + use crate::{ + doublezerocommand::MockCliCommand, + feed::{ + delete::DeleteFeedCliCommand, + guard::fixtures::{device, feed as feed_account, pass, seat, user}, + }, + tests::utils::create_test_client, + }; use doublezero_cli_core::testing::{block_on, cli_context_default_for_tests}; use doublezero_sdk::{ commands::{ exchange::get::GetExchangeCommand, feed::{delete::DeleteFeedCommand, get::GetFeedCommand}, + multicastgroup::subscribe::UpdateMulticastGroupRolesCommand, }, AccountType, Exchange, ExchangeStatus, Feed, }; + use doublezero_serviceability::state::accesspass::AccessPassType; use mockall::predicate; use solana_sdk::{pubkey::Pubkey, signature::Signature}; + use std::{collections::HashMap, net::Ipv4Addr}; + + /// A feed carrying `[group]` and one EdgeSeat user in the feed's exchange, seated on it. + struct GuardFixture { + exchange_pk: Pubkey, + feed_pk: Pubkey, + group: Pubkey, + device_pk: Pubkey, + user_pk: Pubkey, + owner: Pubkey, + client_ip: Ipv4Addr, + } + + impl GuardFixture { + fn new() -> Self { + Self { + exchange_pk: Pubkey::new_unique(), + feed_pk: Pubkey::new_unique(), + group: Pubkey::new_unique(), + device_pk: Pubkey::new_unique(), + user_pk: Pubkey::new_unique(), + owner: Pubkey::new_unique(), + client_ip: [10, 1, 1, 1].into(), + } + } + + /// Stub `get_feed` for a `--pubkey ` lookup. + fn expect_get_feed(&self, client: &mut MockCliCommand) { + let feed_pk = self.feed_pk; + let feed = feed_account(self.exchange_pk, vec![self.group]); + client + .expect_get_feed() + .with(predicate::eq(GetFeedCommand { + pubkey_or_code: feed_pk.to_string(), + exchange: None, + })) + .times(1) + .returning(move |_| Ok((feed_pk, feed.clone()))); + } + + /// Stub one guard scan whose user holds `subscribers`. Stacked expectations match FIFO, + /// so calling this twice lets a second scan see a different snapshot. + fn expect_scan(&self, client: &mut MockCliCommand, subscribers: Vec) { + let user_pk = self.user_pk; + let user_acct = user(self.owner, self.device_pk, self.client_ip, subscribers); + client + .expect_list_user() + .times(1) + .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); + let pass_acct = pass( + self.owner, + self.client_ip, + AccessPassType::EdgeSeat(vec![seat(self.feed_pk)]), + ); + client + .expect_list_accesspass() + .times(1) + .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); + let device_pk = self.device_pk; + let device_acct = device(self.exchange_pk); + client + .expect_list_device() + .times(1) + .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); + let feed_pk = self.feed_pk; + let feed_acct = feed_account(self.exchange_pk, vec![self.group]); + client + .expect_list_feed() + .times(1) + .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); + } + } + + #[test] + fn test_cli_feed_delete_fails_closed_when_it_would_orphan_a_subscriber() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(); + f.expect_get_feed(&mut client); + f.expect_scan(&mut client, vec![f.group]); + client.expect_delete_feed().times(0); + client.expect_update_multicastgroup_roles().times(0); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + DeleteFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + force_unsubscribe: false, + } + .execute(&ctx, &client, &mut output), + ); + let err = res.unwrap_err(); + assert!( + err.to_string().contains("--force-unsubscribe"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_cli_feed_delete_force_unsubscribes_then_deletes() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(); + let signature = Signature::new_unique(); + f.expect_get_feed(&mut client); + f.expect_scan(&mut client, vec![f.group]); + // The mock does not mutate state, so the post-unsubscribe re-scan needs its own snapshot + // with the membership gone. + f.expect_scan(&mut client, vec![]); + client + .expect_update_multicastgroup_roles() + .with(predicate::eq(UpdateMulticastGroupRolesCommand { + user_pk: f.user_pk, + group_pk: f.group, + client_ip: f.client_ip, + publisher: false, + subscriber: false, + device_pk: None, + feed_pk: None, + })) + .times(1) + .returning(|_| Ok(Signature::new_unique())); + client + .expect_delete_feed() + .with(predicate::eq(DeleteFeedCommand { pubkey: f.feed_pk })) + .times(1) + .returning(move |_| Ok(signature)); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + DeleteFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + force_unsubscribe: true, + } + .execute(&ctx, &client, &mut output), + ); + assert!(res.is_ok(), "{res:?}"); + assert!(String::from_utf8(output) + .unwrap() + .contains(&format!("Signature: {signature}"))); + } #[test] fn test_cli_feed_delete_with_exchange_code() { @@ -121,6 +292,7 @@ mod tests { DeleteFeedCliCommand { pubkey: "feed01".to_string(), exchange: Some("xchi".to_string()), + force_unsubscribe: false, } .execute(&ctx, &client, &mut output), ); diff --git a/smartcontract/cli/src/feed/guard.rs b/smartcontract/cli/src/feed/guard.rs new file mode 100644 index 0000000000..fa2f057d6c --- /dev/null +++ b/smartcontract/cli/src/feed/guard.rs @@ -0,0 +1,553 @@ +//! Keeps EdgeSeat multicast subscriptions inside the feeds on the subscriber's access pass. +//! +//! An EdgeSeat user's multicast groups must always be a subset of the groups carried by the feeds +//! seated on their access pass, in their device's metro. Serviceability relies on that invariant +//! when it decides whether an unsubscribe releases a feed's seat. `UpdateFeed` and `DeleteFeed` +//! can break it: dropping a group the feed used to carry leaves every subscriber of that group +//! holding a membership outside their feeds, and the program cannot notice — there is no reverse +//! index from a `Feed` to its subscribers. +//! +//! So the admin side closes the hole: before the feed change is submitted, unsubscribe the users +//! it would orphan. Unsubscribe-then-rotate only ever passes through legal states, since a user +//! holding fewer groups than their feeds offer is fine. Rotate-then-unsubscribe would publish the +//! illegal state for as long as the cleanup takes. + +use crate::doublezerocommand::CliCommand; +use doublezero_sdk::{ + commands::{ + accesspass::list::ListAccessPassCommand, device::list::ListDeviceCommand, + feed::list::ListFeedCommand, multicastgroup::subscribe::UpdateMulticastGroupRolesCommand, + user::list::ListUserCommand, + }, + Device, Feed, User, +}; +use doublezero_serviceability::state::accesspass::{AccessPass, AccessPassType}; +use eyre::Context; +use solana_sdk::pubkey::Pubkey; +use std::{collections::HashMap, io::Write, net::Ipv4Addr}; + +/// Unsubscribe passes to attempt before giving up on a population that keeps changing underneath +/// the scan. Each pass is preceded by a fresh scan, so a clean scan ends the loop early. +const MAX_UNSUBSCRIBE_ROUNDS: usize = 3; + +/// A multicast-group membership the pending feed change would leave outside the user's feeds. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] +pub struct Orphan { + pub user_pk: Pubkey, + pub group_pk: Pubkey, + pub client_ip: Ipv4Addr, + /// The user holds the publisher role on this group. EdgeSeat is meant to be subscribe-only, so + /// this is worth calling out in the plan an operator reads before forcing the change through. + pub publisher: bool, + pub subscriber: bool, +} + +/// The onchain state [`plan`] reads, fetched once per round by [`scan`]. +pub struct Snapshot { + pub users: HashMap, + pub accesspasses: HashMap, + pub devices: HashMap, + pub feeds: HashMap, +} + +/// Unsubscribes every EdgeSeat user that dropping `dropped` from `feed_pk` would orphan, leaving +/// the caller to submit the feed change afterwards. +/// +/// Without `force` this only reports: a non-empty plan fails the command before anything is +/// submitted. With `force` the removals are applied and re-verified against a fresh scan. +/// Removals are idempotent, so a run that fails partway is safe to repeat. +pub fn unsubscribe_orphans( + client: &C, + out: &mut W, + feed_pk: &Pubkey, + feed_code: &str, + dropped: &[Pubkey], + force: bool, +) -> eyre::Result<()> { + if dropped.is_empty() { + return Ok(()); + } + + let mut rounds_done = 0; + loop { + let orphans = plan(feed_pk, dropped, &scan(client)?)?; + if orphans.is_empty() { + return Ok(()); + } + + report(out, feed_code, &orphans, rounds_done)?; + + if !force { + eyre::bail!( + "refusing to change feed '{feed_code}': {} multicast subscription(s) would be left \ + outside their access pass's feeds. Re-run with --force-unsubscribe to unsubscribe \ + them first.", + orphans.len() + ); + } + if rounds_done == MAX_UNSUBSCRIBE_ROUNDS { + eyre::bail!( + "new subscriptions kept appearing across {MAX_UNSUBSCRIBE_ROUNDS} unsubscribe \ + round(s) on feed '{feed_code}'; nothing was changed on the feed. Re-run once \ + subscriptions to its groups have quiesced." + ); + } + + for orphan in &orphans { + client + .update_multicastgroup_roles(UpdateMulticastGroupRolesCommand { + user_pk: orphan.user_pk, + group_pk: orphan.group_pk, + client_ip: orphan.client_ip, + publisher: false, + subscriber: false, + device_pk: None, + feed_pk: None, + }) + .wrap_err_with(|| { + format!( + "failed to unsubscribe user {} from group {}; the feed was left unchanged. \ + Removing another owner's roles needs USER_ADMIN (or foundation \ + membership) on the payer in addition to FEED_AUTHORITY: doublezero \ + permission set --user-payer --add user-admin", + orphan.user_pk, orphan.group_pk + ) + })?; + writeln!( + out, + " unsubscribed user {} from group {}", + orphan.user_pk, orphan.group_pk + )?; + } + rounds_done += 1; + } +} + +/// Reads every account class the planner needs. Users and access passes carry the memberships and +/// seats; devices resolve a user's metro; feeds resolve what the pass's other seats still cover. +fn scan(client: &C) -> eyre::Result { + Ok(Snapshot { + users: client.list_user(ListUserCommand {})?, + accesspasses: client.list_accesspass(ListAccessPassCommand {})?, + devices: client.list_device(ListDeviceCommand {})?, + feeds: client.list_feed(ListFeedCommand {})?, + }) +} + +/// The memberships that dropping `dropped` from `feed_pk` would orphan. +/// +/// A membership survives when some *other* feed seated on the same access pass still carries the +/// group and serves the user's metro — that is the same coverage test the program applies at +/// connect time. The rotated feed itself never counts: every group in `dropped` is by construction +/// absent from its post-change set, and a delete removes it entirely. +pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Result> { + let mut orphans = Vec::new(); + + for (user_pk, user) in &snap.users { + if user.publishers.is_empty() && user.subscribers.is_empty() { + continue; + } + // Non-EdgeSeat memberships come from the pass's own allowlists, not from feeds, so feed + // changes cannot orphan them. + let Some(pass) = resolve_pass(&snap.accesspasses, user) else { + continue; + }; + if !matches!(pass.accesspass_type, AccessPassType::EdgeSeat(_)) { + continue; + } + + // Without the device we cannot tell which metro the user connects from, and therefore + // cannot tell coverage from non-coverage. Refuse rather than guess in either direction. + let device = snap.devices.get(&user.device_pk).ok_or_else(|| { + eyre::eyre!( + "user {user_pk} references unknown device {}; cannot evaluate feed coverage", + user.device_pk + ) + })?; + + for group_pk in dropped { + let publisher = user.publishers.contains(group_pk); + let subscriber = user.subscribers.contains(group_pk); + if !publisher && !subscriber { + continue; + } + let covered = pass.feed_seats().iter().any(|seat| { + seat.feed_key != *feed_pk + && snap.feeds.get(&seat.feed_key).is_some_and(|other| { + other.groups_for(&device.exchange_pk).contains(group_pk) + }) + }); + if !covered { + orphans.push(Orphan { + user_pk: *user_pk, + group_pk: *group_pk, + client_ip: user.client_ip, + publisher, + subscriber, + }); + } + } + } + + // The scan walks HashMaps, so sort for a stable plan and stable output. + orphans.sort(); + Ok(orphans) +} + +/// The access pass serviceability would resolve for `user`, mirroring `GetAccessPassCommand`: a +/// shared dynamic pass at the UNSPECIFIED-IP PDA wins over the exact-IP pass. +fn resolve_pass<'a>( + accesspasses: &'a HashMap, + user: &User, +) -> Option<&'a AccessPass> { + let mut exact = None; + for pass in accesspasses.values() { + if pass.user_payer != user.owner { + continue; + } + if pass.client_ip == Ipv4Addr::UNSPECIFIED { + return Some(pass); + } + if pass.client_ip == user.client_ip { + exact = Some(pass); + } + } + exact +} + +fn report( + out: &mut W, + feed_code: &str, + orphans: &[Orphan], + rounds_done: usize, +) -> eyre::Result<()> { + if rounds_done == 0 { + writeln!( + out, + "Changing feed '{feed_code}' orphans {} multicast subscription(s):", + orphans.len() + )?; + } else { + writeln!( + out, + "{} new orphaned subscription(s) appeared on feed '{feed_code}' during the previous \ + round:", + orphans.len() + )?; + } + for orphan in orphans { + let mut roles = Vec::new(); + if orphan.publisher { + roles.push("publisher"); + } + if orphan.subscriber { + roles.push("subscriber"); + } + writeln!( + out, + " user {} ({}) group {} [{}]", + orphan.user_pk, + orphan.client_ip, + orphan.group_pk, + roles.join(", ") + )?; + } + Ok(()) +} + +/// Account builders shared by the guard's planner tests and the `feed update` / `feed delete` +/// command tests, which stub the same four scans through the mock client. +#[cfg(test)] +pub(crate) mod fixtures { + use super::*; + use doublezero_sdk::{AccountType, DeviceStatus, UserCYOA, UserStatus, UserType}; + use doublezero_serviceability::state::accesspass::{AccessPassStatus, FeedSeat}; + + pub fn device(exchange_pk: Pubkey) -> Device { + Device { + account_type: AccountType::Device, + exchange_pk, + status: DeviceStatus::Activated, + ..Default::default() + } + } + + pub fn feed(exchange: Pubkey, groups: Vec) -> Feed { + Feed { + account_type: AccountType::Feed, + owner: Pubkey::new_unique(), + bump_seed: 255, + code: "feed".to_string(), + name: "Feed".to_string(), + exchange, + groups, + } + } + + pub fn seat(feed_key: Pubkey) -> FeedSeat { + FeedSeat { + feed_key, + max_users: 1, + max_future_users: 1, + current_users: 1, + anniversary_day: 1, + window_end: 0, + terminates_at: 0, + } + } + + pub fn pass( + user_payer: Pubkey, + client_ip: Ipv4Addr, + accesspass_type: AccessPassType, + ) -> AccessPass { + AccessPass { + account_type: AccountType::AccessPass, + owner: Pubkey::new_unique(), + bump_seed: 0, + accesspass_type, + client_ip, + user_payer, + last_access_epoch: u64::MAX, + connection_count: 0, + status: AccessPassStatus::Connected, + mgroup_pub_allowlist: vec![], + mgroup_sub_allowlist: vec![], + flags: 0, + tenant_allowlist: vec![], + unicast_user_count: 0, + max_unicast_users: 1, + multicast_user_count: 1, + max_multicast_users: 1, + } + } + + pub fn user( + owner: Pubkey, + device_pk: Pubkey, + client_ip: Ipv4Addr, + subscribers: Vec, + ) -> User { + User { + account_type: AccountType::User, + owner, + index: 1, + bump_seed: 255, + user_type: UserType::Multicast, + tenant_pk: Pubkey::default(), + device_pk, + cyoa_type: UserCYOA::GREOverDIA, + client_ip, + dz_ip: [10, 0, 0, 1].into(), + tunnel_id: 1, + tunnel_net: "10.0.0.0/31".parse().unwrap(), + status: UserStatus::Activated, + publishers: vec![], + subscribers, + validator_pubkey: Pubkey::default(), + tunnel_endpoint: Ipv4Addr::UNSPECIFIED, + tunnel_flags: 0, + bgp_status: Default::default(), + last_bgp_up_at: 0, + last_bgp_reported_at: 0, + bgp_rtt_ns: 0, + feed_pks: vec![], + } + } +} + +#[cfg(test)] +mod tests { + use super::{fixtures::*, *}; + use doublezero_serviceability::state::accesspass::AccessPassType; + + /// One EdgeSeat user subscribed to `group`, seated on `feed_pk` in `exchange`. + struct Fixture { + exchange: Pubkey, + feed_pk: Pubkey, + group: Pubkey, + user_pk: Pubkey, + client_ip: Ipv4Addr, + snap: Snapshot, + } + + fn fixture() -> Fixture { + let exchange = Pubkey::new_unique(); + let feed_pk = Pubkey::new_unique(); + let group = Pubkey::new_unique(); + let device_pk = Pubkey::new_unique(); + let user_pk = Pubkey::new_unique(); + let owner = Pubkey::new_unique(); + let client_ip: Ipv4Addr = [10, 1, 1, 1].into(); + + Fixture { + exchange, + feed_pk, + group, + user_pk, + client_ip, + snap: Snapshot { + users: HashMap::from([(user_pk, user(owner, device_pk, client_ip, vec![group]))]), + accesspasses: HashMap::from([( + Pubkey::new_unique(), + pass( + owner, + client_ip, + AccessPassType::EdgeSeat(vec![seat(feed_pk)]), + ), + )]), + devices: HashMap::from([(device_pk, device(exchange))]), + feeds: HashMap::from([(feed_pk, feed(exchange, vec![group]))]), + }, + } + } + + #[test] + fn test_plan_flags_dropped_group_the_user_holds() { + let f = fixture(); + let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); + assert_eq!( + orphans, + vec![Orphan { + user_pk: f.user_pk, + group_pk: f.group, + client_ip: f.client_ip, + publisher: false, + subscriber: true, + }] + ); + } + + #[test] + fn test_plan_ignores_dropped_group_the_user_does_not_hold() { + let f = fixture(); + assert!(plan(&f.feed_pk, &[Pubkey::new_unique()], &f.snap) + .unwrap() + .is_empty()); + } + + #[test] + fn test_plan_empty_when_nothing_dropped() { + let f = fixture(); + assert!(plan(&f.feed_pk, &[], &f.snap).unwrap().is_empty()); + } + + #[test] + fn test_plan_skips_group_covered_by_another_seated_feed_in_the_same_metro() { + let mut f = fixture(); + let other_pk = Pubkey::new_unique(); + f.snap + .feeds + .insert(other_pk, feed(f.exchange, vec![f.group])); + let owner = f.snap.users[&f.user_pk].owner; + f.snap.accesspasses = HashMap::from([( + Pubkey::new_unique(), + pass( + owner, + f.client_ip, + AccessPassType::EdgeSeat(vec![seat(f.feed_pk), seat(other_pk)]), + ), + )]); + + assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + } + + #[test] + fn test_plan_does_not_count_another_feed_in_a_different_metro() { + let mut f = fixture(); + let other_pk = Pubkey::new_unique(); + f.snap + .feeds + .insert(other_pk, feed(Pubkey::new_unique(), vec![f.group])); + let owner = f.snap.users[&f.user_pk].owner; + f.snap.accesspasses = HashMap::from([( + Pubkey::new_unique(), + pass( + owner, + f.client_ip, + AccessPassType::EdgeSeat(vec![seat(f.feed_pk), seat(other_pk)]), + ), + )]); + + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } + + /// The sharp edge: the rotated feed's *old* set is not coverage. Its own seat must never + /// excuse a group it is about to stop carrying. + #[test] + fn test_plan_does_not_count_the_rotated_feeds_own_seat_as_coverage() { + let f = fixture(); + // `feeds` still holds the pre-rotation group set, and the pass is seated on it. + assert!(f.snap.feeds[&f.feed_pk].groups.contains(&f.group)); + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } + + #[test] + fn test_plan_covers_delete_by_dropping_every_group() { + let mut f = fixture(); + let second = Pubkey::new_unique(); + f.snap + .feeds + .insert(f.feed_pk, feed(f.exchange, vec![f.group, second])); + f.snap + .users + .get_mut(&f.user_pk) + .unwrap() + .subscribers + .push(second); + + let groups = f.snap.feeds[&f.feed_pk].groups.clone(); + let orphans = plan(&f.feed_pk, &groups, &f.snap).unwrap(); + assert_eq!(orphans.len(), 2); + assert!(orphans.iter().all(|o| o.user_pk == f.user_pk)); + } + + #[test] + fn test_plan_flags_the_publisher_role() { + let mut f = fixture(); + let u = f.snap.users.get_mut(&f.user_pk).unwrap(); + u.subscribers.clear(); + u.publishers.push(f.group); + + let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); + assert_eq!(orphans.len(), 1); + assert!(orphans[0].publisher && !orphans[0].subscriber); + } + + #[test] + fn test_plan_leaves_non_edgeseat_users_alone() { + let mut f = fixture(); + let owner = f.snap.users[&f.user_pk].owner; + f.snap.accesspasses = HashMap::from([( + Pubkey::new_unique(), + pass(owner, f.client_ip, AccessPassType::Prepaid), + )]); + + assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + } + + #[test] + fn test_plan_prefers_the_shared_dynamic_pass() { + let mut f = fixture(); + let owner = f.snap.users[&f.user_pk].owner; + // The exact-IP pass is EdgeSeat, but the shared dynamic pass is what the program reads — + // and it is Prepaid, so this user is not feed-gated. + f.snap.accesspasses.insert( + Pubkey::new_unique(), + pass(owner, Ipv4Addr::UNSPECIFIED, AccessPassType::Prepaid), + ); + + assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + } + + #[test] + fn test_plan_errors_on_a_user_with_an_unknown_device() { + let mut f = fixture(); + f.snap.devices.clear(); + let err = plan(&f.feed_pk, &[f.group], &f.snap).unwrap_err(); + assert!( + err.to_string().contains("unknown device"), + "unexpected error: {err}" + ); + } +} diff --git a/smartcontract/cli/src/feed/mod.rs b/smartcontract/cli/src/feed/mod.rs index 91259509fe..1ada97a183 100644 --- a/smartcontract/cli/src/feed/mod.rs +++ b/smartcontract/cli/src/feed/mod.rs @@ -1,5 +1,6 @@ pub mod create; pub mod delete; pub mod get; +pub mod guard; pub mod list; pub mod update; diff --git a/smartcontract/cli/src/feed/update.rs b/smartcontract/cli/src/feed/update.rs index 57a6837453..31409e5fe6 100644 --- a/smartcontract/cli/src/feed/update.rs +++ b/smartcontract/cli/src/feed/update.rs @@ -1,5 +1,6 @@ use crate::{ doublezerocommand::CliCommand, + feed::guard::unsubscribe_orphans, helpers::{parse_or_resolve_exchange, parse_or_resolve_multicastgroup}, validators::validate_pubkey_or_code, }; @@ -23,6 +24,11 @@ pub struct UpdateFeedCliCommand { /// omitted, the groups are left unchanged. #[arg(long = "group", value_parser = validate_pubkey_or_code, num_args = 1..)] pub groups: Vec, + /// Unsubscribe EdgeSeat users from any group this update drops from the feed, instead of + /// refusing the update. Without it, an update that would leave a user subscribed to a group + /// outside their access pass's feeds fails and changes nothing. + #[arg(long, default_value_t = false)] + pub force_unsubscribe: bool, } impl UpdateFeedCliCommand { @@ -42,7 +48,7 @@ impl UpdateFeedCliCommand { .as_deref() .map(|e| parse_or_resolve_exchange(client, e)) .transpose()?; - let (pubkey, _feed) = client.get_feed(GetFeedCommand { + let (pubkey, feed) = client.get_feed(GetFeedCommand { pubkey_or_code: self.pubkey, exchange, })?; @@ -59,6 +65,25 @@ impl UpdateFeedCliCommand { ) }; + // Only a group removal can orphan a subscriber, so a rename or a purely additive change + // scans nothing and behaves exactly as before. + if let Some(new_groups) = &groups { + let dropped: Vec<_> = feed + .groups + .iter() + .filter(|g| !new_groups.contains(g)) + .copied() + .collect(); + unsubscribe_orphans( + client, + out, + &pubkey, + &feed.code, + &dropped, + self.force_unsubscribe, + )?; + } + let signature = client.update_feed(UpdateFeedCommand { pubkey, name: self.name, @@ -71,18 +96,278 @@ impl UpdateFeedCliCommand { #[cfg(test)] mod tests { - use crate::{feed::update::UpdateFeedCliCommand, tests::utils::create_test_client}; + use crate::{ + doublezerocommand::MockCliCommand, + feed::{ + guard::fixtures::{device, feed as feed_account, pass, seat, user}, + update::UpdateFeedCliCommand, + }, + tests::utils::create_test_client, + }; use doublezero_cli_core::testing::{block_on, cli_context_default_for_tests}; use doublezero_sdk::{ commands::{ exchange::get::GetExchangeCommand, feed::{get::GetFeedCommand, update::UpdateFeedCommand}, - multicastgroup::get::GetMulticastGroupCommand, + multicastgroup::{ + get::GetMulticastGroupCommand, subscribe::UpdateMulticastGroupRolesCommand, + }, }, AccountType, Exchange, ExchangeStatus, Feed, MulticastGroup, MulticastGroupStatus, }; + use doublezero_serviceability::state::accesspass::AccessPassType; use mockall::predicate; use solana_sdk::{pubkey::Pubkey, signature::Signature}; + use std::{collections::HashMap, net::Ipv4Addr}; + + /// A feed carrying `[g1, g2]` and one EdgeSeat user in the feed's exchange, seated on it. + struct GuardFixture { + exchange_pk: Pubkey, + feed_pk: Pubkey, + g1: Pubkey, + g2: Pubkey, + device_pk: Pubkey, + user_pk: Pubkey, + owner: Pubkey, + client_ip: Ipv4Addr, + } + + impl GuardFixture { + fn new() -> Self { + Self { + exchange_pk: Pubkey::new_unique(), + feed_pk: Pubkey::new_unique(), + g1: Pubkey::new_unique(), + g2: Pubkey::new_unique(), + device_pk: Pubkey::new_unique(), + user_pk: Pubkey::new_unique(), + owner: Pubkey::new_unique(), + client_ip: [10, 1, 1, 1].into(), + } + } + + /// Stub `get_feed` for a `--pubkey ` lookup, returning a feed carrying `groups`. + fn expect_get_feed(&self, client: &mut MockCliCommand, groups: Vec) { + let feed_pk = self.feed_pk; + let feed = feed_account(self.exchange_pk, groups); + client + .expect_get_feed() + .with(predicate::eq(GetFeedCommand { + pubkey_or_code: feed_pk.to_string(), + exchange: None, + })) + .times(1) + .returning(move |_| Ok((feed_pk, feed.clone()))); + } + + /// Stub one guard scan whose user holds `subscribers`. Stacked expectations match FIFO, + /// so calling this twice lets a second scan see a different snapshot. + fn expect_scan(&self, client: &mut MockCliCommand, subscribers: Vec) { + let user_pk = self.user_pk; + let user_acct = user(self.owner, self.device_pk, self.client_ip, subscribers); + client + .expect_list_user() + .times(1) + .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); + let pass_acct = pass( + self.owner, + self.client_ip, + AccessPassType::EdgeSeat(vec![seat(self.feed_pk)]), + ); + client + .expect_list_accesspass() + .times(1) + .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); + let device_pk = self.device_pk; + let device_acct = device(self.exchange_pk); + client + .expect_list_device() + .times(1) + .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); + let feed_pk = self.feed_pk; + let feed_acct = feed_account(self.exchange_pk, vec![self.g1, self.g2]); + client + .expect_list_feed() + .times(1) + .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); + } + } + + #[test] + fn test_cli_feed_update_fails_closed_when_it_would_orphan_a_subscriber() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(); + f.expect_get_feed(&mut client, vec![f.g1, f.g2]); + f.expect_scan(&mut client, vec![f.g2]); + client.expect_update_feed().times(0); + client.expect_update_multicastgroup_roles().times(0); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + UpdateFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + name: None, + groups: vec![f.g1.to_string()], + force_unsubscribe: false, + } + .execute(&ctx, &client, &mut output), + ); + let err = res.unwrap_err(); + assert!( + err.to_string().contains("--force-unsubscribe"), + "unexpected error: {err}" + ); + let output = String::from_utf8(output).unwrap(); + assert!( + output.contains(&f.user_pk.to_string()), + "plan does not name the user: {output}" + ); + } + + #[test] + fn test_cli_feed_update_force_unsubscribes_then_updates() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(); + let signature = Signature::new_unique(); + f.expect_get_feed(&mut client, vec![f.g1, f.g2]); + f.expect_scan(&mut client, vec![f.g2]); + // The mock does not mutate state, so the post-unsubscribe re-scan needs its own snapshot + // with the membership gone. + f.expect_scan(&mut client, vec![]); + client + .expect_update_multicastgroup_roles() + .with(predicate::eq(UpdateMulticastGroupRolesCommand { + user_pk: f.user_pk, + group_pk: f.g2, + client_ip: f.client_ip, + publisher: false, + subscriber: false, + device_pk: None, + feed_pk: None, + })) + .times(1) + .returning(|_| Ok(Signature::new_unique())); + client + .expect_update_feed() + .with(predicate::eq(UpdateFeedCommand { + pubkey: f.feed_pk, + name: None, + groups: Some(vec![f.g1]), + })) + .times(1) + .returning(move |_| Ok(signature)); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + UpdateFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + name: None, + groups: vec![f.g1.to_string()], + force_unsubscribe: true, + } + .execute(&ctx, &client, &mut output), + ); + assert!(res.is_ok(), "{res:?}"); + assert!(String::from_utf8(output) + .unwrap() + .contains(&format!("Signature: {signature}"))); + } + + #[test] + fn test_cli_feed_update_additive_change_does_not_scan() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(); + let signature = Signature::new_unique(); + f.expect_get_feed(&mut client, vec![f.g1]); + client.expect_list_user().times(0); + client.expect_list_accesspass().times(0); + client + .expect_update_feed() + .with(predicate::eq(UpdateFeedCommand { + pubkey: f.feed_pk, + name: None, + groups: Some(vec![f.g1, f.g2]), + })) + .times(1) + .returning(move |_| Ok(signature)); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + UpdateFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + name: None, + groups: vec![f.g1.to_string(), f.g2.to_string()], + force_unsubscribe: false, + } + .execute(&ctx, &client, &mut output), + ); + assert!(res.is_ok(), "{res:?}"); + } + + #[test] + fn test_cli_feed_update_aborts_when_orphans_keep_appearing() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(); + f.expect_get_feed(&mut client, vec![f.g1, f.g2]); + // Every scan sees the same still-subscribed user, as if a new subscription raced each + // unsubscribe pass; uncapped so the guard's round limit decides when to stop. + let user_pk = f.user_pk; + let user_acct = user(f.owner, f.device_pk, f.client_ip, vec![f.g2]); + client + .expect_list_user() + .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); + let pass_acct = pass( + f.owner, + f.client_ip, + AccessPassType::EdgeSeat(vec![seat(f.feed_pk)]), + ); + client + .expect_list_accesspass() + .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); + let device_pk = f.device_pk; + let device_acct = device(f.exchange_pk); + client + .expect_list_device() + .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); + let feed_pk = f.feed_pk; + let feed_acct = feed_account(f.exchange_pk, vec![f.g1, f.g2]); + client + .expect_list_feed() + .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); + client + .expect_update_multicastgroup_roles() + .times(3) + .returning(|_| Ok(Signature::new_unique())); + client.expect_update_feed().times(0); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + UpdateFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + name: None, + groups: vec![f.g1.to_string()], + force_unsubscribe: true, + } + .execute(&ctx, &client, &mut output), + ); + assert!(res.is_err(), "{res:?}"); + } #[test] fn test_cli_feed_update_with_codes() { @@ -175,6 +460,7 @@ mod tests { exchange: Some("xchi".to_string()), name: Some("Feed v2".to_string()), groups: vec!["mg01".to_string()], + force_unsubscribe: false, } .execute(&ctx, &client, &mut output), ); diff --git a/smartcontract/cli/src/user/create_subscribe.rs b/smartcontract/cli/src/user/create_subscribe.rs index 36937cb36a..1da6809c05 100644 --- a/smartcontract/cli/src/user/create_subscribe.rs +++ b/smartcontract/cli/src/user/create_subscribe.rs @@ -342,7 +342,10 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - let mgroup_pubkey = Pubkey::new_unique(); + // Fixed short base58 (< 43 chars) so `--group` deterministically takes the + // code-resolution path: `Pubkey::new_unique()` straddles that length threshold + // depending on the process-global counter, which makes the run order matter. + let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, owner: Pubkey::default(), @@ -460,7 +463,10 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - let mgroup_pubkey = Pubkey::new_unique(); + // Fixed short base58 (< 43 chars) so `--group` deterministically takes the + // code-resolution path: `Pubkey::new_unique()` straddles that length threshold + // depending on the process-global counter, which makes the run order matter. + let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, owner: Pubkey::default(), @@ -588,7 +594,10 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - let mgroup_pubkey = Pubkey::new_unique(); + // Fixed short base58 (< 43 chars) so `--group` deterministically takes the + // code-resolution path: `Pubkey::new_unique()` straddles that length threshold + // depending on the process-global counter, which makes the run order matter. + let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, owner: Pubkey::default(), From ed4e71311905de34514268aa1767b1ec2a149788 Mon Sep 17 00:00:00 2001 From: Nik Weidenbacher Date: Thu, 30 Jul 2026 15:51:02 +0000 Subject: [PATCH 2/6] cli: accept an EdgeSeat pass at either PDA and re-derive drops per round Review follow-ups on the feed guard: - The program accepts an access pass at either the exact-IP PDA or the shared UNSPECIFIED one, so mirroring the SDK read path's preference for the dynamic pass let a Prepaid pass hide a live EdgeSeat pass and skip a seated user as "not feed-gated". Coverage is now the union of seats over every accepted EdgeSeat pass, and a consumed seat (`user.feed_pks`) keeps a user in scope on its own. - Re-derive the dropped group set from each round's fresh scan instead of pinning the set read before the loop, so a group added to the feed mid-run is caught too. - Test membership before looking up the device, so an unrelated user with a dangling `device_pk` no longer blocks every rotation. - Stop asserting USER_ADMIN as the cause of every removal failure. Ref malbeclabs/infra#2113 --- smartcontract/cli/src/feed/delete.rs | 115 ++------ smartcontract/cli/src/feed/guard.rs | 256 ++++++++++++++---- smartcontract/cli/src/feed/update.rs | 151 +++-------- .../cli/src/user/create_subscribe.rs | 6 +- 4 files changed, 269 insertions(+), 259 deletions(-) diff --git a/smartcontract/cli/src/feed/delete.rs b/smartcontract/cli/src/feed/delete.rs index 263540c2ec..b72b2a33de 100644 --- a/smartcontract/cli/src/feed/delete.rs +++ b/smartcontract/cli/src/feed/delete.rs @@ -44,15 +44,17 @@ impl DeleteFeedCliCommand { exchange, })?; - // Deleting the feed drops every group it carried. - unsubscribe_orphans( - client, - out, - &pubkey, - &feed.code, - &feed.groups, - self.force_unsubscribe, - )?; + // Deleting the feed drops every group it carried, so the post-change set is empty. + if !feed.groups.is_empty() { + unsubscribe_orphans( + client, + out, + &pubkey, + &feed.code, + &[], + self.force_unsubscribe, + )?; + } let signature = client.delete_feed(DeleteFeedCommand { pubkey })?; print_signature(out, &signature) @@ -62,11 +64,7 @@ impl DeleteFeedCliCommand { #[cfg(test)] mod tests { use crate::{ - doublezerocommand::MockCliCommand, - feed::{ - delete::DeleteFeedCliCommand, - guard::fixtures::{device, feed as feed_account, pass, seat, user}, - }, + feed::{delete::DeleteFeedCliCommand, guard::fixtures::GuardFixture}, tests::utils::create_test_client, }; use doublezero_cli_core::testing::{block_on, cli_context_default_for_tests}; @@ -78,90 +76,18 @@ mod tests { }, AccountType, Exchange, ExchangeStatus, Feed, }; - use doublezero_serviceability::state::accesspass::AccessPassType; use mockall::predicate; use solana_sdk::{pubkey::Pubkey, signature::Signature}; - use std::{collections::HashMap, net::Ipv4Addr}; - - /// A feed carrying `[group]` and one EdgeSeat user in the feed's exchange, seated on it. - struct GuardFixture { - exchange_pk: Pubkey, - feed_pk: Pubkey, - group: Pubkey, - device_pk: Pubkey, - user_pk: Pubkey, - owner: Pubkey, - client_ip: Ipv4Addr, - } - - impl GuardFixture { - fn new() -> Self { - Self { - exchange_pk: Pubkey::new_unique(), - feed_pk: Pubkey::new_unique(), - group: Pubkey::new_unique(), - device_pk: Pubkey::new_unique(), - user_pk: Pubkey::new_unique(), - owner: Pubkey::new_unique(), - client_ip: [10, 1, 1, 1].into(), - } - } - - /// Stub `get_feed` for a `--pubkey ` lookup. - fn expect_get_feed(&self, client: &mut MockCliCommand) { - let feed_pk = self.feed_pk; - let feed = feed_account(self.exchange_pk, vec![self.group]); - client - .expect_get_feed() - .with(predicate::eq(GetFeedCommand { - pubkey_or_code: feed_pk.to_string(), - exchange: None, - })) - .times(1) - .returning(move |_| Ok((feed_pk, feed.clone()))); - } - - /// Stub one guard scan whose user holds `subscribers`. Stacked expectations match FIFO, - /// so calling this twice lets a second scan see a different snapshot. - fn expect_scan(&self, client: &mut MockCliCommand, subscribers: Vec) { - let user_pk = self.user_pk; - let user_acct = user(self.owner, self.device_pk, self.client_ip, subscribers); - client - .expect_list_user() - .times(1) - .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); - let pass_acct = pass( - self.owner, - self.client_ip, - AccessPassType::EdgeSeat(vec![seat(self.feed_pk)]), - ); - client - .expect_list_accesspass() - .times(1) - .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); - let device_pk = self.device_pk; - let device_acct = device(self.exchange_pk); - client - .expect_list_device() - .times(1) - .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); - let feed_pk = self.feed_pk; - let feed_acct = feed_account(self.exchange_pk, vec![self.group]); - client - .expect_list_feed() - .times(1) - .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); - } - } #[test] fn test_cli_feed_delete_fails_closed_when_it_would_orphan_a_subscriber() { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); - let f = GuardFixture::new(); - f.expect_get_feed(&mut client); - f.expect_scan(&mut client, vec![f.group]); + let f = GuardFixture::new(1); + let group = f.groups[0]; + f.expect_get_feed(&mut client, vec![group]); + f.expect_scan(&mut client, vec![group]); client.expect_delete_feed().times(0); client.expect_update_multicastgroup_roles().times(0); @@ -187,10 +113,11 @@ mod tests { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); - let f = GuardFixture::new(); + let f = GuardFixture::new(1); + let group = f.groups[0]; let signature = Signature::new_unique(); - f.expect_get_feed(&mut client); - f.expect_scan(&mut client, vec![f.group]); + f.expect_get_feed(&mut client, vec![group]); + f.expect_scan(&mut client, vec![group]); // The mock does not mutate state, so the post-unsubscribe re-scan needs its own snapshot // with the membership gone. f.expect_scan(&mut client, vec![]); @@ -198,7 +125,7 @@ mod tests { .expect_update_multicastgroup_roles() .with(predicate::eq(UpdateMulticastGroupRolesCommand { user_pk: f.user_pk, - group_pk: f.group, + group_pk: group, client_ip: f.client_ip, publisher: false, subscriber: false, diff --git a/smartcontract/cli/src/feed/guard.rs b/smartcontract/cli/src/feed/guard.rs index fa2f057d6c..5a78340031 100644 --- a/smartcontract/cli/src/feed/guard.rs +++ b/smartcontract/cli/src/feed/guard.rs @@ -50,8 +50,9 @@ pub struct Snapshot { pub feeds: HashMap, } -/// Unsubscribes every EdgeSeat user that dropping `dropped` from `feed_pk` would orphan, leaving -/// the caller to submit the feed change afterwards. +/// Unsubscribes every EdgeSeat user that replacing `feed_pk`'s group set with `new_groups` would +/// orphan, leaving the caller to submit the feed change afterwards. A delete passes an empty +/// `new_groups`, since it drops everything the feed carried. /// /// Without `force` this only reports: a non-empty plan fails the command before anything is /// submitted. With `force` the removals are applied and re-verified against a fresh scan. @@ -61,16 +62,27 @@ pub fn unsubscribe_orphans( out: &mut W, feed_pk: &Pubkey, feed_code: &str, - dropped: &[Pubkey], + new_groups: &[Pubkey], force: bool, ) -> eyre::Result<()> { - if dropped.is_empty() { - return Ok(()); - } - let mut rounds_done = 0; loop { - let orphans = plan(feed_pk, dropped, &scan(client)?)?; + let snap = scan(client)?; + + // Re-derive the dropped set from the freshly scanned feed rather than from the group set + // the command read before the loop: a group added to the feed mid-run is dropped by this + // change too, and pinning the earlier set would let its subscribers through unseen. + let feed = snap.feeds.get(feed_pk).ok_or_else(|| { + eyre::eyre!("feed '{feed_code}' ({feed_pk}) disappeared mid-run; nothing was changed") + })?; + let dropped: Vec = feed + .groups + .iter() + .filter(|g| !new_groups.contains(g)) + .copied() + .collect(); + + let orphans = plan(feed_pk, &dropped, &snap)?; if orphans.is_empty() { return Ok(()); } @@ -107,9 +119,10 @@ pub fn unsubscribe_orphans( .wrap_err_with(|| { format!( "failed to unsubscribe user {} from group {}; the feed was left unchanged. \ - Removing another owner's roles needs USER_ADMIN (or foundation \ - membership) on the payer in addition to FEED_AUTHORITY: doublezero \ - permission set --user-payer --add user-admin", + If this is an authorization failure, removing another owner's roles needs \ + USER_ADMIN (or foundation membership) on the payer in addition to \ + FEED_AUTHORITY: doublezero permission set --user-payer --add \ + user-admin", orphan.user_pk, orphan.group_pk ) })?; @@ -144,15 +157,29 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu let mut orphans = Vec::new(); for (user_pk, user) in &snap.users { - if user.publishers.is_empty() && user.subscribers.is_empty() { + // Cheapest test first: a user holding none of the dropped groups cannot be orphaned, + // whatever their pass or device looks like. Keeping it ahead of the lookups below also + // stops an unrelated user with a dangling `device_pk` from blocking every rotation. + let held: Vec<&Pubkey> = dropped + .iter() + .filter(|g| user.publishers.contains(g) || user.subscribers.contains(g)) + .collect(); + if held.is_empty() { continue; } - // Non-EdgeSeat memberships come from the pass's own allowlists, not from feeds, so feed - // changes cannot orphan them. - let Some(pass) = resolve_pass(&snap.accesspasses, user) else { - continue; - }; - if !matches!(pass.accesspass_type, AccessPassType::EdgeSeat(_)) { + + // Every feed seated on a pass the program would accept for this user. Not the single pass + // the SDK read path prefers: `create_user` and `update_multicastgroup_roles` both accept + // either the exact-IP PDA or the shared UNSPECIFIED one, so a dynamic Prepaid pass must + // not be allowed to hide a live EdgeSeat pass at the other PDA. + let seated: Vec = accepted_passes(&snap.accesspasses, user) + .flat_map(|pass| pass.feed_seats().iter().map(|seat| seat.feed_key)) + .collect(); + // Not feed-gated: these memberships come from the pass's own allowlists, not from feeds, + // so no feed change can orphan them and no seat is at stake. `feed_pks` is the record of + // seats actually consumed at connect, so a user holding one stays in scope even if their + // pass no longer shows the seat. + if seated.is_empty() && user.feed_pks.is_empty() { continue; } @@ -165,15 +192,10 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu ) })?; - for group_pk in dropped { - let publisher = user.publishers.contains(group_pk); - let subscriber = user.subscribers.contains(group_pk); - if !publisher && !subscriber { - continue; - } - let covered = pass.feed_seats().iter().any(|seat| { - seat.feed_key != *feed_pk - && snap.feeds.get(&seat.feed_key).is_some_and(|other| { + for group_pk in held { + let covered = seated.iter().any(|seated_pk| { + seated_pk != feed_pk + && snap.feeds.get(seated_pk).is_some_and(|other| { other.groups_for(&device.exchange_pk).contains(group_pk) }) }); @@ -182,8 +204,8 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu user_pk: *user_pk, group_pk: *group_pk, client_ip: user.client_ip, - publisher, - subscriber, + publisher: user.publishers.contains(group_pk), + subscriber: user.subscribers.contains(group_pk), }); } } @@ -194,25 +216,17 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu Ok(orphans) } -/// The access pass serviceability would resolve for `user`, mirroring `GetAccessPassCommand`: a -/// shared dynamic pass at the UNSPECIFIED-IP PDA wins over the exact-IP pass. -fn resolve_pass<'a>( +/// The EdgeSeat passes the program would accept for `user`: those owned by their payer at either +/// the exact-IP PDA or the shared UNSPECIFIED-IP one. +fn accepted_passes<'a>( accesspasses: &'a HashMap, - user: &User, -) -> Option<&'a AccessPass> { - let mut exact = None; - for pass in accesspasses.values() { - if pass.user_payer != user.owner { - continue; - } - if pass.client_ip == Ipv4Addr::UNSPECIFIED { - return Some(pass); - } - if pass.client_ip == user.client_ip { - exact = Some(pass); - } - } - exact + user: &'a User, +) -> impl Iterator { + accesspasses.values().filter(move |pass| { + pass.user_payer == user.owner + && (pass.client_ip == Ipv4Addr::UNSPECIFIED || pass.client_ip == user.client_ip) + && matches!(pass.accesspass_type, AccessPassType::EdgeSeat(_)) + }) } fn report( @@ -260,7 +274,11 @@ fn report( #[cfg(test)] pub(crate) mod fixtures { use super::*; - use doublezero_sdk::{AccountType, DeviceStatus, UserCYOA, UserStatus, UserType}; + use crate::doublezerocommand::MockCliCommand; + use doublezero_sdk::{ + commands::feed::get::GetFeedCommand, AccountType, DeviceStatus, UserCYOA, UserStatus, + UserType, + }; use doublezero_serviceability::state::accesspass::{AccessPassStatus, FeedSeat}; pub fn device(exchange_pk: Pubkey) -> Device { @@ -354,6 +372,79 @@ pub(crate) mod fixtures { feed_pks: vec![], } } + + /// A feed carrying `groups` and one EdgeSeat user in the feed's exchange, seated on it. + /// Shared by the `feed update` / `feed delete` command tests, which stub the same guard + /// scans through the mock client; `new(group_count)` picks how many groups the feed carries. + pub struct GuardFixture { + pub exchange_pk: Pubkey, + pub feed_pk: Pubkey, + pub groups: Vec, + pub device_pk: Pubkey, + pub user_pk: Pubkey, + pub owner: Pubkey, + pub client_ip: Ipv4Addr, + } + + impl GuardFixture { + pub fn new(group_count: usize) -> Self { + Self { + exchange_pk: Pubkey::new_unique(), + feed_pk: Pubkey::new_unique(), + groups: (0..group_count).map(|_| Pubkey::new_unique()).collect(), + device_pk: Pubkey::new_unique(), + user_pk: Pubkey::new_unique(), + owner: Pubkey::new_unique(), + client_ip: [10, 1, 1, 1].into(), + } + } + + /// Stub `get_feed` for a `--pubkey ` lookup, returning a feed carrying `groups`. + pub fn expect_get_feed(&self, client: &mut MockCliCommand, groups: Vec) { + let feed_pk = self.feed_pk; + let feed_acct = feed(self.exchange_pk, groups); + client + .expect_get_feed() + .with(mockall::predicate::eq(GetFeedCommand { + pubkey_or_code: feed_pk.to_string(), + exchange: None, + })) + .times(1) + .returning(move |_| Ok((feed_pk, feed_acct.clone()))); + } + + /// Stub one guard scan whose user holds `subscribers`. Stacked expectations match FIFO, + /// so calling this twice lets a second scan see a different snapshot. + pub fn expect_scan(&self, client: &mut MockCliCommand, subscribers: Vec) { + let user_pk = self.user_pk; + let user_acct = user(self.owner, self.device_pk, self.client_ip, subscribers); + client + .expect_list_user() + .times(1) + .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); + let pass_acct = pass( + self.owner, + self.client_ip, + AccessPassType::EdgeSeat(vec![seat(self.feed_pk)]), + ); + client + .expect_list_accesspass() + .times(1) + .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); + let device_pk = self.device_pk; + let device_acct = device(self.exchange_pk); + client + .expect_list_device() + .times(1) + .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); + let feed_pk = self.feed_pk; + let feed_acct = feed(self.exchange_pk, self.groups.clone()); + client + .expect_list_feed() + .times(1) + .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); + } + } } #[cfg(test)] @@ -526,20 +617,55 @@ mod tests { assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); } + /// The program accepts a pass at either PDA, so a shared dynamic pass must not hide the + /// EdgeSeat pass at the exact-IP PDA the user is actually seated on. #[test] - fn test_plan_prefers_the_shared_dynamic_pass() { + fn test_plan_sees_an_edgeseat_pass_shadowed_by_a_dynamic_prepaid_pass() { let mut f = fixture(); let owner = f.snap.users[&f.user_pk].owner; - // The exact-IP pass is EdgeSeat, but the shared dynamic pass is what the program reads — - // and it is Prepaid, so this user is not feed-gated. f.snap.accesspasses.insert( Pubkey::new_unique(), pass(owner, Ipv4Addr::UNSPECIFIED, AccessPassType::Prepaid), ); + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } + + /// Coverage is the union over both accepted PDAs, not just one of them. + #[test] + fn test_plan_counts_coverage_from_a_pass_at_the_other_pda() { + let mut f = fixture(); + let owner = f.snap.users[&f.user_pk].owner; + let other_pk = Pubkey::new_unique(); + f.snap + .feeds + .insert(other_pk, feed(f.exchange, vec![f.group])); + f.snap.accesspasses.insert( + Pubkey::new_unique(), + pass( + owner, + Ipv4Addr::UNSPECIFIED, + AccessPassType::EdgeSeat(vec![seat(other_pk)]), + ), + ); + assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); } + /// A consumed seat keeps the user in scope even if their pass no longer shows it. + #[test] + fn test_plan_keeps_a_user_with_a_consumed_seat_but_no_edgeseat_pass() { + let mut f = fixture(); + let owner = f.snap.users[&f.user_pk].owner; + f.snap.accesspasses = HashMap::from([( + Pubkey::new_unique(), + pass(owner, f.client_ip, AccessPassType::Prepaid), + )]); + f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk]; + + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } + #[test] fn test_plan_errors_on_a_user_with_an_unknown_device() { let mut f = fixture(); @@ -550,4 +676,32 @@ mod tests { "unexpected error: {err}" ); } + + /// A dangling `device_pk` on a user who holds none of the dropped groups must not block the + /// rotation — the metro is only needed to judge coverage for a group actually held. + #[test] + fn test_plan_ignores_an_unknown_device_on_an_unaffected_user() { + let mut f = fixture(); + let stray_pk = Pubkey::new_unique(); + let owner = Pubkey::new_unique(); + f.snap.users.insert( + stray_pk, + user( + owner, + Pubkey::new_unique(), + [10, 2, 2, 2].into(), + vec![Pubkey::new_unique()], + ), + ); + f.snap.accesspasses.insert( + Pubkey::new_unique(), + pass( + owner, + [10, 2, 2, 2].into(), + AccessPassType::EdgeSeat(vec![seat(f.feed_pk)]), + ), + ); + + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } } diff --git a/smartcontract/cli/src/feed/update.rs b/smartcontract/cli/src/feed/update.rs index 31409e5fe6..50b466ba3f 100644 --- a/smartcontract/cli/src/feed/update.rs +++ b/smartcontract/cli/src/feed/update.rs @@ -66,22 +66,19 @@ impl UpdateFeedCliCommand { }; // Only a group removal can orphan a subscriber, so a rename or a purely additive change - // scans nothing and behaves exactly as before. + // scans nothing and behaves exactly as before. The guard re-derives the dropped set from + // its own scan, so a group added between here and the update still gets caught. if let Some(new_groups) = &groups { - let dropped: Vec<_> = feed - .groups - .iter() - .filter(|g| !new_groups.contains(g)) - .copied() - .collect(); - unsubscribe_orphans( - client, - out, - &pubkey, - &feed.code, - &dropped, - self.force_unsubscribe, - )?; + if feed.groups.iter().any(|g| !new_groups.contains(g)) { + unsubscribe_orphans( + client, + out, + &pubkey, + &feed.code, + new_groups, + self.force_unsubscribe, + )?; + } } let signature = client.update_feed(UpdateFeedCommand { @@ -97,9 +94,8 @@ impl UpdateFeedCliCommand { #[cfg(test)] mod tests { use crate::{ - doublezerocommand::MockCliCommand, feed::{ - guard::fixtures::{device, feed as feed_account, pass, seat, user}, + guard::fixtures::{device, feed as feed_account, pass, seat, user, GuardFixture}, update::UpdateFeedCliCommand, }, tests::utils::create_test_client, @@ -118,89 +114,17 @@ mod tests { use doublezero_serviceability::state::accesspass::AccessPassType; use mockall::predicate; use solana_sdk::{pubkey::Pubkey, signature::Signature}; - use std::{collections::HashMap, net::Ipv4Addr}; - - /// A feed carrying `[g1, g2]` and one EdgeSeat user in the feed's exchange, seated on it. - struct GuardFixture { - exchange_pk: Pubkey, - feed_pk: Pubkey, - g1: Pubkey, - g2: Pubkey, - device_pk: Pubkey, - user_pk: Pubkey, - owner: Pubkey, - client_ip: Ipv4Addr, - } - - impl GuardFixture { - fn new() -> Self { - Self { - exchange_pk: Pubkey::new_unique(), - feed_pk: Pubkey::new_unique(), - g1: Pubkey::new_unique(), - g2: Pubkey::new_unique(), - device_pk: Pubkey::new_unique(), - user_pk: Pubkey::new_unique(), - owner: Pubkey::new_unique(), - client_ip: [10, 1, 1, 1].into(), - } - } - - /// Stub `get_feed` for a `--pubkey ` lookup, returning a feed carrying `groups`. - fn expect_get_feed(&self, client: &mut MockCliCommand, groups: Vec) { - let feed_pk = self.feed_pk; - let feed = feed_account(self.exchange_pk, groups); - client - .expect_get_feed() - .with(predicate::eq(GetFeedCommand { - pubkey_or_code: feed_pk.to_string(), - exchange: None, - })) - .times(1) - .returning(move |_| Ok((feed_pk, feed.clone()))); - } - - /// Stub one guard scan whose user holds `subscribers`. Stacked expectations match FIFO, - /// so calling this twice lets a second scan see a different snapshot. - fn expect_scan(&self, client: &mut MockCliCommand, subscribers: Vec) { - let user_pk = self.user_pk; - let user_acct = user(self.owner, self.device_pk, self.client_ip, subscribers); - client - .expect_list_user() - .times(1) - .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); - let pass_acct = pass( - self.owner, - self.client_ip, - AccessPassType::EdgeSeat(vec![seat(self.feed_pk)]), - ); - client - .expect_list_accesspass() - .times(1) - .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); - let device_pk = self.device_pk; - let device_acct = device(self.exchange_pk); - client - .expect_list_device() - .times(1) - .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); - let feed_pk = self.feed_pk; - let feed_acct = feed_account(self.exchange_pk, vec![self.g1, self.g2]); - client - .expect_list_feed() - .times(1) - .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); - } - } + use std::collections::HashMap; #[test] fn test_cli_feed_update_fails_closed_when_it_would_orphan_a_subscriber() { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); - let f = GuardFixture::new(); - f.expect_get_feed(&mut client, vec![f.g1, f.g2]); - f.expect_scan(&mut client, vec![f.g2]); + let f = GuardFixture::new(2); + let (g1, g2) = (f.groups[0], f.groups[1]); + f.expect_get_feed(&mut client, vec![g1, g2]); + f.expect_scan(&mut client, vec![g2]); client.expect_update_feed().times(0); client.expect_update_multicastgroup_roles().times(0); @@ -211,7 +135,7 @@ mod tests { pubkey: f.feed_pk.to_string(), exchange: None, name: None, - groups: vec![f.g1.to_string()], + groups: vec![g1.to_string()], force_unsubscribe: false, } .execute(&ctx, &client, &mut output), @@ -233,10 +157,11 @@ mod tests { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); - let f = GuardFixture::new(); + let f = GuardFixture::new(2); + let (g1, g2) = (f.groups[0], f.groups[1]); let signature = Signature::new_unique(); - f.expect_get_feed(&mut client, vec![f.g1, f.g2]); - f.expect_scan(&mut client, vec![f.g2]); + f.expect_get_feed(&mut client, vec![g1, g2]); + f.expect_scan(&mut client, vec![g2]); // The mock does not mutate state, so the post-unsubscribe re-scan needs its own snapshot // with the membership gone. f.expect_scan(&mut client, vec![]); @@ -244,7 +169,7 @@ mod tests { .expect_update_multicastgroup_roles() .with(predicate::eq(UpdateMulticastGroupRolesCommand { user_pk: f.user_pk, - group_pk: f.g2, + group_pk: g2, client_ip: f.client_ip, publisher: false, subscriber: false, @@ -258,7 +183,7 @@ mod tests { .with(predicate::eq(UpdateFeedCommand { pubkey: f.feed_pk, name: None, - groups: Some(vec![f.g1]), + groups: Some(vec![g1]), })) .times(1) .returning(move |_| Ok(signature)); @@ -270,7 +195,7 @@ mod tests { pubkey: f.feed_pk.to_string(), exchange: None, name: None, - groups: vec![f.g1.to_string()], + groups: vec![g1.to_string()], force_unsubscribe: true, } .execute(&ctx, &client, &mut output), @@ -286,9 +211,10 @@ mod tests { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); - let f = GuardFixture::new(); + let f = GuardFixture::new(2); + let (g1, g2) = (f.groups[0], f.groups[1]); let signature = Signature::new_unique(); - f.expect_get_feed(&mut client, vec![f.g1]); + f.expect_get_feed(&mut client, vec![g1]); client.expect_list_user().times(0); client.expect_list_accesspass().times(0); client @@ -296,7 +222,7 @@ mod tests { .with(predicate::eq(UpdateFeedCommand { pubkey: f.feed_pk, name: None, - groups: Some(vec![f.g1, f.g2]), + groups: Some(vec![g1, g2]), })) .times(1) .returning(move |_| Ok(signature)); @@ -308,7 +234,7 @@ mod tests { pubkey: f.feed_pk.to_string(), exchange: None, name: None, - groups: vec![f.g1.to_string(), f.g2.to_string()], + groups: vec![g1.to_string(), g2.to_string()], force_unsubscribe: false, } .execute(&ctx, &client, &mut output), @@ -321,12 +247,15 @@ mod tests { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); - let f = GuardFixture::new(); - f.expect_get_feed(&mut client, vec![f.g1, f.g2]); + let f = GuardFixture::new(2); + let (g1, g2) = (f.groups[0], f.groups[1]); + f.expect_get_feed(&mut client, vec![g1, g2]); // Every scan sees the same still-subscribed user, as if a new subscription raced each - // unsubscribe pass; uncapped so the guard's round limit decides when to stop. + // unsubscribe pass; uncapped so the guard's round limit decides when to stop. Deliberately + // not `expect_scan`: that helper is capped per call, which would hard-code the guard's + // internal round count. let user_pk = f.user_pk; - let user_acct = user(f.owner, f.device_pk, f.client_ip, vec![f.g2]); + let user_acct = user(f.owner, f.device_pk, f.client_ip, vec![g2]); client .expect_list_user() .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); @@ -344,7 +273,7 @@ mod tests { .expect_list_device() .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); let feed_pk = f.feed_pk; - let feed_acct = feed_account(f.exchange_pk, vec![f.g1, f.g2]); + let feed_acct = feed_account(f.exchange_pk, vec![g1, g2]); client .expect_list_feed() .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); @@ -361,7 +290,7 @@ mod tests { pubkey: f.feed_pk.to_string(), exchange: None, name: None, - groups: vec![f.g1.to_string()], + groups: vec![g1.to_string()], force_unsubscribe: true, } .execute(&ctx, &client, &mut output), diff --git a/smartcontract/cli/src/user/create_subscribe.rs b/smartcontract/cli/src/user/create_subscribe.rs index 1da6809c05..14b0c56a8a 100644 --- a/smartcontract/cli/src/user/create_subscribe.rs +++ b/smartcontract/cli/src/user/create_subscribe.rs @@ -342,7 +342,7 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - // Fixed short base58 (< 43 chars) so `--group` deterministically takes the + // Fixed short base58 (< 43 chars) so `--subscriber` deterministically takes the // code-resolution path: `Pubkey::new_unique()` straddles that length threshold // depending on the process-global counter, which makes the run order matter. let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); @@ -463,7 +463,7 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - // Fixed short base58 (< 43 chars) so `--group` deterministically takes the + // Fixed short base58 (< 43 chars) so `--subscriber` deterministically takes the // code-resolution path: `Pubkey::new_unique()` straddles that length threshold // depending on the process-global counter, which makes the run order matter. let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); @@ -594,7 +594,7 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - // Fixed short base58 (< 43 chars) so `--group` deterministically takes the + // Fixed short base58 (< 43 chars) so `--subscriber` deterministically takes the // code-resolution path: `Pubkey::new_unique()` straddles that length threshold // depending on the process-global counter, which makes the run order matter. let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); From 349196715d90b30daf465fc320ef3001c84b29df Mon Sep 17 00:00:00 2001 From: Nik Weidenbacher Date: Thu, 30 Jul 2026 16:41:18 +0000 Subject: [PATCH 3/6] cli: honor pass allowlists in the feed guard and always scan on --group MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the feed guard: - The program authorizes multicast roles through the pass's own mgroup_pub_allowlist / mgroup_sub_allowlist for every pass type, so an EdgeSeat user can legitimately hold a role with no feed involved. The planner now treats a per-role allowlist entry on any accepted pass as coverage, and a forced removal re-asserts the covered role instead of clearing both — only the orphaned role goes away. Leaving the covered role is safe for the invariant: seat release never keys on a group the feeds no longer carry. - Run the guard whenever `--group` is present (and on every delete) instead of deciding drop-vs-additive from the pre-loop feed read, so a group added between that read and the submit is still caught by the guard's fresh per-round scan. An additive update now costs one scan and finds nothing to do. Ref malbeclabs/infra#2113 --- CHANGELOG.md | 2 +- smartcontract/cli/src/feed/delete.rs | 38 ++++-- smartcontract/cli/src/feed/guard.rs | 167 ++++++++++++++++++++++----- smartcontract/cli/src/feed/update.rs | 50 +++++--- 4 files changed, 200 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e529b4889..d50766d647 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All notable changes to this project will be documented in this file. ### Breaking - CLI - - `doublezero feed update` (when `--group` drops a group) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user subscribed to a multicast group no feed on their access pass carries, printing the affected users and groups and submitting nothing. Pass `--force-unsubscribe` to unsubscribe them first and then apply the change. A rename, a purely additive `--group` set, and an update without `--group` are unaffected. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113) + - `doublezero feed update` (with `--group`) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user holding a multicast group role that no feed on their access pass carries and the pass's own allowlists don't authorize, printing the affected users, groups, and roles and submitting nothing. Pass `--force-unsubscribe` to remove those roles first (allowlist-covered roles are re-asserted, not removed) and then apply the change. A rename or an update without `--group` is unaffected; an additive `--group` set scans but finds nothing to do. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113) ### Changes diff --git a/smartcontract/cli/src/feed/delete.rs b/smartcontract/cli/src/feed/delete.rs index b72b2a33de..73c4501029 100644 --- a/smartcontract/cli/src/feed/delete.rs +++ b/smartcontract/cli/src/feed/delete.rs @@ -44,17 +44,17 @@ impl DeleteFeedCliCommand { exchange, })?; - // Deleting the feed drops every group it carried, so the post-change set is empty. - if !feed.groups.is_empty() { - unsubscribe_orphans( - client, - out, - &pubkey, - &feed.code, - &[], - self.force_unsubscribe, - )?; - } + // Deleting the feed drops every group it carried, so the post-change set is empty. The + // guard always runs — it re-derives the dropped set from its own fresh scan, so a group + // added after the `get_feed` read above is still caught. + unsubscribe_orphans( + client, + out, + &pubkey, + &feed.code, + &[], + self.force_unsubscribe, + )?; let signature = client.delete_feed(DeleteFeedCommand { pubkey })?; print_signature(out, &signature) @@ -78,6 +78,7 @@ mod tests { }; use mockall::predicate; use solana_sdk::{pubkey::Pubkey, signature::Signature}; + use std::collections::HashMap; #[test] fn test_cli_feed_delete_fails_closed_when_it_would_orphan_a_subscriber() { @@ -198,6 +199,7 @@ mod tests { exchange: exchange_pk, groups: vec![], }; + let feed_for_get = feed.clone(); client .expect_get_feed() .with(predicate::eq(GetFeedCommand { @@ -205,7 +207,19 @@ mod tests { exchange: Some(exchange_pk), })) .times(1) - .returning(move |_| Ok((feed_pk, feed.clone()))); + .returning(move |_| Ok((feed_pk, feed_for_get.clone()))); + + // Delete always runs the guard; with no users the scan finds nothing to drop. + client.expect_list_user().returning(|_| Ok(HashMap::new())); + client + .expect_list_accesspass() + .returning(|_| Ok(HashMap::new())); + client + .expect_list_device() + .returning(|_| Ok(HashMap::new())); + client + .expect_list_feed() + .returning(move |_| Ok(HashMap::from([(feed_pk, feed.clone())]))); client .expect_delete_feed() diff --git a/smartcontract/cli/src/feed/guard.rs b/smartcontract/cli/src/feed/guard.rs index 5a78340031..9d119f2679 100644 --- a/smartcontract/cli/src/feed/guard.rs +++ b/smartcontract/cli/src/feed/guard.rs @@ -31,15 +31,24 @@ use std::{collections::HashMap, io::Write, net::Ipv4Addr}; const MAX_UNSUBSCRIBE_ROUNDS: usize = 3; /// A multicast-group membership the pending feed change would leave outside the user's feeds. +/// +/// Roles are tracked separately because coverage is per role: the pass's own +/// `mgroup_pub_allowlist` / `mgroup_sub_allowlist` authorize a role with no feed involved, so one +/// role of a group can be orphaned while the other legitimately stays. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] pub struct Orphan { pub user_pk: Pubkey, pub group_pk: Pubkey, pub client_ip: Ipv4Addr, - /// The user holds the publisher role on this group. EdgeSeat is meant to be subscribe-only, so - /// this is worth calling out in the plan an operator reads before forcing the change through. - pub publisher: bool, - pub subscriber: bool, + /// The user holds the publisher role and nothing covers it. EdgeSeat is meant to be + /// subscribe-only, so this is worth calling out in the plan an operator reads before forcing + /// the change through. + pub remove_publisher: bool, + pub remove_subscriber: bool, + /// Held roles kept because an accepted pass's own allowlist still authorizes them; the + /// removal re-asserts these so they survive it. + pub keep_publisher: bool, + pub keep_subscriber: bool, } /// The onchain state [`plan`] reads, fetched once per round by [`scan`]. @@ -111,8 +120,10 @@ pub fn unsubscribe_orphans( user_pk: orphan.user_pk, group_pk: orphan.group_pk, client_ip: orphan.client_ip, - publisher: false, - subscriber: false, + // Desired absolute state, not a toggle: an allowlist-covered role is + // re-asserted so only the orphaned role is removed. + publisher: orphan.keep_publisher, + subscriber: orphan.keep_subscriber, device_pk: None, feed_pk: None, }) @@ -153,6 +164,11 @@ fn scan(client: &C) -> eyre::Result { /// group and serves the user's metro — that is the same coverage test the program applies at /// connect time. The rotated feed itself never counts: every group in `dropped` is by construction /// absent from its post-change set, and a delete removes it entirely. +/// +/// A role also survives when an accepted pass's own allowlist authorizes it +/// (`mgroup_pub_allowlist` for publisher, `mgroup_sub_allowlist` for subscriber): the program runs +/// that check for every pass type, so such a membership is legal with no feed involved, and seat +/// release never keys on a group the feeds don't carry. pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Result> { let mut orphans = Vec::new(); @@ -168,18 +184,23 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu continue; } - // Every feed seated on a pass the program would accept for this user. Not the single pass - // the SDK read path prefers: `create_user` and `update_multicastgroup_roles` both accept - // either the exact-IP PDA or the shared UNSPECIFIED one, so a dynamic Prepaid pass must - // not be allowed to hide a live EdgeSeat pass at the other PDA. - let seated: Vec = accepted_passes(&snap.accesspasses, user) + // Every pass the program would accept for this user. Not the single pass the SDK read + // path prefers: `create_user` and `update_multicastgroup_roles` both accept either the + // exact-IP PDA or the shared UNSPECIFIED one, so a dynamic Prepaid pass must not be + // allowed to hide a live EdgeSeat pass at the other PDA. + let passes: Vec<&AccessPass> = accepted_passes(&snap.accesspasses, user).collect(); + let seated: Vec = passes + .iter() .flat_map(|pass| pass.feed_seats().iter().map(|seat| seat.feed_key)) .collect(); - // Not feed-gated: these memberships come from the pass's own allowlists, not from feeds, + // Not feed-gated: with no EdgeSeat pass, memberships come from the pass's own allowlists, // so no feed change can orphan them and no seat is at stake. `feed_pks` is the record of // seats actually consumed at connect, so a user holding one stays in scope even if their // pass no longer shows the seat. - if seated.is_empty() && user.feed_pks.is_empty() { + let edge_seat = passes + .iter() + .any(|pass| matches!(pass.accesspass_type, AccessPassType::EdgeSeat(_))); + if !edge_seat && user.feed_pks.is_empty() { continue; } @@ -193,19 +214,35 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu })?; for group_pk in held { - let covered = seated.iter().any(|seated_pk| { + // Feed coverage is role-agnostic; allowlist coverage is per role. + let feed_covered = seated.iter().any(|seated_pk| { seated_pk != feed_pk && snap.feeds.get(seated_pk).is_some_and(|other| { other.groups_for(&device.exchange_pk).contains(group_pk) }) }); - if !covered { + let pub_covered = feed_covered + || passes + .iter() + .any(|pass| pass.mgroup_pub_allowlist.contains(group_pk)); + let sub_covered = feed_covered + || passes + .iter() + .any(|pass| pass.mgroup_sub_allowlist.contains(group_pk)); + + let pub_held = user.publishers.contains(group_pk); + let sub_held = user.subscribers.contains(group_pk); + let remove_publisher = pub_held && !pub_covered; + let remove_subscriber = sub_held && !sub_covered; + if remove_publisher || remove_subscriber { orphans.push(Orphan { user_pk: *user_pk, group_pk: *group_pk, client_ip: user.client_ip, - publisher: user.publishers.contains(group_pk), - subscriber: user.subscribers.contains(group_pk), + remove_publisher, + remove_subscriber, + keep_publisher: pub_held && pub_covered, + keep_subscriber: sub_held && sub_covered, }); } } @@ -216,8 +253,8 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu Ok(orphans) } -/// The EdgeSeat passes the program would accept for `user`: those owned by their payer at either -/// the exact-IP PDA or the shared UNSPECIFIED-IP one. +/// The passes the program would accept for `user`: those owned by their payer at either the +/// exact-IP PDA or the shared UNSPECIFIED-IP one. fn accepted_passes<'a>( accesspasses: &'a HashMap, user: &'a User, @@ -225,7 +262,6 @@ fn accepted_passes<'a>( accesspasses.values().filter(move |pass| { pass.user_payer == user.owner && (pass.client_ip == Ipv4Addr::UNSPECIFIED || pass.client_ip == user.client_ip) - && matches!(pass.accesspass_type, AccessPassType::EdgeSeat(_)) }) } @@ -251,19 +287,31 @@ fn report( } for orphan in orphans { let mut roles = Vec::new(); - if orphan.publisher { + if orphan.remove_publisher { roles.push("publisher"); } - if orphan.subscriber { + if orphan.remove_subscriber { roles.push("subscriber"); } + let mut kept = Vec::new(); + if orphan.keep_publisher { + kept.push("publisher"); + } + if orphan.keep_subscriber { + kept.push("subscriber"); + } writeln!( out, - " user {} ({}) group {} [{}]", + " user {} ({}) group {} [{}]{}", orphan.user_pk, orphan.client_ip, orphan.group_pk, - roles.join(", ") + roles.join(", "), + if kept.is_empty() { + String::new() + } else { + format!(" (keeping allowlisted {})", kept.join(", ")) + } )?; } Ok(()) @@ -503,8 +551,10 @@ mod tests { user_pk: f.user_pk, group_pk: f.group, client_ip: f.client_ip, - publisher: false, - subscriber: true, + remove_publisher: false, + remove_subscriber: true, + keep_publisher: false, + keep_subscriber: false, }] ); } @@ -602,7 +652,7 @@ mod tests { let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); assert_eq!(orphans.len(), 1); - assert!(orphans[0].publisher && !orphans[0].subscriber); + assert!(orphans[0].remove_publisher && !orphans[0].remove_subscriber); } #[test] @@ -704,4 +754,67 @@ mod tests { assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); } + + /// The program authorizes a role through the pass's own allowlist for every pass type, so a + /// subscriber the sub allowlist still covers is not orphaned by the feed change. + #[test] + fn test_plan_skips_a_role_covered_by_the_pass_sub_allowlist() { + let mut f = fixture(); + let (_, pass) = f.snap.accesspasses.iter_mut().next().unwrap(); + pass.mgroup_sub_allowlist.push(f.group); + + assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + } + + /// Allowlist coverage is per role: a pub allowlist entry says nothing about the subscriber + /// role. + #[test] + fn test_plan_pub_allowlist_does_not_cover_the_subscriber_role() { + let mut f = fixture(); + let (_, pass) = f.snap.accesspasses.iter_mut().next().unwrap(); + pass.mgroup_pub_allowlist.push(f.group); + + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } + + /// Mixed roles: the uncovered publisher role is removed, the allowlist-covered subscriber + /// role is re-asserted so the removal leaves it in place. + #[test] + fn test_plan_removes_only_the_uncovered_role() { + let mut f = fixture(); + f.snap + .users + .get_mut(&f.user_pk) + .unwrap() + .publishers + .push(f.group); + let (_, pass) = f.snap.accesspasses.iter_mut().next().unwrap(); + pass.mgroup_sub_allowlist.push(f.group); + + let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); + assert_eq!( + orphans, + vec![Orphan { + user_pk: f.user_pk, + group_pk: f.group, + client_ip: f.client_ip, + remove_publisher: true, + remove_subscriber: false, + keep_publisher: false, + keep_subscriber: true, + }] + ); + } + + /// Allowlist coverage counts from any accepted pass, not just the one carrying the seat. + #[test] + fn test_plan_counts_allowlist_coverage_from_the_other_pda() { + let mut f = fixture(); + let owner = f.snap.users[&f.user_pk].owner; + let mut dynamic = pass(owner, Ipv4Addr::UNSPECIFIED, AccessPassType::Prepaid); + dynamic.mgroup_sub_allowlist.push(f.group); + f.snap.accesspasses.insert(Pubkey::new_unique(), dynamic); + + assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + } } diff --git a/smartcontract/cli/src/feed/update.rs b/smartcontract/cli/src/feed/update.rs index 50b466ba3f..a20f790eb2 100644 --- a/smartcontract/cli/src/feed/update.rs +++ b/smartcontract/cli/src/feed/update.rs @@ -65,20 +65,19 @@ impl UpdateFeedCliCommand { ) }; - // Only a group removal can orphan a subscriber, so a rename or a purely additive change - // scans nothing and behaves exactly as before. The guard re-derives the dropped set from - // its own scan, so a group added between here and the update still gets caught. + // Only a group replacement can orphan a subscriber, so a rename leaves the groups alone + // and scans nothing. Whenever `--group` is given the guard runs — even for an apparently + // additive change — because it re-derives the dropped set from its own fresh scan, and + // deciding from the `get_feed` read above would miss a group added in between. if let Some(new_groups) = &groups { - if feed.groups.iter().any(|g| !new_groups.contains(g)) { - unsubscribe_orphans( - client, - out, - &pubkey, - &feed.code, - new_groups, - self.force_unsubscribe, - )?; - } + unsubscribe_orphans( + client, + out, + &pubkey, + &feed.code, + new_groups, + self.force_unsubscribe, + )?; } let signature = client.update_feed(UpdateFeedCommand { @@ -206,8 +205,10 @@ mod tests { .contains(&format!("Signature: {signature}"))); } + /// An additive change scans (the guard re-derives the dropped set from its own snapshot) but + /// finds nothing dropped, so an existing subscriber needs no flag and no removals happen. #[test] - fn test_cli_feed_update_additive_change_does_not_scan() { + fn test_cli_feed_update_additive_change_needs_no_flag() { let mut client = create_test_client(); client.expect_check_requirements().returning(|_| Ok(())); @@ -215,8 +216,10 @@ mod tests { let (g1, g2) = (f.groups[0], f.groups[1]); let signature = Signature::new_unique(); f.expect_get_feed(&mut client, vec![g1]); - client.expect_list_user().times(0); - client.expect_list_accesspass().times(0); + // The scanned feed carries [g1, g2] (the fixture's full set); the new set is a superset, + // so nothing is dropped even though the user subscribes to g2. + f.expect_scan(&mut client, vec![g2]); + client.expect_update_multicastgroup_roles().times(0); client .expect_update_feed() .with(predicate::eq(UpdateFeedCommand { @@ -341,6 +344,7 @@ mod tests { exchange: exchange_pk, groups: vec![], }; + let feed_for_get = feed.clone(); client .expect_get_feed() .with(predicate::eq(GetFeedCommand { @@ -348,7 +352,19 @@ mod tests { exchange: Some(exchange_pk), })) .times(1) - .returning(move |_| Ok((feed_pk, feed.clone()))); + .returning(move |_| Ok((feed_pk, feed_for_get.clone()))); + + // `--group` always runs the guard; with no users the scan finds nothing to drop. + client.expect_list_user().returning(|_| Ok(HashMap::new())); + client + .expect_list_accesspass() + .returning(|_| Ok(HashMap::new())); + client + .expect_list_device() + .returning(|_| Ok(HashMap::new())); + client + .expect_list_feed() + .returning(move |_| Ok(HashMap::from([(feed_pk, feed.clone())]))); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, From 8112c226966df082d42c75b14613116a5b20fe37 Mon Sep 17 00:00:00 2001 From: Nik Weidenbacher Date: Thu, 30 Jul 2026 18:28:47 +0000 Subject: [PATCH 4/6] cli: index passes by payer in the feed guard planner Review follow-ups: pre-index access passes by user_payer once per plan() call instead of rescanning every pass per affected user, and add this PR's reference to the changelog entry per the file's convention. Ref malbeclabs/infra#2113 --- CHANGELOG.md | 2 +- smartcontract/cli/src/feed/guard.rs | 32 +++++++++++++++++------------ 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d50766d647..955748e4c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All notable changes to this project will be documented in this file. ### Breaking - CLI - - `doublezero feed update` (with `--group`) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user holding a multicast group role that no feed on their access pass carries and the pass's own allowlists don't authorize, printing the affected users, groups, and roles and submitting nothing. Pass `--force-unsubscribe` to remove those roles first (allowlist-covered roles are re-asserted, not removed) and then apply the change. A rename or an update without `--group` is unaffected; an additive `--group` set scans but finds nothing to do. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113) + - `doublezero feed update` (with `--group`) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user holding a multicast group role that no feed on their access pass carries and the pass's own allowlists don't authorize, printing the affected users, groups, and roles and submitting nothing. Pass `--force-unsubscribe` to remove those roles first (allowlist-covered roles are re-asserted, not removed) and then apply the change. A rename or an update without `--group` is unaffected; an additive `--group` set scans but finds nothing to do. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113, #4119) ### Changes diff --git a/smartcontract/cli/src/feed/guard.rs b/smartcontract/cli/src/feed/guard.rs index 9d119f2679..46d42f2fd4 100644 --- a/smartcontract/cli/src/feed/guard.rs +++ b/smartcontract/cli/src/feed/guard.rs @@ -172,6 +172,16 @@ fn scan(client: &C) -> eyre::Result { pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Result> { let mut orphans = Vec::new(); + // Index once so the per-user pass lookup doesn't rescan every pass (O(users × passes) when a + // drop touches many users). + let mut passes_by_payer: HashMap> = HashMap::new(); + for pass in snap.accesspasses.values() { + passes_by_payer + .entry(pass.user_payer) + .or_default() + .push(pass); + } + for (user_pk, user) in &snap.users { // Cheapest test first: a user holding none of the dropped groups cannot be orphaned, // whatever their pass or device looks like. Keeping it ahead of the lookups below also @@ -188,7 +198,15 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu // path prefers: `create_user` and `update_multicastgroup_roles` both accept either the // exact-IP PDA or the shared UNSPECIFIED one, so a dynamic Prepaid pass must not be // allowed to hide a live EdgeSeat pass at the other PDA. - let passes: Vec<&AccessPass> = accepted_passes(&snap.accesspasses, user).collect(); + let passes: Vec<&AccessPass> = passes_by_payer + .get(&user.owner) + .into_iter() + .flatten() + .filter(|pass| { + pass.client_ip == Ipv4Addr::UNSPECIFIED || pass.client_ip == user.client_ip + }) + .copied() + .collect(); let seated: Vec = passes .iter() .flat_map(|pass| pass.feed_seats().iter().map(|seat| seat.feed_key)) @@ -253,18 +271,6 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu Ok(orphans) } -/// The passes the program would accept for `user`: those owned by their payer at either the -/// exact-IP PDA or the shared UNSPECIFIED-IP one. -fn accepted_passes<'a>( - accesspasses: &'a HashMap, - user: &'a User, -) -> impl Iterator { - accesspasses.values().filter(move |pass| { - pass.user_payer == user.owner - && (pass.client_ip == Ipv4Addr::UNSPECIFIED || pass.client_ip == user.client_ip) - }) -} - fn report( out: &mut W, feed_code: &str, From 4876bec093c785287f9e8fec6ec8275e20c75c83 Mon Sep 17 00:00:00 2001 From: Nik Weidenbacher Date: Thu, 30 Jul 2026 18:57:26 +0000 Subject: [PATCH 5/6] cli: fail closed on mixed-role orphans; count only consumed seats as coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the feed guard: - Re-asserting an allowlist-covered role as `true` is a grant, not "leave it alone": it flips the required permission to ACCESS_PASS_ADMIN, requires the user be Activated, and re-checks the allowlist against the single pass the SDK resolves. So a mixed-role orphan (one role removable, the other still allowlisted) now fails the run closed for manual handling, and the automated path stays removal-only — keeping the USER_ADMIN hint accurate. - Feed coverage now requires the covering feed to be among the seats the user actually consumed (user.feed_pks), mirroring UnsubscribeFeed's retained-feeds check. A seated feed the user never consumed would keep the group alive with no seat accounting for it. - The disappeared-feed error no longer claims "nothing was changed" (from round 2 onward removals have landed); it says the feed change was not submitted. - Tests: mixed-role force run bails before submitting; the round-limit abort pins its error message; consumed-vs-unconsumed seat coverage. Ref malbeclabs/infra#2113 --- CHANGELOG.md | 2 +- smartcontract/cli/src/feed/guard.rs | 97 ++++++++++++++++++++++------ smartcontract/cli/src/feed/update.rs | 83 ++++++++++++++++++++++-- 3 files changed, 158 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 955748e4c4..4ace4bebfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All notable changes to this project will be documented in this file. ### Breaking - CLI - - `doublezero feed update` (with `--group`) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user holding a multicast group role that no feed on their access pass carries and the pass's own allowlists don't authorize, printing the affected users, groups, and roles and submitting nothing. Pass `--force-unsubscribe` to remove those roles first (allowlist-covered roles are re-asserted, not removed) and then apply the change. A rename or an update without `--group` is unaffected; an additive `--group` set scans but finds nothing to do. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113, #4119) + - `doublezero feed update` (with `--group`) and `doublezero feed delete` now fail closed if the change would leave an EdgeSeat user holding a multicast group role that no consumed feed seat on their access pass covers and the pass's own allowlists don't authorize, printing the affected users, groups, and roles and submitting nothing. Pass `--force-unsubscribe` to remove those roles first and then apply the change; a group where one role must go while the allowlist still authorizes the other fails closed even with the flag and needs manual cleanup. A rename or an update without `--group` is unaffected; an additive `--group` set scans but finds nothing to do. The removals need `USER_ADMIN` (or foundation membership) on the payer in addition to `FEED_AUTHORITY`. (malbeclabs/infra#2113, #4119) ### Changes diff --git a/smartcontract/cli/src/feed/guard.rs b/smartcontract/cli/src/feed/guard.rs index 46d42f2fd4..3eec3262a9 100644 --- a/smartcontract/cli/src/feed/guard.rs +++ b/smartcontract/cli/src/feed/guard.rs @@ -45,12 +45,23 @@ pub struct Orphan { /// the change through. pub remove_publisher: bool, pub remove_subscriber: bool, - /// Held roles kept because an accepted pass's own allowlist still authorizes them; the - /// removal re-asserts these so they survive it. + /// Held roles an accepted pass's own allowlist still authorizes. These cannot be preserved + /// through the automated removal — `UpdateMulticastGroupRoles` reads a `true` as a grant, not + /// "leave it alone", which flips the required permission to ACCESS_PASS_ADMIN, requires the + /// user be Activated, and re-checks the allowlist against the single pass the SDK resolves — + /// so an orphan carrying one of these fails the run closed for manual handling. pub keep_publisher: bool, pub keep_subscriber: bool, } +impl Orphan { + /// One role must go while the other is still authorized — not expressible as the + /// removal-only update the automated path issues. + fn is_mixed(&self) -> bool { + self.keep_publisher || self.keep_subscriber + } +} + /// The onchain state [`plan`] reads, fetched once per round by [`scan`]. pub struct Snapshot { pub users: HashMap, @@ -82,7 +93,10 @@ pub fn unsubscribe_orphans( // the command read before the loop: a group added to the feed mid-run is dropped by this // change too, and pinning the earlier set would let its subscribers through unseen. let feed = snap.feeds.get(feed_pk).ok_or_else(|| { - eyre::eyre!("feed '{feed_code}' ({feed_pk}) disappeared mid-run; nothing was changed") + eyre::eyre!( + "feed '{feed_code}' ({feed_pk}) disappeared mid-run; the feed change was \ + not submitted" + ) })?; let dropped: Vec = feed .groups @@ -113,6 +127,14 @@ pub fn unsubscribe_orphans( subscriptions to its groups have quiesced." ); } + if orphans.iter().any(Orphan::is_mixed) { + eyre::bail!( + "feed '{feed_code}' has membership(s) where one role must be removed while the \ + other is still authorized by the pass allowlist (flagged above). The automated \ + path only issues removal-only updates, so clean those users up manually, then \ + re-run. Nothing was submitted this round." + ); + } for orphan in &orphans { client @@ -120,10 +142,8 @@ pub fn unsubscribe_orphans( user_pk: orphan.user_pk, group_pk: orphan.group_pk, client_ip: orphan.client_ip, - // Desired absolute state, not a toggle: an allowlist-covered role is - // re-asserted so only the orphaned role is removed. - publisher: orphan.keep_publisher, - subscriber: orphan.keep_subscriber, + publisher: false, + subscriber: false, device_pk: None, feed_pk: None, }) @@ -160,10 +180,13 @@ fn scan(client: &C) -> eyre::Result { /// The memberships that dropping `dropped` from `feed_pk` would orphan. /// -/// A membership survives when some *other* feed seated on the same access pass still carries the -/// group and serves the user's metro — that is the same coverage test the program applies at -/// connect time. The rotated feed itself never counts: every group in `dropped` is by construction -/// absent from its post-change set, and a delete removes it entirely. +/// A membership survives when some *other* feed still carries the group in the user's metro, +/// provided that feed is both seated on an accepted pass and among the seats the user actually +/// consumed (`user.feed_pks`) — the same set `UnsubscribeFeed`'s retained-feeds check releases +/// seats against. A seated feed the user never consumed is not coverage: keeping the group on its +/// strength would leave usage no seat accounts for. The rotated feed itself never counts: every +/// group in `dropped` is by construction absent from its post-change set, and a delete removes it +/// entirely. /// /// A role also survives when an accepted pass's own allowlist authorizes it /// (`mgroup_pub_allowlist` for publisher, `mgroup_sub_allowlist` for subscriber): the program runs @@ -232,9 +255,11 @@ pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Resu })?; for group_pk in held { - // Feed coverage is role-agnostic; allowlist coverage is per role. + // Feed coverage is role-agnostic; allowlist coverage is per role. Only a seat the + // user consumed counts, mirroring the retained-feeds set seat release checks. let feed_covered = seated.iter().any(|seated_pk| { seated_pk != feed_pk + && user.feed_pks.contains(seated_pk) && snap.feeds.get(seated_pk).is_some_and(|other| { other.groups_for(&device.exchange_pk).contains(group_pk) }) @@ -316,7 +341,10 @@ fn report( if kept.is_empty() { String::new() } else { - format!(" (keeping allowlisted {})", kept.join(", ")) + format!( + " ({} still allowlisted — resolve manually)", + kept.join(", ") + ) } )?; } @@ -580,7 +608,7 @@ mod tests { } #[test] - fn test_plan_skips_group_covered_by_another_seated_feed_in_the_same_metro() { + fn test_plan_skips_group_covered_by_another_consumed_feed_in_the_same_metro() { let mut f = fixture(); let other_pk = Pubkey::new_unique(); f.snap @@ -595,10 +623,35 @@ mod tests { AccessPassType::EdgeSeat(vec![seat(f.feed_pk), seat(other_pk)]), ), )]); + f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk, other_pk]; assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); } + /// A seated feed whose seat the user never consumed (absent from `feed_pks`) is not coverage: + /// seat release only retains against consumed seats, so the membership would outlive its + /// accounting. + #[test] + fn test_plan_does_not_count_a_seated_feed_the_user_never_consumed() { + let mut f = fixture(); + let other_pk = Pubkey::new_unique(); + f.snap + .feeds + .insert(other_pk, feed(f.exchange, vec![f.group])); + let owner = f.snap.users[&f.user_pk].owner; + f.snap.accesspasses = HashMap::from([( + Pubkey::new_unique(), + pass( + owner, + f.client_ip, + AccessPassType::EdgeSeat(vec![seat(f.feed_pk), seat(other_pk)]), + ), + )]); + f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk]; + + assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + } + #[test] fn test_plan_does_not_count_another_feed_in_a_different_metro() { let mut f = fixture(); @@ -615,6 +668,8 @@ mod tests { AccessPassType::EdgeSeat(vec![seat(f.feed_pk), seat(other_pk)]), ), )]); + // Consumed, so the metro mismatch is the only reason it doesn't cover. + f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk, other_pk]; assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); } @@ -623,8 +678,10 @@ mod tests { /// excuse a group it is about to stop carrying. #[test] fn test_plan_does_not_count_the_rotated_feeds_own_seat_as_coverage() { - let f = fixture(); - // `feeds` still holds the pre-rotation group set, and the pass is seated on it. + let mut f = fixture(); + // `feeds` still holds the pre-rotation group set, the pass is seated on it, and the seat + // is even consumed — none of which excuses a group the feed is about to stop carrying. + f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk]; assert!(f.snap.feeds[&f.feed_pk].groups.contains(&f.group)); assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); } @@ -704,6 +761,7 @@ mod tests { AccessPassType::EdgeSeat(vec![seat(other_pk)]), ), ); + f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk, other_pk]; assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); } @@ -783,10 +841,10 @@ mod tests { assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); } - /// Mixed roles: the uncovered publisher role is removed, the allowlist-covered subscriber - /// role is re-asserted so the removal leaves it in place. + /// Mixed roles: the uncovered publisher role must go while the allowlist still authorizes the + /// subscriber role — flagged as mixed so the run fails closed for manual handling. #[test] - fn test_plan_removes_only_the_uncovered_role() { + fn test_plan_flags_a_mixed_role_orphan() { let mut f = fixture(); f.snap .users @@ -810,6 +868,7 @@ mod tests { keep_subscriber: true, }] ); + assert!(orphans[0].is_mixed()); } /// Allowlist coverage counts from any accepted pass, not just the one carrying the seat. diff --git a/smartcontract/cli/src/feed/update.rs b/smartcontract/cli/src/feed/update.rs index a20f790eb2..0141b2654a 100644 --- a/smartcontract/cli/src/feed/update.rs +++ b/smartcontract/cli/src/feed/update.rs @@ -254,9 +254,9 @@ mod tests { let (g1, g2) = (f.groups[0], f.groups[1]); f.expect_get_feed(&mut client, vec![g1, g2]); // Every scan sees the same still-subscribed user, as if a new subscription raced each - // unsubscribe pass; uncapped so the guard's round limit decides when to stop. Deliberately - // not `expect_scan`: that helper is capped per call, which would hard-code the guard's - // internal round count. + // unsubscribe pass. Deliberately not `expect_scan` (capped per call): uncapped scans let + // the guard run until its own round limit trips, which the removal count below then pins + // at one removal per round. let user_pk = f.user_pk; let user_acct = user(f.owner, f.device_pk, f.client_ip, vec![g2]); client @@ -298,7 +298,82 @@ mod tests { } .execute(&ctx, &client, &mut output), ); - assert!(res.is_err(), "{res:?}"); + let err = res.unwrap_err(); + assert!( + err.to_string().contains("kept appearing"), + "expected the round-limit error, got: {err}" + ); + } + + /// A mixed-role orphan (one role must go, the other still allowlist-authorized) cannot be + /// expressed as the removal-only update the automated path issues, so even `--force` fails + /// closed before submitting anything. + #[test] + fn test_cli_feed_update_force_fails_closed_on_a_mixed_role_orphan() { + let mut client = create_test_client(); + client.expect_check_requirements().returning(|_| Ok(())); + + let f = GuardFixture::new(2); + let (g1, g2) = (f.groups[0], f.groups[1]); + f.expect_get_feed(&mut client, vec![g1, g2]); + + // One scan: the user publishes and subscribes g2; the sub allowlist still authorizes the + // subscriber role, so only the publisher role is removable. + let user_pk = f.user_pk; + let mut user_acct = user(f.owner, f.device_pk, f.client_ip, vec![g2]); + user_acct.publishers.push(g2); + client + .expect_list_user() + .times(1) + .returning(move |_| Ok(HashMap::from([(user_pk, user_acct.clone())]))); + let mut pass_acct = pass( + f.owner, + f.client_ip, + AccessPassType::EdgeSeat(vec![seat(f.feed_pk)]), + ); + pass_acct.mgroup_sub_allowlist.push(g2); + client + .expect_list_accesspass() + .times(1) + .returning(move |_| Ok(HashMap::from([(Pubkey::new_unique(), pass_acct.clone())]))); + let device_pk = f.device_pk; + let device_acct = device(f.exchange_pk); + client + .expect_list_device() + .times(1) + .returning(move |_| Ok(HashMap::from([(device_pk, device_acct.clone())]))); + let feed_pk = f.feed_pk; + let feed_acct = feed_account(f.exchange_pk, vec![g1, g2]); + client + .expect_list_feed() + .times(1) + .returning(move |_| Ok(HashMap::from([(feed_pk, feed_acct.clone())]))); + + client.expect_update_multicastgroup_roles().times(0); + client.expect_update_feed().times(0); + + let ctx = cli_context_default_for_tests(); + let mut output = Vec::new(); + let res = block_on( + UpdateFeedCliCommand { + pubkey: f.feed_pk.to_string(), + exchange: None, + name: None, + groups: vec![g1.to_string()], + force_unsubscribe: true, + } + .execute(&ctx, &client, &mut output), + ); + let err = res.unwrap_err(); + assert!( + err.to_string().contains("manually"), + "expected the mixed-role error, got: {err}" + ); + let output = String::from_utf8(output).unwrap(); + assert!( + output.contains("still allowlisted"), + "plan does not flag the kept role: {output}" + ); } #[test] From a56974d99d4f47af3d0c29c0b31c6d5f63d7f425 Mon Sep 17 00:00:00 2001 From: Nik Weidenbacher Date: Thu, 30 Jul 2026 19:20:53 +0000 Subject: [PATCH 6/6] cli: rename the guard planner and dedupe a test comment Review nits: plan() becomes find_orphans() with its doc comment trimmed to the coverage rule, and the thrice-repeated pinned-pubkey explanation in the create_subscribe tests moves into one short_mgroup_pubkey() helper. Ref malbeclabs/infra#2113 --- smartcontract/cli/src/feed/guard.rs | 95 ++++++++++++------- .../cli/src/user/create_subscribe.rs | 22 ++--- 2 files changed, 71 insertions(+), 46 deletions(-) diff --git a/smartcontract/cli/src/feed/guard.rs b/smartcontract/cli/src/feed/guard.rs index 3eec3262a9..6b885b5d98 100644 --- a/smartcontract/cli/src/feed/guard.rs +++ b/smartcontract/cli/src/feed/guard.rs @@ -62,7 +62,7 @@ impl Orphan { } } -/// The onchain state [`plan`] reads, fetched once per round by [`scan`]. +/// The onchain state [`find_orphans`] reads, fetched once per round by [`scan`]. pub struct Snapshot { pub users: HashMap, pub accesspasses: HashMap, @@ -105,7 +105,7 @@ pub fn unsubscribe_orphans( .copied() .collect(); - let orphans = plan(feed_pk, &dropped, &snap)?; + let orphans = find_orphans(feed_pk, &dropped, &snap)?; if orphans.is_empty() { return Ok(()); } @@ -180,19 +180,15 @@ fn scan(client: &C) -> eyre::Result { /// The memberships that dropping `dropped` from `feed_pk` would orphan. /// -/// A membership survives when some *other* feed still carries the group in the user's metro, -/// provided that feed is both seated on an accepted pass and among the seats the user actually -/// consumed (`user.feed_pks`) — the same set `UnsubscribeFeed`'s retained-feeds check releases -/// seats against. A seated feed the user never consumed is not coverage: keeping the group on its -/// strength would leave usage no seat accounts for. The rotated feed itself never counts: every -/// group in `dropped` is by construction absent from its post-change set, and a delete removes it -/// entirely. -/// -/// A role also survives when an accepted pass's own allowlist authorizes it -/// (`mgroup_pub_allowlist` for publisher, `mgroup_sub_allowlist` for subscriber): the program runs -/// that check for every pass type, so such a membership is legal with no feed involved, and seat -/// release never keys on a group the feeds don't carry. -pub fn plan(feed_pk: &Pubkey, dropped: &[Pubkey], snap: &Snapshot) -> eyre::Result> { +/// A role survives when another feed the user holds a consumed seat on (`user.feed_pks`, the set +/// seat release retains against) still carries the group in their metro, or when the pass's own +/// per-role allowlist authorizes it. The rotated feed itself never counts: every group in +/// `dropped` is by construction absent from its post-change set. +pub fn find_orphans( + feed_pk: &Pubkey, + dropped: &[Pubkey], + snap: &Snapshot, +) -> eyre::Result> { let mut orphans = Vec::new(); // Index once so the per-user pass lookup doesn't rescan every pass (O(users × passes) when a @@ -578,7 +574,7 @@ mod tests { #[test] fn test_plan_flags_dropped_group_the_user_holds() { let f = fixture(); - let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); + let orphans = find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap(); assert_eq!( orphans, vec![Orphan { @@ -596,7 +592,7 @@ mod tests { #[test] fn test_plan_ignores_dropped_group_the_user_does_not_hold() { let f = fixture(); - assert!(plan(&f.feed_pk, &[Pubkey::new_unique()], &f.snap) + assert!(find_orphans(&f.feed_pk, &[Pubkey::new_unique()], &f.snap) .unwrap() .is_empty()); } @@ -604,7 +600,7 @@ mod tests { #[test] fn test_plan_empty_when_nothing_dropped() { let f = fixture(); - assert!(plan(&f.feed_pk, &[], &f.snap).unwrap().is_empty()); + assert!(find_orphans(&f.feed_pk, &[], &f.snap).unwrap().is_empty()); } #[test] @@ -625,7 +621,9 @@ mod tests { )]); f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk, other_pk]; - assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + assert!(find_orphans(&f.feed_pk, &[f.group], &f.snap) + .unwrap() + .is_empty()); } /// A seated feed whose seat the user never consumed (absent from `feed_pks`) is not coverage: @@ -649,7 +647,10 @@ mod tests { )]); f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk]; - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } #[test] @@ -671,7 +672,10 @@ mod tests { // Consumed, so the metro mismatch is the only reason it doesn't cover. f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk, other_pk]; - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } /// The sharp edge: the rotated feed's *old* set is not coverage. Its own seat must never @@ -683,7 +687,10 @@ mod tests { // is even consumed — none of which excuses a group the feed is about to stop carrying. f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk]; assert!(f.snap.feeds[&f.feed_pk].groups.contains(&f.group)); - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } #[test] @@ -701,7 +708,7 @@ mod tests { .push(second); let groups = f.snap.feeds[&f.feed_pk].groups.clone(); - let orphans = plan(&f.feed_pk, &groups, &f.snap).unwrap(); + let orphans = find_orphans(&f.feed_pk, &groups, &f.snap).unwrap(); assert_eq!(orphans.len(), 2); assert!(orphans.iter().all(|o| o.user_pk == f.user_pk)); } @@ -713,7 +720,7 @@ mod tests { u.subscribers.clear(); u.publishers.push(f.group); - let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); + let orphans = find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap(); assert_eq!(orphans.len(), 1); assert!(orphans[0].remove_publisher && !orphans[0].remove_subscriber); } @@ -727,7 +734,9 @@ mod tests { pass(owner, f.client_ip, AccessPassType::Prepaid), )]); - assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + assert!(find_orphans(&f.feed_pk, &[f.group], &f.snap) + .unwrap() + .is_empty()); } /// The program accepts a pass at either PDA, so a shared dynamic pass must not hide the @@ -741,7 +750,10 @@ mod tests { pass(owner, Ipv4Addr::UNSPECIFIED, AccessPassType::Prepaid), ); - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } /// Coverage is the union over both accepted PDAs, not just one of them. @@ -763,7 +775,9 @@ mod tests { ); f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk, other_pk]; - assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + assert!(find_orphans(&f.feed_pk, &[f.group], &f.snap) + .unwrap() + .is_empty()); } /// A consumed seat keeps the user in scope even if their pass no longer shows it. @@ -777,14 +791,17 @@ mod tests { )]); f.snap.users.get_mut(&f.user_pk).unwrap().feed_pks = vec![f.feed_pk]; - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } #[test] fn test_plan_errors_on_a_user_with_an_unknown_device() { let mut f = fixture(); f.snap.devices.clear(); - let err = plan(&f.feed_pk, &[f.group], &f.snap).unwrap_err(); + let err = find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap_err(); assert!( err.to_string().contains("unknown device"), "unexpected error: {err}" @@ -816,7 +833,10 @@ mod tests { ), ); - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } /// The program authorizes a role through the pass's own allowlist for every pass type, so a @@ -827,7 +847,9 @@ mod tests { let (_, pass) = f.snap.accesspasses.iter_mut().next().unwrap(); pass.mgroup_sub_allowlist.push(f.group); - assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + assert!(find_orphans(&f.feed_pk, &[f.group], &f.snap) + .unwrap() + .is_empty()); } /// Allowlist coverage is per role: a pub allowlist entry says nothing about the subscriber @@ -838,7 +860,10 @@ mod tests { let (_, pass) = f.snap.accesspasses.iter_mut().next().unwrap(); pass.mgroup_pub_allowlist.push(f.group); - assert_eq!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), 1); + assert_eq!( + find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap().len(), + 1 + ); } /// Mixed roles: the uncovered publisher role must go while the allowlist still authorizes the @@ -855,7 +880,7 @@ mod tests { let (_, pass) = f.snap.accesspasses.iter_mut().next().unwrap(); pass.mgroup_sub_allowlist.push(f.group); - let orphans = plan(&f.feed_pk, &[f.group], &f.snap).unwrap(); + let orphans = find_orphans(&f.feed_pk, &[f.group], &f.snap).unwrap(); assert_eq!( orphans, vec![Orphan { @@ -880,6 +905,8 @@ mod tests { dynamic.mgroup_sub_allowlist.push(f.group); f.snap.accesspasses.insert(Pubkey::new_unique(), dynamic); - assert!(plan(&f.feed_pk, &[f.group], &f.snap).unwrap().is_empty()); + assert!(find_orphans(&f.feed_pk, &[f.group], &f.snap) + .unwrap() + .is_empty()); } } diff --git a/smartcontract/cli/src/user/create_subscribe.rs b/smartcontract/cli/src/user/create_subscribe.rs index 14b0c56a8a..27eea03b75 100644 --- a/smartcontract/cli/src/user/create_subscribe.rs +++ b/smartcontract/cli/src/user/create_subscribe.rs @@ -146,6 +146,13 @@ impl CreateSubscribeUserCliCommand { #[cfg(test)] mod tests { + /// A fixed short (< 43 chars) base58 pubkey, so a `--subscriber` argument deterministically + /// takes the code-resolution path: `Pubkey::new_unique()` straddles `parse_pubkey`'s length + /// threshold depending on the process-global counter, which makes the run order matter. + fn short_mgroup_pubkey() -> solana_sdk::pubkey::Pubkey { + solana_sdk::pubkey::Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo") + } + use doublezero_cli_core::testing::{block_on, cli_context_default_for_tests}; use crate::{ @@ -342,10 +349,7 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - // Fixed short base58 (< 43 chars) so `--subscriber` deterministically takes the - // code-resolution path: `Pubkey::new_unique()` straddles that length threshold - // depending on the process-global counter, which makes the run order matter. - let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); + let mgroup_pubkey = short_mgroup_pubkey(); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, owner: Pubkey::default(), @@ -463,10 +467,7 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - // Fixed short base58 (< 43 chars) so `--subscriber` deterministically takes the - // code-resolution path: `Pubkey::new_unique()` straddles that length threshold - // depending on the process-global counter, which makes the run order matter. - let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); + let mgroup_pubkey = short_mgroup_pubkey(); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, owner: Pubkey::default(), @@ -594,10 +595,7 @@ mod tests { .times(1) .returning(move |_| Ok((device_pubkey, device.clone()))); - // Fixed short base58 (< 43 chars) so `--subscriber` deterministically takes the - // code-resolution path: `Pubkey::new_unique()` straddles that length threshold - // depending on the process-global counter, which makes the run order matter. - let mgroup_pubkey = Pubkey::from_str_const("11111115RidqCHAoz6dzmXxGcfWLNzevYqNpaRAUo"); + let mgroup_pubkey = short_mgroup_pubkey(); let mgroup = MulticastGroup { account_type: AccountType::MulticastGroup, owner: Pubkey::default(),