chmod: Implement --dereference, -L, -H, etc

Should fix tests/chmod/symlinks.sh

src/uu/chmod/src/chmod.rs

@@ -16,6 +16,7 @@ use uucore::fs::display_permissions_unix;
 use uucore::libc::mode_t;
 #[cfg(not(windows))]
 use uucore::mode;
+use uucore::perms::TraverseSymlinks;
 use uucore::{format_usage, help_about, help_section, help_usage, show, show_error};
 
 const ABOUT: &str = help_about!("chmod.md");
@@ -33,6 +34,11 @@ mod options {
     pub const RECURSIVE: &str = "recursive";
     pub const MODE: &str = "MODE";
     pub const FILE: &str = "FILE";
+    // TODO remove duplication with perms.rs
+    pub mod dereference {
+        pub const DEREFERENCE: &str = "dereference";
+        pub const NO_DEREFERENCE: &str = "no-dereference";
+    }
 }
 
 /// Extract negative modes (starting with '-') from the rest of the arguments.
@@ -99,7 +105,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let quiet = matches.get_flag(options::QUIET);
     let verbose = matches.get_flag(options::VERBOSE);
     let preserve_root = matches.get_flag(options::PRESERVE_ROOT);
-    let recursive = matches.get_flag(options::RECURSIVE);
     let fmode = match matches.get_one::<String>(options::REFERENCE) {
         Some(fref) => match fs::metadata(fref) {
             Ok(meta) => Some(meta.mode() & 0o7777),
@@ -138,6 +143,34 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         return Err(UUsageError::new(1, "missing operand".to_string()));
     }
 
+    let mut dereference = if matches.get_flag(options::dereference::DEREFERENCE) {
+        Some(true) // Follow symlinks
+    } else if matches.get_flag(options::dereference::NO_DEREFERENCE) {
+        Some(false) // Do not follow symlinks
+    } else {
+        None // Default behavior
+    };
+
+    let mut traverse_symlinks = if matches.get_flag("L") {
+        TraverseSymlinks::All
+    } else if matches.get_flag("H") {
+        TraverseSymlinks::First
+    } else {
+        TraverseSymlinks::None
+    };
+
+    let recursive = matches.get_flag(options::RECURSIVE);
+    if recursive {
+        if traverse_symlinks == TraverseSymlinks::None {
+            if dereference == Some(true) {
+                return Err(USimpleError::new(1, "-R --dereference requires -H or -L"));
+            }
+            dereference = Some(false);
+        }
+    } else {
+        traverse_symlinks = TraverseSymlinks::None;
+    }
+
     let chmoder = Chmoder {
         changes,
         quiet,
@@ -146,6 +179,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         recursive,
         fmode,
         cmode,
+        traverse_symlinks,
+        dereference: dereference.unwrap_or(true),
     };
 
     chmoder.chmod(&files)
@@ -237,6 +272,8 @@ struct Chmoder {
     recursive: bool,
     fmode: Option<u32>,
     cmode: Option<String>,
+    traverse_symlinks: TraverseSymlinks,
+    dereference: bool,
 }
 
 impl Chmoder {
@@ -248,12 +285,19 @@ impl Chmoder {
             let file = Path::new(filename);
             if !file.exists() {
                 if file.is_symlink() {
+                    if !self.dereference && !self.recursive {
+                        // The file is a symlink and we should not follow it
+                        // Don't try to change the mode of the symlink itself
+                        continue;
+                    }
                     if !self.quiet {
                         show!(USimpleError::new(
                             1,
                             format!("cannot operate on dangling symlink {}", filename.quote()),
                         ));
+                        set_exit_code(1);
                     }
+
                     if self.verbose {
                         println!(
                             "failed to change mode of {} from 0000 (---------) to 1500 (r-x-----T)",
@@ -273,6 +317,11 @@ impl Chmoder {
                 // So we set the exit code, because it hasn't been set yet if `self.quiet` is true.
                 set_exit_code(1);
                 continue;
+            } else if !self.dereference && file.is_symlink() {
+                // The file is a symlink and we should not follow it
+                // chmod 755 --no-dereference a/link
+                // should not change the permissions in this case
+                continue;
             }
             if self.recursive && self.preserve_root && filename == "/" {
                 return Err(USimpleError::new(
@@ -294,11 +343,23 @@ impl Chmoder {
 
     fn walk_dir(&self, file_path: &Path) -> UResult<()> {
         let mut r = self.chmod_file(file_path);
-        if !file_path.is_symlink() && file_path.is_dir() {
+        // Determine whether to traverse symlinks based on `self.traverse_symlinks`
+        let should_follow_symlink = match self.traverse_symlinks {
+            TraverseSymlinks::All => true,
+            TraverseSymlinks::First => {
+                file_path == file_path.canonicalize().unwrap_or(file_path.to_path_buf())
+            }
+            TraverseSymlinks::None => false,
+        };
+
+        // If the path is a directory (or we should follow symlinks), recurse into it
+        if (!file_path.is_symlink() || should_follow_symlink) && file_path.is_dir() {
             for dir_entry in file_path.read_dir()? {
                 let path = dir_entry?.path();
                 if !path.is_symlink() {
                     r = self.walk_dir(path.as_path());
+                } else if should_follow_symlink {
+                    r = self.chmod_file(path.as_path()).and(r);
                 }
             }
         }
@@ -316,17 +377,25 @@ impl Chmoder {
     fn chmod_file(&self, file: &Path) -> UResult<()> {
         use uucore::mode::get_umask;
 
-        let fperm = match fs::metadata(file) {
+        // Determine metadata based on dereference flag
+        let metadata = if self.dereference {
+            file.metadata() // Follow symlinks
+        } else {
+            file.symlink_metadata() // Act on the symlink itself
+        };
+
+        let fperm = match metadata {
             Ok(meta) => meta.mode() & 0o7777,
             Err(err) => {
-                if file.is_symlink() {
+                // Handle dangling symlinks or other errors
+                if file.is_symlink() && !self.dereference {
                     if self.verbose {
                         println!(
                             "neither symbolic link {} nor referent has been changed",
                             file.quote()
                         );
                     }
-                    return Ok(());
+                    return Ok(()); // Skip dangling symlinks
                 } else if err.kind() == std::io::ErrorKind::PermissionDenied {
                     // These two filenames would normally be conditionally
                     // quoted, but GNU's tests expect them to always be quoted
@@ -339,6 +408,8 @@ impl Chmoder {
                 }
             }
         };
+
+        // Determine the new permissions to apply
         match self.fmode {
             Some(mode) => self.change_file(fperm, mode, file)?,
             None => {

tests/by-util/test_chmod.rs

@@ -231,20 +231,50 @@ fn test_chmod_ugoa() {
     run_tests(tests);
 }
 
+#[test]
+#[cfg(any(target_os = "linux", target_os = "macos"))]
+// TODO fix android, it has 0777
+// We should force for the umask on startup
+fn test_chmod_umask_expected() {
+    let current_umask = uucore::mode::get_umask();
+    assert_eq!(
+        current_umask,
+        0o022,
+        "Unexpected umask value: expected 022 (octal), but got {:03o}. Please adjust the test environment.",
+        current_umask
+    );
+}
+
+fn get_expected_symlink_permissions() -> u32 {
+    #[cfg(any(target_os = "linux", target_os = "android"))]
+    {
+        0o120_777
+    }
+    #[cfg(not(any(target_os = "linux", target_os = "android")))]
+    {
+        0o120_755
+    }
+}
+
 #[test]
 fn test_chmod_error_permissions() {
     // check that we print an error if umask prevents us from removing a permission
     let (at, mut ucmd) = at_and_ucmd!();
+
     at.touch("file");
     set_permissions(at.plus("file"), Permissions::from_mode(0o777)).unwrap();
+
     ucmd.args(&["-w", "file"])
+        .umask(0o022)
         .fails()
         .code_is(1)
+        .stderr_is(
             // spell-checker:disable-next-line
-        .stderr_is("chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x\n");
+            "chmod: file: new permissions are r-xrwxrwx, not r-xr-xr-x\n",
+        );
     assert_eq!(
         metadata(at.plus("file")).unwrap().permissions().mode(),
-        0o100577
+        0o100_577
     );
 }
 
@@ -645,7 +675,10 @@ fn test_chmod_file_symlink_after_non_existing_file() {
         .stderr_contains(expected_stderr);
     assert_eq!(
         at.metadata(test_existing_symlink).permissions().mode(),
-        0o100_764
+        0o100_764,
+        "Expected mode: {:o}, but got: {:o}",
+        0o100_764,
+        at.metadata(test_existing_symlink).permissions().mode()
     );
 }
 
@@ -749,3 +782,252 @@ fn test_gnu_special_options() {
     scene.ucmd().arg("--").arg("--").arg("file").succeeds();
     scene.ucmd().arg("--").arg("--").fails();
 }
+
+#[test]
+fn test_chmod_dereference_symlink() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+
+    at.touch(target);
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+    at.symlink_file(target, symlink);
+
+    // Use --dereference: should modify the target file's permissions
+    scene
+        .ucmd()
+        .arg("--dereference")
+        .arg("u+x")
+        .arg(symlink)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(at.metadata(target).permissions().mode(), 0o100_764);
+    assert_eq!(
+        at.symlink_metadata(symlink).permissions().mode(),
+        get_expected_symlink_permissions()
+    );
+}
+
+#[test]
+fn test_chmod_no_dereference_symlink() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+
+    at.touch(target);
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+    at.symlink_file(target, symlink);
+
+    // Use --no-dereference: should modify the symlink itself
+    scene
+        .ucmd()
+        .arg("--no-dereference")
+        .arg("u+x")
+        .arg(symlink)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(at.metadata(target).permissions().mode(), 0o100_664);
+    assert_eq!(
+        at.symlink_metadata(symlink).permissions().mode(),
+        get_expected_symlink_permissions()
+    );
+}
+
+#[test]
+fn test_chmod_traverse_symlink_h() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+    let directory = "dir";
+
+    at.mkdir(directory);
+    at.touch(target);
+    at.symlink_file(target, &format!("{directory}/{symlink}"));
+
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+
+    // Use -H: should only follow symlinks specified as arguments
+    scene
+        .ucmd()
+        .arg("-R")
+        .arg("-H")
+        .arg("u+x")
+        .arg(directory)
+        .succeeds()
+        .no_stderr();
+
+    let target_permissions = at.metadata(target).permissions().mode();
+    assert_eq!(
+        target_permissions, 0o100_664,
+        "Expected target permissions to be 0o100_664, but got {:o}",
+        target_permissions
+    );
+
+    let symlink_path = format!("{directory}/{symlink}");
+    let symlink_permissions = at.symlink_metadata(&symlink_path).permissions().mode();
+    assert_eq!(
+        symlink_permissions,
+        get_expected_symlink_permissions(),
+        "Expected symlink permissions to be 0o120_777, but got {:o} for symlink at {}",
+        symlink_permissions,
+        symlink_path
+    );
+}
+
+#[test]
+fn test_chmod_traverse_symlink_l() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+    let directory = "dir";
+
+    at.mkdir(directory);
+    at.touch(target);
+    at.symlink_file(target, &format!("{directory}/{symlink}"));
+
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+
+    // Use -L: should follow all symlinks
+    scene
+        .ucmd()
+        .arg("-R")
+        .arg("-L")
+        .arg("u+x")
+        .umask(0o022)
+        .arg(directory)
+        .succeeds()
+        .no_stderr();
+
+    let target_permissions = at.metadata(target).permissions().mode();
+    assert_eq!(
+        target_permissions, 0o100_764,
+        "Expected target permissions to be 0o100_764, but got {:o}",
+        target_permissions
+    );
+
+    let symlink_path = format!("{directory}/{symlink}");
+    let symlink_permissions = at.symlink_metadata(&symlink_path).permissions().mode();
+    assert_eq!(
+        symlink_permissions,
+        get_expected_symlink_permissions(),
+        "Expected symlink permissions to be 0o120_777, but got {:o} for symlink at {}",
+        symlink_permissions,
+        symlink_path
+    );
+}
+
+#[test]
+fn test_chmod_traverse_symlink_p() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let target = "file";
+    let symlink = "symlink";
+    let directory = "dir";
+
+    at.mkdir(directory);
+    at.touch(target);
+    at.symlink_file(target, &format!("{directory}/{symlink}"));
+
+    set_permissions(at.plus(target), Permissions::from_mode(0o664)).unwrap();
+
+    // Use -P: should not follow any symlinks
+    scene
+        .ucmd()
+        .arg("-R")
+        .arg("-P")
+        .arg("u+x")
+        .umask(0o022)
+        .arg(directory)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(at.metadata(target).permissions().mode(), 0o100_664);
+    assert_eq!(
+        at.symlink_metadata(&format!("{directory}/{symlink}"))
+            .permissions()
+            .mode(),
+        get_expected_symlink_permissions()
+    );
+}
+
+#[test]
+fn test_chmod_symlink_to_dangling_target_dereference() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let dangling_target = "nonexistent_file";
+    let symlink = "symlink";
+
+    at.symlink_file(dangling_target, symlink);
+
+    // Use --dereference: should fail due to dangling symlink
+    scene
+        .ucmd()
+        .arg("--dereference")
+        .arg("u+x")
+        .arg(symlink)
+        .fails()
+        .stderr_contains(format!("cannot operate on dangling symlink '{}'", symlink));
+}
+
+#[test]
+fn test_chmod_symlink_target_no_dereference() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let file = "a";
+    let symlink = "symlink";
+    at.touch(file);
+    at.symlink_file(file, symlink);
+    set_permissions(at.plus(file), Permissions::from_mode(0o644)).unwrap();
+
+    scene
+        .ucmd()
+        .arg("--no-dereference")
+        .arg("755")
+        .arg(symlink)
+        .succeeds()
+        .no_stderr();
+    assert_eq!(
+        at.symlink_metadata(file).permissions().mode(),
+        0o100_644,
+        "Expected symlink permissions: {:o}, but got: {:o}",
+        0o100_644,
+        at.symlink_metadata(file).permissions().mode()
+    );
+}
+
+#[test]
+fn test_chmod_symlink_to_dangling_recursive() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+
+    let dangling_target = "nonexistent_file";
+    let symlink = "symlink";
+
+    at.symlink_file(dangling_target, symlink);
+
+    // Use --no-dereference: should succeed and modify the symlink itself
+    scene
+        .ucmd()
+        .arg("755")
+        .arg("-R")
+        .arg(symlink)
+        .fails()
+        .stderr_is("chmod: cannot operate on dangling symlink 'symlink'\n");
+    assert_eq!(
+        at.symlink_metadata(symlink).permissions().mode(),
+        get_expected_symlink_permissions(),
+        "Expected symlink permissions: {:o}, but got: {:o}",
+        get_expected_symlink_permissions(),
+        at.symlink_metadata(symlink).permissions().mode()
+    );
+}