diff --git a/src/octoprint/filemanager/__init__.py b/src/octoprint/filemanager/__init__.py index ec4d1eea..0db223cc 100644 --- a/src/octoprint/filemanager/__init__.py +++ b/src/octoprint/filemanager/__init__.py @@ -427,7 +427,7 @@ class FileManager(object): pos=pos, date=time.time()) try: - with atomic_write(self._recovery_file) as f: + with atomic_write(self._recovery_file, max_permissions=0o666) as f: yaml.safe_dump(data, stream=f, default_flow_style=False, indent=" ", allow_unicode=True) except: self._logger.exception("Could not write recovery data to file {}".format(self._recovery_file)) diff --git a/src/octoprint/plugins/cura/__init__.py b/src/octoprint/plugins/cura/__init__.py index 0aa5cabc..bbce724f 100644 --- a/src/octoprint/plugins/cura/__init__.py +++ b/src/octoprint/plugins/cura/__init__.py @@ -391,7 +391,7 @@ class CuraPlugin(octoprint.plugin.SlicerPlugin, def _save_profile(self, path, profile, allow_overwrite=True): import yaml - with octoprint.util.atomic_write(path, "wb") as f: + with octoprint.util.atomic_write(path, "wb", max_permissions=0o666) as f: yaml.safe_dump(profile, f, default_flow_style=False, indent=" ", allow_unicode=True) def _convert_to_engine(self, profile_path, printer_profile, posX, posY): diff --git a/src/octoprint/plugins/softwareupdate/__init__.py b/src/octoprint/plugins/softwareupdate/__init__.py index f391ad72..03c6e324 100644 --- a/src/octoprint/plugins/softwareupdate/__init__.py +++ b/src/octoprint/plugins/softwareupdate/__init__.py @@ -129,7 +129,7 @@ class SoftwareUpdatePlugin(octoprint.plugin.BlueprintPlugin, octoprint_version = get_versions()["version"] self._version_cache["__version"] = octoprint_version - with atomic_write(self._version_cache_path) as file_obj: + with atomic_write(self._version_cache_path, max_permissions=0o666) as file_obj: yaml.safe_dump(self._version_cache, stream=file_obj, default_flow_style=False, indent=" ", allow_unicode=True) self._version_cache_dirty = False diff --git a/src/octoprint/printer/profile.py b/src/octoprint/printer/profile.py index 6102f523..52199c14 100644 --- a/src/octoprint/printer/profile.py +++ b/src/octoprint/printer/profile.py @@ -332,7 +332,7 @@ class PrinterProfileManager(object): from octoprint.util import atomic_write import yaml try: - with atomic_write(path, "wb") as f: + with atomic_write(path, "wb", max_permissions=0o666) as f: yaml.safe_dump(profile, f, default_flow_style=False, indent=" ", allow_unicode=True) except Exception as e: self._logger.exception("Error while trying to save profile %s" % profile["id"]) diff --git a/src/octoprint/server/util/flask.py b/src/octoprint/server/util/flask.py index 6ed17dcd..87c198b2 100644 --- a/src/octoprint/server/util/flask.py +++ b/src/octoprint/server/util/flask.py @@ -682,7 +682,7 @@ class PreemptiveCache(object): with self._lock: try: - with atomic_write(self.cachefile, "wb") as handle: + with atomic_write(self.cachefile, "wb", max_permissions=0o666) as handle: yaml.safe_dump(data, handle,default_flow_style=False, indent=" ", allow_unicode=True) except: self._logger.exception("Error while writing {}".format(self.cachefile)) diff --git a/src/octoprint/settings.py b/src/octoprint/settings.py index df8bb3f0..6076caf7 100644 --- a/src/octoprint/settings.py +++ b/src/octoprint/settings.py @@ -843,7 +843,7 @@ class Settings(object): from octoprint.util import atomic_write try: - with atomic_write(self._configfile, "wb", prefix="octoprint-config-", suffix=".yaml") as configFile: + with atomic_write(self._configfile, "wb", prefix="octoprint-config-", suffix=".yaml", permissions=0o600, max_permissions=0o666) as configFile: yaml.safe_dump(self._config, configFile, default_flow_style=False, indent=" ", allow_unicode=True) self._dirty = False except: @@ -1154,7 +1154,7 @@ class Settings(object): path, _ = os.path.split(filename) if not os.path.exists(path): os.makedirs(path) - with atomic_write(filename, "wb") as f: + with atomic_write(filename, "wb", max_permissions=0o666) as f: f.write(script) def _default_basedir(applicationName): diff --git a/src/octoprint/users.py b/src/octoprint/users.py index b6425dd7..11c23314 100644 --- a/src/octoprint/users.py +++ b/src/octoprint/users.py @@ -234,7 +234,7 @@ class FilebasedUserManager(UserManager): "settings": user._settings } - with atomic_write(self._userfile, "wb") as f: + with atomic_write(self._userfile, "wb", permissions=0o600, max_permissions=0o666) as f: yaml.safe_dump(data, f, default_flow_style=False, indent=" ", allow_unicode=True) self._dirty = False self._load() diff --git a/src/octoprint/util/__init__.py b/src/octoprint/util/__init__.py index f21a2006..97aa8a45 100644 --- a/src/octoprint/util/__init__.py +++ b/src/octoprint/util/__init__.py @@ -693,12 +693,17 @@ def address_for_client(host, port): @contextlib.contextmanager -def atomic_write(filename, mode="w+b", prefix="tmp", suffix=""): +def atomic_write(filename, mode="w+b", prefix="tmp", suffix="", permissions=0o644, max_permissions=0o777): + if os.path.exists(filename): + permissions |= os.stat(filename).st_mode + permissions &= max_permissions + temp_config = tempfile.NamedTemporaryFile(mode=mode, prefix=prefix, suffix=suffix, delete=False) try: yield temp_config finally: temp_config.close() + os.chmod(temp_config.name, permissions) shutil.move(temp_config.name, filename) diff --git a/tests/util/test_file_helpers.py b/tests/util/test_file_helpers.py index 160f776c..e0336488 100644 --- a/tests/util/test_file_helpers.py +++ b/tests/util/test_file_helpers.py @@ -83,13 +83,16 @@ class TestAtomicWrite(unittest.TestCase): @mock.patch("shutil.move") @mock.patch("tempfile.NamedTemporaryFile") - def test_atomic_write(self, mock_tempfile, mock_move): + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + def test_atomic_write(self, mock_exists, mock_chmod, mock_tempfile, mock_move): """Tests the regular basic "good" case.""" # setup mock_file = mock.MagicMock() mock_file.name = "tempfile.tmp" mock_tempfile.return_value = mock_file + mock_exists.return_value = False # test with octoprint.util.atomic_write("somefile.yaml") as f: @@ -99,11 +102,14 @@ class TestAtomicWrite(unittest.TestCase): mock_tempfile.assert_called_once_with(mode="w+b", prefix="tmp", suffix="", delete=False) mock_file.write.assert_called_once_with("test") mock_file.close.assert_called_once_with() + mock_chmod.assert_called_once_with("tempfile.tmp", 0o644) mock_move.assert_called_once_with("tempfile.tmp", "somefile.yaml") @mock.patch("shutil.move") @mock.patch("tempfile.NamedTemporaryFile") - def test_atomic_write_error_on_write(self, mock_tempfile, mock_move): + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + def test_atomic_write_error_on_write(self, mock_exists, mock_chmod, mock_tempfile, mock_move): """Tests the error case where something in the wrapped code fails.""" # setup @@ -111,6 +117,7 @@ class TestAtomicWrite(unittest.TestCase): mock_file.name = "tempfile.tmp" mock_file.write.side_effect = RuntimeError() mock_tempfile.return_value = mock_file + mock_exists.return_value = False # test try: @@ -124,16 +131,20 @@ class TestAtomicWrite(unittest.TestCase): mock_tempfile.assert_called_once_with(mode="w+b", prefix="tmp", suffix="", delete=False) mock_file.close.assert_called_once_with() self.assertFalse(mock_move.called) + self.assertFalse(mock_chmod.called) @mock.patch("shutil.move") @mock.patch("tempfile.NamedTemporaryFile") - def test_atomic_write_error_on_move(self, mock_tempfile, mock_move): + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + def test_atomic_write_error_on_move(self, mock_exists, mock_chmod, mock_tempfile, mock_move): """Tests the error case where the final move fails.""" # setup mock_file = mock.MagicMock() mock_file.name = "tempfile.tmp" mock_tempfile.return_value = mock_file mock_move.side_effect = RuntimeError() + mock_exists.return_value = False # test try: @@ -147,16 +158,20 @@ class TestAtomicWrite(unittest.TestCase): mock_tempfile.assert_called_once_with(mode="w+b", prefix="tmp", suffix="", delete=False) mock_file.close.assert_called_once_with() self.assertTrue(mock_move.called) + self.assertTrue(mock_chmod.called) @mock.patch("shutil.move") @mock.patch("tempfile.NamedTemporaryFile") - def test_atomic_write_parameters(self, mock_tempfile, mock_move): + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + def test_atomic_write_parameters(self, mock_exists, mock_chmod, mock_tempfile, mock_move): """Tests that the open parameters are propagated properly.""" # setup mock_file = mock.MagicMock() mock_file.name = "tempfile.tmp" mock_tempfile.return_value = mock_file + mock_exists.return_value = False # test with octoprint.util.atomic_write("somefile.yaml", mode="w", prefix="foo", suffix="bar") as f: @@ -165,6 +180,87 @@ class TestAtomicWrite(unittest.TestCase): # assert mock_tempfile.assert_called_once_with(mode="w", prefix="foo", suffix="bar", delete=False) mock_file.close.assert_called_once_with() + mock_chmod.assert_called_once_with("tempfile.tmp", 0o644) + mock_move.assert_called_once_with("tempfile.tmp", "somefile.yaml") + + + @mock.patch("shutil.move") + @mock.patch("tempfile.NamedTemporaryFile") + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + def test_atomic_custom_permissions(self, mock_exists, mock_chmod, mock_tempfile, mock_move): + """Tests that custom permissions may be set.""" + + # setup + mock_file = mock.MagicMock() + mock_file.name = "tempfile.tmp" + mock_tempfile.return_value = mock_file + mock_exists.return_value = False + + # test + with octoprint.util.atomic_write("somefile.yaml", permissions=0o755) as f: + f.write("test") + + # assert + mock_tempfile.assert_called_once_with(mode="w+b", prefix="tmp", suffix="", delete=False) + mock_file.close.assert_called_once_with() + mock_chmod.assert_called_once_with("tempfile.tmp", 0o755) + mock_move.assert_called_once_with("tempfile.tmp", "somefile.yaml") + + @mock.patch("shutil.move") + @mock.patch("tempfile.NamedTemporaryFile") + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + @mock.patch("os.stat") + def test_atomic_permissions_combined(self, mock_stat, mock_exists, mock_chmod, mock_tempfile, mock_move): + """Tests that the permissions of an existing file are combined with the requested permissions.""" + + # setup + mock_file = mock.MagicMock() + mock_file.name = "tempfile.tmp" + mock_tempfile.return_value = mock_file + mock_exists.return_value = True + + mock_stat_result = mock.MagicMock() + mock_stat_result.st_mode = 0o666 + mock_stat.return_value = mock_stat_result + + # test + with octoprint.util.atomic_write("somefile.yaml", permissions=0o755) as f: + f.write("test") + + # assert + mock_tempfile.assert_called_once_with(mode="w+b", prefix="tmp", suffix="", delete=False) + mock_file.close.assert_called_once_with() + mock_chmod.assert_called_once_with("tempfile.tmp", 0o777) # 0o755 | 0o666 + mock_move.assert_called_once_with("tempfile.tmp", "somefile.yaml") + + @mock.patch("shutil.move") + @mock.patch("tempfile.NamedTemporaryFile") + @mock.patch("os.chmod") + @mock.patch("os.path.exists") + @mock.patch("os.stat") + def test_atomic_permissions_limited(self, mock_stat, mock_exists, mock_chmod, mock_tempfile, mock_move): + """Tests that max_permissions limit the combined file permissions.""" + + # setup + mock_file = mock.MagicMock() + mock_file.name = "tempfile.tmp" + mock_tempfile.return_value = mock_file + mock_exists.return_value = True + + mock_stat_result = mock.MagicMock() + mock_stat_result.st_mode = 0o755 + mock_stat.return_value = mock_stat_result + + # test + with octoprint.util.atomic_write("somefile.yaml", permissions=0o600, max_permissions=0o666) as f: + f.write("test") + + # assert + mock_tempfile.assert_called_once_with(mode="w+b", prefix="tmp", suffix="", delete=False) + mock_file.close.assert_called_once_with() + mock_chmod.assert_called_once_with("tempfile.tmp", 0o644) # (0o600 | 0o755) & 0o666 = 0o755 & 0o666 mock_move.assert_called_once_with("tempfile.tmp", "somefile.yaml")