Extend atomic_write to allow setting & persisting file permissions

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))

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):

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

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"])

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))

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):

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()

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)
 
 

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")