Merge pull request #2371 from jhscheer/fix_get_groups

uucore: fix order of group IDs return from entries::get_groups()

Cargo.toml

@@ -349,7 +349,7 @@ sha1 = { version="0.6", features=["std"] }
 tempfile = "3.2.0"
 time = "0.1"
 unindent = "0.1"
-uucore = { version=">=0.0.8", package="uucore", path="src/uucore", features=["entries"] }
+uucore = { version=">=0.0.8", package="uucore", path="src/uucore", features=["entries", "process"] }
 walkdir = "2.2"
 atty = "0.2.14"
 

src/uu/groups/src/groups.rs

@@ -10,7 +10,7 @@
 
 #[macro_use]
 extern crate uucore;
-use uucore::entries::{get_groups, gid2grp, Locate, Passwd};
+use uucore::entries::{get_groups_gnu, gid2grp, Locate, Passwd};
 
 use clap::{crate_version, App, Arg};
 
@@ -35,7 +35,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         None => {
             println!(
                 "{}",
-                get_groups()
+                get_groups_gnu(None)
                     .unwrap()
                     .iter()
                     .map(|&g| gid2grp(g).unwrap())

src/uu/id/src/id.rs

@@ -10,7 +10,7 @@
 //  http://ftp-archive.freebsd.org/mirror/FreeBSD-Archive/old-releases/i386/1.0-RELEASE/ports/shellutils/src/id.c
 //  http://www.opensource.apple.com/source/shell_cmds/shell_cmds-118/id/id.c
 
-// spell-checker:ignore (ToDO) asid auditid auditinfo auid cstr egid emod euid getaudit getlogin gflag nflag pline rflag termid uflag
+// spell-checker:ignore (ToDO) asid auditid auditinfo auid cstr egid emod euid getaudit getlogin gflag nflag pline rflag termid uflag gsflag
 
 #![allow(non_camel_case_types)]
 #![allow(dead_code)]
@@ -79,7 +79,7 @@ static OPT_GROUPS: &str = "groups";
 static OPT_HUMAN_READABLE: &str = "human-readable";
 static OPT_NAME: &str = "name";
 static OPT_PASSWORD: &str = "password";
-static OPT_REAL_ID: &str = "real-id";
+static OPT_REAL_ID: &str = "real";
 
 static ARG_USERS: &str = "users";
 
@@ -135,7 +135,10 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         .arg(
             Arg::with_name(OPT_REAL_ID)
                 .short("r")
-                .help("Display the real ID for the -g and -u options"),
+                .long(OPT_REAL_ID)
+                .help(
+                "Display the real ID for the -G, -g and -u options instead of the effective ID.",
+            ),
         )
         .arg(Arg::with_name(ARG_USERS).multiple(true).takes_value(true))
         .get_matches_from(args);
@@ -162,6 +165,7 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
     let nflag = matches.is_present(OPT_NAME);
     let uflag = matches.is_present(OPT_EFFECTIVE_USER);
     let gflag = matches.is_present(OPT_GROUP);
+    let gsflag = matches.is_present(OPT_GROUPS);
     let rflag = matches.is_present(OPT_REAL_ID);
 
     if gflag {
@@ -194,26 +198,23 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         return 0;
     }
 
-    if matches.is_present(OPT_GROUPS) {
+    if gsflag {
+        let id = possible_pw
+            .map(|p| p.gid())
+            .unwrap_or(if rflag { getgid() } else { getegid() });
         println!(
             "{}",
-            if nflag {
-                possible_pw
-                    .map(|p| p.belongs_to())
-                    .unwrap_or_else(|| entries::get_groups().unwrap())
-                    .iter()
-                    .map(|&id| entries::gid2grp(id).unwrap())
-                    .collect::<Vec<_>>()
-                    .join(" ")
-            } else {
-                possible_pw
-                    .map(|p| p.belongs_to())
-                    .unwrap_or_else(|| entries::get_groups().unwrap())
-                    .iter()
-                    .map(|&id| id.to_string())
-                    .collect::<Vec<_>>()
-                    .join(" ")
-            }
+            possible_pw
+                .map(|p| p.belongs_to())
+                .unwrap_or_else(|| entries::get_groups_gnu(Some(id)).unwrap())
+                .iter()
+                .map(|&id| if nflag {
+                    entries::gid2grp(id).unwrap_or_else(|_| id.to_string())
+                } else {
+                    id.to_string()
+                })
+                .collect::<Vec<_>>()
+                .join(" ")
         );
         return 0;
     }
@@ -280,7 +281,7 @@ fn pretty(possible_pw: Option<Passwd>) {
 
         println!(
             "groups\t{}",
-            entries::get_groups()
+            entries::get_groups_gnu(None)
                 .unwrap()
                 .iter()
                 .map(|&gr| entries::gid2grp(gr).unwrap())

src/uucore/src/lib/features/entries.rs

@@ -5,7 +5,7 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-// spell-checker:ignore (vars) Passwd cstr fnam gecos ngroups
+// spell-checker:ignore (vars) Passwd cstr fnam gecos ngroups egid
 
 //! Get password/group file entry
 //!
@@ -72,6 +72,41 @@ pub fn get_groups() -> IOResult<Vec<gid_t>> {
     }
 }
 
+/// The list of group IDs returned from GNU's `groups` and GNU's `id --groups`
+/// starts with the effective group ID (egid).
+/// This is a wrapper for `get_groups()` to mimic this behavior.
+///
+/// If `arg_id` is `None` (default), `get_groups_gnu` moves the effective
+/// group id (egid) to the first entry in the returned Vector.
+/// If `arg_id` is `Some(x)`, `get_groups_gnu` moves the id with value `x`
+/// to the first entry in the returned Vector. This might be necessary
+/// for `id --groups --real` if `gid` and `egid` are not equal.
+///
+/// From: https://www.man7.org/linux/man-pages/man3/getgroups.3p.html
+/// As implied by the definition of supplementary groups, the
+/// effective group ID may appear in the array returned by
+/// getgroups() or it may be returned only by getegid().  Duplication
+/// may exist, but the application needs to call getegid() to be sure
+/// of getting all of the information. Various implementation
+/// variations and administrative sequences cause the set of groups
+/// appearing in the result of getgroups() to vary in order and as to
+/// whether the effective group ID is included, even when the set of
+/// groups is the same (in the mathematical sense of ``set''). (The
+/// history of a process and its parents could affect the details of
+/// the result.)
+pub fn get_groups_gnu(arg_id: Option<u32>) -> IOResult<Vec<gid_t>> {
+    let mut groups = get_groups()?;
+    let egid = arg_id.unwrap_or_else(crate::features::process::getegid);
+    if !groups.is_empty() && *groups.first().unwrap() == egid {
+        return Ok(groups);
+    } else if let Some(index) = groups.iter().position(|&x| x == egid) {
+        groups.remove(index);
+    }
+    groups.insert(0, egid);
+    Ok(groups)
+}
+
+#[derive(Copy, Clone)]
 pub struct Passwd {
     inner: passwd,
 }
@@ -268,3 +303,18 @@ pub fn usr2uid(name: &str) -> IOResult<uid_t> {
 pub fn grp2gid(name: &str) -> IOResult<gid_t> {
     Group::locate(name).map(|p| p.gid())
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+
+    #[test]
+    fn test_entries_get_groups_gnu() {
+        if let Ok(mut groups) = get_groups() {
+            if let Some(last) = groups.pop() {
+                groups.insert(0, last);
+                assert_eq!(get_groups_gnu(Some(last)).unwrap(), groups);
+            }
+        }
+    }
+}

tests/by-util/test_groups.rs

@@ -1,41 +1,53 @@
 use crate::common::util::*;
 
 #[test]
+#[cfg(any(target_vendor = "apple", target_os = "linux"))]
 fn test_groups() {
-    let result = new_ucmd!().run();
-    println!("result.stdout = {}", result.stdout_str());
-    println!("result.stderr = {}", result.stderr_str());
-    if is_ci() && result.stdout_str().trim().is_empty() {
-        // In the CI, some server are failing to return the group.
-        // As seems to be a configuration issue, ignoring it
-        return;
+    if !is_ci() {
+        new_ucmd!().succeeds().stdout_is(expected_result(&[]));
+    } else {
+        // TODO: investigate how this could be tested in CI
+        // stderr = groups: cannot find name for group ID 116
+        println!("test skipped:");
     }
-    result.success();
-    assert!(!result.stdout_str().trim().is_empty());
 }
 
 #[test]
-fn test_groups_arg() {
-    // get the username with the "id -un" command
-    let result = TestScenario::new("id").ucmd_keepenv().arg("-un").run();
-    println!("result.stdout = {}", result.stdout_str());
-    println!("result.stderr = {}", result.stderr_str());
-    let s1 = String::from(result.stdout_str().trim());
-    if is_ci() && s1.parse::<f64>().is_ok() {
-        // In the CI, some server are failing to return id -un.
-        // So, if we are getting a uid, just skip this test
-        // As seems to be a configuration issue, ignoring it
+#[cfg(any(target_os = "linux"))]
+#[ignore = "fixme: 'groups USERNAME' needs more debugging"]
+fn test_groups_username() {
+    let scene = TestScenario::new(util_name!());
+    let whoami_result = scene.cmd("whoami").run();
+
+    let username = if whoami_result.succeeded() {
+        whoami_result.stdout_move_str()
+    } else if is_ci() {
+        String::from("docker")
+    } else {
+        println!("test skipped:");
         return;
-    }
+    };
 
-    println!("result.stdout = {}", result.stdout_str());
-    println!("result.stderr = {}", result.stderr_str());
-    result.success();
-    assert!(!result.stdout_str().is_empty());
-    let username = result.stdout_str().trim();
+    // TODO: stdout should be in the form: "username : group1 group2 group3"
 
-    // call groups with the user name to check that we
-    // are getting something
-    new_ucmd!().arg(username).succeeds();
-    assert!(!result.stdout_str().is_empty());
+    scene
+        .ucmd()
+        .arg(&username)
+        .succeeds()
+        .stdout_is(expected_result(&[&username]));
+}
+
+#[cfg(any(target_vendor = "apple", target_os = "linux"))]
+fn expected_result(args: &[&str]) -> String {
+    #[cfg(target_os = "linux")]
+    let util_name = util_name!();
+    #[cfg(target_vendor = "apple")]
+    let util_name = format!("g{}", util_name!());
+
+    TestScenario::new(&util_name)
+        .cmd_keepenv(util_name)
+        .env("LANGUAGE", "C")
+        .args(args)
+        .succeeds()
+        .stdout_move_str()
 }

tests/by-util/test_id.rs

@@ -104,19 +104,23 @@ fn test_id_group() {
 }
 
 #[test]
+#[cfg(any(target_vendor = "apple", target_os = "linux"))]
 fn test_id_groups() {
     let scene = TestScenario::new(util_name!());
-
-    let result = scene.ucmd().arg("-G").succeeds();
-    let groups = result.stdout_str().trim().split_whitespace();
-    for s in groups {
-        assert!(s.parse::<f64>().is_ok());
-    }
-
-    let result = scene.ucmd().arg("--groups").succeeds();
-    let groups = result.stdout_str().trim().split_whitespace();
-    for s in groups {
-        assert!(s.parse::<f64>().is_ok());
+    for g_flag in &["-G", "--groups"] {
+        scene
+            .ucmd()
+            .arg(g_flag)
+            .succeeds()
+            .stdout_is(expected_result(&[g_flag], false));
+        for &r_flag in &["-r", "--real"] {
+            let args = [g_flag, r_flag];
+            scene
+                .ucmd()
+                .args(&args)
+                .succeeds()
+                .stdout_is(expected_result(&args, false));
+        }
     }
 }
 
@@ -167,3 +171,32 @@ fn test_id_password_style() {
 
     assert!(result.stdout_str().starts_with(&username));
 }
+
+#[cfg(any(target_vendor = "apple", target_os = "linux"))]
+fn expected_result(args: &[&str], exp_fail: bool) -> String {
+    #[cfg(target_os = "linux")]
+    let util_name = util_name!();
+    #[cfg(target_vendor = "apple")]
+    let util_name = format!("g{}", util_name!());
+
+    let result = if !exp_fail {
+        TestScenario::new(&util_name)
+            .cmd_keepenv(util_name)
+            .env("LANGUAGE", "C")
+            .args(args)
+            .succeeds()
+            .stdout_move_str()
+    } else {
+        TestScenario::new(&util_name)
+            .cmd_keepenv(util_name)
+            .env("LANGUAGE", "C")
+            .args(args)
+            .fails()
+            .stderr_move_str()
+    };
+    return if cfg!(target_os = "macos") && result.starts_with("gid") {
+        result[1..].to_string()
+    } else {
+        result
+    };
+}