From c159ea2042656d0e0559c166e6e801fbc5c19193 Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Wed, 2 Nov 2022 22:39:22 -0600 Subject: [PATCH 1/2] Fix panic when using --owner Unfortunately, clap_derive can't combine a value_parser of Option with an optional argument to get a merged Option so we need to do the check for the nop outside of the value parser. Also adds some tests for --owner Fixes: #1163 --- src/filter/owner.rs | 40 +++++++++++++++++++++++++--------------- src/main.rs | 2 +- tests/tests.rs | 44 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 70 insertions(+), 16 deletions(-) diff --git a/src/filter/owner.rs b/src/filter/owner.rs index b69c2ef..842c711 100644 --- a/src/filter/owner.rs +++ b/src/filter/owner.rs @@ -1,13 +1,13 @@ use anyhow::{anyhow, Result}; use std::fs; -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct OwnerFilter { uid: Check, gid: Check, } -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] enum Check { Equal(T), NotEq(T), @@ -15,10 +15,15 @@ enum Check { } impl OwnerFilter { + const IGNORE: Self = OwnerFilter { + uid: Check::Ignore, + gid: Check::Ignore, + }; + /// Parses an owner constraint /// Returns an error if the string is invalid /// Returns Ok(None) when string is acceptable but a noop (such as "" or ":") - pub fn from_string(input: &str) -> Result> { + pub fn from_string(input: &str) -> Result { let mut it = input.split(':'); let (fst, snd) = (it.next(), it.next()); @@ -42,10 +47,15 @@ impl OwnerFilter { .ok_or_else(|| anyhow!("'{}' is not a recognized group name", s)) })?; - if let (Check::Ignore, Check::Ignore) = (uid, gid) { - Ok(None) + Ok(OwnerFilter { uid, gid }) + } + + /// If self is a no-op (ignore both uid and gid) then return `None`, otherwise wrap in a `Some` + pub fn filter_ignore(self) -> Option { + if self == Self::IGNORE { + None } else { - Ok(Some(OwnerFilter { uid, gid })) + Some(self) } } @@ -106,16 +116,16 @@ mod owner_parsing { use super::Check::*; owner_tests! { - empty: "" => Ok(None), - uid_only: "5" => Ok(Some(OwnerFilter { uid: Equal(5), gid: Ignore })), - uid_gid: "9:3" => Ok(Some(OwnerFilter { uid: Equal(9), gid: Equal(3) })), - gid_only: ":8" => Ok(Some(OwnerFilter { uid: Ignore, gid: Equal(8) })), - colon_only: ":" => Ok(None), - trailing: "5:" => Ok(Some(OwnerFilter { uid: Equal(5), gid: Ignore })), + empty: "" => Ok(OwnerFilter::IGNORE), + uid_only: "5" => Ok(OwnerFilter { uid: Equal(5), gid: Ignore }), + uid_gid: "9:3" => Ok(OwnerFilter { uid: Equal(9), gid: Equal(3) }), + gid_only: ":8" => Ok(OwnerFilter { uid: Ignore, gid: Equal(8) }), + colon_only: ":" => Ok(OwnerFilter::IGNORE), + trailing: "5:" => Ok(OwnerFilter { uid: Equal(5), gid: Ignore }), - uid_negate: "!5" => Ok(Some(OwnerFilter { uid: NotEq(5), gid: Ignore })), - both_negate:"!4:!3" => Ok(Some(OwnerFilter { uid: NotEq(4), gid: NotEq(3) })), - uid_not_gid:"6:!8" => Ok(Some(OwnerFilter { uid: Equal(6), gid: NotEq(8) })), + uid_negate: "!5" => Ok(OwnerFilter { uid: NotEq(5), gid: Ignore }), + both_negate:"!4:!3" => Ok(OwnerFilter { uid: NotEq(4), gid: NotEq(3) }), + uid_not_gid:"6:!8" => Ok(OwnerFilter { uid: Equal(6), gid: NotEq(8) }), more_colons:"3:5:" => Err(_), only_colons:"::" => Err(_), diff --git a/src/main.rs b/src/main.rs index 9fa4edc..08a9881 100644 --- a/src/main.rs +++ b/src/main.rs @@ -184,7 +184,7 @@ fn construct_config(mut opts: Opts, pattern_regex: &str) -> Result { let size_limits = std::mem::take(&mut opts.size); let time_constraints = extract_time_constraints(&opts)?; #[cfg(unix)] - let owner_constraint: Option = opts.owner; + let owner_constraint: Option = opts.owner.and_then(OwnerFilter::filter_ignore); #[cfg(windows)] let ansi_colors_support = diff --git a/tests/tests.rs b/tests/tests.rs index 1cc561f..8d0f093 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1919,6 +1919,50 @@ fn test_modified_absolute() { ); } +#[cfg(unix)] +#[test] +fn test_owner_ignore_all() { + let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + te.assert_output(&["--owner", ":", "a.foo"], "a.foo"); + te.assert_output(&["--owner", "", "a.foo"], "a.foo"); +} + +#[cfg(unix)] +#[test] +fn test_owner_current_user() { + let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + let uid = users::get_current_uid(); + te.assert_output(&["--owner", &uid.to_string(), "a.foo"], "a.foo"); + if let Some(username) = users::get_current_username().map(|u| u.into_string().unwrap()) { + te.assert_output(&["--owner", &username, "a.foo"], "a.foo"); + } +} + +#[cfg(unix)] +#[test] +fn test_owner_current_group() { + let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + let gid = users::get_current_gid(); + te.assert_output(&["--owner", &format!(":{}", gid), "a.foo"], "a.foo"); + if let Some(groupname) = users::get_current_groupname().map(|u| u.into_string().unwrap()) { + te.assert_output(&["--owner", &format!(":{}", groupname), "a.foo"], "a.foo"); + } +} + +#[cfg(unix)] +#[test] +fn test_owner_root() { + // This test assumes the current user isn't root + if users::get_current_uid() == 0 || users::get_current_gid() == 0 { + return; + } + let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); + te.assert_output(&["--owner", "root", "a.foo"], ""); + te.assert_output(&["--owner", "0", "a.foo"], ""); + te.assert_output(&["--owner", ":root", "a.foo"], ""); + te.assert_output(&["--owner", ":0", "a.foo"], ""); +} + #[test] fn test_custom_path_separator() { let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES); From b04cae2ca07ac114b5abcd2e1be517ed9410c8aa Mon Sep 17 00:00:00 2001 From: Thayne McCombs Date: Wed, 2 Nov 2022 23:55:31 -0600 Subject: [PATCH 2/2] Only run owner root test on linux Because macos doesn't have a "root" user --- tests/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tests.rs b/tests/tests.rs index 8d0f093..eb24472 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -1949,7 +1949,7 @@ fn test_owner_current_group() { } } -#[cfg(unix)] +#[cfg(target_os = "linux")] #[test] fn test_owner_root() { // This test assumes the current user isn't root