Bug 62940 fix cron inconsistency in _write_cron_lines

changelog/62940.fixed

@@ -0,0 +1 @@
+Allow root user to modify crontab lines for non-root users (except AIX and Solaris). Align crontab line changes with the file ones and also with listing crontab.

salt/modules/cron.py

@@ -266,22 +266,34 @@ def _write_cron_lines(user, lines):
     """
     lines = [salt.utils.stringutils.to_str(_l) for _l in lines]
     path = salt.utils.files.mkstemp()
-    if _check_instance_uid_match("root") or __grains__.get("os_family") in (
-        "Solaris",
-        "AIX",
-    ):
-        # In some cases crontab command should be executed as user rather than root
+    # Some OS' do not support specifying user via the `crontab` command
+    if __grains__.get("os_family") in ("Solaris", "AIX"):
         with salt.utils.files.fpopen(
             path, "w+", uid=__salt__["file.user_to_uid"](user), mode=0o600
         ) as fp_:
             fp_.writelines(lines)
         ret = __salt__["cmd.run_all"](
             _get_cron_cmdstr(path), runas=user, python_shell=False
         )
-    else:
+    # If Salt is running from same user as requested in cron module we don't need any user switch
+    elif _check_instance_uid_match(user):
         with salt.utils.files.fpopen(path, "w+", mode=0o600) as fp_:
             fp_.writelines(lines)
         ret = __salt__["cmd.run_all"](_get_cron_cmdstr(path), python_shell=False)
+    # If Salt is running from root user it could modify any user's crontab
+    elif _check_instance_uid_match("root"):
+        with salt.utils.files.fpopen(path, "w+", mode=0o600) as fp_:
+            fp_.writelines(lines)
+        ret = __salt__["cmd.run_all"](_get_cron_cmdstr(path, user), python_shell=False)
+    # Edge cases here, let's try do a runas
+    else:
+        with salt.utils.files.fpopen(
+            path, "w+", uid=__salt__["file.user_to_uid"](user), mode=0o600
+        ) as fp_:
+            fp_.writelines(lines)
+        ret = __salt__["cmd.run_all"](
+            _get_cron_cmdstr(path), runas=user, python_shell=False
+        )
     os.remove(path)
     return ret
 

tests/unit/modules/test_cron.py

@@ -1421,10 +1421,11 @@ def test_rm_job_is_absent(self):
             ret = cron.rm_job("DUMMY_USER", "/bin/echo NOT A DROID", 1, 2, 3, 4, 5)
             self.assertEqual("absent", ret)
 
-    def test_write_cron_lines_root_rh(self):
+    def test_write_cron_lines_euid_match_user_rh(self):
         """
         Assert that _write_cron_lines() is called with the correct cron command and user
-        OS: RedHat. User: root. Expected to run with runas argument.
+        OS: RedHat. EUID match User (either root, root or user, user).
+        Expected to run without runas argument.
         """
         temp_path = "some_temp_path"
         crontab_cmd = "crontab {}".format(temp_path)
@@ -1436,35 +1437,32 @@ def test_write_cron_lines_root_rh(self):
             new=MagicMock(return_value=True),
         ), patch(
             "salt.utils.files.fpopen", mock_open()
-        ), patch.dict(
-            cron.__salt__, {"file.user_to_uid": MagicMock(return_value=1)}
         ), patch(
             "salt.utils.files.mkstemp", MagicMock(return_value=temp_path)
         ), patch(
             "os.remove", MagicMock()
         ):
             cron._write_cron_lines("root", "test 123")
             cron.__salt__["cmd.run_all"].assert_called_with(
-                crontab_cmd, python_shell=False, runas="root"
+                crontab_cmd, python_shell=False
             )
 
-    def test_write_cron_lines_non_root_rh(self):
+    def test_write_cron_lines_root_non_root_rh(self):
         """
         Assert that _write_cron_lines() is called with the correct cron command and user
