Self Removals

dev/gen_protos.sh

@@ -6,7 +6,8 @@ if ! cargo install --list | grep "protoc-gen-prost-crate" > /dev/null; then
     fi
 fi
 
-if ! buf generate https://github.com/xmtp/proto.git#branch=main,subdir=proto; then
+# if ! buf generate https://github.com/xmtp/proto.git#branch=main,subdir=proto; then
+if ! buf generate /Users/insipx/Projects/xmtp/workspace-proto/insipx/new-base-policy/proto; then
     echo "Failed to generate protobuf definitions"
     exit 1
 fi

xmtp_mls/src/groups/group_membership.rs

@@ -37,13 +37,20 @@ impl GroupMembership {
             .collect()
     }
 
+    pub fn merge<'inbox_id>(&'inbox_id self, other: &'inbox_id Self) -> GroupMembership {
+        let mut merged = self.members.clone();
+        merged.extend(other.members.clone());
+        Self { members: merged }
+    }
+
     pub fn diff<'inbox_id>(
         &'inbox_id self,
         new_group_membership: &'inbox_id Self,
     ) -> MembershipDiff {
         let mut removed_inboxes: Vec<&String> = vec![];
         let mut updated_inboxes: Vec<&String> = vec![];
 
+        log::debug!("NEW_MEMBERSHIP = {:?}", new_group_membership);
         for (inbox_id, last_sequence_id) in self.members.iter() {
             match new_group_membership.get(inbox_id) {
                 Some(new_last_sequence_id) => {
@@ -52,6 +59,7 @@ impl GroupMembership {
                     }
                 }
                 None => {
+                    log::debug!("ADDING TO REMOVED INBOXES");
                     removed_inboxes.push(inbox_id);
                 }
             }
@@ -68,6 +76,7 @@ impl GroupMembership {
                 }
             })
             .collect::<Vec<&String>>();
