Merge pull request #180 from bobbyjnichols/fix/atomic-smb-permissions

atomic: skip redundant chmod for matching replacement modes

crates/atomic/README.md

@@ -97,8 +97,11 @@ sensitive payloads; changing permissions inside a callback is too late to guaran
 other process opened the stage. Private stages request `0600`; actual enforcement depends on
 the filesystem's permission support. Neither policy adds Windows ACL guarantees.
 
-Replacements still stage privately and restore the existing Unix mode after writing. A new
-file keeps any mode selected by its callback. In the normal exclusive-copy fallback, a changed
+Replacements still stage privately and restore the existing Unix mode after writing. When the
+stage already has that mode, no `chmod` is attempted. This lets replacements work on fixed-mode
+GVfs/SMB mounts and document portals that reject permission changes even when the reported mode
+already matches. The comparison happens after the callback, so its mode changes are still
+restored. A new file keeps any mode selected by its callback. In the normal exclusive-copy fallback, a changed
 mode is applied before copying bytes; explicit private copies stay private during copying and
 apply callback-selected modes afterward. A required mode change that fails is reported, not
 silently ignored. Existing-file replacements and deliberate callback changes can therefore

crates/atomic/src/lib.rs

@@ -31,7 +31,8 @@
 //! ### Permissions
 //! New documents and exports use normal OS creation permissions, including while staging.
 //! Use [`WriteOptions`] with [`NewFilePermissions::Private`] for settings or sensitive payloads.
-//! Existing destinations always stage privately and retain their original Unix mode.
+//! Existing destinations always stage privately and retain their original Unix mode. Restoring
+//! an already-matching mode needs no chmod, including on fixed-mode network filesystems.
 //!
 //! ### Platform Characteristics
 //! - **Windows**: Antivirus scanners, search indexers, and backup utilities briefly hold files
@@ -342,8 +343,9 @@ fn split(target: &Path, path: &Path) -> io::Result<(PathBuf, String)> {
 }
 
 /// Create a temporary file in `dir`, fill it with `fill`, give it the permissions of the file it
