From e5748fdebb8f8c55580aa61c280b85fb879e3f35 Mon Sep 17 00:00:00 2001
From: Sebastiano Tocci <seba.tocci@gmail.com>
Date: Wed, 19 Jul 2023 01:44:51 +0200
Subject: [PATCH] Unix gid duplicate fix (#1876)

* added gid removal only when the gid is actually set and updated tests

---------

Signed-off-by: Sebastiano Tocci <seba.tocci@gmail.com>
---
 server/core/src/actors/v1_write.rs    | 18 ++++++++++++------
 server/lib/src/server/modify.rs       |  1 -
 server/testkit/tests/proto_v1_test.rs |  9 +++++++++
 3 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/server/core/src/actors/v1_write.rs b/server/core/src/actors/v1_write.rs
index bad1a4753..f8c3b5911 100644
--- a/server/core/src/actors/v1_write.rs
+++ b/server/core/src/actors/v1_write.rs
@@ -1,6 +1,5 @@
-use std::iter;
-use std::sync::Arc;
 use std::time::Duration;
+use std::{iter, sync::Arc};
 
 use kanidm_proto::v1::{
     AccountUnixExtend, CUIntentToken, CUSessionToken, CUStatus, CreateRequest, DeleteRequest,
@@ -1102,15 +1101,22 @@ impl QueryServerWriteV1 {
         gx: GroupUnixExtend,
         eventid: Uuid,
     ) -> Result<(), OperationError> {
-        // The filter_map here means we only create the mods if the gidnumber or shell are set
+        // The if let Some here means we only create the mods if the gidnumber is set
         // in the actual request.
+
+        let gidnumber_mods = if let Some(gid) = gx.gidnumber {
+            [
+                Some(Modify::Purged("gidnumber".into())),
+                Some(Modify::Present("gidnumber".into(), Value::new_uint32(gid))),
+            ]
+        } else {
+            [None, None]
+        };
         let mods: Vec<_> = iter::once(Some(Modify::Present(
             "class".into(),
             Value::new_class("posixgroup"),
         )))
-        .chain(iter::once(gx.gidnumber.map(|n| {
-            Modify::Present("gidnumber".into(), Value::new_uint32(n))
-        })))
+        .chain(gidnumber_mods)
         .flatten()
         .collect();
 
diff --git a/server/lib/src/server/modify.rs b/server/lib/src/server/modify.rs
index 8b9aeeaa8..1cbc0d34e 100644
--- a/server/lib/src/server/modify.rs
+++ b/server/lib/src/server/modify.rs
@@ -121,7 +121,6 @@ impl<'a> QueryServerWriteTransaction<'a> {
         }
 
         // Pre mod plugins
-        // We should probably supply the pre-post cands here.
         Plugins::run_pre_modify(self, &pre_candidates, &mut candidates, me).map_err(|e| {
             admin_error!("Pre-Modify operation failed (plugin), {:?}", e);
             e
diff --git a/server/testkit/tests/proto_v1_test.rs b/server/testkit/tests/proto_v1_test.rs
index c69fe1761..0ec46416d 100644
--- a/server/testkit/tests/proto_v1_test.rs
+++ b/server/testkit/tests/proto_v1_test.rs
@@ -532,6 +532,15 @@ async fn test_server_rest_posix_lifecycle(rsclient: KanidmClient) {
         .idm_group_unix_extend("posix_group", None)
         .await
         .unwrap();
+    // here we check that we can successfully change the gid without breaking anything
+
+    let res = rsclient
+        .idm_group_unix_extend("posix_group", Some(123123))
+        .await;
+    assert!(res.is_ok());
+
+    let res = rsclient.idm_group_unix_extend("posix_group", None).await;
+    assert!(res.is_ok());
 
     // Open a new connection as anonymous
     let res = rsclient.auth_anonymous().await;