Implement channel member removal, permission check for member retrieval

crates/client/src/channel_store.rs

@@ -121,6 +121,33 @@ impl ChannelStore {
         })
     }
 
+    pub fn remove_member(
+        &mut self,
+        channel_id: ChannelId,
+        user_id: u64,
+        cx: &mut ModelContext<Self>,
+    ) -> Task<Result<()>> {
+        if !self.outgoing_invites.insert((channel_id, user_id)) {
+            return Task::ready(Err(anyhow!("invite request already in progress")));
+        }
+
+        cx.notify();
+        let client = self.client.clone();
+        cx.spawn(|this, mut cx| async move {
+            client
+                .request(proto::RemoveChannelMember {
+                    channel_id,
+                    user_id,
+                })
+                .await?;
+            this.update(&mut cx, |this, cx| {
+                this.outgoing_invites.remove(&(channel_id, user_id));
+                cx.notify();
+            });
+            Ok(())
+        })
+    }
+
     pub fn respond_to_channel_invite(
         &mut self,
         channel_id: ChannelId,
@@ -181,16 +208,6 @@ impl ChannelStore {
         self.outgoing_invites.contains(&(channel_id, user_id))
     }
 
-    pub fn remove_member(
-        &self,
-        _channel_id: ChannelId,
-        _user_id: u64,
-        _cx: &mut ModelContext<Self>,
-    ) -> Task<Result<()>> {
-        dbg!("TODO");
-        Task::Ready(Some(Ok(())))
-    }
-
     async fn handle_update_channels(
         this: ModelHandle<Self>,
         message: TypedEnvelope<proto::UpdateChannels>,

crates/collab/src/db.rs

@@ -3165,30 +3165,17 @@ impl Database {
         creator_id: UserId,
     ) -> Result<ChannelId> {
         self.transaction(move |tx| async move {
-            let tx = tx;
-
             if let Some(parent) = parent {
-                let channels = self.get_channel_ancestors(parent, &*tx).await?;
+                self.check_user_is_channel_admin(parent, creator_id, &*tx)
-                channel_member::Entity::find()
+                    .await?;
-                    .filter(channel_member::Column::ChannelId.is_in(channels.iter().copied()))
-                    .filter(
-                        channel_member::Column::UserId
-                            .eq(creator_id)
-                            .and(channel_member::Column::Accepted.eq(true)),
-                    )
-                    .one(&*tx)
-                    .await?
-                    .ok_or_else(|| {
-                        anyhow!("User does not have the permissions to create this channel")
-                    })?;
             }
 
             let channel = channel::ActiveModel {
                 name: ActiveValue::Set(name.to_string()),
                 ..Default::default()
-            };
+            }
-
+            .insert(&*tx)
-            let channel = channel.insert(&*tx).await?;
+            .await?;
 
             if let Some(parent) = parent {
                 channel_parent::ActiveModel {
@@ -3228,45 +3215,36 @@ impl Database {
         user_id: UserId,
     ) -> Result<(Vec<ChannelId>, Vec<UserId>)> {
         self.transaction(move |tx| async move {
-            let tx = tx;
+            self.check_user_is_channel_admin(channel_id, user_id, &*tx)
-
-            // Check if user is an admin
-            channel_member::Entity::find()
-                .filter(
-                    channel_member::Column::ChannelId
-                        .eq(channel_id)
-                        .and(channel_member::Column::UserId.eq(user_id))
-                        .and(channel_member::Column::Admin.eq(true)),
-                )
-                .one(&*tx)
-                .await?
-                .ok_or_else(|| anyhow!("user is not allowed to remove this channel"))?;
-
-            let mut descendants = self.get_channel_descendants([channel_id], &*tx).await?;
-
-            // Keep channels which have another active
-            let mut channels_to_keep = channel_parent::Entity::find()
-                .filter(
-                    channel_parent::Column::ChildId
-                        .is_in(descendants.keys().copied().filter(|&id| id != channel_id))
-                        .and(
-                            channel_parent::Column::ParentId.is_not_in(descendants.keys().copied()),
-                        ),
-                )
-                .stream(&*tx)
                 .await?;
 
-            while let Some(row) = channels_to_keep.next().await {
+            // Don't remove descendant channels that have additional parents.
-                let row = row?;
+            let mut channels_to_remove = self.get_channel_descendants([channel_id], &*tx).await?;
-                descendants.remove(&row.child_id);
+            {
+                let mut channels_to_keep = channel_parent::Entity::find()
+                    .filter(
+                        channel_parent::Column::ChildId
+                            .is_in(
+                                channels_to_remove
+                                    .keys()
+                                    .copied()
+                                    .filter(|&id| id != channel_id),
+                            )
+                            .and(
+                                channel_parent::Column::ParentId
+                                    .is_not_in(channels_to_remove.keys().copied()),
+                            ),
+                    )
+                    .stream(&*tx)
+                    .await?;
+                while let Some(row) = channels_to_keep.next().await {
+                    let row = row?;
+                    channels_to_remove.remove(&row.child_id);
+                }
             }
 
-            drop(channels_to_keep);
-
-            let channels_to_remove = descendants.keys().copied().collect::<Vec<_>>();
-
             let members_to_notify: Vec<UserId> = channel_member::Entity::find()
-                .filter(channel_member::Column::ChannelId.is_in(channels_to_remove.iter().copied()))
+                .filter(channel_member::Column::ChannelId.is_in(channels_to_remove.keys().copied()))
                 .select_only()
                 .column(channel_member::Column::UserId)
                 .distinct()
@@ -3274,13 +3252,12 @@ impl Database {
                 .all(&*tx)
                 .await?;
 
-            // Channel members and parents should delete via cascade
             channel::Entity::delete_many()
-                .filter(channel::Column::Id.is_in(channels_to_remove.iter().copied()))
+                .filter(channel::Column::Id.is_in(channels_to_remove.keys().copied()))
                 .exec(&*tx)
                 .await?;
 
-            Ok((channels_to_remove, members_to_notify))
+            Ok((channels_to_remove.into_keys().collect(), members_to_notify))
         })
         .await
     }
@@ -3293,31 +3270,18 @@ impl Database {
         is_admin: bool,
     ) -> Result<()> {
         self.transaction(move |tx| async move {
-            let tx = tx;
+            self.check_user_is_channel_admin(channel_id, inviter_id, &*tx)
+                .await?;
 
-            // Check if inviter is a member
+            channel_member::ActiveModel {
-            channel_member::Entity::find()
-                .filter(
-                    channel_member::Column::ChannelId
-                        .eq(channel_id)
-                        .and(channel_member::Column::UserId.eq(inviter_id))
-                        .and(channel_member::Column::Admin.eq(true)),
-                )
-                .one(&*tx)
-                .await?
-                .ok_or_else(|| {
-                    anyhow!("Inviter does not have permissions to invite the invitee")
-                })?;
-
-            let channel_membership = channel_member::ActiveModel {
                 channel_id: ActiveValue::Set(channel_id),
                 user_id: ActiveValue::Set(invitee_id),
                 accepted: ActiveValue::Set(false),
                 admin: ActiveValue::Set(is_admin),
                 ..Default::default()
-            };
+            }
-
+            .insert(&*tx)
-            channel_membership.insert(&*tx).await?;
+            .await?;
 
             Ok(())
         })
@@ -3331,8 +3295,6 @@ impl Database {
         accept: bool,
     ) -> Result<()> {
         self.transaction(move |tx| async move {
-            let tx = tx;
-
             let rows_affected = if accept {
                 channel_member::Entity::update_many()
                     .set(channel_member::ActiveModel {
@@ -3368,10 +3330,36 @@ impl Database {
         .await
     }
 
-    pub async fn get_channel_invites(&self, user_id: UserId) -> Result<Vec<Channel>> {
+    pub async fn remove_channel_member(
+        &self,
+        channel_id: ChannelId,
+        member_id: UserId,
+        remover_id: UserId,
+    ) -> Result<()> {
         self.transaction(|tx| async move {
-            let tx = tx;
+            self.check_user_is_channel_admin(channel_id, remover_id, &*tx)
+                .await?;
 
+            let result = channel_member::Entity::delete_many()
+                .filter(
+                    channel_member::Column::ChannelId
+                        .eq(channel_id)
+                        .and(channel_member::Column::UserId.eq(member_id)),
+                )
+                .exec(&*tx)
+                .await?;
+
+            if result.rows_affected == 0 {
+                Err(anyhow!("no such member"))?;
+            }
+
+            Ok(())
+        })
+        .await
+    }
+
+    pub async fn get_channel_invites_for_user(&self, user_id: UserId) -> Result<Vec<Channel>> {
+        self.transaction(|tx| async move {
             let channel_invites = channel_member::Entity::find()
                 .filter(
                     channel_member::Column::UserId
@@ -3406,7 +3394,7 @@ impl Database {
         .await
     }
 
-    pub async fn get_channels(
+    pub async fn get_channels_for_user(
         &self,
         user_id: UserId,
     ) -> Result<(Vec<Channel>, HashMap<ChannelId, Vec<UserId>>)> {
@@ -3430,47 +3418,48 @@ impl Database {
                 .await?;
 
             let mut channels = Vec::with_capacity(parents_by_child_id.len());
-            let mut rows = channel::Entity::find()
+            {
-                .filter(channel::Column::Id.is_in(parents_by_child_id.keys().copied()))
+                let mut rows = channel::Entity::find()
-                .stream(&*tx)
+                    .filter(channel::Column::Id.is_in(parents_by_child_id.keys().copied()))
-                .await?;
+                    .stream(&*tx)
-
+                    .await?;
-            while let Some(row) = rows.next().await {
+                while let Some(row) = rows.next().await {
-                let row = row?;
+                    let row = row?;
-                channels.push(Channel {
+                    channels.push(Channel {
-                    id: row.id,
+                        id: row.id,
-                    name: row.name,
+                        name: row.name,
-                    parent_id: parents_by_child_id.get(&row.id).copied().flatten(),
+                        parent_id: parents_by_child_id.get(&row.id).copied().flatten(),
-                });
+                    });
+                }
             }
 
-            drop(rows);
-
             #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)]
             enum QueryUserIdsAndChannelIds {
                 ChannelId,
                 UserId,
             }
 
-            let mut participants = room_participant::Entity::find()
+            let mut participants_by_channel: HashMap<ChannelId, Vec<UserId>> = HashMap::default();
-                .inner_join(room::Entity)
+            {
-                .filter(room::Column::ChannelId.is_in(channels.iter().map(|c| c.id)))
+                let mut rows = room_participant::Entity::find()
-                .select_only()
+                    .inner_join(room::Entity)
-                .column(room::Column::ChannelId)
+                    .filter(room::Column::ChannelId.is_in(channels.iter().map(|c| c.id)))
-                .column(room_participant::Column::UserId)
+                    .select_only()
-                .into_values::<_, QueryUserIdsAndChannelIds>()
+                    .column(room::Column::ChannelId)
-                .stream(&*tx)
+                    .column(room_participant::Column::UserId)
-                .await?;
+                    .into_values::<_, QueryUserIdsAndChannelIds>()
-
+                    .stream(&*tx)
-            let mut participant_map: HashMap<ChannelId, Vec<UserId>> = HashMap::default();
+                    .await?;
-            while let Some(row) = participants.next().await {
+                while let Some(row) = rows.next().await {
-                let row: (ChannelId, UserId) = row?;
+                    let row: (ChannelId, UserId) = row?;
-                participant_map.entry(row.0).or_default().push(row.1)
+                    participants_by_channel
+                        .entry(row.0)
+                        .or_default()
+                        .push(row.1)
+                }
             }
 
-            drop(participants);
+            Ok((channels, participants_by_channel))
-
-            Ok((channels, participant_map))
         })
         .await
     }
@@ -3480,12 +3469,15 @@ impl Database {
             .await
     }
 
-    // TODO: Add a chekc whether this user is allowed to read this channel
     pub async fn get_channel_member_details(
         &self,
-        id: ChannelId,
+        channel_id: ChannelId,
+        user_id: UserId,
     ) -> Result<Vec<proto::ChannelMember>> {
         self.transaction(|tx| async move {
+            self.check_user_is_channel_admin(channel_id, user_id, &*tx)
+                .await?;
+
             #[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)]
             enum QueryMemberDetails {
                 UserId,
@@ -3494,14 +3486,14 @@ impl Database {
             }
 
             let tx = tx;
-            let ancestor_ids = self.get_channel_ancestors(id, &*tx).await?;
+            let ancestor_ids = self.get_channel_ancestors(channel_id, &*tx).await?;
             let mut stream = channel_member::Entity::find()
                 .distinct()
                 .filter(channel_member::Column::ChannelId.is_in(ancestor_ids.iter().copied()))
                 .select_only()
                 .column(channel_member::Column::UserId)
                 .column_as(
-                    channel_member::Column::ChannelId.eq(id),
+                    channel_member::Column::ChannelId.eq(channel_id),
                     QueryMemberDetails::IsDirectMember,
                 )
                 .column(channel_member::Column::Accepted)
@@ -3552,9 +3544,29 @@ impl Database {
         Ok(user_ids)
     }
 
+    async fn check_user_is_channel_admin(
+        &self,
+        channel_id: ChannelId,
+        user_id: UserId,
+        tx: &DatabaseTransaction,
+    ) -> Result<()> {
+        let channel_ids = self.get_channel_ancestors(channel_id, tx).await?;
+        channel_member::Entity::find()
+            .filter(
+                channel_member::Column::ChannelId
+                    .is_in(channel_ids)
+                    .and(channel_member::Column::UserId.eq(user_id))
+                    .and(channel_member::Column::Admin.eq(true)),
+            )
+            .one(&*tx)
+            .await?
+            .ok_or_else(|| anyhow!("user is not allowed to remove this channel"))?;
+        Ok(())
+    }
+
     async fn get_channel_ancestors(
         &self,
-        id: ChannelId,
+        channel_id: ChannelId,
         tx: &DatabaseTransaction,
     ) -> Result<Vec<ChannelId>> {
         let sql = format!(
@@ -3570,7 +3582,7 @@ impl Database {
             SELECT DISTINCT channel_tree.parent_id
             FROM channel_tree
             "#,
-            id
+            channel_id
         );
 
         #[derive(FromQueryResult, Debug, PartialEq)]

crates/collab/src/db/tests.rs

@@ -951,7 +951,7 @@ test_both_dbs!(test_channels_postgres, test_channels_sqlite, db, {
         .await
         .unwrap();
 
-    let (channels, _) = db.get_channels(a_id).await.unwrap();
+    let (channels, _) = db.get_channels_for_user(a_id).await.unwrap();
 
     assert_eq!(
         channels,
@@ -1144,7 +1144,7 @@ test_both_dbs!(
             .unwrap();
 
         let user_2_invites = db
-            .get_channel_invites(user_2) // -> [channel_1_1, channel_1_2]
+            .get_channel_invites_for_user(user_2) // -> [channel_1_1, channel_1_2]
             .await
             .unwrap()
             .into_iter()
@@ -1154,7 +1154,7 @@ test_both_dbs!(
         assert_eq!(user_2_invites, &[channel_1_1, channel_1_2]);
 
         let user_3_invites = db
-            .get_channel_invites(user_3) // -> [channel_1_1]
+            .get_channel_invites_for_user(user_3) // -> [channel_1_1]
             .await
             .unwrap()
             .into_iter()
@@ -1163,7 +1163,10 @@ test_both_dbs!(
 
         assert_eq!(user_3_invites, &[channel_1_1]);
 
-        let members = db.get_channel_member_details(channel_1_1).await.unwrap();
+        let members = db
+            .get_channel_member_details(channel_1_1, user_1)
+            .await
+            .unwrap();
         assert_eq!(
             members,
             &[
@@ -1191,7 +1194,10 @@ test_both_dbs!(
             .await
             .unwrap();
 
-        let members = db.get_channel_member_details(channel_1_3).await.unwrap();
+        let members = db
+            .get_channel_member_details(channel_1_3, user_1)
+            .await
+            .unwrap();
         assert_eq!(
             members,
             &[

crates/collab/src/rpc.rs

@@ -530,8 +530,8 @@ impl Server {
             let (contacts, invite_code, (channels, channel_participants), channel_invites) = future::try_join4(
                 this.app_state.db.get_contacts(user_id),
                 this.app_state.db.get_invite_code_for_user(user_id),
-                this.app_state.db.get_channels(user_id),
+                this.app_state.db.get_channels_for_user(user_id),
-                this.app_state.db.get_channel_invites(user_id)
+                this.app_state.db.get_channel_invites_for_user(user_id)
             ).await?;
 
             {
@@ -2230,10 +2230,16 @@ async fn invite_channel_member(
 }
 
 async fn remove_channel_member(
-    _request: proto::RemoveChannelMember,
+    request: proto::RemoveChannelMember,
-    _response: Response<proto::RemoveChannelMember>,
+    response: Response<proto::RemoveChannelMember>,
-    _session: Session,
+    session: Session,
 ) -> Result<()> {
+    let db = session.db().await;
+    let channel_id = ChannelId::from_proto(request.channel_id);
+    let member_id = UserId::from_proto(request.user_id);
+    db.remove_channel_member(channel_id, member_id, session.user_id)
+        .await?;
+    response.send(proto::Ack {})?;
     Ok(())
 }
 
@@ -2244,7 +2250,9 @@ async fn get_channel_members(
 ) -> Result<()> {
     let db = session.db().await;
     let channel_id = ChannelId::from_proto(request.channel_id);
-    let members = db.get_channel_member_details(channel_id).await?;
+    let members = db
+        .get_channel_member_details(channel_id, session.user_id)
+        .await?;
     response.send(proto::GetChannelMembersResponse { members })?;
     Ok(())
 }

crates/collab_ui/src/collab_panel/channel_modal.rs

@@ -260,9 +260,12 @@ impl PickerDelegate for ChannelModalDelegate {
     fn confirm(&mut self, _: bool, cx: &mut ViewContext<Picker<Self>>) {
         if let Some((user, _)) = self.matches.get(self.selected_index) {
             match self.mode {
-                Mode::ManageMembers => {
+                Mode::ManageMembers => self
-                    //
+                    .channel_store
-                }
+                    .update(cx, |store, cx| {
+                        store.remove_member(self.channel_id, user.id, cx)
+                    })
+                    .detach(),
                 Mode::InviteMembers => match self.member_status(user.id, cx) {
                     Some(proto::channel_member::Kind::Member) => {}
                     Some(proto::channel_member::Kind::Invitee) => self