-/// replaces (`replaced`, Unix) and flush it. A leftover file with the same name is skipped. When
-/// any step fails, the temporary file is removed.
+/// replaces (`replaced`, Unix) and flush it. Permission restoration is skipped when the mode
+/// already matches after `fill`. A leftover file with the same name is skipped. When any step
+/// fails, the temporary file is removed.
 fn stage(
     fs: &dyn AtomicFs,
     dir: &Path,
@@ -368,7 +370,24 @@ fn stage(
         };
         let written = fill(&mut file)
             .map_err(|e| write_error(e, path, dir, existed))
-            .and_then(|()| perms.map_or(Ok(()), |p| fs.set_permissions(&file, p).map_err(|e| permissions_error(e, path))))
+            .and_then(|()| {
+                perms
+                    .map_or(Ok(()), |p| {
+                        #[cfg(unix)]
+                        {
+                            use std::os::unix::fs::PermissionsExt;
+                            // GVfs/SMB and document portals can expose a fixed mode but reject
+                            // chmod, even when the requested mode is already in effect. Check
+                            // after fill: a streaming callback may have changed the stage's mode.
+                            // Compare permission and special bits, not file-type bits.
+                            if (file.metadata()?.permissions().mode() & 0o7777) == (p.mode() & 0o7777) {
+                                return Ok(());
+                            }
+                        }
+                        fs.set_permissions(&file, p)
+                    })
+                    .map_err(|e| permissions_error(e, path))
+            })
             .and_then(|()| fs.sync(&file).map_err(|e| write_error(e, path, dir, existed)));
         drop(file);
         if written.is_err() {
@@ -645,6 +664,10 @@ mod tests {
         renames: Cell<u32>,
         no_hard_links: bool,
         fail_permissions: bool,
+        permission_error: Option<io::ErrorKind>,
+        /// Simulate a network filesystem exposing the same fixed mode on every new file.
+        #[cfg(unix)]
+        fixed_mode: Option<u32>,
         fail_sync_dir: bool,
         fail_normal_create: bool,
         normal_collisions: Cell<u32>,
@@ -653,6 +676,8 @@ mod tests {
     }
 
     impl AtomicFs for Faulty {
+        /// Create a real stage with injected failures or a simulated fixed network mode,
+        /// recording the creation policy and the Unix mode exposed by the filesystem.
         fn create_new(&self, path: &Path, permissions: NewFilePermissions) -> io::Result<File> {
             let normal = permissions == NewFilePermissions::PlatformDefault;
             self.events.borrow_mut().push(if normal { "create_new_normal" } else { "create_new" });
@@ -667,6 +692,9 @@ mod tests {
             #[cfg(unix)]
             {
                 use std::os::unix::fs::PermissionsExt;
+                if let Some(mode) = self.fixed_mode {
+                    file.set_permissions(std::fs::Permissions::from_mode(mode))?;
+                }
                 self.created_modes.borrow_mut().push(file.metadata()?.permissions().mode() & 0o777);
             }
             Ok(file)
@@ -712,10 +740,15 @@ mod tests {
             }
             RealFs.sync_dir(dir)
         }
+        /// Record permission restoration and payload length, then inject a configured error
+        /// or delegate the mode change to the real filesystem.
         fn set_permissions(&self, file: &File, perms: std::fs::Permissions) -> io::Result<()> {
             self.events.borrow_mut().push("set_permissions");
             #[cfg(unix)]
             self.permission_lengths.borrow_mut().push(file.metadata()?.len());
+            if let Some(kind) = self.permission_error {
+                return Err(io::Error::new(kind, "injected permission failure"));
+            }
             if self.fail_permissions {
                 return Err(io::Error::new(io::ErrorKind::PermissionDenied, "injected: operation not permitted"));
             }
@@ -917,18 +950,87 @@ mod tests {
         Ok(())
     }
 
+    /// Matching fixed modes permit byte and streaming replacements under both creation
+    /// policies without calling a permission operation that the mount rejects.
     #[cfg(unix)]
     #[test]
-    fn failing_to_keep_permissions_fails_the_save() {
+    fn unchanged_replacement_modes_need_no_chmod() -> io::Result<()> {
+        use std::os::unix::fs::PermissionsExt;
+        let d = TempDir::new("fixed-replacement-mode");
+        // GVfs SMB reports 0700 regardless of the requested creation mode; also cover a
+        // private stage whose normal 0600 mode already matches the original.
+        for mode in [0o600, 0o700] {
+            for policy in [NewFilePermissions::Private, NewFilePermissions::PlatformDefault] {
+                for streaming in [false, true] {
+                    let p = d.0.join(format!("{mode:o}-{policy:?}-{streaming}"));
+                    std::fs::write(&p, ORIGINAL)?;
+                    std::fs::set_permissions(&p, std::fs::Permissions::from_mode(mode))?;
+                    let fs = Faulty { fixed_mode: Some(mode), permission_error: Some(io::ErrorKind::Unsupported), ..Default::default() };
+                    let options = WriteOptions::default().with_new_file_permissions(policy);
+                    if streaming {
+                        options.atomic_write_with_custom(&fs, NO_RETRY, &p, |f| f.write_all(b"replacement"))?;
+                    } else {
+                        options.atomic_write_custom(&fs, NO_RETRY, &p, b"replacement")?;
+                    }
+                    assert_eq!(std::fs::read(&p)?, b"replacement");
+                    assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, mode);
+                    assert!(!fs.events.borrow().contains(&"set_permissions"));
+                    assert_eq!(fs.renames.get(), 1);
+                    assert_no_temp(&d.0);
+                }
+            }
+        }
+        Ok(())
+    }
+
+    /// A required mode change that fails must preserve the original bytes and mode,
+    /// report the error, and remove the stage without publishing it.
+    #[cfg(unix)]
+    #[test]
+    fn failing_to_keep_permissions_fails_the_save() -> io::Result<()> {
+        use std::os::unix::fs::PermissionsExt;
         let d = TempDir::new("perms");
-        let p = d.0.join("doc.craft");
-        std::fs::write(&p, ORIGINAL).ok();
-        let fs = Faulty { fail_permissions: true, ..Default::default() };
-        let msg = atomic_write_custom(&fs, NO_RETRY, &p, b"new").err().map(|e| e.to_string()).unwrap_or_default();
-        assert!(msg.contains("permissions"), "{msg}");
-        assert_eq!(std::fs::read(&p).ok(), Some(ORIGINAL.to_vec()));
-        assert_eq!(fs.renames.get(), 0);
-        assert_no_temp(&d.0);
+        for kind in [io::ErrorKind::Unsupported, io::ErrorKind::PermissionDenied, io::ErrorKind::Other] {
+            let p = d.0.join(format!("{kind:?}.craft"));
+            std::fs::write(&p, ORIGINAL)?;
+            std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o640))?;
+            let fs = Faulty { permission_error: Some(kind), ..Default::default() };
+            let error =
+                atomic_write_custom(&fs, NO_RETRY, &p, b"new").err().ok_or_else(|| io::Error::other("a required mode change must succeed"))?;
+            assert_eq!(error.kind(), kind);
+            assert!(error.to_string().contains("permissions"), "{error}");
+            assert_eq!(std::fs::read(&p)?, ORIGINAL);
+            assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, 0o640);
+            assert_eq!(fs.renames.get(), 0);
+            assert_no_temp(&d.0);
+        }
+        Ok(())
+    }
+
+    /// A streaming callback that changes permissions or setuid/setgid/sticky bits must not
+    /// bypass restoration of the original mode, even when the modes matched at creation.
+    #[cfg(unix)]
+    #[test]
+    fn replacement_checks_mode_after_streaming_callback() -> io::Result<()> {
+        use std::os::unix::fs::PermissionsExt;
+        let d = TempDir::new("callback-mode-change");
+        for callback_mode in [0o640, 0o1600, 0o2600, 0o4600] {
+            let p = d.0.join(format!("doc-{callback_mode:o}.craft"));
+            std::fs::write(&p, ORIGINAL)?;
+            std::fs::set_permissions(&p, std::fs::Permissions::from_mode(0o600))?;
+            let fs = Faulty::default();
+            WriteOptions::default().atomic_write_with_custom(&fs, NO_RETRY, &p, |f| {
+                f.write_all(b"replacement")?;
+                f.set_permissions(std::fs::Permissions::from_mode(callback_mode))?;
+                assert_eq!(f.metadata()?.permissions().mode() & 0o7777, callback_mode);
+                Ok(())
+            })?;
+            assert_eq!(std::fs::read(&p)?, b"replacement");
+            assert_eq!(std::fs::metadata(&p)?.permissions().mode() & 0o7777, 0o600);
+            assert!(fs.events.borrow().contains(&"set_permissions"));
+            assert_no_temp(&d.0);
+        }
+        Ok(())
     }
 
     #[cfg(unix)]