Fix groups with duplicate GIDs are not returned by get_group_list

changelog/62377.fixed

@@ -0,0 +1 @@
+Fix groups with duplicate GIDs are not returned by get_group_list

salt/modules/aix_group.py

@@ -33,7 +33,7 @@ def __virtual__():
     )
 
 
-def add(name, gid=None, system=False, root=None):
+def add(name, gid=None, system=False, root=None, **kwargs):
     """
     Add the specified group
 

salt/modules/groupadd.py

@@ -40,8 +40,10 @@ def __virtual__():
     )
 
 
-def add(name, gid=None, system=False, root=None):
+def add(name, gid=None, system=False, root=None, non_unique=False):
     """
+    .. versionchanged:: 3006.0
+
     Add the specified group
 
     name
@@ -56,6 +58,11 @@ def add(name, gid=None, system=False, root=None):
     root
         Directory to chroot into
 
+    non_unique
+        Allow creating groups with duplicate (non-unique) GIDs
+
+        .. versionadded:: 3006.0
+
     CLI Example:
 
     .. code-block:: bash
@@ -65,6 +72,8 @@ def add(name, gid=None, system=False, root=None):
     cmd = ["groupadd"]
     if gid:
         cmd.append("-g {}".format(gid))
+        if non_unique:
+            cmd.append("-o")
     if system and __grains__["kernel"] != "OpenBSD":
         cmd.append("-r")
 
@@ -200,8 +209,10 @@ def _chattrib(name, key, value, param, root=None):
     return info(name, root=root).get(key) == value
 
 
-def chgid(name, gid, root=None):
+def chgid(name, gid, root=None, non_unique=False):
     """
+    .. versionchanged:: 3006.0
+
     Change the gid for a named group
 
     name
@@ -213,13 +224,21 @@ def chgid(name, gid, root=None):
     root
         Directory to chroot into
 
+    non_unique
+        Allow modifying groups with duplicate (non-unique) GIDs
+
+        .. versionadded:: 3006.0
+
     CLI Example:
 
     .. code-block:: bash
 
         salt '*' group.chgid foo 4376
     """
-    return _chattrib(name, "gid", gid, "-g", root=root)
+    param = "-g"
+    if non_unique:
+        param = "-og"
+    return _chattrib(name, "gid", gid, param, root=root)
 
 
 def adduser(name, username, root=None):

salt/modules/mac_group.py

@@ -1,6 +1,7 @@
 """
 Manage groups on Mac OS 10.7+
 """
+import logging
 
 import salt.utils.functools
 import salt.utils.itertools
@@ -13,6 +14,8 @@
 except ImportError:
     pass
 
+log = logging.getLogger(__name__)
+
 
 # Define the module's virtual name
 __virtualname__ = "group"
@@ -35,8 +38,16 @@ def __virtual__():
 
 def add(name, gid=None, **kwargs):
     """
+    .. versionchanged:: 3006.0
+
     Add the specified group
 
+    name
+        Name of the new group
+
+    gid
+        Use GID for the new group
+
     CLI Example:
 
     .. code-block:: bash
@@ -55,6 +66,8 @@ def add(name, gid=None, **kwargs):
         )
     if gid is not None and not isinstance(gid, int):
         raise SaltInvocationError("gid must be an integer")
+    if "non_unique" in kwargs:
+        log.warning("The non_unique parameter is not supported on this platform.")
     # check if gid is already in use
     gid_list = _list_gids()
     if str(gid) in gid_list:

salt/modules/pw_group.py

@@ -38,8 +38,16 @@ def __virtual__():
 
 def add(name, gid=None, **kwargs):
     """
+    .. versionchanged:: 3006.0
+
     Add the specified group
 
+    name
+        Name of the new group
+
+    gid
+        Use GID for the new group
+
     CLI Example:
 
     .. code-block:: bash
@@ -49,6 +57,8 @@ def add(name, gid=None, **kwargs):
     kwargs = salt.utils.args.clean_kwargs(**kwargs)
     if salt.utils.data.is_true(kwargs.pop("system", False)):
         log.warning("pw_group module does not support the 'system' argument")
