From b6fe813dfabe1eaa58a0996f4dc8679e6b6c1259 Mon Sep 17 00:00:00 2001
From: Stephen Touset <stephen@squareup.com>
Date: Thu, 28 Mar 2019 11:40:35 -0700
Subject: [PATCH] Store groups_exempted separately from gids_exempted

---
 sudo_pair/src/lib.rs | 70 ++++++++++++++++++++++----------------------
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/sudo_pair/src/lib.rs b/sudo_pair/src/lib.rs
index 77ef8c3..3cad82e 100644
--- a/sudo_pair/src/lib.rs
+++ b/sudo_pair/src/lib.rs
@@ -407,13 +407,13 @@ impl SudoPair {
     }
 
     fn is_sudoing_from_exempted_gid(&self) -> bool {
-        !self.options.gids_exempted.is_disjoint(
+        !self.options.gids_exempted().is_disjoint(
             &self.plugin.user_info.groups.iter().cloned().collect()
         )
     }
 
     fn is_sudoing_to_enforced_gid(&self) -> bool {
-        !self.options.gids_enforced.is_disjoint(
+        !self.options.gids_enforced().is_disjoint(
             &self.plugin.runas_gids()
         )
     }
@@ -582,7 +582,8 @@ struct PluginOptions {
     /// pair approve their session.
     ///
     /// Default: `[0]` (e.g., root)
-    gids_enforced: HashSet<gid_t>,
+    gids_enforced:   HashSet<gid_t>,
+    groups_enforced: HashSet<String>,
 
     /// `gids_exempted` is a comma-separated list of gids whose users
     /// will be exempted from the requirements of sudo_pair. Note that
@@ -593,7 +594,8 @@ struct PluginOptions {
     /// outages without needing to find a pair.
     ///
     /// Default: `[]` (however, root is *always* exempt)
-    gids_exempted: HashSet<gid_t>,
+    gids_exempted:   HashSet<gid_t>,
+    groups_exempted: HashSet<String>,
 }
 
 impl PluginOptions {
@@ -602,46 +604,35 @@ impl PluginOptions {
             self.binary_path.as_os_str()
         ).as_bytes()
     }
-}
-
-// TODO: single_use_lifetimes was committed, but I'm not sure there's
-// actually a way to satisfy the linter for the time being
-#[allow(single_use_lifetimes)]
-impl<'a> From<&'a OptionMap> for PluginOptions {
-    fn from(map: &'a OptionMap) -> Self {
-        let groups_enforced : HashSet<String> = map.get("groups_enforced")
-            .unwrap_or_default();
 
-        let mut gids_enforced : HashSet<gid_t> = groups_enforced
+    fn gids_enforced(&self) -> HashSet<gid_t> {
+        let gids_enforced : HashSet<gid_t> = self.groups_enforced
             .iter()
             .filter_map(|groupname| groupname_to_gid(groupname))
-            .chain(map.get("gids_enforced"))
+            .chain(self.gids_enforced.clone())
             .collect();
 
-        // TODO: I feel like this is a bit of a hack, but we want to use the
-        // value of `DEFAULT_GIDS_ENFORCED` if neither `gids_enforced` nor
-        // `groups_enforced` were specified in the plugin options. There's
-        // probably a cleaner way to do this, but at least this approach does
-        // work so we can always clean it up later.
-        if !map.contains_key("gids_enforced") && !map.contains_key("groups_enforced") {
-            // if neither of these is provided, the `gids_enforced` shouldn't
-            // have any elements inside it
-            debug_assert!(gids_enforced.is_empty());
-
-            for gid in &DEFAULT_GIDS_ENFORCED {
-                let _ = gids_enforced.insert(*gid);
-            }
+        if gids_enforced.is_empty() {
+            return DEFAULT_GIDS_ENFORCED.iter().cloned().collect()
         }
 
-        let groups_exempted : HashSet<String> = map.get("groups_exempted")
-            .unwrap_or_default();
+        gids_enforced
+    }
 
-        let gids_exempted : HashSet<gid_t> = groups_exempted
+    fn gids_exempted(&self) -> HashSet<gid_t> {
+        self.groups_exempted
             .iter()
             .filter_map(|groupname| groupname_to_gid(groupname))
-            .chain(map.get("gids_exempted"))
-            .collect();
+            .chain(self.gids_exempted.clone())
+            .collect()
+    }
+}
 
+// TODO: single_use_lifetimes was committed, but I'm not sure there's
+// actually a way to satisfy the linter for the time being
+#[allow(single_use_lifetimes)]
+impl<'a> From<&'a OptionMap> for PluginOptions {
+    fn from(map: &'a OptionMap) -> Self {
         Self {
             binary_path: map.get("binary_path")
                 .unwrap_or_else(|_| DEFAULT_BINARY_PATH.into()),
@@ -655,8 +646,17 @@ impl<'a> From<&'a OptionMap> for PluginOptions {
             socket_dir: map.get("socket_dir")
                 .unwrap_or_else(|_| DEFAULT_SOCKET_DIR.into()),
 
-            gids_enforced,
-            gids_exempted,
+            gids_enforced: map.get("gids_enforced")
+                .unwrap_or_default(),
+
+            groups_enforced: map.get("groups_enforced")
+                .unwrap_or_default(),
+
+            gids_exempted: map.get("gids_exempted")
+                .unwrap_or_default(),
+
+            groups_exempted: map.get("groups_exempted")
+                .unwrap_or_default(),
         }
     }
 }