test passes

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

@@ -672,7 +672,9 @@ impl MlsGroup {
         &self,
         client: &Client<ApiClient>,
     ) -> Result<(), GroupError> {
-        self.remove_members_by_inbox_id(client, vec![client.identity().inbox_id()])
+        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

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,6 +1258,7 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
             &old_group_membership,
             &new_group_membership,
             &membership_diff,
+            &client.inbox_id(),
         )
         .await?;
 
@@ -1260,6 +1307,7 @@ async fn apply_update_group_membership_intent<ApiClient: XmtpApi>(
     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);
     }
@@ -1355,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();
+    }
 }

xmtp_mls/src/groups/validated_commit.rs

@@ -264,6 +264,7 @@ impl ValidatedCommit {
             &mutable_metadata,
         )?;
 
+        let old_group_membership = extract_group_membership(existing_group_context.extensions())?;
         // Get the expected diff of installations added and removed based on the difference between the current
         // group membership and the new group membership.
         // Also gets back the added and removed inbox ids from the expected diff
@@ -276,9 +277,10 @@ impl ValidatedCommit {
             conn,
             client,
             staged_commit,
-            existing_group_context,
+            &old_group_membership,
             &immutable_metadata,
             &mutable_metadata,
+            &actor.inbox_id,
         )
         .await?;
 
@@ -297,10 +299,12 @@ impl ValidatedCommit {
         // 2. Anyone referenced in an update proposal
         // Satisfies Rule 4
         for participant in credentials_to_verify {
-            let to_sequence_id = new_group_membership
+            let all_members = old_group_membership.merge(&new_group_membership);
+            let to_sequence_id = all_members
                 .get(&participant.inbox_id)
                 .ok_or(CommitValidationError::SubjectDoesNotExist)?;
-
+            log::debug!("\nMERGED = {:#?}", all_members);
+            log::debug!("\nGETTING ASSOCIATION STATE\n");
             let inbox_state = client
                 .get_association_state(
                     conn,
@@ -320,6 +324,8 @@ impl ValidatedCommit {
             }
         }
 
+        log::debug!("\nBuilding verified commit\n");
+
         let verified_commit = Self {
             actor,
             added_inboxes,
@@ -418,7 +424,7 @@ fn get_proposal_changes(
     })
 }
 
-fn get_latest_group_membership(
+pub(super) fn get_latest_group_membership(
     staged_commit: &StagedCommit,
 ) -> Result<GroupMembership, CommitValidationError> {
     for proposal in staged_commit.queued_proposals() {
@@ -454,16 +460,17 @@ async fn extract_expected_diff<'diff, ApiClient: XmtpApi>(
     conn: &DbConnection,
     client: &Client<ApiClient>,
     staged_commit: &StagedCommit,
-    existing_group_context: &GroupContext,
+    old_group_membership: &GroupMembership,
     immutable_metadata: &GroupMetadata,
     mutable_metadata: &GroupMutableMetadata,
+    sender_inbox_id: &str,
+    // sender_installation_id: Vec<u8>
 ) -> Result<ExpectedDiff, CommitValidationError> {
-    let old_group_membership = extract_group_membership(existing_group_context.extensions())?;
     let new_group_membership = get_latest_group_membership(staged_commit)?;
     let membership_diff = old_group_membership.diff(&new_group_membership);
 
     validate_membership_diff(
-        &old_group_membership,
+        old_group_membership,
         &new_group_membership,
         &membership_diff,
     )?;
@@ -486,6 +493,7 @@ async fn extract_expected_diff<'diff, ApiClient: XmtpApi>(
             &old_group_membership,
             &new_group_membership,
             &membership_diff,
+            sender_inbox_id,
         )
         .await?;