Merge pull request #521 from tavianator/user-group-ids

find: User/group ID fixes

Author: sylvestre

src/find/matchers/group.rs

@@ -3,71 +3,47 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-use super::{Matcher, MatcherIO, WalkEntry};
+use super::{ComparableValue, Matcher, MatcherIO, WalkEntry};
 
 #[cfg(unix)]
 use nix::unistd::Group;
 #[cfg(unix)]
 use std::os::unix::fs::MetadataExt;
 
 pub struct GroupMatcher {
-    gid: Option<u32>,
+    gid: ComparableValue,
 }
 
 impl GroupMatcher {
     #[cfg(unix)]
-    pub fn from_group_name(group: &str) -> Self {
+    pub fn from_group_name(group: &str) -> Option<Self> {
         // get gid from group name
-        let Ok(group) = Group::from_name(group) else {
-            return Self { gid: None };
-        };
-
-        let Some(group) = group else {
-            // This if branch is to determine whether a certain group exists in the system.
-            // If a certain group does not exist in the system,
-            // the result will need to be returned according to
-            // the flag bit of whether to invert the result.
-            return Self { gid: None };
-        };
-
-        Self {
-            gid: Some(group.gid.as_raw()),
-        }
+        let group = Group::from_name(group).ok()??;
+        let gid = group.gid.as_raw();
+        Some(Self::from_gid(gid))
     }
 
-    #[cfg(unix)]
     pub fn from_gid(gid: u32) -> Self {
-        Self { gid: Some(gid) }
+        Self::from_comparable(ComparableValue::EqualTo(gid as u64))
     }
 
-    #[cfg(windows)]
-    pub fn from_group_name(_group: &str) -> GroupMatcher {
-        GroupMatcher { gid: None }
+    pub fn from_comparable(gid: ComparableValue) -> Self {
+        Self { gid }
     }
 
     #[cfg(windows)]
-    pub fn from_gid(_gid: u32) -> GroupMatcher {
-        GroupMatcher { gid: None }
-    }
-
-    pub fn gid(&self) -> &Option<u32> {
-        &self.gid
+    pub fn from_group_name(_group: &str) -> Option<Self> {
+        None
     }
 }
 
 impl Matcher for GroupMatcher {
     #[cfg(unix)]
     fn matches(&self, file_info: &WalkEntry, _: &mut MatcherIO) -> bool {
-        let Ok(metadata) = file_info.metadata() else {
-            return false;
-        };
-
-        let file_gid = metadata.gid();
-
-        // When matching the -group parameter in find/matcher/mod.rs,
-        // it has been judged that the group does not exist and an error is returned.
-        // So use unwarp() directly here.
-        self.gid.unwrap() == file_gid
+        match file_info.metadata() {
+            Ok(metadata) => self.gid.matches(metadata.gid().into()),
+            Err(_) => false,
+        }
     }
 
     #[cfg(windows)]
@@ -136,7 +112,8 @@ mod tests {
             .unwrap()
             .name;
 
-        let matcher = super::GroupMatcher::from_group_name(file_group.as_str());
+        let matcher =
+            super::GroupMatcher::from_group_name(file_group.as_str()).expect("group should exist");
         assert!(
             matcher.matches(&file_info, &mut matcher_io),
             "group should match"
@@ -146,18 +123,13 @@ mod tests {
         let time_string = Local::now().format("%Y%m%d%H%M%S").to_string();
         let matcher = GroupMatcher::from_group_name(time_string.as_str());
         assert!(
-            matcher.gid().is_none(),
+            matcher.is_none(),
             "group name {} should not exist",
             time_string
         );
 
         // Testing group id
         let matcher = GroupMatcher::from_gid(file_gid);
-        assert!(
-            matcher.gid().is_some(),
-            "group id {} should exist",
-            file_gid
-        );
         assert!(
             matcher.matches(&file_info, &mut matcher_io),
             "group id should match"

src/find/matchers/mod.rs

@@ -717,29 +717,20 @@ fn build_matcher_tree(
                 }
 
                 i += 1;
-                let matcher = UserMatcher::from_user_name(user);
-                match matcher.uid() {
-                    Some(_) => Some(matcher.into_box()),
-                    None => {
-                        return Err(From::from(format!(
-                            "{} is not the name of a known user",
-                            user
-                        )))
-                    }
-                }
+                let matcher = UserMatcher::from_user_name(user)
+                    .or_else(|| Some(UserMatcher::from_uid(user.parse::<u32>().ok()?)))
+                    .ok_or_else(|| format!("{user} is not the name of a known user"))?;
+                Some(matcher.into_box())
             }
             "-nouser" => Some(NoUserMatcher {}.into_box()),
             "-uid" => {
                 if i >= args.len() - 1 {
                     return Err(From::from(format!("missing argument to {}", args[i])));
                 }
                 // check if the argument is a number
-                let uid = args[i + 1].parse::<u32>();
-                if uid.is_err() {
-                    return Err(From::from(format!("{} is not a number", args[i + 1])));
-                }
+                let uid = convert_arg_to_comparable_value(args[i], args[i + 1])?;
                 i += 1;
-                Some(UserMatcher::from_uid(uid.unwrap()).into_box())
+                Some(UserMatcher::from_comparable(uid).into_box())
             }
             "-group" => {
                 if i >= args.len() - 1 {
@@ -755,32 +746,20 @@ fn build_matcher_tree(
                 }
 
                 i += 1;
-                let matcher = GroupMatcher::from_group_name(group);
-                match matcher.gid() {
-                    Some(_) => Some(matcher.into_box()),
-                    None => {
-                        return Err(From::from(format!(
-                            "{} is not the name of an existing group",
-                            group
-                        )))
-                    }
-                }
+                let matcher = GroupMatcher::from_group_name(group)
+                    .or_else(|| Some(GroupMatcher::from_gid(group.parse::<u32>().ok()?)))
+                    .ok_or_else(|| format!("{group} is not the name of an existing group"))?;
+                Some(matcher.into_box())
             }
             "-nogroup" => Some(NoGroupMatcher {}.into_box()),
             "-gid" => {
                 if i >= args.len() - 1 {
                     return Err(From::from(format!("missing argument to {}", args[i])));
                 }
                 // check if the argument is a number
-                let gid = args[i + 1].parse::<u32>();
-                if gid.is_err() {
-                    return Err(From::from(format!(
-                        "find: invalid argument `{}' to `-gid'",
-                        args[i + 1]
-                    )));
-                }
+                let gid = convert_arg_to_comparable_value(args[i], args[i + 1])?;
                 i += 1;
-                Some(GroupMatcher::from_gid(gid.unwrap()).into_box())
+                Some(GroupMatcher::from_comparable(gid).into_box())
             }
             "-executable" => Some(AccessMatcher::Executable.into_box()),
             "-perm" => {

src/find/matchers/user.rs

@@ -3,71 +3,47 @@
 // For the full copyright and license information, please view the LICENSE
 // file that was distributed with this source code.
 
-use super::{Matcher, MatcherIO, WalkEntry};
+use super::{ComparableValue, Matcher, MatcherIO, WalkEntry};
 
 #[cfg(unix)]
 use nix::unistd::User;
 #[cfg(unix)]
 use std::os::unix::fs::MetadataExt;
 
 pub struct UserMatcher {
-    uid: Option<u32>,
+    uid: ComparableValue,
 }
 
 impl UserMatcher {
     #[cfg(unix)]
-    pub fn from_user_name(user: &str) -> Self {
+    pub fn from_user_name(user: &str) -> Option<Self> {
         // get uid from user name
-        let Ok(user) = User::from_name(user) else {
-            return Self { uid: None };
-        };
-
-        let Some(user) = user else {
-            // This if branch is to determine whether a certain user exists in the system.
-            // If a certain user does not exist in the system,
-            // the result will need to be returned according to
-            // the flag bit of whether to invert the result.
-            return Self { uid: None };
-        };
-
-        Self {
-            uid: Some(user.uid.as_raw()),
-        }
+        let user = User::from_name(user).ok()??;
+        let uid = user.uid.as_raw();
+        Some(Self::from_uid(uid))
     }
 
-    #[cfg(unix)]
     pub fn from_uid(uid: u32) -> Self {
-        Self { uid: Some(uid) }
+        Self::from_comparable(ComparableValue::EqualTo(uid as u64))
     }
 
-    #[cfg(windows)]
-    pub fn from_user_name(_user: &str) -> UserMatcher {
-        UserMatcher { uid: None }
+    pub fn from_comparable(uid: ComparableValue) -> Self {
+        Self { uid }
     }
 
     #[cfg(windows)]
-    pub fn from_uid(_uid: u32) -> UserMatcher {
-        UserMatcher { uid: None }
-    }
-
-    pub fn uid(&self) -> &Option<u32> {
-        &self.uid
+    pub fn from_user_name(_user: &str) -> Option<Self> {
+        None
     }
 }
 
 impl Matcher for UserMatcher {
     #[cfg(unix)]
     fn matches(&self, file_info: &WalkEntry, _: &mut MatcherIO) -> bool {
-        let Ok(metadata) = file_info.metadata() else {
-            return false;
-        };
-
-        let file_uid = metadata.uid();
-
-        // When matching the -user parameter in find/matcher/mod.rs,
-        // it has been judged that the user does not exist and an error is returned.
-        // So use unwarp() directly here.
-        self.uid.unwrap() == file_uid
+        match file_info.metadata() {
+            Ok(metadata) => self.uid.matches(metadata.uid().into()),
+            Err(_) => false,
+        }
     }
 
     #[cfg(windows)]
@@ -134,7 +110,7 @@ mod tests {
             .unwrap()
             .name;
 
-        let matcher = UserMatcher::from_user_name(file_user.as_str());
+        let matcher = UserMatcher::from_user_name(file_user.as_str()).expect("user should exist");
         assert!(
             matcher.matches(&file_info, &mut matcher_io),
             "user should be the same"
@@ -144,14 +120,13 @@ mod tests {
         let time_string = Local::now().format("%Y%m%d%H%M%S").to_string();
         let matcher = UserMatcher::from_user_name(time_string.as_str());
         assert!(
-            matcher.uid().is_none(),
+            matcher.is_none(),
             "user {} should not be the same",
             time_string
         );
 
         // Testing user id
         let matcher = UserMatcher::from_uid(file_uid);
-        assert!(matcher.uid().is_some(), "user id {} should exist", file_uid);
         assert!(
             matcher.matches(&file_info, &mut matcher_io),
             "user id should match"

src/find/mod.rs

@@ -1163,17 +1163,35 @@ mod tests {
         );
 
         // test uid
-        let deps = FakeDependencies::new();
-        let rc = find_main(
-            &[
-                "find",
-                "./test_data/simple/subdir",
-                "-uid",
-                &uid.to_string(),
-            ],
-            &deps,
-        );
-        assert_eq!(rc, 0);
+        for arg in ["-uid", "-user"] {
+            let deps = FakeDependencies::new();
+            let rc = find_main(
+                &["find", "./test_data/simple/subdir", arg, &uid.to_string()],
+                &deps,
+            );
+            assert_eq!(rc, 0);
+        }
+
+        // test -uid +N, -uid -N
+        if uid > 0 {
+            let deps = FakeDependencies::new();
+            let rc = find_main(
+                &[
+                    "find",
+                    "./test_data/simple/subdir",
+                    "-uid",
+                    &format!("+{}", uid - 1),
+                    "-uid",
+                    &format!("-{}", uid + 1),
+                ],
+                &deps,
+            );
+            assert_eq!(rc, 0);
+            assert_eq!(
+                deps.get_output_as_string(),
+                "./test_data/simple/subdir\n./test_data/simple/subdir/ABBBC\n"
+            );
+        }
 
         // test empty uid
         let deps = FakeDependencies::new();
@@ -1233,17 +1251,35 @@ mod tests {
         );
 
         // test gid
-        let deps = FakeDependencies::new();
-        let rc = find_main(
-            &[
-                "find",
-                "./test_data/simple/subdir",
-                "-gid",
-                gid.to_string().as_str(),
-            ],
-            &deps,
-        );
-        assert_eq!(rc, 0);
+        for arg in ["-gid", "-group"] {
+            let deps = FakeDependencies::new();
+            let rc = find_main(
+                &["find", "./test_data/simple/subdir", arg, &gid.to_string()],
+                &deps,
+            );
+            assert_eq!(rc, 0);
+        }
+
+        // test -gid +N, -gid -N
+        if gid > 0 {
+            let deps = FakeDependencies::new();
+            let rc = find_main(
+                &[
+                    "find",
+                    "./test_data/simple/subdir",
+                    "-gid",
+                    &format!("+{}", gid - 1),
+                    "-gid",
+                    &format!("-{}", gid + 1),
+                ],
+                &deps,
+            );
+            assert_eq!(rc, 0);
+            assert_eq!(
+                deps.get_output_as_string(),
+                "./test_data/simple/subdir\n./test_data/simple/subdir/ABBBC\n"
+            );
+        }
 
         // test empty gid
         let deps = FakeDependencies::new();