From a3bc48261e49d7cb483c5371fade67cc568ebc9d Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Mon, 8 Jan 2024 12:23:40 -0700 Subject: [PATCH 1/3] First pass of real access control Co-Authored-By: Max --- crates/collab/src/db/ids.rs | 8 ++ crates/collab/src/db/queries/projects.rs | 38 ++++++++ crates/collab/src/rpc.rs | 86 ++++++++++++------- .../collab/src/tests/channel_guest_tests.rs | 10 ++- 4 files changed, 112 insertions(+), 30 deletions(-) diff --git a/crates/collab/src/db/ids.rs b/crates/collab/src/db/ids.rs index 9bb766147f5b0b2084b76665262962179a62e6eb..2e2218c4d9da181719cf6404245a4d84d4285dab 100644 --- a/crates/collab/src/db/ids.rs +++ b/crates/collab/src/db/ids.rs @@ -140,6 +140,14 @@ impl ChannelRole { Guest | Banned => false, } } + + pub fn can_edit_projects(&self) -> bool { + use ChannelRole::*; + match self { + Admin | Member => true, + Guest | Banned => false, + } + } } impl From for ChannelRole { diff --git a/crates/collab/src/db/queries/projects.rs b/crates/collab/src/db/queries/projects.rs index 5b8d54f8d36a746b2c09a77cbf9e75b77b557543..ca59c851e738865c6ffc50c970490fa3e2a93d1f 100644 --- a/crates/collab/src/db/queries/projects.rs +++ b/crates/collab/src/db/queries/projects.rs @@ -777,6 +777,44 @@ impl Database { .await } + pub async fn host_for_mutating_project_request( + &self, + project_id: ProjectId, + connection_id: ConnectionId, + ) -> Result { + let room_id = self.room_id_for_project(project_id).await?; + self.room_transaction(room_id, |tx| async move { + let current_participant = room_participant::Entity::find() + .filter(room_participant::Column::RoomId.eq(room_id)) + .filter(room_participant::Column::AnsweringConnectionId.eq(connection_id.id)) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("no such room"))?; + + if !current_participant + .role + .unwrap_or(ChannelRole::Guest) + .can_edit_projects() + { + Err(anyhow!("not authorized to edit projects"))?; + } + + let host = project_collaborator::Entity::find() + .filter( + project_collaborator::Column::ProjectId + .eq(project_id) + .and(project_collaborator::Column::IsHost.eq(true)), + ) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("failed to read project host"))?; + + Ok(host.connection()) + }) + .await + .map(|guard| guard.into_inner()) + } + pub async fn project_collaborators( &self, project_id: ProjectId, diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 835b48809da94dc60cd872d473e564a7456da81e..68774c22e6ec67782c76c6878dad974dfd7db984 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -217,39 +217,43 @@ impl Server { .add_message_handler(update_diagnostic_summary) .add_message_handler(update_worktree_settings) .add_message_handler(refresh_inlay_hints) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) - .add_request_handler(forward_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_read_only_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler( + forward_mutating_project_request::, + ) + .add_request_handler( + forward_mutating_project_request::, + ) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_mutating_project_request::) .add_message_handler(create_buffer_for_peer) .add_request_handler(update_buffer) .add_message_handler(update_buffer_file) .add_message_handler(buffer_reloaded) .add_message_handler(buffer_saved) - .add_request_handler(forward_project_request::) .add_request_handler(get_users) .add_request_handler(fuzzy_search_users) .add_request_handler(request_contact) @@ -1741,7 +1745,7 @@ async fn update_language_server( Ok(()) } -async fn forward_project_request( +async fn forward_read_only_project_request( request: T, response: Response, session: Session, @@ -1772,6 +1776,30 @@ where Ok(()) } +async fn forward_mutating_project_request( + request: T, + response: Response, + session: Session, +) -> Result<()> +where + T: EntityMessage + RequestMessage, +{ + let project_id = ProjectId::from_proto(request.remote_entity_id()); + let host_connection_id = session + .db() + .await + .host_for_mutating_project_request(project_id, session.connection_id) + .await?; + + let payload = session + .peer + .forward_request(session.connection_id, host_connection_id, request) + .await?; + + response.send(payload)?; + Ok(()) +} + async fn create_buffer_for_peer( request: proto::CreateBufferForPeer, session: Session, diff --git a/crates/collab/src/tests/channel_guest_tests.rs b/crates/collab/src/tests/channel_guest_tests.rs index e2051c44a038ced0724b616ac4e3b4cd851b4508..32cc074ec96a11e3806a256eedd8f74191689ace 100644 --- a/crates/collab/src/tests/channel_guest_tests.rs +++ b/crates/collab/src/tests/channel_guest_tests.rs @@ -82,5 +82,13 @@ async fn test_channel_guests( project_b.read_with(cx_b, |project, _| project.remote_id()), Some(project_id), ); - assert!(project_b.read_with(cx_b, |project, _| project.is_read_only())) + assert!(project_b.read_with(cx_b, |project, _| project.is_read_only())); + + assert!(project_b + .update(cx_b, |project, cx| { + let worktree_id = project.worktrees().next().unwrap().read(cx).id(); + project.create_entry((worktree_id, "b.txt"), false, cx) + }) + .await + .is_err()) } From 18b31f1552db6f7a25ae8011fe5ffd467095cd79 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 8 Jan 2024 12:04:59 -0800 Subject: [PATCH 2/3] Check user is host for host-broadcasted project messages --- crates/collab/src/db/queries/projects.rs | 28 ++++++++ crates/collab/src/rpc.rs | 92 +++++------------------- crates/rpc/src/macros.rs | 4 +- crates/rpc/src/proto.rs | 5 +- 4 files changed, 51 insertions(+), 78 deletions(-) diff --git a/crates/collab/src/db/queries/projects.rs b/crates/collab/src/db/queries/projects.rs index ca59c851e738865c6ffc50c970490fa3e2a93d1f..04c77c80771b29a6be3d1603898b681da3e79569 100644 --- a/crates/collab/src/db/queries/projects.rs +++ b/crates/collab/src/db/queries/projects.rs @@ -777,6 +777,34 @@ impl Database { .await } + pub async fn check_user_is_project_host( + &self, + project_id: ProjectId, + connection_id: ConnectionId, + ) -> Result<()> { + let room_id = self.room_id_for_project(project_id).await?; + self.room_transaction(room_id, |tx| async move { + project_collaborator::Entity::find() + .filter( + Condition::all() + .add(project_collaborator::Column::ProjectId.eq(project_id)) + .add(project_collaborator::Column::IsHost.eq(true)) + .add(project_collaborator::Column::ConnectionId.eq(connection_id.id)) + .add( + project_collaborator::Column::ConnectionServerId + .eq(connection_id.owner_id), + ), + ) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("failed to read project host"))?; + + Ok(()) + }) + .await + .map(|guard| guard.into_inner()) + } + pub async fn host_for_mutating_project_request( &self, project_id: ProjectId, diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 68774c22e6ec67782c76c6878dad974dfd7db984..572670d78f4dca73cff7e57b8d0f0b030d9bdd11 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -42,7 +42,7 @@ use prometheus::{register_int_gauge, IntGauge}; use rpc::{ proto::{ self, Ack, AnyTypedEnvelope, EntityMessage, EnvelopedMessage, LiveKitConnectionInfo, - RequestMessage, UpdateChannelBufferCollaborators, + RequestMessage, ShareProject, UpdateChannelBufferCollaborators, }, Connection, ConnectionId, Peer, Receipt, TypedEnvelope, }; @@ -216,7 +216,6 @@ impl Server { .add_message_handler(update_language_server) .add_message_handler(update_diagnostic_summary) .add_message_handler(update_worktree_settings) - .add_message_handler(refresh_inlay_hints) .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_read_only_project_request::) @@ -251,9 +250,11 @@ impl Server { .add_request_handler(forward_mutating_project_request::) .add_message_handler(create_buffer_for_peer) .add_request_handler(update_buffer) - .add_message_handler(update_buffer_file) - .add_message_handler(buffer_reloaded) - .add_message_handler(buffer_saved) + .add_message_handler(broadcast_project_message_from_host::) + .add_message_handler(broadcast_project_message_from_host::) + .add_message_handler(broadcast_project_message_from_host::) + .add_message_handler(broadcast_project_message_from_host::) + .add_message_handler(broadcast_project_message_from_host::) .add_request_handler(get_users) .add_request_handler(fuzzy_search_users) .add_request_handler(request_contact) @@ -285,7 +286,6 @@ impl Server { .add_request_handler(follow) .add_message_handler(unfollow) .add_message_handler(update_followers) - .add_message_handler(update_diff_base) .add_request_handler(get_private_user_info) .add_message_handler(acknowledge_channel_message) .add_message_handler(acknowledge_buffer_version); @@ -1697,10 +1697,6 @@ async fn update_worktree_settings( Ok(()) } -async fn refresh_inlay_hints(request: proto::RefreshInlayHints, session: Session) -> Result<()> { - broadcast_project_message(request.project_id, request, session).await -} - async fn start_language_server( request: proto::StartLanguageServer, session: Session, @@ -1804,6 +1800,14 @@ async fn create_buffer_for_peer( request: proto::CreateBufferForPeer, session: Session, ) -> Result<()> { + session + .db() + .await + .check_user_is_project_host( + ProjectId::from_proto(request.project_id), + session.connection_id, + ) + .await?; let peer_id = request.peer_id.ok_or_else(|| anyhow!("invalid peer id"))?; session .peer @@ -1856,60 +1860,17 @@ async fn update_buffer( Ok(()) } -async fn update_buffer_file(request: proto::UpdateBufferFile, session: Session) -> Result<()> { - let project_id = ProjectId::from_proto(request.project_id); - let project_connection_ids = session - .db() - .await - .project_connection_ids(project_id, session.connection_id) - .await?; - - broadcast( - Some(session.connection_id), - project_connection_ids.iter().copied(), - |connection_id| { - session - .peer - .forward_send(session.connection_id, connection_id, request.clone()) - }, - ); - Ok(()) -} - -async fn buffer_reloaded(request: proto::BufferReloaded, session: Session) -> Result<()> { - let project_id = ProjectId::from_proto(request.project_id); - let project_connection_ids = session - .db() - .await - .project_connection_ids(project_id, session.connection_id) - .await?; - broadcast( - Some(session.connection_id), - project_connection_ids.iter().copied(), - |connection_id| { - session - .peer - .forward_send(session.connection_id, connection_id, request.clone()) - }, - ); - Ok(()) -} - -async fn buffer_saved(request: proto::BufferSaved, session: Session) -> Result<()> { - broadcast_project_message(request.project_id, request, session).await -} - -async fn broadcast_project_message( - project_id: u64, +async fn broadcast_project_message_from_host>( request: T, session: Session, ) -> Result<()> { - let project_id = ProjectId::from_proto(project_id); + let project_id = ProjectId::from_proto(request.remote_entity_id()); let project_connection_ids = session .db() .await .project_connection_ids(project_id, session.connection_id) .await?; + broadcast( Some(session.connection_id), project_connection_ids.iter().copied(), @@ -3138,25 +3099,6 @@ async fn mark_notification_as_read( Ok(()) } -async fn update_diff_base(request: proto::UpdateDiffBase, session: Session) -> Result<()> { - let project_id = ProjectId::from_proto(request.project_id); - let project_connection_ids = session - .db() - .await - .project_connection_ids(project_id, session.connection_id) - .await?; - broadcast( - Some(session.connection_id), - project_connection_ids.iter().copied(), - |connection_id| { - session - .peer - .forward_send(session.connection_id, connection_id, request.clone()) - }, - ); - Ok(()) -} - async fn get_private_user_info( _request: proto::GetPrivateUserInfo, response: Response, diff --git a/crates/rpc/src/macros.rs b/crates/rpc/src/macros.rs index 89e605540da1157f5530ad7236b23358dc127c1a..85e2b0cf879608aec655d8eb663a73baf65fe7f0 100644 --- a/crates/rpc/src/macros.rs +++ b/crates/rpc/src/macros.rs @@ -60,8 +60,10 @@ macro_rules! request_messages { #[macro_export] macro_rules! entity_messages { - ($id_field:ident, $($name:ident),* $(,)?) => { + ({$id_field:ident, $entity_type:ty}, $($name:ident),* $(,)?) => { $(impl EntityMessage for $name { + type Entity = $entity_type; + fn remote_entity_id(&self) -> u64 { self.$id_field } diff --git a/crates/rpc/src/proto.rs b/crates/rpc/src/proto.rs index 336c252630ea51ea9d11a0658fc59d0cb09b214f..25b8b00dae99fe3e4c11677e480c6c48fa9f08c8 100644 --- a/crates/rpc/src/proto.rs +++ b/crates/rpc/src/proto.rs @@ -31,6 +31,7 @@ pub trait EnvelopedMessage: Clone + Debug + Serialize + Sized + Send + Sync + 's } pub trait EntityMessage: EnvelopedMessage { + type Entity; fn remote_entity_id(&self) -> u64; } @@ -369,7 +370,7 @@ request_messages!( ); entity_messages!( - project_id, + {project_id, ShareProject}, AddProjectCollaborator, ApplyCodeAction, ApplyCompletionAdditionalEdits, @@ -422,7 +423,7 @@ entity_messages!( ); entity_messages!( - channel_id, + {channel_id, Channel}, ChannelMessageSent, RemoveChannelMessage, UpdateChannelBuffer, From d7c5d29237b1a010f09559b291fc45c3bde450d2 Mon Sep 17 00:00:00 2001 From: Conrad Irwin Date: Mon, 8 Jan 2024 15:39:24 -0700 Subject: [PATCH 3/3] Only allow read-write users to update buffers --- crates/collab/src/db/ids.rs | 8 ++++ crates/collab/src/db/queries/projects.rs | 56 ++++++++++++++++++++++-- crates/collab/src/rpc.rs | 26 ++++------- 3 files changed, 69 insertions(+), 21 deletions(-) diff --git a/crates/collab/src/db/ids.rs b/crates/collab/src/db/ids.rs index 2e2218c4d9da181719cf6404245a4d84d4285dab..9f77225fb704c1d4d53051cb7ee29d77c3536f80 100644 --- a/crates/collab/src/db/ids.rs +++ b/crates/collab/src/db/ids.rs @@ -148,6 +148,14 @@ impl ChannelRole { Guest | Banned => false, } } + + pub fn can_read_projects(&self) -> bool { + use ChannelRole::*; + match self { + Admin | Member | Guest => true, + Banned => false, + } + } } impl From for ChannelRole { diff --git a/crates/collab/src/db/queries/projects.rs b/crates/collab/src/db/queries/projects.rs index 04c77c80771b29a6be3d1603898b681da3e79569..6e1bf16309bd69315903a37ce7b57d389e749161 100644 --- a/crates/collab/src/db/queries/projects.rs +++ b/crates/collab/src/db/queries/projects.rs @@ -805,6 +805,43 @@ impl Database { .map(|guard| guard.into_inner()) } + pub async fn host_for_read_only_project_request( + &self, + project_id: ProjectId, + connection_id: ConnectionId, + ) -> Result { + let room_id = self.room_id_for_project(project_id).await?; + self.room_transaction(room_id, |tx| async move { + let current_participant = room_participant::Entity::find() + .filter(room_participant::Column::RoomId.eq(room_id)) + .filter(room_participant::Column::AnsweringConnectionId.eq(connection_id.id)) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("no such room"))?; + + if !current_participant + .role + .map_or(false, |role| role.can_read_projects()) + { + Err(anyhow!("not authorized to read projects"))?; + } + + let host = project_collaborator::Entity::find() + .filter( + project_collaborator::Column::ProjectId + .eq(project_id) + .and(project_collaborator::Column::IsHost.eq(true)), + ) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("failed to read project host"))?; + + Ok(host.connection()) + }) + .await + .map(|guard| guard.into_inner()) + } + pub async fn host_for_mutating_project_request( &self, project_id: ProjectId, @@ -821,8 +858,7 @@ impl Database { if !current_participant .role - .unwrap_or(ChannelRole::Guest) - .can_edit_projects() + .map_or(false, |role| role.can_edit_projects()) { Err(anyhow!("not authorized to edit projects"))?; } @@ -843,13 +879,27 @@ impl Database { .map(|guard| guard.into_inner()) } - pub async fn project_collaborators( + pub async fn project_collaborators_for_buffer_update( &self, project_id: ProjectId, connection_id: ConnectionId, ) -> Result>> { let room_id = self.room_id_for_project(project_id).await?; self.room_transaction(room_id, |tx| async move { + let current_participant = room_participant::Entity::find() + .filter(room_participant::Column::RoomId.eq(room_id)) + .filter(room_participant::Column::AnsweringConnectionId.eq(connection_id.id)) + .one(&*tx) + .await? + .ok_or_else(|| anyhow!("no such room"))?; + + if !current_participant + .role + .map_or(false, |role| role.can_edit_projects()) + { + Err(anyhow!("not authorized to edit projects"))?; + } + let collaborators = project_collaborator::Entity::find() .filter(project_collaborator::Column::ProjectId.eq(project_id)) .all(&*tx) diff --git a/crates/collab/src/rpc.rs b/crates/collab/src/rpc.rs index 572670d78f4dca73cff7e57b8d0f0b030d9bdd11..da0d281f102602a3f7ec38a0d97b9e801a9e35ff 100644 --- a/crates/collab/src/rpc.rs +++ b/crates/collab/src/rpc.rs @@ -227,7 +227,7 @@ impl Server { .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_read_only_project_request::) - .add_request_handler(forward_mutating_project_request::) + .add_request_handler(forward_read_only_project_request::) .add_request_handler(forward_mutating_project_request::) .add_request_handler( forward_mutating_project_request::, @@ -1750,24 +1750,15 @@ where T: EntityMessage + RequestMessage, { let project_id = ProjectId::from_proto(request.remote_entity_id()); - let host_connection_id = { - let collaborators = session - .db() - .await - .project_collaborators(project_id, session.connection_id) - .await?; - collaborators - .iter() - .find(|collaborator| collaborator.is_host) - .ok_or_else(|| anyhow!("host not found"))? - .connection_id - }; - + let host_connection_id = session + .db() + .await + .host_for_read_only_project_request(project_id, session.connection_id) + .await?; let payload = session .peer .forward_request(session.connection_id, host_connection_id, request) .await?; - response.send(payload)?; Ok(()) } @@ -1786,12 +1777,10 @@ where .await .host_for_mutating_project_request(project_id, session.connection_id) .await?; - let payload = session .peer .forward_request(session.connection_id, host_connection_id, request) .await?; - response.send(payload)?; Ok(()) } @@ -1823,11 +1812,12 @@ async fn update_buffer( let project_id = ProjectId::from_proto(request.project_id); let mut guest_connection_ids; let mut host_connection_id = None; + { let collaborators = session .db() .await - .project_collaborators(project_id, session.connection_id) + .project_collaborators_for_buffer_update(project_id, session.connection_id) .await?; guest_connection_ids = Vec::with_capacity(collaborators.len() - 1); for collaborator in collaborators.iter() {