From c6d33d4bb98e5a60774e113fa869193df59fefd2 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Fri, 26 Jan 2024 09:40:41 -0700 Subject: [PATCH] Only allow Manage Members on root channels --- 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(-) diff --git a/crates/channel/src/channel_store.rs b/crates/channel/src/channel_store.rs index 1883a7e06d..20a0955678 100644 --- a/crates/channel/src/channel_store.rs +++ b/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) diff --git a/crates/collab/src/db/queries/channels.rs b/crates/collab/src/db/queries/channels.rs index b1df4e7cbe..7276d6cc17 100644 --- a/crates/collab/src/db/queries/channels.rs +++ b/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())?; } } diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index aee9e19f61..71f0510a69 100644 --- a/crates/collab/src/rpc.rs +++ b/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(); } } diff --git a/crates/collab/src/tests/channel_tests.rs b/crates/collab/src/tests/channel_tests.rs index 56511df207..950d096df2 100644 --- a/crates/collab/src/tests/channel_tests.rs +++ b/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] diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index f16c634bc6..e82ab60bc9 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/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.show_channel_modal(channel_id, channel_modal::Mode::InviteMembers, cx); - } - fn manage_members(&mut self, channel_id: ChannelId, cx: &mut ViewContext) { 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.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.channel_clipboard = Some(ChannelMoveClipboard { channel_id }); } diff --git a/crates/rpc/proto/zed.proto b/crates/rpc/proto/zed.proto index 5278bbba66..a6a34a9e5e 100644 --- a/crates/rpc/proto/zed.proto +++ b/crates/rpc/proto/zed.proto @@ -213,6 +213,7 @@ enum ErrorCode { WrongReleaseChannel = 6; NeedsCla = 7; NotARootChannel = 8; + BadPublicNesting = 9; } message Test {