diff --git a/core/conversations/src/conversation.rs b/core/conversations/src/conversation.rs index 3581eba..ba0dd46 100644 --- a/core/conversations/src/conversation.rs +++ b/core/conversations/src/conversation.rs @@ -48,6 +48,12 @@ pub(crate) trait GroupConvo: Convo + std::fmt::Debug + S /// Each current member's MLS leaf-credential content (hex-encoded), self /// included. fn members(&self) -> Result>, ChatError>; + + /// Each member this conversation invited and the group has not committed + /// yet, in the same encoding as [`Self::members`]. Covers only invites + /// [`Self::add_member`] made here, and is empty for a conversation kind + /// whose add takes effect within that call. + fn pending_members(&self) -> Result>, ChatError>; // All GroupConvos MUST return ConvoMetadata // the return type is Option<_> to support legacy ConvoTypes which // are being phased out. diff --git a/core/conversations/src/conversation/group_v1.rs b/core/conversations/src/conversation/group_v1.rs index 6d6b64a..aa3e3d0 100644 --- a/core/conversations/src/conversation/group_v1.rs +++ b/core/conversations/src/conversation/group_v1.rs @@ -352,6 +352,12 @@ impl GroupConvo for GroupV1Convo { .collect()) } + /// Always empty: `add_member` merges its own commit, so an added member is + /// on the roster by the time the call returns. + fn pending_members(&self) -> Result>, ChatError> { + Ok(Vec::new()) + } + fn metadata(&self) -> Option { None } diff --git a/core/conversations/src/conversation/group_v2.rs b/core/conversations/src/conversation/group_v2.rs index c547e5a..9e1e696 100644 --- a/core/conversations/src/conversation/group_v2.rs +++ b/core/conversations/src/conversation/group_v2.rs @@ -386,6 +386,14 @@ where Ok(members) } + fn pending_members(&self) -> Result>, ChatError> { + Ok(self + .pending_invites + .iter() + .map(|(member_id, _)| member_id.clone()) + .collect()) + } + fn metadata(&self) -> Option { let res = self.conversation.extensions().iter().find_map(|ext| { if let Extension::Unknown(ext_type, UnknownExtension(bytes)) = ext diff --git a/core/conversations/src/core.rs b/core/conversations/src/core.rs index 99ad312..d4a9b0c 100644 --- a/core/conversations/src/core.rs +++ b/core/conversations/src/core.rs @@ -306,6 +306,24 @@ impl<'a, S: ExternalServices + 'static> Core { } } + /// Each member invited here and still awaiting the group's commit, in the + /// same encoding as [`Self::group_members`]; errors if `convo_id` names a + /// direct (non-group) conversation. + pub fn group_pending_members(&mut self, convo_id: &str) -> Result>, ChatError> { + let convo = self + .cached_convos + .get(convo_id) + .ok_or_else(|| ChatError::NoConvo(convo_id.to_string()))?; + + match convo { + ConvoTypeOwned::Group(group_convo) => group_convo.pending_members(), + ConvoTypeOwned::Direct(convo) => Err(ChatError::UnsupportedFunction( + convo.id().into(), + "List Pending Members".into(), + )), + } + } + pub fn list_conversations(&self) -> Result, ChatError> { // Check Legacy load_convo store let records = self.services.store.load_conversations()?; diff --git a/crates/generic-chat/src/client.rs b/crates/generic-chat/src/client.rs index a684ecd..3e0f438 100644 --- a/crates/generic-chat/src/client.rs +++ b/crates/generic-chat/src/client.rs @@ -26,12 +26,19 @@ type LocalSignerId = IdentId; /// Shares [`MessageSender`]'s field semantics: `account` is set only when the /// member's credential claimed an account *and* the directory confirmed this /// device belongs to it. Unlike a message sender, an unconfirmable claim does -/// not hide the member: it is cryptographically in the group, so it is listed -/// by `local_identity` (its device) with `account: None`. +/// not hide the member: a committed member is cryptographically in the group, +/// so it is listed by `local_identity` (its device) with `account: None`. +/// +/// `pending` marks a member whose add the group has not committed yet, so it +/// cannot read the conversation. Only invites this client sent are reported; +/// an add another member proposed is invisible until it commits. The flag +/// clears when the commit admitting the member lands, and an invite the group +/// never commits stays pending for the life of the conversation. #[derive(Debug, Clone, PartialEq, Eq)] pub struct GroupMember { pub account: Option, pub local_identity: IdentId, + pub pending: bool, } /// Metadata a caller supplies when creating a group: its shared name and @@ -222,17 +229,31 @@ where .map_err(Into::into) } - /// The group's roster, one [`GroupMember`] per account (self included). An - /// account's several devices collapse to a single entry surfacing that - /// account; a member whose account claim the directory can't confirm stays - /// on the roster individually, keyed by its device. Costs one directory - /// lookup per member that claims an account, the same per-member cost a - /// received message's sender check pays. + /// The group's roster, one [`GroupMember`] per account (self included), + /// committed members first and this client's uncommitted invites after + /// them, flagged `pending`. An account's several devices collapse to a + /// single entry surfacing that account; a member whose account claim the + /// directory can't confirm stays on the roster individually, keyed by its + /// device. An account that is both committed and pending collapses to its + /// committed entry. Costs one directory lookup per member that claims an + /// account, the same per-member cost a received message's sender check pays. pub fn group_members(&mut self, convo_id: &str) -> Result, ClientError> { - let credentials = self.core.lock().group_members(convo_id)?; - let members = credentials + let (committed, pending) = { + let mut core = self.core.lock(); + ( + core.group_members(convo_id)?, + core.group_pending_members(convo_id)?, + ) + }; + let members = committed .iter() - .filter_map(|credential| roster_member(&self.directory, credential)); + .filter_map(|credential| roster_member(&self.directory, credential)) + .chain(pending.iter().filter_map(|credential| { + roster_member(&self.directory, credential).map(|member| GroupMember { + pending: true, + ..member + }) + })); Ok(dedup_members(members)) } @@ -497,6 +518,7 @@ fn roster_member(directory: &impl AccountDirectory, encoded: &[u8]) -> Option; /// the endorsing bundle, and builds the client on the shared bus/registry with /// the fast GroupV2 timers. Returns the account address peers invite by. fn create_test_client( + message_bus: MessageBus, + reg: EphemeralRegistry, +) -> (TestClient, Receiver, String) { + create_test_client_with(message_bus, reg, fast_group_v2_config()) +} + +/// [`create_test_client`] with explicit GroupV2 timers, for a test that needs +/// to observe the group between two protocol steps. +fn create_test_client_with( message_bus: MessageBus, mut reg: EphemeralRegistry, + config: GroupV2Config, ) -> (TestClient, Receiver, String) { let account = TestLogosAccount::new(); let delegate = DelegateSigner::random(); @@ -52,7 +62,7 @@ fn create_test_client( .ident(delegate) .transport(InProcessDelivery::new(message_bus)) .registration(reg) - .group_v2_config(fast_group_v2_config()) + .group_v2_config(config) .build() .expect("client create"); let addr = client.addr().to_string(); @@ -92,10 +102,11 @@ fn wait_for_group_started(events: &Receiver, label: &str) -> String { }) } -/// Poll a client's roster for `convo_id` until its verified accounts equal -/// `expected` (order-independent), or panic after a timeout. The roster settles -/// asynchronously as each member applies the add commit, so it is polled rather -/// than snapshotted. +/// Poll a client's roster for `convo_id` until its verified *committed* +/// accounts equal `expected` (order-independent), or panic after a timeout. The +/// roster settles asynchronously as each member applies the add commit, so it is +/// polled rather than snapshotted; members still awaiting that commit are +/// skipped so an invite alone never reads as convergence. fn wait_for_members(client: &mut TestClient, convo_id: &str, expected: &[&str]) { use std::collections::BTreeSet; let want: BTreeSet<&str> = expected.iter().copied().collect(); @@ -104,6 +115,7 @@ fn wait_for_members(client: &mut TestClient, convo_id: &str, expected: &[&str]) let roster = client.group_members(convo_id).expect("group_members"); let got: BTreeSet<&str> = roster .iter() + .filter(|m| !m.pending) .filter_map(|m| m.account.as_ref().map(|a| a.as_str())) .collect(); if got == want { @@ -275,6 +287,77 @@ fn group_creator_is_in_own_roster() { assert_eq!(accounts, vec![Some(saro_addr.as_str())]); } +/// An invited member joins the roster immediately, flagged pending: the add is +/// staged as a proposal, so the invitee is not a member until the group commits. +#[test] +fn invited_member_is_pending_until_the_group_commits() { + let bus = MessageBus::default(); + let reg = EphemeralRegistry::new(); + + // A commit window far longer than the assertions below, so the add provably + // cannot merge while they run. + let deferred_commit = GroupV2Config { + commit_inactivity_duration: Duration::from_secs(30), + ..fast_group_v2_config() + }; + let (mut saro, _saro_events, saro_addr) = + create_test_client_with(bus.clone(), reg.clone(), deferred_commit); + let (_raya, _raya_events, raya_addr) = create_test_client(bus.clone(), reg.clone()); + + let convo_id = saro + .create_group_conversation(&[], unnamed_group()) + .expect("empty group"); + saro.add_group_members(&convo_id, &[&raya_addr]) + .expect("saro invites raya"); + + let roster = saro.group_members(&convo_id).expect("group_members"); + let accounts = |pending: bool| -> Vec<&str> { + roster + .iter() + .filter(|m| m.pending == pending) + .filter_map(|m| m.account.as_ref().map(|a| a.as_str())) + .collect() + }; + assert_eq!(accounts(false), vec![saro_addr.as_str()]); + assert_eq!(accounts(true), vec![raya_addr.as_str()]); +} + +/// The pending flag is transient: once the group commits the add, the invitee +/// is an ordinary roster member and nothing is left pending. The joiner, which +/// invited nobody, never reports a pending member at all: the flag is local to +/// the client that sent the invite. +#[test] +fn pending_clears_once_the_add_commits() { + let bus = MessageBus::default(); + let reg = EphemeralRegistry::new(); + + let (mut saro, _saro_events, saro_addr) = create_test_client(bus.clone(), reg.clone()); + let (mut raya, raya_events, raya_addr) = create_test_client(bus.clone(), reg.clone()); + + let convo_id = saro + .create_group_conversation(&[], unnamed_group()) + .expect("empty group"); + saro.add_group_members(&convo_id, &[&raya_addr]) + .expect("saro invites raya"); + + let raya_convo_id = wait_for_group_started(&raya_events, "raya ConversationStarted"); + wait_for_members(&mut saro, &convo_id, &[&saro_addr, &raya_addr]); + + let roster = saro.group_members(&convo_id).expect("group_members"); + assert!( + roster.iter().all(|m| !m.pending), + "committed roster still reports a pending member: {roster:?}" + ); + + let joiner_roster = raya + .group_members(&raya_convo_id) + .expect("joiner group_members"); + assert!( + joiner_roster.iter().all(|m| !m.pending), + "joiner reports a pending member it never invited: {joiner_roster:?}" + ); +} + /// A batch add is validated before any member is proposed: a member whose /// account is endorsed in the directory but whose device registered no key /// package fails the whole call, the resolvable member in the same batch is