From e5aeea7a949ca7e18a4594a0f74a67ccc1a0c54b Mon Sep 17 00:00:00 2001 From: Hamza Date: Sat, 17 Oct 2026 16:59:27 +0500 Subject: [PATCH] Sheet protection: obey the Sort and Use AutoFilter boxes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SheetProtection` carries eleven permissions. `format_cells` is checked in `apply_style`, and an edit to a locked cell is caught after the fact by `check_protection`, but `sort` and `autofilter` had no reader anywhere outside `sheet_read.rs`, `sheet_write.rs` and a round-trip test: they only ever travelled through the file. So a protected sheet could still be sorted, and Ctrl+Shift+L still put an AutoFilter on it. `refuse_unless_allowed` sits beside `locked_cell_error`, with three call sites: `do_sort` (which both Sort… and the A-to-Z buttons come through), `toggle_filter` and `filter_by`. Where the box decides is worth knowing. Sorting a range of locked cells was already refused, by the locked-cell guard rather than the permission, because a sort moves values between locked cells — which is Excel's rule too ("users cannot sort ranges that contain locked cells on a protected worksheet, regardless of this setting"). So the Sort box decides for a range that has been unlocked, while an AutoFilter changes no cell's contents and the box is the only thing that ever decided it. The test asserts both, and that an unprotected sheet still sorts and filters. Not fixed here: Delete Rows and Delete Columns are refused even when their boxes are ticked, because deleting removes locked cells and trips the same post-hoc guard, while Insert Rows and Insert Columns work. Letting the guard know which operation ran is a change to code every command passes through and wants its own PR. --- crates/engine/src/cmd/data.rs | 5 +++ crates/engine/src/cmd/mod.rs | 12 +++++ crates/engine/src/tests.rs | 85 +++++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/crates/engine/src/cmd/data.rs b/crates/engine/src/cmd/data.rs index 24bd043f7..e003cfffe 100644 --- a/crates/engine/src/cmd/data.rs +++ b/crates/engine/src/cmd/data.rs @@ -232,6 +232,9 @@ fn sort(s: &mut Session, p: &Json) -> Result { } fn do_sort(s: &mut Session, r: RangeRef, header: bool, keys: Vec, by_cols: bool, case: bool) -> Result { + // Protect Sheet ▸ Sort. Both Sort… and the A-to-Z buttons come through here. + let sheet = s.doc()?.wb.active_sheet; + super::refuse_unless_allowed(s, sheet, "Sorting", |p| p.sort)?; let body = if by_cols { if header && r.width() > 1 { RangeRef::new(CellRef::new(r.start.row, r.start.col + 1), r.end) } else { r } } else if header && r.height() > 1 { @@ -438,6 +441,7 @@ fn sorted_anchor(sh: &Sheet, a: Anchor, bottom: (u32, u32, f32, f32), by_cols: b fn toggle_filter(s: &mut Session, p: &Json) -> Result { let sheet = s.doc()?.wb.active_sheet; + super::refuse_unless_allowed(s, sheet, "Filtering", |p| p.autofilter)?; let has = s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.autofilter.is_some()); let in_table = s.doc()?.wb.sheet(sheet).and_then(|sh| sh.table_at(s.doc().map(|d| d.selection.active).unwrap_or_default()).map(|t| t.name.clone())); @@ -492,6 +496,7 @@ fn install_filter(s: &mut Session, p: &Json) -> Result { fn filter_by(s: &mut Session, p: &Json) -> Result { let sheet = s.doc()?.wb.active_sheet; + super::refuse_unless_allowed(s, sheet, "Filtering", |p| p.autofilter)?; let Some(sh) = s.doc()?.wb.sheet(sheet) else { return Err(EngineError::NoDocument) }; let af = match &sh.autofilter { Some(a) => a.clone(), diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 50865a2b7..9b44e6741 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -394,6 +394,18 @@ fn check_protection(old: &Workbook, new: &Workbook, unlocked: &[(String, String) Ok(()) } +/// Refuses an action a protected sheet does not allow, by the tick box for it in Review ▸ Protect +/// Sheet. `what` names the action in the message. An unprotected sheet allows everything. +/// +/// Cell edits are guarded separately, by whether the cell is locked (`locked_cell_error`): these +/// permissions are the ones that are not about one cell's contents. +pub(crate) fn refuse_unless_allowed(s: &Session, sheet: usize, what: &str, pick: fn(&gridcraft_model::SheetProtection) -> bool) -> Result<()> { + if s.doc()?.wb.sheet(sheet).is_some_and(|sh| sh.protection.as_ref().is_some_and(|pr| !pick(pr))) { + return Err(EngineError::Other(format!("{what} isn't allowed on this protected sheet (Review › Unprotect Sheet)."))); + } + Ok(()) +} + /// Whether a locked cell on a protected sheet stays locked: `None` when it's in an Allow Edit /// Range without a password or one unlocked in this session, else the message to show. pub(crate) fn locked_cell_error(sh: &gridcraft_model::Sheet, c: CellRef, unlocked: &[(String, String)]) -> Option { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 2ac290654..0e648fd22 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3344,3 +3344,88 @@ fn csv_as_text_keeps_identifier_columns_as_written() { let saved = s.execute("file.saveBytes", json!({"format": "csv"})).unwrap(); assert_eq!(crate::io::base64_decode(saved["base64"].as_str().unwrap()).unwrap(), csv.replace('\n', "\r\n").as_bytes()); } + +/// Protect Sheet's Sort and Use AutoFilter tick boxes are obeyed. They were read from the file and +/// written back but never consulted, so a protected sheet could still be sorted and filtered. +/// +/// Sorting locked cells stays refused whichever way the box is set, which is Excel's rule too +/// ("users cannot sort ranges that contain locked cells on a protected worksheet"), so the box +/// decides for a range that has been unlocked. An AutoFilter changes no cell's contents, so there +/// the box is the only thing that decides. +#[test] +fn a_protected_sheet_allows_sorting_and_filtering_only_when_it_says_so() { + let allow = |s: &mut Session, f: fn(&mut gridcraft_model::SheetProtection)| { + let d = s.doc_mut().unwrap(); + let mut wb = (*d.wb).clone(); + if let Some(p) = wb.sheet_mut(0).and_then(|sh| sh.protection.as_mut()) { + f(p); + } + d.wb = std::sync::Arc::new(wb); + }; + // Three rows under a header, and the cells unlocked so that the tick box is what decides. + let book = || { + let mut s = s(); + for (c, v) in [("A1", "Head"), ("A2", "2"), ("A3", "1")] { + s.execute("cell.set", json!({"cell": c, "input": v})).unwrap(); + } + s.execute("home.lockCell", json!({"range": "A1:A3", "on": false})).unwrap(); + s + }; + let order = |s: &Session| (v(s, "A2"), v(s, "A3")); + + // Unprotected: both work, so the guard has not made the ordinary case stricter. + let mut s = book(); + s.execute("data.sortAscending", json!({})).unwrap(); + assert_eq!(order(&s), (Value::Number(1.0), Value::Number(2.0))); + s.execute("data.filter", json!({})).unwrap(); + + // Protected with nothing ticked, which is what Excel's dialog opens with: both refused. + let mut s = book(); + s.execute("review.protectSheet", json!({})).unwrap(); + for (cmd, args) in [ + ("data.sortAscending", json!({})), + ("data.sortDescending", json!({})), + // Sort… needs a key to be a sort at all: with none it does nothing either way. + ("data.sort", json!({"keys": [{"column": "A", "order": "asc"}]})), + ("data.filter", json!({})), + ("data.filterBy", json!({})), + ] { + let e = s.execute(cmd, args).expect_err(cmd).to_string(); + assert!(e.contains("protected sheet"), "{cmd}: {e}"); + } + // Refused, not half-applied: the rows are where they were. + assert_eq!(order(&s), (Value::Number(2.0), Value::Number(1.0))); + assert!(s.doc().unwrap().wb.sheet(0).unwrap().autofilter.is_none()); + + // Sort ticked: sorting works on the unlocked range; filtering is still refused. + let mut s = book(); + s.execute("review.protectSheet", json!({})).unwrap(); + allow(&mut s, |p| p.sort = true); + s.execute("data.sortAscending", json!({})).unwrap(); + assert_eq!(order(&s), (Value::Number(1.0), Value::Number(2.0)), "sorted under the header"); + assert!(s.execute("data.filter", json!({})).is_err(), "Use AutoFilter is still off"); + + // Use AutoFilter ticked: the filter goes on, and sorting is refused. + let mut s = book(); + s.execute("review.protectSheet", json!({})).unwrap(); + allow(&mut s, |p| p.autofilter = true); + s.execute("data.filter", json!({})).unwrap(); + assert!(s.doc().unwrap().wb.sheet(0).unwrap().autofilter.is_some(), "the AutoFilter is on"); + assert!(s.execute("data.sortAscending", json!({})).is_err(), "Sort is still off"); + // Unprotecting gives sorting back. + s.execute("review.unprotectSheet", json!({})).unwrap(); + s.execute("data.sortAscending", json!({})).unwrap(); + + // Locked cells: ticking Sort does not let them be reordered, as in Excel. + let mut s = { + let mut t = super::tests::s(); + for (c, v) in [("A1", "Head"), ("A2", "2"), ("A3", "1")] { + t.execute("cell.set", json!({"cell": c, "input": v})).unwrap(); + } + t + }; + s.execute("review.protectSheet", json!({})).unwrap(); + allow(&mut s, |p| p.sort = true); + assert!(s.execute("data.sortAscending", json!({})).is_err(), "locked cells cannot be sorted"); + assert_eq!(order(&s), (Value::Number(2.0), Value::Number(1.0))); +}