From 08b6dd497521e71cc3d0c743061c611686b0f819 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sun, 8 May 2022 23:02:32 -0400 Subject: [PATCH 1/6] uucore(perms): better support nameless uids, gids Update the `wrap_chown()` function to support user IDs and group IDs that do not correspond to named users or groups, respectively. Before this commit, the result from `uid2usr()` and `gid2grp()` calls were unwrapped because we assumed a user name or group name, respectively, existed. However, this is not always true: for example, running the command `sudo chown 12345 f` works even if there is no named user with ID 12345. This commit expands `wrap_chown()` to work even if no name exists for the user or group. --- src/uucore/src/lib/features/perms.rs | 34 ++++++++++++++++------------ 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index d8e345313..ed9e1c001 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -92,22 +92,25 @@ pub fn wrap_chown>( ); if level == VerbosityLevel::Verbose { out = if verbosity.groups_only { + let gid = meta.gid(); format!( "{}\nfailed to change group of {} from {} to {}", out, path.quote(), - entries::gid2grp(meta.gid()).unwrap(), - entries::gid2grp(dest_gid).unwrap() + entries::gid2grp(gid).unwrap_or(gid.to_string()), + entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) ) } else { + let uid = meta.uid(); + let gid = meta.gid(); format!( "{}\nfailed to change ownership of {} from {}:{} to {}:{}", out, path.quote(), - entries::uid2usr(meta.uid()).unwrap(), - entries::gid2grp(meta.gid()).unwrap(), - entries::uid2usr(dest_uid).unwrap(), - entries::gid2grp(dest_gid).unwrap() + entries::uid2usr(uid).unwrap_or(uid.to_string()), + entries::gid2grp(gid).unwrap_or(gid.to_string()), + entries::uid2usr(dest_uid).unwrap_or(dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) ) }; }; @@ -119,21 +122,24 @@ pub fn wrap_chown>( if changed { match verbosity.level { VerbosityLevel::Changes | VerbosityLevel::Verbose => { + let gid = meta.gid(); out = if verbosity.groups_only { format!( "changed group of {} from {} to {}", path.quote(), - entries::gid2grp(meta.gid()).unwrap(), - entries::gid2grp(dest_gid).unwrap() + entries::gid2grp(gid).unwrap_or(gid.to_string()), + entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) ) } else { + let gid = meta.gid(); + let uid = meta.uid(); format!( "changed ownership of {} from {}:{} to {}:{}", path.quote(), - entries::uid2usr(meta.uid()).unwrap(), - entries::gid2grp(meta.gid()).unwrap(), - entries::uid2usr(dest_uid).unwrap(), - entries::gid2grp(dest_gid).unwrap() + entries::uid2usr(uid).unwrap_or(uid.to_string()), + entries::gid2grp(gid).unwrap_or(gid.to_string()), + entries::uid2usr(dest_uid).unwrap_or(dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) ) }; } @@ -150,8 +156,8 @@ pub fn wrap_chown>( format!( "ownership of {} retained as {}:{}", path.quote(), - entries::uid2usr(dest_uid).unwrap(), - entries::gid2grp(dest_gid).unwrap() + entries::uid2usr(dest_uid).unwrap_or(dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) ) }; } From 55550e1a6ec98e8657bb7b0807f2675424f4fc78 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Sat, 23 Apr 2022 12:39:43 -0400 Subject: [PATCH 2/6] chown: allow setting arbitrary numeric user ID Update `chown` to allow setting the owner of a file to a numeric user ID regardless of whether a corresponding username exists on the system. For example, $ touch f && sudo chown 12345 f succeeds even though there is no named user with ID 12345. Fixes #3380. --- src/uu/chown/src/chown.rs | 15 +++++++++++---- tests/by-util/test_chown.rs | 23 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 3add9df1f..c58696121 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -197,10 +197,17 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { // So, try to parse it this way return parse_spec(spec, '.'); } else { - return Err(USimpleError::new( - 1, - format!("invalid user: {}", spec.quote()), - )); + // It's possible that the `user` string contains a + // numeric user ID, in which case, we respect that. + match user.parse() { + Ok(uid) => uid, + Err(_) => { + return Err(USimpleError::new( + 1, + format!("invalid user: {}", spec.quote()), + )) + } + } } } }) diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 4470260f4..05a10fb65 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -418,6 +418,29 @@ fn test_chown_only_user_id() { .stderr_contains(&"failed to change"); } +/// Test for setting the owner to a user ID for a user that does not exist. +/// +/// For example: +/// +/// $ touch f && chown 12345 f +/// +/// succeeds with exit status 0 and outputs nothing. The owner of the +/// file is set to 12345, even though no user with that ID exists. +/// +/// This test must be run as root, because only the root user can +/// transfer ownership of a file. +#[test] +fn test_chown_only_user_id_nonexistent_user() { + let ts = TestScenario::new(util_name!()); + let at = ts.fixtures.clone(); + at.touch("f"); + if let Ok(result) = run_ucmd_as_root(&ts, &["12345", "f"]) { + result.success().no_stdout().no_stderr(); + } else { + print!("Test skipped; requires root user"); + } +} + #[test] // FixME: stderr = chown: ownership of 'test_chown_file1' retained as cuuser:wheel #[cfg(not(target_os = "freebsd"))] From 163df8abc1954fee060d354c775ed1f4ff99f66c Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Mon, 9 May 2022 10:19:57 -0400 Subject: [PATCH 3/6] fixup! chown: allow setting arbitrary numeric user ID --- src/uu/chown/src/chown.rs | 47 +++++++++++++++++++++++++++---------- tests/by-util/test_chown.rs | 23 ++++++++++++++++++ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index c58696121..18c64d60e 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -167,17 +167,18 @@ pub fn uu_app<'a>() -> Command<'a> { ) } -/// Parse the username and groupname +/// Parse the owner/group specifier string into a user ID and a group ID. /// -/// In theory, it should be username:groupname -/// but ... -/// it can user.name:groupname -/// or username.groupname +/// The `spec` can be of the form: /// -/// # Arguments +/// * `"owner:group"`, +/// * `"owner"`, +/// * `":group"`, /// -/// * `spec` - The input from the user -/// * `sep` - Should be ':' or '.' +/// and the owner or group can be specified either as an ID or a +/// name. The `sep` argument specifies which character to use as a +/// separator between the owner and group; calling code should set +/// this to `':'`. fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { assert!(['.', ':'].contains(&sep)); let mut args = spec.splitn(2, sep); @@ -215,11 +216,18 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { None }; let gid = if !group.is_empty() { - Some( - Group::locate(group) - .map_err(|_| USimpleError::new(1, format!("invalid group: {}", spec.quote())))? - .gid, - ) + Some(match Group::locate(group) { + Ok(g) => g.gid, + Err(_) => match group.parse() { + Ok(gid) => gid, + Err(_) => { + return Err(USimpleError::new( + 1, + format!("invalid group: {}", spec.quote()), + )); + } + }, + }) } else { None }; @@ -238,4 +246,17 @@ mod test { assert!(format!("{}", parse_spec("::", ':').err().unwrap()).starts_with("invalid group: ")); assert!(format!("{}", parse_spec("..", ':').err().unwrap()).starts_with("invalid group: ")); } + + /// Test for parsing IDs that don't correspond to a named user or group. + #[test] + fn test_parse_spec_nameless_ids() { + // This assumes that there is no named user with ID 12345. + assert!(matches!(parse_spec("12345", ':'), Ok((Some(12345), None)))); + // This assumes that there is no named group with ID 54321. + assert!(matches!(parse_spec(":54321", ':'), Ok((None, Some(54321))))); + assert!(matches!( + parse_spec("12345:54321", ':'), + Ok((Some(12345), Some(54321))) + )); + } } diff --git a/tests/by-util/test_chown.rs b/tests/by-util/test_chown.rs index 05a10fb65..ef3fc0d33 100644 --- a/tests/by-util/test_chown.rs +++ b/tests/by-util/test_chown.rs @@ -484,6 +484,29 @@ fn test_chown_only_group_id() { .stderr_contains(&"failed to change"); } +/// Test for setting the group to a group ID for a group that does not exist. +/// +/// For example: +/// +/// $ touch f && chown :12345 f +/// +/// succeeds with exit status 0 and outputs nothing. The group of the +/// file is set to 12345, even though no group with that ID exists. +/// +/// This test must be run as root, because only the root user can +/// transfer ownership of a file. +#[test] +fn test_chown_only_group_id_nonexistent_group() { + let ts = TestScenario::new(util_name!()); + let at = ts.fixtures.clone(); + at.touch("f"); + if let Ok(result) = run_ucmd_as_root(&ts, &[":12345", "f"]) { + result.success().no_stdout().no_stderr(); + } else { + print!("Test skipped; requires root user"); + } +} + #[test] fn test_chown_owner_group_id() { // test chown 1111:1111 file.txt From f56903493c3717401dc2191584844f9c510e2340 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Mon, 9 May 2022 20:29:11 -0400 Subject: [PATCH 4/6] WIP Trying to diagnose 'invalid group: 1001:121' error in CI environment --- src/uu/chown/src/chown.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 18c64d60e..9d2cad1c3 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -220,7 +220,8 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { Ok(g) => g.gid, Err(_) => match group.parse() { Ok(gid) => gid, - Err(_) => { + Err(e) => { + eprintln!("error {}, user {}, group {}, uid {:?}", e, user, group, uid); return Err(USimpleError::new( 1, format!("invalid group: {}", spec.quote()), From 3029d83a36af853a95cebdd8363a41c3143659ab Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Mon, 9 May 2022 20:47:46 -0400 Subject: [PATCH 5/6] Revert "WIP Trying to diagnose 'invalid group: 1001:121' error in CI environment" This reverts commit 291fb3ad71a0e93705509a352fd95de7539402ed. --- src/uu/chown/src/chown.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/uu/chown/src/chown.rs b/src/uu/chown/src/chown.rs index 9d2cad1c3..18c64d60e 100644 --- a/src/uu/chown/src/chown.rs +++ b/src/uu/chown/src/chown.rs @@ -220,8 +220,7 @@ fn parse_spec(spec: &str, sep: char) -> UResult<(Option, Option)> { Ok(g) => g.gid, Err(_) => match group.parse() { Ok(gid) => gid, - Err(e) => { - eprintln!("error {}, user {}, group {}, uid {:?}", e, user, group, uid); + Err(_) => { return Err(USimpleError::new( 1, format!("invalid group: {}", spec.quote()), From 5713de4d937ac7e03cbf32e926fb4044bd782d67 Mon Sep 17 00:00:00 2001 From: Jeffrey Finkelstein Date: Mon, 9 May 2022 20:50:20 -0400 Subject: [PATCH 6/6] fixup! uucore(perms): better support nameless uids, gids --- src/uucore/src/lib/features/perms.rs | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/uucore/src/lib/features/perms.rs b/src/uucore/src/lib/features/perms.rs index ed9e1c001..edb714646 100644 --- a/src/uucore/src/lib/features/perms.rs +++ b/src/uucore/src/lib/features/perms.rs @@ -97,8 +97,8 @@ pub fn wrap_chown>( "{}\nfailed to change group of {} from {} to {}", out, path.quote(), - entries::gid2grp(gid).unwrap_or(gid.to_string()), - entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) ) } else { let uid = meta.uid(); @@ -107,10 +107,10 @@ pub fn wrap_chown>( "{}\nfailed to change ownership of {} from {}:{} to {}:{}", out, path.quote(), - entries::uid2usr(uid).unwrap_or(uid.to_string()), - entries::gid2grp(gid).unwrap_or(gid.to_string()), - entries::uid2usr(dest_uid).unwrap_or(dest_uid.to_string()), - entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) + entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), + entries::uid2usr(dest_uid).unwrap_or_else(|_| dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) ) }; }; @@ -127,8 +127,8 @@ pub fn wrap_chown>( format!( "changed group of {} from {} to {}", path.quote(), - entries::gid2grp(gid).unwrap_or(gid.to_string()), - entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) ) } else { let gid = meta.gid(); @@ -136,10 +136,10 @@ pub fn wrap_chown>( format!( "changed ownership of {} from {}:{} to {}:{}", path.quote(), - entries::uid2usr(uid).unwrap_or(uid.to_string()), - entries::gid2grp(gid).unwrap_or(gid.to_string()), - entries::uid2usr(dest_uid).unwrap_or(dest_uid.to_string()), - entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) + entries::uid2usr(uid).unwrap_or_else(|_| uid.to_string()), + entries::gid2grp(gid).unwrap_or_else(|_| gid.to_string()), + entries::uid2usr(dest_uid).unwrap_or_else(|_| dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) ) }; } @@ -156,8 +156,8 @@ pub fn wrap_chown>( format!( "ownership of {} retained as {}:{}", path.quote(), - entries::uid2usr(dest_uid).unwrap_or(dest_uid.to_string()), - entries::gid2grp(dest_gid).unwrap_or(dest_gid.to_string()) + entries::uid2usr(dest_uid).unwrap_or_else(|_| dest_uid.to_string()), + entries::gid2grp(dest_gid).unwrap_or_else(|_| dest_gid.to_string()) ) }; }