+        log::debug!("REMOVED_INBOXES={:?}", removed_inboxes);
 
         MembershipDiff {
             added_inboxes,

xmtp_mls/src/groups/group_permissions.rs

@@ -598,6 +598,7 @@ pub enum PolicyError {
     FromProtoUpdatePermissionsInvalidPolicy,
 }
 
+/// Order must be same as proto
 #[derive(Clone, Copy, Debug, PartialEq)]
 #[allow(dead_code)]
 #[repr(u8)]
@@ -608,6 +609,7 @@ pub enum BasePolicies {
     AllowSameMember,
     AllowIfAdminOrSuperAdmin,
     AllowIfSuperAdmin,
+    AllowIfAdminOrSuperAdminOrTarget,
 }
 
 impl MembershipPolicy for BasePolicies {
@@ -618,6 +620,9 @@ impl MembershipPolicy for BasePolicies {
             BasePolicies::AllowSameMember => inbox.inbox_id == actor.inbox_id,
             BasePolicies::AllowIfAdminOrSuperAdmin => actor.is_admin || actor.is_super_admin,
             BasePolicies::AllowIfSuperAdmin => actor.is_super_admin,
+            BasePolicies::AllowIfAdminOrSuperAdminOrTarget => {
+                actor.is_admin || actor.is_super_admin || actor.inbox_id == inbox.inbox_id
+            }
         }
     }
 
@@ -630,6 +635,9 @@ impl MembershipPolicy for BasePolicies {
                 BasePolicyProto::AllowIfAdminOrSuperAdmin as i32
             }
             BasePolicies::AllowIfSuperAdmin => BasePolicyProto::AllowIfSuperAdmin as i32,
+            BasePolicies::AllowIfAdminOrSuperAdminOrTarget => {
+                BasePolicyProto::AllowIfAdminOrSuperAdminOrTarget as i32
+            }
         };
 
         Ok(MembershipPolicyProto {
@@ -660,11 +668,16 @@ impl MembershipPolicies {
         MembershipPolicies::Standard(BasePolicies::AllowSameMember)
     }
 
-    #[allow(dead_code)]
     pub fn allow_if_actor_admin() -> Self {
         MembershipPolicies::Standard(BasePolicies::AllowIfAdminOrSuperAdmin)
     }
 
+    /// Allow a remove if admin is removing or remover
+    /// is themselves (both commit sender and target)
+    pub fn allow_if_actor_admin_or_superadmin_or_target() -> Self {
+        MembershipPolicies::Standard(BasePolicies::AllowIfAdminOrSuperAdminOrTarget)
+    }
+
     #[allow(dead_code)]
     pub fn allow_if_actor_super_admin() -> Self {
         MembershipPolicies::Standard(BasePolicies::AllowIfSuperAdmin)
@@ -689,6 +702,7 @@ impl TryFrom<MembershipPolicyProto> for MembershipPolicies {
                 2 => Ok(MembershipPolicies::deny()),
                 3 => Ok(MembershipPolicies::allow_if_actor_admin()),
                 4 => Ok(MembershipPolicies::allow_if_actor_super_admin()),
+                5 => Ok(MembershipPolicies::allow_if_actor_admin_or_superadmin_or_target()),
                 _ => Err(PolicyError::InvalidMembershipPolicy),
             },
             Some(PolicyKindProto::AndCondition(inner)) => {
@@ -1087,7 +1101,7 @@ pub(crate) fn policy_all_members() -> PolicySet {
     }
     PolicySet::new(
         MembershipPolicies::allow(),
-        MembershipPolicies::allow_if_actor_admin(),
+        MembershipPolicies::allow_if_actor_admin_or_superadmin_or_target(),
         metadata_policies_map,
         PermissionsPolicies::allow_if_actor_super_admin(),
         PermissionsPolicies::allow_if_actor_super_admin(),

xmtp_mls/src/groups/mod.rs

@@ -666,10 +666,22 @@ impl MlsGroup {
             .await
     }
 
+    /// Initiate the self-removal process for
+    /// the [`InboxId`] of this [`Client`]
+    pub async fn remove_self<ApiClient: XmtpApi>(
+        &self,
+        client: &Client<ApiClient>,
+    ) -> Result<(), GroupError> {
+        log::info!("Removing myself inbox_id={} from the group", client.inbox_id());
+        self.remove_members_by_inbox_id(client, vec![client.identity().inbox_id().to_string()])
+            .await
+    }
+
+    /// Remove members by Ethereum Account Address
     pub async fn remove_members<ApiClient: XmtpApi>(
         &self,
         client: &Client<ApiClient>,
-        account_addresses_to_remove: Vec<InboxId>,
+        account_addresses_to_remove: Vec<String>,
     ) -> Result<(), GroupError> {
         let account_addresses = sanitize_evm_addresses(account_addresses_to_remove)?;
         let inbox_id_map = client.api_client.get_inbox_ids(account_addresses).await?;
@@ -678,6 +690,7 @@ impl MlsGroup {
             .await
     }
 
+    /// Remove members by [`InboxId`]
     pub async fn remove_members_by_inbox_id<ApiClient: XmtpApi>(
         &self,
         client: &Client<ApiClient>,

xmtp_mls/src/groups/sync.rs

@@ -10,7 +10,8 @@ use super::{
         Installation, PostCommitAction, SendMessageIntentData, SendWelcomesAction,
         UpdateAdminListIntentData, UpdateGroupMembershipIntentData, UpdatePermissionIntentData,
     },
-    validated_commit::extract_group_membership,
+    members::GroupMember,
+    validated_commit::{extract_group_membership, get_latest_group_membership},
     GroupError, MlsGroup,
 };
 #[cfg(feature = "message-history")]
@@ -22,6 +23,7 @@ use crate::{
         GRPC_DATA_LIMIT, MAX_GROUP_SIZE, MAX_INTENT_PUBLISH_ATTEMPTS, MAX_PAST_EPOCHS,
         SYNC_UPDATE_INSTALLATIONS_INTERVAL_NS,
     },
+    groups::GroupMembership,
     groups::{intents::UpdateMetadataIntentData, validated_commit::ValidatedCommit},
     hpke::{encrypt_welcome, HpkeError},
     identity::parse_credential,
@@ -499,12 +501,17 @@ impl MlsGroup {
                     "[{}] staged commit is valid, will attempt to merge",
                     self.context.inbox_id()
                 );
+                let group_membership = get_latest_group_membership(&sc)?;
                 openmls_group.merge_staged_commit(provider, sc)?;
                 self.save_transcript_message(
                     provider.conn_ref(),
                     validated_commit,
                     envelope_timestamp_ns,
                 )?;
+
+                self.remove_orphaned_installations(provider, client, group_membership)
+                    .await
+                    .map_err(Box::new)?;
             }
         };
 
@@ -757,7 +764,6 @@ impl MlsGroup {
                         .await
                 })
             );
-
             match result {
                 Err(err) => {
                     log::error!("error getting publish intent data {:?}", err);
@@ -812,7 +818,12 @@ impl MlsGroup {
                     }
                 }
                 Ok(None) => {
-                    log::info!("Skipping intent because no publish data returned");
+                    log::info!("{} Skipping intent id={},kind={},state={:?} because no publish data returned",
+                        client.inbox_id(),
+                        intent.id,
+                        intent.kind,
+                        intent.state
+                    );
                     let deleter: &dyn Delete<StoredGroupIntent, Key = i32> = provider.conn_ref();
                     deleter.delete(intent.id)?;
                 }
@@ -1038,10 +1049,39 @@ impl MlsGroup {
             intent_data.into(),
         ))?;
 
+        // list of installations to remove because no member in member list
+        // construct commit that removes those members
         self.sync_until_intent_resolved(provider, intent.id, client)
             .await
     }
 
+    pub(super) async fn remove_orphaned_installations<ApiClient: XmtpApi>(
+        &self,
+        provider: &XmtpOpenMlsProvider,
+        client: &Client<ApiClient>,
+        received_members: GroupMembership,
+    ) -> Result<(), GroupError> {
+        // time stuff for thundering herd
+        // commit for every found installation that needs removing
+        let received_members: HashSet<String> = received_members.members.keys().cloned().collect();
+        let local_members = self.members_with_provider(client, provider).await?;
+        for GroupMember {
+            inbox_id,
+            installation_ids: _,
+            ..
+        } in local_members
+        {
+            if !received_members.contains(&inbox_id) {
+                tracing::info!("Orphaned local inbox_id={inbox_id}. Prune installations");
+                // get_membership_update_intent(client, provider, vec![], vec![])
+                // prune/create new intent
+                // check that we're not trying to remove our installations
+            }
+        }
+
+        Ok(())
+    }
+
     /**
      * get_membership_update_intent will query the network for any new [`IdentityUpdate`]s for any of the existing
      * group members
@@ -1201,6 +1241,12 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
     let old_group_membership = extract_group_membership(&extensions)?;
     let new_group_membership = intent_data.apply_to_group_membership(&old_group_membership);
 
+    log::debug!(
+        "OLD_MEMBERSHIP={:?},\nNEW_MEMBERSHIP={:?}",
+        old_group_membership,
+        new_group_membership
+    );
+
     // Diff the two membership hashmaps getting a list of inboxes that have been added, removed, or updated
     let membership_diff = old_group_membership.diff(&new_group_membership);
 
@@ -1212,19 +1258,20 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
             &old_group_membership,
             &new_group_membership,
             &membership_diff,
+            &client.inbox_id(),
         )
         .await?;
 
     let mut new_installations: Vec<Installation> = vec![];
     let mut new_key_packages: Vec<KeyPackage> = vec![];
 
-    if !installation_diff.added_installations.is_empty() {
+    if !installation_diff.added_installations().is_empty() {
         let my_installation_id = &client.installation_public_key();
         // Go to the network and load the key packages for any new installation
         let key_packages = client
             .get_key_packages_for_installation_ids(
                 installation_diff
-                    .added_installations
+                    .added_installations()
                     .into_iter()
                     .filter(|installation| my_installation_id.ne(installation))
                     .collect(),
@@ -1238,12 +1285,29 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
         }
     }
 
+    // for everyone else OK
+    // if removed installations includes ourselves need to modify
+    // if !installations_diff.is
+    // make sure not to send a commit to remove me and my wallet ID
+    // if removing myself, cannot do both things (update membership & remove my client b/c fail)
+    // if any leaf nodes have same identity as myself, cannot remove them
+    let own_inbox_id = client.identity().inbox_id();
+
+    let removed_installations = installation_diff
+        .removed_installations
+        .iter()
+        .cloned()
+        .filter(|i| &i.inbox_id != own_inbox_id)
+        .map(|i| i.installation)
+        .collect::<HashSet<Vec<u8>>>();
+
     let leaf_nodes_to_remove: Vec<LeafNodeIndex> =
-        get_removed_leaf_nodes(openmls_group, &installation_diff.removed_installations);
+        get_removed_leaf_nodes(openmls_group, &removed_installations);
 
     if leaf_nodes_to_remove.is_empty()
         && new_key_packages.is_empty()
         && membership_diff.updated_inboxes.is_empty()
+        && membership_diff.removed_inboxes.is_empty()
     {
         return Ok(None);
     }
@@ -1279,6 +1343,8 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
     }))
 }
 
+// Filter out own identity from leaf nodes to return right list
+// for self-removes -- i.e to send the commit to the group
 fn get_removed_leaf_nodes(
     openmls_group: &mut OpenMlsGroup,
     removed_installations: &HashSet<Vec<u8>>,
@@ -1337,4 +1403,36 @@ mod tests {
         }
         future::join_all(futures).await;
     }
+
+    #[tokio::test(flavor = "current_thread")]
+    async fn self_removal_basic_case() {
+        let alix = Arc::new(ClientBuilder::new_test_client(&generate_local_wallet()).await);
+        let bo = Arc::new(ClientBuilder::new_test_client(&generate_local_wallet()).await);
+        let amal = Arc::new(ClientBuilder::new_test_client(&generate_local_wallet()).await);
+
+        log::info!(
+            "alix={},bo={},amal={}",
+            alix.inbox_id(),
+            bo.inbox_id(),
+            amal.inbox_id()
+        );
+
+        let amal_group: Arc<MlsGroup> =
+            Arc::new(amal.create_group(None, Default::default()).unwrap());
+        amal_group.sync(&amal).await.unwrap();
+        // log::debug!("amal_group={:?}", amal_group);
+        amal_group
+            .add_members_by_inbox_id(&amal, vec![bo.inbox_id(), alix.inbox_id()])
+            .await
+            .unwrap();
+
+        alix.sync_welcomes().await.unwrap();
+
+        let alix_group = alix.group(amal_group.group_id.clone()).unwrap();
+        log::info!("Got alix group");
+        amal_group.sync(&amal).await.unwrap();
+        log::info!("\nABOUT TO REMOVE SELF\n");
+        alix_group.remove_self(&alix).await.unwrap();
+        amal_group.sync(&amal).await.unwrap();
+    }
 }