-        OS: RedHat. User: non-root. Expected to run without runas argument.
+        OS: RedHat. EUID: root. User: non-root.
+        Expected to run without runas argument and with -u non-root argument.
         """
         temp_path = "some_temp_path"
-        crontab_cmd = "crontab {}".format(temp_path)
+        crontab_cmd = "crontab -u {} {}".format("non-root", temp_path)
 
         with patch.dict(cron.__grains__, {"os_family": "RedHat"}), patch.dict(
             cron.__salt__, {"cmd.run_all": MagicMock()}
         ), patch(
             "salt.modules.cron._check_instance_uid_match",
-            new=MagicMock(return_value=False),
+            new=MagicMock(side_effect=[False, True]),
         ), patch(
             "salt.utils.files.fpopen", mock_open()
-        ), patch.dict(
-            cron.__salt__, {"file.user_to_uid": MagicMock(return_value=1)}
         ), patch(
             "salt.utils.files.mkstemp", MagicMock(return_value=temp_path)
         ), patch(
@@ -1475,15 +1473,16 @@ def test_write_cron_lines_non_root_rh(self):
                 crontab_cmd, python_shell=False
             )
 
-    def test_write_cron_lines_non_root_aix(self):
+    def test_write_cron_lines_non_root_euid_doesnt_match_user_rh(self):
         """
         Assert that _write_cron_lines() is called with the correct cron command and user
-        OS: AIX. User: non-root. Expected to run with runas argument.
+        OS: RedHat. EUID: non-root. EUID doesn't match User.
+        Expected to run with runas argument.
         """
         temp_path = "some_temp_path"
         crontab_cmd = "crontab {}".format(temp_path)
 
-        with patch.dict(cron.__grains__, {"os_family": "AIX"}), patch.dict(
+        with patch.dict(cron.__grains__, {"os_family": "RedHat"}), patch.dict(
             cron.__salt__, {"cmd.run_all": MagicMock()}
         ), patch(
             "salt.modules.cron._check_instance_uid_match",
@@ -1502,22 +1501,41 @@ def test_write_cron_lines_non_root_aix(self):
                 crontab_cmd, python_shell=False, runas="non-root"
             )
 
-    def test_write_cron_lines_non_root_solaris(self):
+    def test_write_cron_lines_non_root_aix(self):
         """
         Assert that _write_cron_lines() is called with the correct cron command and user
-        OS: Solaris. User: non-root. Expected to run with runas argument.
+        OS: AIX. User: non-root.
+        Expected to run with runas argument.
         """
         temp_path = "some_temp_path"
         crontab_cmd = "crontab {}".format(temp_path)
 
         with patch.dict(cron.__grains__, {"os_family": "AIX"}), patch.dict(
             cron.__salt__, {"cmd.run_all": MagicMock()}
+        ), patch("salt.utils.files.fpopen", mock_open()), patch.dict(
+            cron.__salt__, {"file.user_to_uid": MagicMock(return_value=1)}
         ), patch(
-            "salt.modules.cron._check_instance_uid_match",
-            new=MagicMock(return_value=False),
+            "salt.utils.files.mkstemp", MagicMock(return_value=temp_path)
         ), patch(
-            "salt.utils.files.fpopen", mock_open()
-        ), patch.dict(
+            "os.remove", MagicMock()
+        ):
+            cron._write_cron_lines("non-root", "test 123")
+            cron.__salt__["cmd.run_all"].assert_called_with(
+                crontab_cmd, python_shell=False, runas="non-root"
+            )
+
+    def test_write_cron_lines_non_root_solaris(self):
+        """
+        Assert that _write_cron_lines() is called with the correct cron command and user
+        OS: Solaris. User: non-root.
+        Expected to run with runas argument.
+        """
+        temp_path = "some_temp_path"
+        crontab_cmd = "crontab {}".format(temp_path)
+
+        with patch.dict(cron.__grains__, {"os_family": "Solaris"}), patch.dict(
+            cron.__salt__, {"cmd.run_all": MagicMock()}
+        ), patch("salt.utils.files.fpopen", mock_open()), patch.dict(
             cron.__salt__, {"file.user_to_uid": MagicMock(return_value=1)}
         ), patch(
             "salt.utils.files.mkstemp", MagicMock(return_value=temp_path)