fix(membership): always set reason & allow new events if reason changed

src/api/client_server/membership.rs

@@ -186,15 +186,7 @@ pub async fn kick_user_route(
 ) -> Result<kick_user::v3::Response> {
     let sender_user = body.sender_user.as_ref().expect("user is authenticated");
 
-    if let Ok(true) = services()
+    let event: RoomMemberEventContent = serde_json::from_str(
-        .rooms
-        .state_cache
-        .is_left(sender_user, &body.room_id)
-    {
-        return Ok(kick_user::v3::Response {});
-    }
-
-    let mut event: RoomMemberEventContent = serde_json::from_str(
         services()
             .rooms
             .state_accessor
@@ -205,15 +197,26 @@ pub async fn kick_user_route(
             )?
             .ok_or(Error::BadRequest(
                 ErrorKind::BadState,
-                "Cannot kick member that's not in the room.",
+                "Cannot kick a user who is not in the room.",
             ))?
             .content
             .get(),
     )
     .map_err(|_| Error::bad_database("Invalid member event in database."))?;
 
-    event.membership = MembershipState::Leave;
+    // If they are already kicked and the reason is unchanged, there isn't any point in sending a new event.
-    event.reason.clone_from(&body.reason);
+    if event.membership == MembershipState::Leave && event.reason == body.reason {
+        return Ok(kick_user::v3::Response {});
+    }
+
+    let event = RoomMemberEventContent {
+        is_direct: None,
+        membership: MembershipState::Leave,
+        third_party_invite: None,
+        reason: body.reason.clone(),
+        join_authorized_via_users_server: None,
+        ..event
+    };
 
     let mutex_state = Arc::clone(
         services()
@@ -254,17 +257,7 @@ pub async fn kick_user_route(
 pub async fn ban_user_route(body: Ruma<ban_user::v3::Request>) -> Result<ban_user::v3::Response> {
     let sender_user = body.sender_user.as_ref().expect("user is authenticated");
 
-    if let Ok(Some(membership_event)) = services()
+    let event = if let Some(event) = services()
-        .rooms
-        .state_accessor
-        .get_member(&body.room_id, sender_user)
-    {
-        if membership_event.membership == MembershipState::Ban {
-            return Ok(ban_user::v3::Response {});
-        }
-    }
-
-    let event = services()
         .rooms
         .state_accessor
         .room_state_get(
@@ -272,27 +265,30 @@ pub async fn ban_user_route(body: Ruma<ban_user::v3::Request>) -> Result<ban_use
             &StateEventType::RoomMember,
             body.user_id.as_ref(),
         )?
-        .map_or(
+        // Even when the previous member content is invalid, we should let the ban go through anyways.
-            Ok(RoomMemberEventContent {
+        .and_then(|event| serde_json::from_str::<RoomMemberEventContent>(event.content.get()).ok())
+    {
+        // If they are already banned and the reason is unchanged, there isn't any point in sending a new event.
+        if event.membership == MembershipState::Ban && event.reason == body.reason {
+            return Ok(ban_user::v3::Response {});
+        }
+
+        RoomMemberEventContent {
             membership: MembershipState::Ban,
-                displayname: services().users.displayname(&body.user_id)?,
+            join_authorized_via_users_server: None,
-                avatar_url: services().users.avatar_url(&body.user_id)?,
-                is_direct: None,
-                third_party_invite: None,
-                blurhash: services().users.blurhash(&body.user_id)?,
             reason: body.reason.clone(),
-                join_authorized_via_users_server: None,
+            third_party_invite: None,
-            }),
+            is_direct: None,
-            |event| {
+            avatar_url: event.avatar_url,
-                serde_json::from_str(event.content.get())
+            displayname: event.displayname,
-                    .map(|event: RoomMemberEventContent| RoomMemberEventContent {
+            blurhash: event.blurhash,
-                        membership: MembershipState::Ban,
+        }
-                        join_authorized_via_users_server: None,
+    } else {
-                        ..event
+        RoomMemberEventContent {
-                    })
+            reason: body.reason.clone(),
-                    .map_err(|_| Error::bad_database("Invalid member event in database."))
+            ..RoomMemberEventContent::new(MembershipState::Ban)
-            },
+        }
-        )?;
+    };
 
     let mutex_state = Arc::clone(
         services()
@@ -335,17 +331,7 @@ pub async fn unban_user_route(
 ) -> Result<unban_user::v3::Response> {
     let sender_user = body.sender_user.as_ref().expect("user is authenticated");
 
-    if let Ok(Some(membership_event)) = services()
+    let event: RoomMemberEventContent = serde_json::from_str(
-        .rooms
-        .state_accessor
-        .get_member(&body.room_id, sender_user)
-    {
-        if membership_event.membership != MembershipState::Ban {
-            return Ok(unban_user::v3::Response {});
-        }
-    }
-
-    let mut event: RoomMemberEventContent = serde_json::from_str(
         services()
             .rooms
             .state_accessor
@@ -363,8 +349,19 @@ pub async fn unban_user_route(
     )
     .map_err(|_| Error::bad_database("Invalid member event in database."))?;
 
-    event.membership = MembershipState::Leave;
+    // If they are already unbanned and the reason is unchanged, there isn't any point in sending a new event.
-    event.reason.clone_from(&body.reason);
+    if event.membership == MembershipState::Leave && event.reason == body.reason {
+        return Ok(unban_user::v3::Response {});
+    }
+
+    let event = RoomMemberEventContent {
+        is_direct: None,
+        membership: MembershipState::Leave,
+        third_party_invite: None,
+        reason: body.reason.clone(),
+        join_authorized_via_users_server: None,
+        ..event
+    };
 
     let mutex_state = Arc::clone(
         services()
@@ -1319,10 +1316,7 @@ pub(crate) async fn invite_helper<'a>(
             .filter(|server| &**server != services().globals.server_name());
 
         services().sending.send_pdu(servers, &pdu_id)?;
-
+    } else {
-        return Ok(());
-    }
-
         if !services()
             .rooms
             .state_cache
@@ -1372,7 +1366,9 @@ pub(crate) async fn invite_helper<'a>(
             )
             .await?;
 
+        // Critical point ends
         drop(state_lock);
+    }
 
     Ok(())
 }
@@ -1470,12 +1466,15 @@ pub async fn leave_room(user_id: &UserId, room_id: &RoomId, reason: Option<Strin
             Some(e) => e,
         };
 
-        let mut event: RoomMemberEventContent = serde_json::from_str(member_event.content.get())
+        let event = RoomMemberEventContent {
-            .map_err(|_| Error::bad_database("Invalid member event in database."))?;
+            is_direct: None,
-
+            membership: MembershipState::Leave,
-        event.membership = MembershipState::Leave;
+            third_party_invite: None,
-        event.reason = reason;
+            reason,
-        event.join_authorized_via_users_server = None;
+            join_authorized_via_users_server: None,
+            ..serde_json::from_str(member_event.content.get())
+                .map_err(|_| Error::bad_database("Invalid member event in database."))?
+        };
 
         services()
             .rooms