usermod: move home directory files to the new primary group with -g

src/uu/usermod/src/usermod.rs

@@ -163,8 +163,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         return Err(UsermodError::UserNotFound(format!("user '{login}' does not exist")).into());
     };
 
-    // Save the old UID and home dir before mutation so we can chown if needed.
+    // Save the old UID, GID and home dir before mutation so we can chown if needed.
     let old_uid = entries[idx].uid;
+    let old_primary_gid = entries[idx].gid;
     let home_for_chown = entries[idx].home.clone();
     let home_is_changing = matches.get_one::<String>(options::HOME).is_some();
 
@@ -224,6 +225,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     }
 
     let new_uid = entries[idx].uid;
+    let new_primary_gid = entries[idx].gid;
 
     passwd_file
         .commit()
@@ -232,14 +234,25 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     // Restore signals before potentially long-running recursive chown.
     drop(signals);
 
-    // If the UID changed and the home directory was not explicitly moved,
-    // recursively chown the existing home directory to the new UID.
-    // Only files owned by old_uid are touched (files owned by other users
-    // are left alone, matching GNU shadow-utils behavior).
-    if new_uid != old_uid && !home_is_changing && !home_for_chown.is_empty() {
+    // If the UID or the primary group changed and the home directory was not
+    // explicitly moved, recursively chown the existing home directory.
+    // usermod(8): only files owned by the old UID get the new UID, and only
+    // files in the old primary group get the new group.
+    if (new_uid != old_uid || new_primary_gid != old_primary_gid)
+        && !home_is_changing
+        && !home_for_chown.is_empty()
+    {
+        use std::os::unix::fs::MetadataExt;
+
+        // Like GNU, only a home directory the user owns is walked, and a
+        // symlinked home is not followed.
         let home_path = root.resolve(&home_for_chown);
-        if home_path.exists() {
-            recursive_chown(&home_path, old_uid, new_uid);
+        if std::fs::symlink_metadata(&home_path).is_ok_and(|m| m.is_dir() && m.uid() == old_uid) {
+            recursive_chown(
+                &home_path,
+                (old_uid, new_uid),
+                (old_primary_gid, new_primary_gid),
+            );
         }
     }
 
@@ -507,42 +520,47 @@ fn apply_subid_changes(
     Ok(())
 }
 
-/// Recursively chown all files and directories under `path` that are owned by
-/// `old_uid` to `new_uid`. Files owned by other users are left untouched.
+/// Recursively chown all files and directories under `path`: those owned by
+/// the old UID get the new UID, and those whose group is the old GID get the
+/// new GID. Other owners and groups are left untouched.
 ///
 /// Uses `fchownat` with `AT_SYMLINK_NOFOLLOW` so symlinks themselves are
 /// re-owned without following them.
-fn recursive_chown(path: &Path, old_uid: u32, new_uid: u32) {
-    use std::os::unix::fs::MetadataExt;
-
+fn recursive_chown(path: &Path, owner: (u32, u32), group: (u32, u32)) {
     if let Ok(entries) = std::fs::read_dir(path) {
         for entry in entries.flatten() {
             let entry_path = entry.path();
             if let Ok(meta) = std::fs::symlink_metadata(&entry_path) {
-                if meta.uid() == old_uid {
-                    let _ = rustix::fs::chownat(
-                        rustix::fs::CWD,
-                        &entry_path,
-                        Some(rustix::process::Uid::from_raw(new_uid)),
-                        None,
-                        rustix::fs::AtFlags::SYMLINK_NOFOLLOW,
-                    );
-                }
+                chown_matching(&entry_path, &meta, owner, group);
                 if meta.is_dir() {
-                    recursive_chown(&entry_path, old_uid, new_uid);
+                    recursive_chown(&entry_path, owner, group);
                 }
             }
         }
     }
     // Also chown the directory itself.
-    if let Ok(meta) = std::fs::symlink_metadata(path)
-        && meta.uid() == old_uid
-    {
+    if let Ok(meta) = std::fs::symlink_metadata(path) {
+        chown_matching(path, &meta, owner, group);
+    }
+}
+
+/// Give `path` the new UID if it has the old one, and the new GID if it has
+/// the old one.
+fn chown_matching(path: &Path, meta: &std::fs::Metadata, owner: (u32, u32), group: (u32, u32)) {
+    use std::os::unix::fs::MetadataExt;
+
+    let (old_owner, new_owner) = owner;
+    let (old_group, new_group) = group;
+    let set_owner = (old_owner != new_owner && meta.uid() == old_owner)
+        .then(|| rustix::process::Uid::from_raw(new_owner));
+    let set_group = (old_group != new_group && meta.gid() == old_group)
+        .then(|| rustix::process::Gid::from_raw(new_group));
+    if set_owner.is_some() || set_group.is_some() {
         let _ = rustix::fs::chownat(
             rustix::fs::CWD,
             path,
-            Some(rustix::process::Uid::from_raw(new_uid)),
-            None,
+            set_owner,
+            set_group,
             rustix::fs::AtFlags::SYMLINK_NOFOLLOW,
         );
     }

tests/by-util/test_usermod.rs

@@ -209,6 +209,147 @@ fn test_change_uid() {
     );
 }
 
+/// Owner and group of `path`, without following a symlink.
+fn owner_of(path: &std::path::Path) -> (u32, u32) {
+    use std::os::unix::fs::MetadataExt;
+
+    let meta = std::fs::symlink_metadata(path).expect("stat failed");
+    (meta.uid(), meta.gid())
+}
+
+/// A prefix with testuser (1000:1000) and group dev (2000). The home holds a
+/// file of testuser's, a root-owned file in the old group, a file in another
+/// group, and a symlink in the old group to a file outside the home.
+fn setup_home_with_files() -> (tempfile::TempDir, std::path::PathBuf) {
+    let dir = setup_prefix(
+        "testuser:x:1000:1000:Test:/home/testuser:/bin/bash\n",
+        "testuser:$6$hash:19500:0:99999:7:::\n",
+        "testuser:x:1000:\ndev:x:2000:\n",
+    );
+    let home = dir.path().join("home/testuser");
+    std::fs::create_dir_all(home.join("sub")).expect("failed to create home");
+    for (path, uid, gid) in [
+        (home.join("mine"), 1000, 1000),
+        (home.join("root-owned"), 0, 1000),
+        (home.join("other-group"), 1000, 0),
+        (dir.path().join("outside"), 1000, 1000),
+    ] {
+        std::fs::write(&path, "").expect("failed to write file");
+        std::os::unix::fs::chown(&path, Some(uid), Some(gid)).expect("chown failed");
+    }
+    std::os::unix::fs::symlink(dir.path().join("outside"), home.join("link"))
+        .expect("failed to create symlink");
+    std::os::unix::fs::lchown(home.join("link"), Some(1000), Some(1000)).expect("chown failed");
+    std::os::unix::fs::chown(&home, Some(1000), Some(1000)).expect("chown failed");
+    std::os::unix::fs::chown(home.join("sub"), Some(1000), Some(1000)).expect("chown failed");
+    (dir, home)
+}
+
+#[test]
+fn test_change_gid_regroups_home_files() {
+    if crate::common::skip_unless_root() {
+        return;
+    }
+
+    let (dir, home) = setup_home_with_files();
+    let code = run_with_prefix(&dir, &["-g", "dev", "testuser"]);
+    assert_eq!(code, 0, "changing the primary group should succeed");
+
+    for (name, expected) in [
+        ("", (1000, 2000)),
+        ("sub", (1000, 2000)),
+        ("mine", (1000, 2000)),
+        ("root-owned", (0, 2000)),
+        ("other-group", (1000, 0)),
+        ("link", (1000, 2000)),
+    ] {
+        assert_eq!(owner_of(&home.join(name)), expected, "home entry '{name}'");
+    }
+    assert_eq!(
+        owner_of(&dir.path().join("outside")),
+        (1000, 1000),
+        "the symlink target outside the home should be left alone"
+    );
+}
+
+#[test]
+fn test_change_uid_and_gid_rechowns_home_files() {
+    if crate::common::skip_unless_root() {
+        return;
+    }
+
+    let (dir, home) = setup_home_with_files();
+    let code = run_with_prefix(&dir, &["-u", "1500", "-g", "dev", "testuser"]);
+    assert_eq!(code, 0, "changing the UID and primary group should succeed");
+
+    for (name, expected) in [
+        ("", (1500, 2000)),
+        ("sub", (1500, 2000)),
+        ("mine", (1500, 2000)),
+        ("root-owned", (0, 2000)),
+        ("other-group", (1500, 0)),
+        ("link", (1500, 2000)),
+    ] {
+        assert_eq!(owner_of(&home.join(name)), expected, "home entry '{name}'");
+    }
+    assert_eq!(
+        owner_of(&dir.path().join("outside")),
+        (1000, 1000),
+        "the symlink target outside the home should be left alone"
+    );
+}
+
+#[test]
+fn test_home_not_owned_by_user_is_left_alone() {
+    if crate::common::skip_unless_root() {
+        return;
+    }
+
+    for args in [["-g", "dev"], ["-u", "1500"]] {
+        for symlinked in [false, true] {
+            let dir = setup_prefix(
+                "testuser:x:1000:1000:Test:/home/testuser:/bin/bash\n",
+                "testuser:$6$hash:19500:0:99999:7:::\n",
+                "testuser:x:1000:\ndev:x:2000:\n",
+            );
+            let home = dir.path().join("home/testuser");
+            // A symlinked home points at a real directory the user owns.
+            let real = if symlinked {
+                dir.path().join("real-home")
+            } else {
+                home.clone()
+            };
+            std::fs::create_dir_all(&real).expect("failed to create home");
+            std::fs::write(real.join("mine"), "").expect("failed to write file");
+            std::os::unix::fs::chown(real.join("mine"), Some(1000), Some(1000))
+                .expect("chown failed");
+            let real_owner = if symlinked { 1000 } else { 0 };
+            std::os::unix::fs::chown(&real, Some(real_owner), Some(1000)).expect("chown failed");
+            if symlinked {
+                std::fs::create_dir_all(home.parent().expect("home has a parent"))
+                    .expect("failed to create /home");
+                std::os::unix::fs::symlink(&real, &home).expect("failed to create symlink");
+            }
+
+            let code = run_with_prefix(&dir, &[args[0], args[1], "testuser"]);
+            // GNU exits 12 for a symlinked home: #351.
+            if !symlinked {
+                assert_eq!(code, 0, "{args:?} should succeed");
+            }
+            assert_eq!(
+                owner_of(&real),
+                (real_owner, 1000),
+                "{args:?}, symlinked {symlinked}: home should be left alone"
+            );
+            assert_eq!(
+                owner_of(&real.join("mine")),
+                (1000, 1000),
+                "{args:?}, symlinked {symlinked}: files under it should be left alone"
+            );
+        }
+    }
+}
+
 #[test]
 fn test_add_to_groups() {
     if crate::common::skip_unless_root() {