Only allow Manage Members on root channels

Conrad Irwin created 2 years ago

Change summary

crates/channel/src/channel_store.rs      | 36 +++++++++-
crates/collab/src/db/queries/channels.rs |  8 +
crates/collab/src/rpc.rs                 | 14 ++++
crates/collab/src/tests/channel_tests.rs | 37 ++++++++++
crates/collab_ui/src/collab_panel.rs     | 85 ++++++++++++++++++-------
crates/rpc/proto/zed.proto               |  1 
6 files changed, 150 insertions(+), 31 deletions(-)

Detailed changes

crates/channel/src/channel_store.rs

@@ -79,6 +79,17 @@ impl Channel {
             + &self.id.to_string()
     }
 
+    pub fn is_root_channel(&self) -> bool {
+        self.parent_path.is_empty()
+    }
+
+    pub fn root_id(&self) -> ChannelId {
+        self.parent_path
+            .first()
+            .map(|id| *id as ChannelId)
+            .unwrap_or(self.id)
+    }
+
     pub fn slug(&self) -> String {
         let slug: String = self
             .name
@@ -473,6 +484,22 @@ impl ChannelStore {
         self.channel_role(channel_id) == proto::ChannelRole::Admin
     }
 
+    pub fn is_root_channel(&self, channel_id: ChannelId) -> bool {
+        self.channel_index
+            .by_id()
+            .get(&channel_id)
+            .map_or(false, |channel| channel.is_root_channel())
+    }
+
+    pub fn is_public_channel(&self, channel_id: ChannelId) -> bool {
+        self.channel_index
+            .by_id()
+            .get(&channel_id)
+            .map_or(false, |channel| {
+                channel.visibility == ChannelVisibility::Public
+            })
+    }
+
     pub fn channel_capability(&self, channel_id: ChannelId) -> Capability {
         match self.channel_role(channel_id) {
             ChannelRole::Admin | ChannelRole::Member => Capability::ReadWrite,
@@ -482,10 +509,11 @@ impl ChannelStore {
 
     pub fn channel_role(&self, channel_id: ChannelId) -> proto::ChannelRole {
         maybe!({
-            let channel = self.channel_for_id(channel_id)?;
-            let root_channel_id = channel.parent_path.first()?;
-            let root_channel_state = self.channel_states.get(&root_channel_id);
-            debug_assert!(root_channel_state.is_some());
+            let mut channel = self.channel_for_id(channel_id)?;
+            if !channel.is_root_channel() {
+                channel = self.channel_for_id(channel.root_id())?;
+            }
+            let root_channel_state = self.channel_states.get(&channel.id);
             root_channel_state?.role
         })
         .unwrap_or(proto::ChannelRole::Guest)

crates/collab/src/db/queries/channels.rs

@@ -190,7 +190,9 @@ impl Database {
                     let parent = self.get_channel_internal(parent_id, &*tx).await?;
 
                     if parent.visibility != ChannelVisibility::Public {
-                        Err(anyhow!("public channels must descend from public channels"))?;
+                        Err(ErrorCode::BadPublicNesting
+                            .with_tag("direction", "parent")
+                            .anyhow())?;
                     }
                 }
             } else if visibility == ChannelVisibility::Members {
@@ -202,7 +204,9 @@ impl Database {
                         channel.id != channel_id && channel.visibility == ChannelVisibility::Public
                     })
                 {
-                    Err(anyhow!("cannot make a parent of a public channel private"))?;
+                    Err(ErrorCode::BadPublicNesting
+                        .with_tag("direction", "children")
+                        .anyhow())?;
                 }
             }
 

crates/collab/src/rpc.rs

@@ -3281,6 +3281,18 @@ fn notify_membership_updated(
     user_id: UserId,
     peer: &Peer,
 ) {
+    let user_channels_update = proto::UpdateUserChannels {
+        channel_memberships: result
+            .new_channels
+            .channel_memberships
+            .iter()
+            .map(|cm| proto::ChannelMembership {
+                channel_id: cm.channel_id.to_proto(),
+                role: cm.role.into(),
+            })
+            .collect(),
+        ..Default::default()
+    };
     let mut update = build_channels_update(result.new_channels, vec![]);
     update.delete_channels = result
         .removed_channels
@@ -3290,6 +3302,8 @@ fn notify_membership_updated(
     update.remove_channel_invitations = vec![result.channel_id.to_proto()];
 
     for connection_id in connection_pool.user_connection_ids(user_id) {
+        peer.send(connection_id, user_channels_update.clone())
+            .trace_err();
         peer.send(connection_id, update.clone()).trace_err();
     }
 }

crates/collab/src/tests/channel_tests.rs

@@ -1100,16 +1100,21 @@ async fn test_channel_membership_notifications(
     let user_b = client_b.user_id().unwrap();
 
     let channels = server
-        .make_channel_tree(&[("zed", None), ("vim", Some("zed"))], (&client_a, cx_a))
+        .make_channel_tree(
+            &[("zed", None), ("vim", Some("zed")), ("opensource", None)],
+            (&client_a, cx_a),
+        )
         .await;
     let zed_channel = channels[0];
     let vim_channel = channels[1];
+    let opensource_channel = channels[2];
 
     try_join_all(client_a.channel_store().update(cx_a, |channel_store, cx| {
         [
             channel_store.set_channel_visibility(zed_channel, proto::ChannelVisibility::Public, cx),
             channel_store.set_channel_visibility(vim_channel, proto::ChannelVisibility::Public, cx),
-            channel_store.invite_member(zed_channel, user_b, proto::ChannelRole::Guest, cx),
+            channel_store.invite_member(zed_channel, user_b, proto::ChannelRole::Admin, cx),
+            channel_store.invite_member(opensource_channel, user_b, proto::ChannelRole::Member, cx),
         ]
     }))
     .await
@@ -1144,6 +1149,34 @@ async fn test_channel_membership_notifications(
             },
         ],
     );
+
+    client_b.channel_store().update(cx_b, |channel_store, _| {
+        channel_store.is_channel_admin(zed_channel)
+    });
+
+    client_b
+        .channel_store()
+        .update(cx_b, |channel_store, cx| {
+            channel_store.respond_to_channel_invite(opensource_channel, true, cx)
+        })
+        .await
+        .unwrap();
+
+    cx_a.run_until_parked();
+
+    client_a
+        .channel_store()
+        .update(cx_a, |channel_store, cx| {
+            channel_store.set_member_role(opensource_channel, user_b, ChannelRole::Admin, cx)
+        })
+        .await
+        .unwrap();
+
+    cx_a.run_until_parked();
+
+    client_b.channel_store().update(cx_b, |channel_store, _| {
+        channel_store.is_channel_admin(opensource_channel)
+    });
 }
 
 #[gpui::test]

crates/collab_ui/src/collab_panel.rs

@@ -23,7 +23,7 @@ use gpui::{
 use menu::{Cancel, Confirm, SecondaryConfirm, SelectNext, SelectPrev};
 use project::{Fs, Project};
 use rpc::{
-    proto::{self, PeerId},
+    proto::{self, ChannelVisibility, PeerId},
     ErrorCode, ErrorExt,
 };
 use serde_derive::{Deserialize, Serialize};
@@ -1134,13 +1134,6 @@ impl CollabPanel {
                         "Rename",
                         Some(Box::new(SecondaryConfirm)),
                         cx.handler_for(&this, move |this, cx| this.rename_channel(channel_id, cx)),
-                    )
-                    .entry(
-                        "Move this channel",
-                        None,
-                        cx.handler_for(&this, move |this, cx| {
-                            this.start_move_channel(channel_id, cx)
-                        }),
                     );
 
                 if let Some(channel_name) = clipboard_channel_name {
@@ -1153,23 +1146,52 @@ impl CollabPanel {
                     );
                 }
 
-                context_menu = context_menu
-                    .separator()
-                    .entry(
-                        "Invite Members",
-                        None,
-                        cx.handler_for(&this, move |this, cx| this.invite_members(channel_id, cx)),
-                    )
-                    .entry(
+                if self.channel_store.read(cx).is_root_channel(channel_id) {
+                    context_menu = context_menu.separator().entry(
                         "Manage Members",
                         None,
                         cx.handler_for(&this, move |this, cx| this.manage_members(channel_id, cx)),
                     )
-                    .entry(
-                        "Delete",
+                } else {
+                    context_menu = context_menu.entry(
+                        "Move this channel",
                         None,
-                        cx.handler_for(&this, move |this, cx| this.remove_channel(channel_id, cx)),
+                        cx.handler_for(&this, move |this, cx| {
+                            this.start_move_channel(channel_id, cx)
+                        }),
                     );
+                    if self.channel_store.read(cx).is_public_channel(channel_id) {
+                        context_menu = context_menu.separator().entry(
+                            "Make Channel Private",
+                            None,
+                            cx.handler_for(&this, move |this, cx| {
+                                this.set_channel_visibility(
+                                    channel_id,
+                                    ChannelVisibility::Members,
+                                    cx,
+                                )
+                            }),
+                        )
+                    } else {
+                        context_menu = context_menu.separator().entry(
+                            "Make Channel Public",
+                            None,
+                            cx.handler_for(&this, move |this, cx| {
+                                this.set_channel_visibility(
+                                    channel_id,
+                                    ChannelVisibility::Public,
+                                    cx,
+                                )
+                            }),
+                        )
+                    }
+                }
+
+                context_menu = context_menu.entry(
+                    "Delete",
+                    None,
+                    cx.handler_for(&this, move |this, cx| this.remove_channel(channel_id, cx)),
+                );
             }
 
             context_menu
@@ -1490,10 +1512,6 @@ impl CollabPanel {
         cx.notify();
     }
 
-    fn invite_members(&mut self, channel_id: ChannelId, cx: &mut ViewContext<Self>) {
-        self.show_channel_modal(channel_id, channel_modal::Mode::InviteMembers, cx);
-    }
-
     fn manage_members(&mut self, channel_id: ChannelId, cx: &mut ViewContext<Self>) {
         self.show_channel_modal(channel_id, channel_modal::Mode::ManageMembers, cx);
     }
@@ -1530,6 +1548,27 @@ impl CollabPanel {
         }
     }
 
+    fn set_channel_visibility(
+        &mut self,
+        channel_id: ChannelId,
+        visibility: ChannelVisibility,
+        cx: &mut ViewContext<Self>,
+    ) {
+        self.channel_store
+            .update(cx, |channel_store, cx| {
+                channel_store.set_channel_visibility(channel_id, visibility, cx)
+            })
+            .detach_and_prompt_err("Failed to set channel visibility", cx, |e, _| match e.error_code() {
+                ErrorCode::BadPublicNesting =>
+                    if e.error_tag("direction") == Some("parent") {
+                        Some("To make a channel public, its parent channel must be public.".to_string())
+                    } else {
+                        Some("To make a channel private, all of its subchannels must be private.".to_string())
+                    },
+                _ => None
+            });
+    }
+
     fn start_move_channel(&mut self, channel_id: ChannelId, _cx: &mut ViewContext<Self>) {
         self.channel_clipboard = Some(ChannelMoveClipboard { channel_id });
     }

crates/rpc/proto/zed.proto

@@ -213,6 +213,7 @@ enum ErrorCode {
     WrongReleaseChannel = 6;
     NeedsCla = 7;
     NotARootChannel = 8;
+    BadPublicNesting = 9;
 }
 
 message Test {