diff --git a/core/conversations/src/conversation.rs b/core/conversations/src/conversation.rs index ba0dd46..2d016f3 100644 --- a/core/conversations/src/conversation.rs +++ b/core/conversations/src/conversation.rs @@ -35,6 +35,10 @@ pub(crate) trait Convo: Identified + Send { /// Advances any time-driven protocol work (de-mls consensus deadlines) and /// reports what it observed, mirroring [`Self::handle_frame`]. fn wakeup(&mut self, service_ctx: &mut ServiceContext) -> Result; + + /// Each current member's MLS leaf-credential content (hex-encoded), self + /// included. + fn members(&self) -> Result>, ChatError>; } /// Group-only operations. @@ -45,10 +49,6 @@ pub(crate) trait GroupConvo: Convo + std::fmt::Debug + S members: &[IdentIdRef], ) -> Result<(), ChatError>; - /// 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 diff --git a/core/conversations/src/conversation/direct_v1.rs b/core/conversations/src/conversation/direct_v1.rs index 8a6cb01..b1cf04a 100644 --- a/core/conversations/src/conversation/direct_v1.rs +++ b/core/conversations/src/conversation/direct_v1.rs @@ -61,4 +61,8 @@ where ) -> Result { self.inner_group.wakeup(service_ctx) } + + fn members(&self) -> Result>, ChatError> { + Convo::::members(&self.inner_group) + } } diff --git a/core/conversations/src/conversation/group_v1.rs b/core/conversations/src/conversation/group_v1.rs index aa3e3d0..d275b2f 100644 --- a/core/conversations/src/conversation/group_v1.rs +++ b/core/conversations/src/conversation/group_v1.rs @@ -295,6 +295,14 @@ impl Convo for GroupV1Convo { fn wakeup(&mut self, _: &mut ServiceContext) -> Result { Ok(ConvoOutcome::empty(self.id().to_string())) } + + fn members(&self) -> Result>, ChatError> { + Ok(self + .mls_group + .members() + .map(|m| m.credential.serialized_content().to_vec()) + .collect()) + } } impl GroupConvo for GroupV1Convo { @@ -344,14 +352,6 @@ impl GroupConvo for GroupV1Convo { self.send_payload(cx, commit.to_bytes()?) } - fn members(&self) -> Result>, ChatError> { - Ok(self - .mls_group - .members() - .map(|m| m.credential.serialized_content().to_vec()) - .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> { diff --git a/core/conversations/src/conversation/group_v2.rs b/core/conversations/src/conversation/group_v2.rs index 9e1e696..c82c5ae 100644 --- a/core/conversations/src/conversation/group_v2.rs +++ b/core/conversations/src/conversation/group_v2.rs @@ -291,6 +291,16 @@ where let events = self.after_op(ctx)?; // publish what poll produced + re-arm alarm Ok(self.outcome_from_events(&events)) } + + fn members(&self) -> Result>, ChatError> { + // Guarantee the local member is listed so callers see the full roster. + let mut members = self.conversation.members()?; + let self_id = self.conversation.member_id_bytes().to_vec(); + if !members.contains(&self_id) { + members.push(self_id); + } + Ok(members) + } } impl GroupConvo for GroupV2Convo @@ -376,16 +386,6 @@ where result.and(flushed) } - fn members(&self) -> Result>, ChatError> { - // Guarantee the local member is listed so callers see the full roster. - let mut members = self.conversation.members()?; - let self_id = self.conversation.member_id_bytes().to_vec(); - if !members.contains(&self_id) { - members.push(self_id); - } - Ok(members) - } - fn pending_members(&self) -> Result>, ChatError> { Ok(self .pending_invites diff --git a/core/conversations/src/core.rs b/core/conversations/src/core.rs index d4a9b0c..6511f4c 100644 --- a/core/conversations/src/core.rs +++ b/core/conversations/src/core.rs @@ -263,52 +263,36 @@ impl<'a, S: ExternalServices + 'static> Core { convo_id: &str, members: &[IdentIdRef], ) -> Result<(), ChatError> { - if self.cached_convos.contains_key(convo_id) { - let convo = self - .cached_convos - .get_mut(convo_id) - .ok_or_else(|| ChatError::NoConvo(convo_id.to_string()))?; + let convo = self + .cached_convos + .get_mut(convo_id) + .ok_or_else(|| ChatError::NoConvo(convo_id.to_string()))?; - match convo { - ConvoTypeOwned::Group(group_convo) => { - group_convo.add_member(&mut self.services, members) - } - ConvoTypeOwned::Direct(convo) => Err(ChatError::UnsupportedFunction( - convo.id().into(), - "Add Member".into(), - )), + match convo { + ConvoTypeOwned::Group(group_convo) => { + group_convo.add_member(&mut self.services, members) } - } else { - let mut convo = self.load_group_convo(convo_id)?; - convo.add_member(&mut self.services, members) + ConvoTypeOwned::Direct(convo) => Err(ChatError::UnsupportedFunction( + convo.id().into(), + "Add Member".into(), + )), } } - /// Each member's MLS leaf-credential content (hex-encoded); errors if - /// `convo_id` names a direct (non-group) conversation. + /// Each member's MLS leaf-credential content (hex-encoded), for a direct + /// conversation as for a group. pub fn group_members(&mut self, convo_id: &str) -> Result>, ChatError> { - if self.cached_convos.contains_key(convo_id) { - let convo = self - .cached_convos - .get(convo_id) - .ok_or_else(|| ChatError::NoConvo(convo_id.to_string()))?; + 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.members(), - ConvoTypeOwned::Direct(convo) => Err(ChatError::UnsupportedFunction( - convo.id().into(), - "List Members".into(), - )), - } - } else { - let convo = self.load_group_convo(convo_id)?; - convo.members() - } + convo.members() } /// 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. + /// same encoding as [`Self::group_members`]. A direct conversation has no + /// pending members and reports none. pub fn group_pending_members(&mut self, convo_id: &str) -> Result>, ChatError> { let convo = self .cached_convos @@ -317,10 +301,7 @@ impl<'a, S: ExternalServices + 'static> Core { match convo { ConvoTypeOwned::Group(group_convo) => group_convo.pending_members(), - ConvoTypeOwned::Direct(convo) => Err(ChatError::UnsupportedFunction( - convo.id().into(), - "List Pending Members".into(), - )), + ConvoTypeOwned::Direct(_) => Ok(Vec::new()), } } @@ -464,17 +445,6 @@ impl<'a, S: ExternalServices + 'static> Core { }) } - /// Rebuilds a group conversation; errors if `convo_id` names a non-group. - fn load_group_convo(&mut self, convo_id: &str) -> Result>, ChatError> { - let record = self.load_conversation_meta(convo_id)?; - match record.kind { - ConversationKind::GroupV1 => Ok(Box::new(self.load_mls_convo(&record.local_convo_id)?)), - ConversationKind::Unknown(_) => { - Err(ChatError::UnsupportedConvoType(record.kind.as_str().into())) - } - } - } - /// Rebuilds a group conversation from storage so an operation can run against it. fn load_mls_convo(&mut self, convo_id: &str) -> Result { let group_id_bytes = hex::decode(convo_id).map_err(ChatError::generic)?; @@ -563,4 +533,11 @@ impl Convo for ConvoTypeOwned { ConvoTypeOwned::Direct(convo) => convo.wakeup(service_ctx), } } + + fn members(&self) -> Result>, ChatError> { + match self { + ConvoTypeOwned::Group(group_convo) => group_convo.members(), + ConvoTypeOwned::Direct(convo) => convo.members(), + } + } } diff --git a/crates/generic-chat/src/client.rs b/crates/generic-chat/src/client.rs index 3e0f438..f4e0df4 100644 --- a/crates/generic-chat/src/client.rs +++ b/crates/generic-chat/src/client.rs @@ -229,14 +229,15 @@ where .map_err(Into::into) } - /// 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. + /// The conversation's roster, one [`GroupMember`] per account (self + /// included), for a direct conversation as for a group: 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 (committed, pending) = { let mut core = self.core.lock(); diff --git a/crates/generic-chat/tests/saro_and_raya.rs b/crates/generic-chat/tests/saro_and_raya.rs index 01b954f..4c7f0d5 100644 --- a/crates/generic-chat/tests/saro_and_raya.rs +++ b/crates/generic-chat/tests/saro_and_raya.rs @@ -304,6 +304,43 @@ fn group_metadata_on_direct_conversation_errors() { .expect_err("direct conversation has no group metadata"); } +/// A direct conversation reports its participants like any conversation. Add +/// Member errors on the creator's handle, though the joiner holds the same +/// conversation as a plain group, so the rejection is not conversation-wide. +#[test] +fn direct_conversation_lists_its_participants() { + let bus = MessageBus::default(); + let reg = EphemeralRegistry::new(); + + let (mut saro, _saro_events) = + create_test_client(bus.clone(), reg.clone()).expect("client create"); + let (raya, _raya_events) = create_test_client(bus.clone(), reg.clone()).expect("client create"); + + let saro_addr = saro.addr().to_string(); + let raya_addr = raya.addr().to_string(); + let convo_id = saro + .create_direct_conversation(&raya_addr) + .expect("convo create"); + + let roster = saro.group_members(&convo_id).expect("group_members"); + let mut accounts: Vec> = roster + .iter() + .map(|m| m.account.as_ref().map(|a| a.as_str())) + .collect(); + accounts.sort(); + let mut expected = vec![Some(saro_addr.as_str()), Some(raya_addr.as_str())]; + expected.sort(); + assert_eq!(accounts, expected); + + let err = saro + .add_group_members(&convo_id, &[&raya_addr]) + .expect_err("add member is unsupported on a direct conversation"); + assert!(matches!( + err, + logos_generic_chat::ClientError::Chat(libchat::ChatError::UnsupportedFunction(..)) + )); +} + #[derive(Debug)] struct FailingDelivery { inbound_tx: Sender>,