+    if "non_unique" in kwargs:
+        log.warning("The non_unique parameter is not supported on this platform.")
     if kwargs:
         log.warning("Invalid kwargs passed to group.add")
 

salt/states/group.py

@@ -100,8 +100,18 @@ def _changes(name, gid=None, addusers=None, delusers=None, members=None):
     return change
 
 
-def present(name, gid=None, system=False, addusers=None, delusers=None, members=None):
+def present(
+    name,
+    gid=None,
+    system=False,
+    addusers=None,
+    delusers=None,
+    members=None,
+    non_unique=False,
+):
     r"""
+    .. versionchanged:: 3006.0
+
     Ensure that a group is present
 
     Args:
@@ -131,6 +141,11 @@ def present(name, gid=None, system=False, addusers=None, delusers=None, members=
             Replace existing group members with a list of new members. Cannot be
             used in conjunction with addusers or delusers.
 
+        non_unique (bool):
+            Allow creating groups with duplicate (non-unique) GIDs
+
+            .. versionadded:: 3006.0
+
     Example:
 
     .. code-block:: yaml
@@ -190,7 +205,7 @@ def present(name, gid=None, system=False, addusers=None, delusers=None, members=
 
         for key, val in changes.items():
             if key == "gid":
-                __salt__["group.chgid"](name, gid)
+                __salt__["group.chgid"](name, gid, non_unique=non_unique)
                 continue
             if key == "addusers":
                 for user in val:
@@ -224,7 +239,7 @@ def present(name, gid=None, system=False, addusers=None, delusers=None, members=
 
         grps = __salt__["group.getent"]()
         # Test if gid is free
-        if gid is not None:
+        if gid is not None and not non_unique:
             gid_group = None
             for lgrp in grps:
                 if lgrp["gid"] == gid:
@@ -240,7 +255,7 @@ def present(name, gid=None, system=False, addusers=None, delusers=None, members=
                 return ret
 
         # Group is not present, make it.
-        if __salt__["group.add"](name, gid=gid, system=system):
+        if __salt__["group.add"](name, gid=gid, system=system, non_unique=non_unique):
             # if members to be added
             grp_members = None
             if members:

salt/utils/user.py

@@ -293,9 +293,11 @@ def get_group_list(user, include_default=True):
         # Try os.getgrouplist, available in python >= 3.3
         log.trace("Trying os.getgrouplist for '%s'", user)
         try:
+            user_group_list = os.getgrouplist(user, pwd.getpwnam(user).pw_gid)
             group_names = [
-                grp.getgrgid(grpid).gr_name
-                for grpid in os.getgrouplist(user, pwd.getpwnam(user).pw_gid)
+                _group.gr_name
+                for _group in grp.getgrall()
+                if _group.gr_gid in user_group_list
             ]
         except Exception:  # pylint: disable=broad-except
             pass

tests/pytests/functional/utils/user/test_get_group_list.py

@@ -0,0 +1,39 @@
+import logging
+
+import pytest
+
+import salt.utils.platform
+import salt.utils.user
+
+log = logging.getLogger(__name__)
+
+pytestmark = [
+    pytest.mark.destructive_test,
+    pytest.mark.skip_if_not_root,
+    pytest.mark.skip_on_windows,
+]
+
+
+@pytest.fixture(scope="module")
+def user():
+    with pytest.helpers.create_account(create_group=True) as _account:
+        yield _account
+
+
+@pytest.fixture(scope="module")
+def dupegroup(user):
+    grpid = user.group.info.gid
+    with pytest.helpers.create_group(
+        name="dupegroup", gid=grpid, members=user.username
+    ) as _group:
+        yield _group
+
+
+@pytest.mark.skipif(
+    salt.utils.platform.is_darwin() or salt.utils.platform.is_freebsd(),
+    reason="This test should not run on FreeBSD and Mac due to lack of duplicate GID support",
+)
+def test_get_group_list_with_duplicate_gid_group(user, dupegroup):
+    group_list = salt.utils.user.get_group_list(user.username)
+    assert user.group.info.name in group_list
+    assert dupegroup.name in group_list

tests/pytests/unit/states/test_group.py

@@ -0,0 +1,78 @@
+import pytest
+
+import salt.states.group as group
+from tests.support.mock import MagicMock, patch
+
+__context__ = {}
+
+
+def ping():
+    ...
+
+
+@pytest.fixture
+def configure_loader_modules():
+    return {group: {"__salt__": {"test.ping": ping}, "__opts__": {"test": False}}}
+
+
+def test_present_with_non_unique_gid():
+    with patch(
+        "salt.states.group._changes", MagicMock(side_effect=[False, {}, False])
+    ), patch.dict(
+        group.__salt__,
+        {"group.getent": MagicMock(side_effect=[[], [{"name": "salt", "gid": 1}]])},
+    ), patch.dict(
+        group.__salt__, {"group.add": MagicMock(return_value=True)}
+    ), patch.dict(
+        group.__salt__, {"group.info": MagicMock(return_value={"things": "stuff"})}
+    ):
+        ret = group.present("salt", gid=1, non_unique=True)
+        assert ret == {
+            "changes": {"things": "stuff"},
+            "comment": "New group salt created",
+            "name": "salt",
+            "result": True,
+        }
+        ret = group.present("salt", gid=1, non_unique=False)
+        assert ret == {
+            "changes": {},
+            "comment": "Group salt is not present but gid 1 is already taken by group salt",
+            "name": "salt",
+            "result": False,
+        }
+
+
+def test_present_with_existing_group_and_non_unique_gid():
+    with patch(
+        "salt.states.group._changes",
+        MagicMock(side_effect=[{"gid": 1}, {}, {"gid": 1}, {"gid": 1}]),
+    ), patch.dict(
+        group.__salt__,
+        {
+            "group.getent": MagicMock(
+                side_effect=[[{"name": "salt", "gid": 1}], [{"name": "salt", "gid": 1}]]
+            )
+        },
+    ), patch.dict(
+        group.__salt__, {"group.add": MagicMock(return_value=True)}
+    ), patch.dict(
+        group.__salt__, {"group.chgid": MagicMock(return_value=True)}
+    ), patch.dict(
+        group.__salt__, {"group.info": MagicMock(return_value={"things": "stuff"})}
+    ):
+        ret = group.present("salt", gid=1, non_unique=True)
+        assert ret == {
+            "changes": {"Final": "All changes applied successfully"},
+            "comment": "The following group attributes are set to be changed:\ngid: 1\n",
+            "name": "salt",
+            "result": True,
+        }
+        ret = group.present("salt", gid=1, non_unique=False)
+        assert ret == {
+            "changes": {"Failed": {"gid": 1}},
+            "comment": "The following group attributes are set to be changed:\n"
+            "gid: 1\n"
+            "Some changes could not be applied",
+            "name": "salt",
+            "result": False,
+        }

tests/support/pytest/helpers.py

@@ -192,6 +192,8 @@ def remove_stale_proxy_minion_cache_file(proxy_minion, minion_id=None):
 class TestGroup:
     sminion = attr.ib(repr=False)
     name = attr.ib()
+    gid = attr.ib(default=None)
+    members = attr.ib(default=None)
     _delete_group = attr.ib(init=False, repr=False, default=False)
 
     @sminion.default
@@ -209,12 +211,19 @@ def info(self):
     def __enter__(self):
         group = self.sminion.functions.group.info(self.name)
         if not group:
-            ret = self.sminion.functions.group.add(self.name)
+            ret = self.sminion.functions.group.add(
+                self.name, gid=self.gid, non_unique=True
+            )
             assert ret
             self._delete_group = True
             log.debug("Created system group: %s", self)
         else:
             log.debug("Reusing exising system group: %s", self)
+        if self.members:
+            ret = self.sminion.functions.group.members(
+                self.name, members_list=self.members
+            )
+            assert ret
         # Run tests
         return self
 
@@ -231,8 +240,8 @@ def __exit__(self, *_):
 
 @pytest.helpers.register
 @contextmanager
-def create_group(name=attr.NOTHING, sminion=attr.NOTHING):
-    with TestGroup(sminion=sminion, name=name) as group:
+def create_group(name=attr.NOTHING, sminion=attr.NOTHING, gid=None, members=None):
+    with TestGroup(sminion=sminion, name=name, gid=gid, members=members) as group:
         yield group