From 6f780211dda0706aa474bcc9be4cec99149acef3 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 23:49:39 -0500 Subject: [PATCH] fix(collab): build no repair for a batch refused for lack of permission (#1980) --- crates/collab/src/server.rs | 10 ++++++++-- crates/collab/src/server_tests.rs | 20 ++++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/crates/collab/src/server.rs b/crates/collab/src/server.rs index d5af681b2..0f11792b7 100644 --- a/crates/collab/src/server.rs +++ b/crates/collab/src/server.rs @@ -413,10 +413,14 @@ impl Server { // client learned what happened to it from the catch-up. ClientMsg::Changes { batch_id, .. } if self.issued.get(&from).is_some_and(|p| batch_id <= p.last_batch) => Vec::new(), ClientMsg::Changes { base_seq, batch_id, changes } => { + let mut denied = true; let result = match self.limited.get(&from) { Some(Access::View) => Err(Error::Invalid(VIEW_ONLY.into())), Some(Access::Comment) if !changes.iter().all(Change::is_comment) => Err(Error::Invalid(COMMENT_ONLY.into())), - _ => self.accept(from, base_seq, &changes), + _ => { + denied = false; + self.accept(from, base_seq, &changes) + } }; if let Some(p) = self.issued.get_mut(&from) { p.note(batch_id, result.is_err()); @@ -430,7 +434,9 @@ impl Server { } Err(e) => { self.note(Journal::Refused { client: from, batch_id }); - let fixes = self.fixes(&changes).unwrap_or_default(); + // A refusal for lack of permission costs no repair: the client's confirmed copy + // already is the server's state, so the fixes would only repeat it. + let fixes = if denied { Vec::new() } else { self.fixes(&changes).unwrap_or_default() }; vec![Outgoing::To(from, ServerMsg::Reject { batch_id, reason: reason(&e), fixes })] } } diff --git a/crates/collab/src/server_tests.rs b/crates/collab/src/server_tests.rs index da12f7f35..aafff946d 100644 --- a/crates/collab/src/server_tests.rs +++ b/crates/collab/src/server_tests.rs @@ -182,6 +182,26 @@ fn deleted_nodes_can_come_back_but_ids_are_never_handed_out_twice() { assert_eq!(s.document(), gone.document()); } +#[test] +fn a_viewers_refused_deletion_gets_no_repair() { + let (d, _, frame) = doc(); + let mut s = Server::new(d, "tok", Limits::default()); + let (id, out) = s.join_as(hello("tok"), crate::message::Access::View).unwrap(); + let Some(Outgoing::To(_, welcome)) = out.into_iter().next() else { panic!() }; + let mut v = Client::join(welcome).unwrap(); + let del = edit(&mut v, |d| d.remove(frame).unwrap()); + let (reason, fixes) = rejected( + &mut s, + id, + match del { + ClientMsg::Changes { changes, .. } => changes, + _ => panic!(), + }, + ); + assert_eq!(reason, crate::VIEW_ONLY); + assert!(fixes.is_empty(), "{fixes:?}"); +} + #[test] fn fixes_roll_a_rejected_client_back_to_the_server_state() { let (d, _, frame) = doc();