chroot: fix parsing of --userspec argument

Fix the parsing of the `--userspec=USER:GROUP` argument so that the
both the user and the group are optional, and update the error message
to match that of GNU `chroot`. This commit also removes the incorrect
`clap` arguments for `--user` and `--group`. In `chroot --user=USER`,
the `--user` is an abbreviation of `--userspec`, and in `chroot
--group=GROUP`, the `--group` is an abbreviation of `--groups`.

Closes #7040.

src/uu/chroot/src/chroot.rs

@@ -11,7 +11,7 @@ use clap::{crate_version, Arg, ArgAction, Command};
 use std::ffi::CString;
 use std::io::Error;
 use std::os::unix::prelude::OsStrExt;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 use std::process;
 use uucore::error::{set_exit_code, UClapError, UResult, UUsageError};
 use uucore::fs::{canonicalize, MissingHandling, ResolveMode};
@@ -23,14 +23,79 @@ static USAGE: &str = help_usage!("chroot.md");
 
 mod options {
     pub const NEWROOT: &str = "newroot";
-    pub const USER: &str = "user";
-    pub const GROUP: &str = "group";
     pub const GROUPS: &str = "groups";
     pub const USERSPEC: &str = "userspec";
     pub const COMMAND: &str = "command";
     pub const SKIP_CHDIR: &str = "skip-chdir";
 }
 
+/// A user and group specification, where each is optional.
+enum UserSpec {
+    NeitherGroupNorUser,
+    UserOnly(String),
+    GroupOnly(String),
+    UserAndGroup(String, String),
+}
+
+struct Options {
+    /// Path to the new root directory.
+    newroot: PathBuf,
+    /// Whether to change to the new root directory.
+    skip_chdir: bool,
+    /// List of groups under which the command will be run.
+    groups: Vec<String>,
+    /// The user and group (each optional) under which the command will be run.
+    userspec: Option<UserSpec>,
+}
+
+/// Parse a user and group from the argument to `--userspec`.
+///
+/// The `spec` must be of the form `[USER][:[GROUP]]`, otherwise an
+/// error is returned.
+fn parse_userspec(spec: &str) -> UResult<UserSpec> {
+    match &spec.splitn(2, ':').collect::<Vec<&str>>()[..] {
+        // ""
+        [""] => Ok(UserSpec::NeitherGroupNorUser),
+        // "usr"
+        [usr] => Ok(UserSpec::UserOnly(usr.to_string())),
+        // ":"
+        ["", ""] => Ok(UserSpec::NeitherGroupNorUser),
+        // ":grp"
+        ["", grp] => Ok(UserSpec::GroupOnly(grp.to_string())),
+        // "usr:"
+        [usr, ""] => Ok(UserSpec::UserOnly(usr.to_string())),
+        // "usr:grp"
+        [usr, grp] => Ok(UserSpec::UserAndGroup(usr.to_string(), grp.to_string())),
+        // everything else
+        _ => Err(ChrootError::InvalidUserspec(spec.to_string()).into()),
+    }
+}
+
+impl Options {
+    /// Parse parameters from the command-line arguments.
+    fn from(matches: &clap::ArgMatches) -> UResult<Self> {
+        let newroot = match matches.get_one::<String>(options::NEWROOT) {
+            Some(v) => Path::new(v).to_path_buf(),
+            None => return Err(ChrootError::MissingNewRoot.into()),
+        };
+        let groups = match matches.get_one::<String>(options::GROUPS) {
+            None => vec![],
+            Some(s) => s.split(",").map(str::to_string).collect(),
+        };
+        let skip_chdir = matches.get_flag(options::SKIP_CHDIR);
+        let userspec = match matches.get_one::<String>(options::USERSPEC) {
+            None => None,
+            Some(s) => Some(parse_userspec(s)?),
+        };
+        Ok(Self {
+            newroot,
+            skip_chdir,
+            groups,
+            userspec,
+        })
+    }
+}
+
 #[uucore::main]
 pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let matches = uu_app().try_get_matches_from(args).with_exit_code(125)?;
@@ -39,15 +104,15 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let default_option: &'static str = "-i";
     let user_shell = std::env::var("SHELL");
 
-    let newroot: &Path = match matches.get_one::<String>(options::NEWROOT) {
+    let options = Options::from(&matches)?;
-        Some(v) => Path::new(v),
-        None => return Err(ChrootError::MissingNewRoot.into()),
-    };
 
