fixes saltstack/salt#62657 groupadd.* functions hard code relative command name

changelog/62657.fixed

@@ -0,0 +1 @@
+Fix groupadd.* functions hard code relative command name

salt/modules/groupadd.py

@@ -13,7 +13,9 @@ import logging
 import os
 
 import salt.utils.files
+import salt.utils.path
 import salt.utils.stringutils
+from salt.exceptions import CommandExecutionError
 
 try:
     import grp
@@ -40,6 +42,16 @@ def __virtual__():
     )
 
 
+def _which(cmd):
+    """
+    Utility function wrapper to error out early if a command is not found
+    """
+    _cmd = salt.utils.path.which(cmd)
+    if not _cmd:
+        raise CommandExecutionError("Command '{}' cannot be found".format(cmd))
+    return _cmd
+
+
 def add(name, gid=None, system=False, root=None, non_unique=False):
     """
     .. versionchanged:: 3006.0
@@ -69,7 +81,7 @@ def add(name, gid=None, system=False, root=None, non_unique=False):
 
         salt '*' group.add foo 3456
     """
-    cmd = ["groupadd"]
+    cmd = [_which("groupadd")]
     if gid:
         cmd.append("-g {}".format(gid))
         if non_unique:
@@ -103,7 +115,7 @@ def delete(name, root=None):
 
         salt '*' group.delete foo
     """
-    cmd = ["groupdel"]
+    cmd = [_which("groupdel")]
 
     if root is not None:
         cmd.extend(("-R", root))
@@ -198,7 +210,7 @@ def _chattrib(name, key, value, param, root=None):
     if value == pre_info[key]:
         return True
 
-    cmd = ["groupmod"]
+    cmd = [_which("groupmod")]
 
     if root is not None:
         cmd.extend(("-R", root))
@@ -274,15 +286,15 @@ def adduser(name, username, root=None):
 
     if __grains__["kernel"] == "Linux":
         if on_redhat_5:
-            cmd = ["gpasswd", "-a", username, name]
+            cmd = [_which("gpasswd"), "-a", username, name]
         elif on_suse_11:
-            cmd = ["usermod", "-A", name, username]
+            cmd = [_which("usermod"), "-A", name, username]
         else:
-            cmd = ["gpasswd", "--add", username, name]
+            cmd = [_which("gpasswd"), "--add", username, name]
         if root is not None:
             cmd.extend(("--root", root))
     else:
-        cmd = ["usermod", "-G", name, username]
+        cmd = [_which("usermod"), "-G", name, username]
         if root is not None:
             cmd.extend(("-R", root))
 
@@ -327,11 +339,11 @@ def deluser(name, username, root=None):
         if username in grp_info["members"]:
             if __grains__["kernel"] == "Linux":
                 if on_redhat_5:
-                    cmd = ["gpasswd", "-d", username, name]
+                    cmd = [_which("gpasswd"), "-d", username, name]
                 elif on_suse_11:
-                    cmd = ["usermod", "-R", name, username]
+                    cmd = [_which("usermod"), "-R", name, username]
                 else:
-                    cmd = ["gpasswd", "--del", username, name]
+                    cmd = [_which("gpasswd"), "--del", username, name]
                 if root is not None:
                     cmd.extend(("--root", root))
                 retcode = __salt__["cmd.retcode"](cmd, python_shell=False)
@@ -339,7 +351,7 @@ def deluser(name, username, root=None):
                 out = __salt__["cmd.run_stdout"](
                     "id -Gn {}".format(username), python_shell=False
                 )
-                cmd = ["usermod", "-S"]
+                cmd = [_which("usermod"), "-S"]
                 cmd.append(",".join([g for g in out.split() if g != str(name)]))
                 cmd.append("{}".format(username))
                 retcode = __salt__["cmd.retcode"](cmd, python_shell=False)
@@ -386,15 +398,16 @@ def members(name, members_list, root=None):
 
     if __grains__["kernel"] == "Linux":
         if on_redhat_5:
-            cmd = ["gpasswd", "-M", members_list, name]
+            cmd = [_which("gpasswd"), "-M", members_list, name]
         elif on_suse_11:
             for old_member in __salt__["group.info"](name).get("members"):
                 __salt__["cmd.run"](
-                    "groupmod -R {} {}".format(old_member, name), python_shell=False
+                    "{} -R {} {}".format(_which("groupmod"), old_member, name),
+                    python_shell=False,
                 )
-            cmd = ["groupmod", "-A", members_list, name]
+            cmd = [_which("groupmod"), "-A", members_list, name]
         else:
-            cmd = ["gpasswd", "--members", members_list, name]
+            cmd = [_which("gpasswd"), "--members", members_list, name]
         if root is not None:
             cmd.extend(("--root", root))
         retcode = __salt__["cmd.retcode"](cmd, python_shell=False)
@@ -402,14 +415,17 @@ def members(name, members_list, root=None):
         retcode = 1
         grp_info = __salt__["group.info"](name)
         if grp_info and name in grp_info["name"]:
-            __salt__["cmd.run"]("groupdel {}".format(name), python_shell=False)
             __salt__["cmd.run"](
-                "groupadd -g {} {}".format(grp_info["gid"], name), python_shell=False
+                "{} {}".format(_which("groupdel"), name), python_shell=False
+            )
+            __salt__["cmd.run"](
+                "{} -g {} {}".format(_which("groupadd"), grp_info["gid"], name),
+                python_shell=False,
             )
             for user in members_list.split(","):
                 if user:
                     retcode = __salt__["cmd.retcode"](
-                        ["usermod", "-G", name, user], python_shell=False
+                        [_which("usermod"), "-G", name, user], python_shell=False
                     )
                     if not retcode == 0:
                         break

tests/pytests/unit/modules/test_groupadd.py

@@ -0,0 +1,271 @@
+import pytest
+
+import salt.modules.groupadd as groupadd
+from tests.support.mock import MagicMock, patch
+
+try:
+    import grp
+except ImportError:
+    pass
+
+
+pytestmark = [
+    pytest.mark.skip_on_windows,
+]
+
+
+@pytest.fixture
+def configure_loader_modules():
+    return {groupadd: {}}
+
+
+def test_add():
+    """
+    Tests if specified group was added
+    """
+    mock = MagicMock(return_value={"retcode": 0})
+    with patch.dict(groupadd.__salt__, {"cmd.run_all": mock}):
+        assert groupadd.add("test", 100) is True
+
+    with patch.dict(groupadd.__grains__, {"kernel": "Linux"}):
+        with patch.dict(groupadd.__salt__, {"cmd.run_all": mock}):
+            assert groupadd.add("test", 100, True) is True
+
+
+def test_info():
+    """
+    Tests the return of group information
+    """
+    getgrnam = grp.struct_group(("foo", "*", 20, ["test"]))
+    with patch("grp.getgrnam", MagicMock(return_value=getgrnam)):
+        ret = {"passwd": "*", "gid": 20, "name": "foo", "members": ["test"]}
+        assert groupadd.info("foo") == ret
+
+
+def test_format_info():
+    """
+    Tests the formatting of returned group information
+    """
+    group = {"passwd": "*", "gid": 0, "name": "test", "members": ["root"]}
+    with patch("salt.modules.groupadd._format_info", MagicMock(return_value=group)):
+        data = grp.struct_group(("wheel", "*", 0, ["root"]))
+        ret = {"passwd": "*", "gid": 0, "name": "test", "members": ["root"]}
+        assert groupadd._format_info(data) == ret
+
+
+def test_getent():
+    """
+    Tests the return of information on all groups
+    """
+    getgrnam = grp.struct_group(("foo", "*", 20, ["test"]))
+    with patch("grp.getgrall", MagicMock(return_value=[getgrnam])):
+        ret = [{"passwd": "*", "gid": 20, "name": "foo", "members": ["test"]}]
+        assert groupadd.getent() == ret
+
+
+def test_chgid_gid_same():
+    """
+    Tests if the group id is the same as argument
+    """
+    mock = MagicMock(return_value={"gid": 10})
+    with patch.object(groupadd, "info", mock):
+        assert groupadd.chgid("test", 10) is True
+
+
+def test_chgid():
+    """
+    Tests the gid for a named group was changed
+    """
+    mock = MagicMock(return_value=None)
+    with patch.dict(groupadd.__salt__, {"cmd.run": mock}):
+        mock = MagicMock(side_effect=[{"gid": 10}, {"gid": 500}])
+        with patch.object(groupadd, "info", mock):
+            assert groupadd.chgid("test", 500) is True
+
+
+def test_delete():
+    """
+    Tests if the specified group was deleted
+    """
+    mock_ret = MagicMock(return_value={"retcode": 0})
+    with patch.dict(groupadd.__salt__, {"cmd.run_all": mock_ret}):
+        assert groupadd.delete("test") is True
+
+
+def test_adduser():
+    """
+    Tests if specified user gets added in the group.
+    """
+    os_version_list = [
+        {
+            "grains": {
+                "kernel": "Linux",
+                "os_family": "RedHat",
+                "osmajorrelease": "5",
+            },
+            "cmd": ["/bin/gpasswd", "-a", "root", "test"],
+        },
+        {
+            "grains": {
+                "kernel": "Linux",
+                "os_family": "Suse",
+                "osmajorrelease": "11",
+            },
+            "cmd": ["/bin/usermod", "-A", "test", "root"],
+        },
+        {
+            "grains": {"kernel": "Linux"},
+            "cmd": ["/bin/gpasswd", "--add", "root", "test"],
+        },
+        {
+            "grains": {"kernel": "OTHERKERNEL"},
+            "cmd": ["/bin/usermod", "-G", "test", "root"],
+        },
+    ]
+    with patch(
+        "salt.utils.path.which",
+        MagicMock(
+            side_effect=["/bin/gpasswd", "/bin/usermod", "/bin/gpasswd", "/bin/usermod"]
+        ),
+    ):
+        for os_version in os_version_list:
+            mock = MagicMock(return_value={"retcode": 0})
+            with patch.dict(groupadd.__grains__, os_version["grains"]), patch.dict(
+                groupadd.__salt__, {"cmd.retcode": mock}
+            ):
+                assert groupadd.adduser("test", "root") is False
+                groupadd.__salt__["cmd.retcode"].assert_called_once_with(
+                    os_version["cmd"], python_shell=False
+                )
+
+
+def test_deluser():
+    """
+    Tests if specified user gets deleted from the group.
+    """
+    os_version_list = [
+        {
+            "grains": {
+                "kernel": "Linux",
+                "os_family": "RedHat",
+                "osmajorrelease": "5",
+            },
+            "cmd": ["/bin/gpasswd", "-d", "root", "test"],
+        },
+        {
+            "grains": {
+                "kernel": "Linux",
+                "os_family": "Suse",
+                "osmajorrelease": "11",
+            },
+            "cmd": ["/bin/usermod", "-R", "test", "root"],
+        },
+        {
+            "grains": {"kernel": "Linux"},
+            "cmd": ["/bin/gpasswd", "--del", "root", "test"],
+        },
+        {"grains": {"kernel": "OpenBSD"}, "cmd": ["/bin/usermod", "-S", "foo", "root"]},
+    ]
+
+    with patch(
+        "salt.utils.path.which",
+        MagicMock(
+            side_effect=["/bin/gpasswd", "/bin/usermod", "/bin/gpasswd", "/bin/usermod"]
+        ),
+    ):
+        for os_version in os_version_list:
+            mock_retcode = MagicMock(return_value=0)
+            mock_stdout = MagicMock(return_value="test foo")
+            mock_info = MagicMock(
+                return_value={
+                    "passwd": "*",
+                    "gid": 0,
+                    "name": "test",
+                    "members": ["root"],
+                }
+            )
+
+            with patch.dict(groupadd.__grains__, os_version["grains"]):
+                with patch.dict(
+                    groupadd.__salt__,
+                    {
+                        "cmd.retcode": mock_retcode,
+                        "group.info": mock_info,
+                        "cmd.run_stdout": mock_stdout,
+                    },
+                ):
+                    assert groupadd.deluser("test", "root") is True
+                    groupadd.__salt__["cmd.retcode"].assert_called_once_with(
+                        os_version["cmd"], python_shell=False
+                    )
+
+
+def test_members():
+    """
+    Tests if members of the group, get replaced with a provided list.
+    """
+    os_version_list = [
+        {
+            "grains": {
+                "kernel": "Linux",
+                "os_family": "RedHat",
+                "osmajorrelease": "5",
+            },
+            "cmd": ["/bin/gpasswd", "-M", "foo", "test"],
+        },
+        {
+            "grains": {
+                "kernel": "Linux",
+                "os_family": "Suse",
+                "osmajorrelease": "11",
+            },
+            "cmd": ["/bin/groupmod", "-A", "foo", "test"],
+        },
+        {
+            "grains": {"kernel": "Linux"},
+            "cmd": ["/bin/gpasswd", "--members", "foo", "test"],
+        },
+        {"grains": {"kernel": "OpenBSD"}, "cmd": ["/bin/usermod", "-G", "test", "foo"]},
+    ]
+
+    with patch(
+        "salt.utils.path.which",
+        MagicMock(
+            side_effect=[
+                "/bin/gpasswd",
+                "/bin/gpasswd",
+                "/bin/groupmod",
+                "/bin/gpasswd",
+                "/bin/groupdel",
+                "/bin/groupadd",
+                "/bin/usermod",
+            ]
+        ),
+    ):
+        for os_version in os_version_list:
+            mock_ret = MagicMock(return_value={"retcode": 0})
+            mock_stdout = MagicMock(return_value={"cmd.run_stdout": 1})
+            mock_info = MagicMock(
+                return_value={
+                    "passwd": "*",
+                    "gid": 0,
+                    "name": "test",
+                    "members": ["root"],
+                }
+            )
+            mock = MagicMock(return_value=True)
+
+            with patch.dict(groupadd.__grains__, os_version["grains"]):
+                with patch.dict(
+                    groupadd.__salt__,
+                    {
+                        "cmd.retcode": mock_ret,
+                        "group.info": mock_info,
+                        "cmd.run_stdout": mock_stdout,
+                        "cmd.run": mock,
+                    },
+                ):
+                    assert groupadd.members("test", "foo") is False
+                    groupadd.__salt__["cmd.retcode"].assert_called_once_with(
+                        os_version["cmd"], python_shell=False
+                    )

tests/unit/modules/test_groupadd.py

@@ -1,259 +0,0 @@
-"""
-    :codeauthor: Jayesh Kariya <jayeshk@saltstack.com>
-"""
-
-
-import salt.modules.groupadd as groupadd
-import salt.utils.platform
-from tests.support.mixins import LoaderModuleMockMixin
-from tests.support.mock import MagicMock, patch
-from tests.support.unit import TestCase, skipIf
-
-try:
-    import grp
-except ImportError:
-    pass
-
-
-@skipIf(salt.utils.platform.is_windows(), "Module not available on Windows")
-class GroupAddTestCase(TestCase, LoaderModuleMockMixin):
-    """
-    TestCase for salt.modules.groupadd
-    """
-
-    def setup_loader_modules(self):
-        return {groupadd: {}}
-
-    # 'add' function tests: 1
-
-    def test_add(self):
-        """
-        Tests if specified group was added
-        """
-        mock = MagicMock(return_value={"retcode": 0})
-        with patch.dict(groupadd.__salt__, {"cmd.run_all": mock}):
-            self.assertTrue(groupadd.add("test", 100))
-
-        with patch.dict(groupadd.__grains__, {"kernel": "Linux"}):
-            with patch.dict(groupadd.__salt__, {"cmd.run_all": mock}):
-                self.assertTrue(groupadd.add("test", 100, True))
-
-    # 'info' function tests: 1
-
-    def test_info(self):
-        """
-        Tests the return of group information
-        """
-        getgrnam = grp.struct_group(("foo", "*", 20, ["test"]))
-        with patch("grp.getgrnam", MagicMock(return_value=getgrnam)):
-            ret = {"passwd": "*", "gid": 20, "name": "foo", "members": ["test"]}
-            self.assertEqual(groupadd.info("foo"), ret)
-
-    # '_format_info' function tests: 1
-
-    def test_format_info(self):
-        """
-        Tests the formatting of returned group information
-        """
-        group = {"passwd": "*", "gid": 0, "name": "test", "members": ["root"]}
-        with patch("salt.modules.groupadd._format_info", MagicMock(return_value=group)):
-            data = grp.struct_group(("wheel", "*", 0, ["root"]))
-            ret = {"passwd": "*", "gid": 0, "name": "test", "members": ["root"]}
-            self.assertDictEqual(groupadd._format_info(data), ret)
-
-    # 'getent' function tests: 1
-
-    def test_getent(self):
-        """
-        Tests the return of information on all groups
-        """
-        getgrnam = grp.struct_group(("foo", "*", 20, ["test"]))
-        with patch("grp.getgrall", MagicMock(return_value=[getgrnam])):
-            ret = [{"passwd": "*", "gid": 20, "name": "foo", "members": ["test"]}]
-            self.assertEqual(groupadd.getent(), ret)
-
-    # 'chgid' function tests: 2
-
-    def test_chgid_gid_same(self):
-        """
-        Tests if the group id is the same as argument
-        """
-        mock = MagicMock(return_value={"gid": 10})
-        with patch.object(groupadd, "info", mock):
-            self.assertTrue(groupadd.chgid("test", 10))
-
-    def test_chgid(self):
-        """
-        Tests the gid for a named group was changed
-        """
-        mock = MagicMock(return_value=None)
-        with patch.dict(groupadd.__salt__, {"cmd.run": mock}):
-            mock = MagicMock(side_effect=[{"gid": 10}, {"gid": 500}])
-            with patch.object(groupadd, "info", mock):
-                self.assertTrue(groupadd.chgid("test", 500))
-
-    # 'delete' function tests: 1
-
-    def test_delete(self):
-        """
-        Tests if the specified group was deleted
-        """
-        mock_ret = MagicMock(return_value={"retcode": 0})
-        with patch.dict(groupadd.__salt__, {"cmd.run_all": mock_ret}):
-            self.assertTrue(groupadd.delete("test"))
-
-    # 'adduser' function tests: 1
-
-    def test_adduser(self):
-        """
-        Tests if specified user gets added in the group.
-        """
-        os_version_list = [
-            {
-                "grains": {
-                    "kernel": "Linux",
-                    "os_family": "RedHat",
-                    "osmajorrelease": "5",
-                },
-                "cmd": ["gpasswd", "-a", "root", "test"],
-            },
-            {
-                "grains": {
-                    "kernel": "Linux",
-                    "os_family": "Suse",
-                    "osmajorrelease": "11",
-                },
-                "cmd": ["usermod", "-A", "test", "root"],
-            },
-            {
-                "grains": {"kernel": "Linux"},
-                "cmd": ["gpasswd", "--add", "root", "test"],
-            },
-            {
-                "grains": {"kernel": "OTHERKERNEL"},
-                "cmd": ["usermod", "-G", "test", "root"],
-            },
-        ]
-
-        for os_version in os_version_list:
-            mock = MagicMock(return_value={"retcode": 0})
-            with patch.dict(groupadd.__grains__, os_version["grains"]):
-                with patch.dict(groupadd.__salt__, {"cmd.retcode": mock}):
-                    self.assertFalse(groupadd.adduser("test", "root"))
-                    groupadd.__salt__["cmd.retcode"].assert_called_once_with(
-                        os_version["cmd"], python_shell=False
-                    )
-
-    # 'deluser' function tests: 1
-
-    def test_deluser(self):
-        """
-        Tests if specified user gets deleted from the group.
-        """
-        os_version_list = [
-            {
-                "grains": {
-                    "kernel": "Linux",
-                    "os_family": "RedHat",
-                    "osmajorrelease": "5",
-                },
-                "cmd": ["gpasswd", "-d", "root", "test"],
-            },
-            {
-                "grains": {
-                    "kernel": "Linux",
-                    "os_family": "Suse",
-                    "osmajorrelease": "11",
-                },
-                "cmd": ["usermod", "-R", "test", "root"],
-            },
-            {
-                "grains": {"kernel": "Linux"},
-                "cmd": ["gpasswd", "--del", "root", "test"],
-            },
-            {"grains": {"kernel": "OpenBSD"}, "cmd": ["usermod", "-S", "foo", "root"]},
-        ]
-
-        for os_version in os_version_list:
-            mock_retcode = MagicMock(return_value=0)
-            mock_stdout = MagicMock(return_value="test foo")
-            mock_info = MagicMock(
-                return_value={
-                    "passwd": "*",
-                    "gid": 0,
-                    "name": "test",
-                    "members": ["root"],
-                }
-            )
-
-            with patch.dict(groupadd.__grains__, os_version["grains"]):
-                with patch.dict(
-                    groupadd.__salt__,
-                    {
-                        "cmd.retcode": mock_retcode,
-                        "group.info": mock_info,
-                        "cmd.run_stdout": mock_stdout,
-                    },
-                ):
-                    self.assertTrue(groupadd.deluser("test", "root"))
-                    groupadd.__salt__["cmd.retcode"].assert_called_once_with(
-                        os_version["cmd"], python_shell=False
-                    )
-
-    # 'deluser' function tests: 1
-
-    def test_members(self):
-        """
-        Tests if members of the group, get replaced with a provided list.
-        """
-        os_version_list = [
-            {
-                "grains": {
-                    "kernel": "Linux",
-                    "os_family": "RedHat",
-                    "osmajorrelease": "5",
-                },
-                "cmd": ["gpasswd", "-M", "foo", "test"],
-            },
-            {
-                "grains": {
-                    "kernel": "Linux",
-                    "os_family": "Suse",
-                    "osmajorrelease": "11",
-                },
-                "cmd": ["groupmod", "-A", "foo", "test"],
-            },
-            {
-                "grains": {"kernel": "Linux"},
-                "cmd": ["gpasswd", "--members", "foo", "test"],
-            },
-            {"grains": {"kernel": "OpenBSD"}, "cmd": ["usermod", "-G", "test", "foo"]},
-        ]
-
-        for os_version in os_version_list:
-            mock_ret = MagicMock(return_value={"retcode": 0})
-            mock_stdout = MagicMock(return_value={"cmd.run_stdout": 1})
-            mock_info = MagicMock(
-                return_value={
-                    "passwd": "*",
-                    "gid": 0,
-                    "name": "test",
-                    "members": ["root"],
-                }
-            )
-            mock = MagicMock(return_value=True)
-
-            with patch.dict(groupadd.__grains__, os_version["grains"]):
-                with patch.dict(
-                    groupadd.__salt__,
-                    {
-                        "cmd.retcode": mock_ret,
-                        "group.info": mock_info,
-                        "cmd.run_stdout": mock_stdout,
-                        "cmd.run": mock,
-                    },
-                ):
-                    self.assertFalse(groupadd.members("test", "foo"))
-                    groupadd.__salt__["cmd.retcode"].assert_called_once_with(
-                        os_version["cmd"], python_shell=False
-                    )