-    let skip_chdir = matches.get_flag(options::SKIP_CHDIR);
     // We are resolving the path in case it is a symlink or /. or /../
-    if skip_chdir
+    if options.skip_chdir
-        && canonicalize(newroot, MissingHandling::Normal, ResolveMode::Logical)
+        && canonicalize(
+            &options.newroot,
+            MissingHandling::Normal,
+            ResolveMode::Logical,
+        )
         .unwrap()
         .to_str()
             != Some("/")
@@ -58,8 +123,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
         ));
     }
 
-    if !newroot.is_dir() {
+    if !options.newroot.is_dir() {
-        return Err(ChrootError::NoSuchDirectory(format!("{}", newroot.display())).into());
+        return Err(ChrootError::NoSuchDirectory(format!("{}", options.newroot.display())).into());
     }
 
     let commands = match matches.get_many::<String>(options::COMMAND) {
@@ -85,7 +150,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let chroot_args = &command[1..];
 
     // NOTE: Tests can only trigger code beyond this point if they're invoked with root permissions
-    set_context(newroot, &matches)?;
+    set_context(&options)?;
 
     let pstatus = match process::Command::new(chroot_command)
         .args(chroot_args)
@@ -125,20 +190,6 @@ pub fn uu_app() -> Command {
                 .required(true)
                 .index(1),
         )
-        .arg(
-            Arg::new(options::USER)
-                .short('u')
-                .long(options::USER)
-                .help("User (ID or name) to switch before running the program")
-                .value_name("USER"),
-        )
-        .arg(
-            Arg::new(options::GROUP)
-                .short('g')
-                .long(options::GROUP)
-                .help("Group (ID or name) to switch to")
-                .value_name("GROUP"),
-        )
         .arg(
             Arg::new(options::GROUPS)
                 .short('G')
@@ -175,43 +226,18 @@ pub fn uu_app() -> Command {
         )
 }
 
-fn set_context(root: &Path, options: &clap::ArgMatches) -> UResult<()> {
+fn set_context(options: &Options) -> UResult<()> {
-    let userspec_str = options.get_one::<String>(options::USERSPEC);
+    enter_chroot(&options.newroot, options.skip_chdir)?;
-    let user_str = options
+    set_groups_from_str(&options.groups)?;
-        .get_one::<String>(options::USER)
+    match &options.userspec {
-        .map(|s| s.as_str())
+        None | Some(UserSpec::NeitherGroupNorUser) => {}
-        .unwrap_or_default();
+        Some(UserSpec::UserOnly(user)) => set_user(user)?,
-    let group_str = options
+        Some(UserSpec::GroupOnly(group)) => set_main_group(group)?,
-        .get_one::<String>(options::GROUP)
+        Some(UserSpec::UserAndGroup(user, group)) => {
-        .map(|s| s.as_str())
-        .unwrap_or_default();
-    let groups_str = options
-        .get_one::<String>(options::GROUPS)
-        .map(|s| s.as_str())
-        .unwrap_or_default();
-    let skip_chdir = options.contains_id(options::SKIP_CHDIR);
-    let userspec = match userspec_str {
-        Some(u) => {
-            let s: Vec<&str> = u.split(':').collect();
-            if s.len() != 2 || s.iter().any(|&spec| spec.is_empty()) {
-                return Err(ChrootError::InvalidUserspec(u.to_string()).into());
-            };
-            s
-        }
-        None => Vec::new(),
-    };
-
-    let (user, group) = if userspec.is_empty() {
-        (user_str, group_str)
-    } else {
-        (userspec[0], userspec[1])
-    };
-
-    enter_chroot(root, skip_chdir)?;
-
-    set_groups_from_str(groups_str)?;
             set_main_group(group)?;
             set_user(user)?;
+        }
+    }
     Ok(())
 }
 
@@ -239,7 +265,7 @@ fn set_main_group(group: &str) -> UResult<()> {
     if !group.is_empty() {
         let group_id = match entries::grp2gid(group) {
             Ok(g) => g,
-            _ => return Err(ChrootError::NoSuchGroup(group.to_string()).into()),
+            _ => return Err(ChrootError::NoSuchGroup.into()),
         };
         let err = unsafe { setgid(group_id) };
         if err != 0 {
@@ -261,13 +287,13 @@ fn set_groups(groups: &[libc::gid_t]) -> libc::c_int {
     unsafe { setgroups(groups.len() as libc::size_t, groups.as_ptr()) }
 }
 
-fn set_groups_from_str(groups: &str) -> UResult<()> {
+fn set_groups_from_str(groups: &[String]) -> UResult<()> {
     if !groups.is_empty() {
         let mut groups_vec = vec![];
-        for group in groups.split(',') {
+        for group in groups {
             let gid = match entries::grp2gid(group) {
                 Ok(g) => g,
-                Err(_) => return Err(ChrootError::NoSuchGroup(group.to_string()).into()),
+                Err(_) => return Err(ChrootError::NoSuchGroup.into()),
             };
             groups_vec.push(gid);
         }
@@ -281,8 +307,7 @@ fn set_groups_from_str(groups: &str) -> UResult<()> {
 
 fn set_user(user: &str) -> UResult<()> {
     if !user.is_empty() {
-        let user_id =
+        let user_id = entries::usr2uid(user).map_err(|_| ChrootError::NoSuchUser)?;
-            entries::usr2uid(user).map_err(|_| ChrootError::NoSuchUser(user.to_string()))?;
         let err = unsafe { setuid(user_id as libc::uid_t) };
         if err != 0 {
             return Err(

src/uu/chroot/src/error.rs

@@ -28,10 +28,10 @@ pub enum ChrootError {
     MissingNewRoot,
 
     /// Failed to find the specified user.
-    NoSuchUser(String),
+    NoSuchUser,
 
     /// Failed to find the specified group.
-    NoSuchGroup(String),
+    NoSuchGroup,
 
     /// The given directory does not exist.
     NoSuchDirectory(String),
@@ -74,8 +74,8 @@ impl Display for ChrootError {
                 "Missing operand: NEWROOT\nTry '{} --help' for more information.",
                 uucore::execution_phrase(),
             ),
-            Self::NoSuchUser(s) => write!(f, "no such user: {}", s.maybe_quote(),),
+            Self::NoSuchUser => write!(f, "invalid user"),
-            Self::NoSuchGroup(s) => write!(f, "no such group: {}", s.maybe_quote(),),
+            Self::NoSuchGroup => write!(f, "invalid group"),
             Self::NoSuchDirectory(s) => write!(
                 f,
                 "cannot change root directory to {}: no such directory",

tests/by-util/test_chroot.rs

@@ -55,13 +55,34 @@ fn test_no_such_directory() {
 
 #[test]
 fn test_invalid_user_spec() {
-    let (at, mut ucmd) = at_and_ucmd!();
+    let ts = TestScenario::new(util_name!());
 
-    at.mkdir("a");
+    if let Ok(result) = run_ucmd_as_root(&ts, &["--userspec=ARABA:", "/"]) {
+        result
+            .failure()
+            .code_is(125)
+            .stderr_is("chroot: invalid user");
+    } else {
+        print!("Test skipped; requires root user");
+    }
 
-    let result = ucmd.arg("a").arg("--userspec=ARABA:").fails();
+    if let Ok(result) = run_ucmd_as_root(&ts, &["--userspec=ARABA:ARABA", "/"]) {
-    result.code_is(125);
+        result
-    assert!(result.stderr_str().starts_with("chroot: invalid userspec"));
+            .failure()
+            .code_is(125)
+            .stderr_is("chroot: invalid user");
+    } else {
+        print!("Test skipped; requires root user");
+    }
+
+    if let Ok(result) = run_ucmd_as_root(&ts, &["--userspec=:ARABA", "/"]) {
+        result
+            .failure()
+            .code_is(125)
+            .stderr_is("chroot: invalid group");
+    } else {
+        print!("Test skipped; requires root user");
+    }
 }
 
 #[test]
@@ -77,10 +98,9 @@ fn test_invalid_user() {
         print!("Test skipped; requires root user");
     }
 
+    // `--user` is an abbreviation of `--userspec`.
     if let Ok(result) = run_ucmd_as_root(&ts, &["--user=nobody:+65535", dir, "pwd"]) {
-        result
+        result.failure().stderr_is("chroot: invalid user");
-            .failure()
-            .stderr_contains("no such user: nobody:+65535");
     } else {
         print!("Test skipped; requires root user");
     }
@@ -116,6 +136,7 @@ fn test_preference_of_userspec() {
 
     at.mkdir("a");
 
+    // `--user` is an abbreviation of `--userspec`.
     let result = ucmd
         .arg("a")
         .arg("--user")