Fix write_to_file function (#336)

* Fix write_to_file function

Probably because of the usage of low level functions, as for example os.open and os.fdopen, it's harder to deal with buffering which can result into files with inconsistent content, which breaks the configuration and the hardware-exporter service.

After adding using high level functions and make the code simpler, the bug seems to be fixed

Closes: #305

src/service.py

@@ -277,25 +277,25 @@ def validate_exporter_configs(self) -> Tuple[bool, str]:
 
 
 def write_to_file(path: Path, content: str, mode: Optional[int] = None) -> bool:
-    """Write to file with provided content."""
-    success = True
+    """Write to file with provided content.
+
+    It's important to first set the permissions to then write the content because it might have
+    sensitive information like password.
+    """
     try:
-        logger.info("Writing file to %s.", path)
-        fileobj = (
-            os.fdopen(os.open(path, os.O_CREAT | os.O_WRONLY, mode), "w", encoding="utf-8")
-            if mode
-            # create file with default permissions based on default OS umask
-            else open(path, "w", encoding="utf-8")  # pylint: disable=consider-using-with
-        )
-        with fileobj as file:
+        path.touch()
+        if mode is not None:
+            os.chmod(path, mode)
+
+        with open(path, "w", encoding="utf-8") as file:
             file.write(content)
     except (NotADirectoryError, PermissionError) as err:
         logger.error(err)
         logger.info("Writing file to %s - Failed.", path)
-        success = False
-    else:
-        logger.info("Writing file to %s - Done.", path)
-    return success
+        return False
+
+    logger.info("Writing file to %s - Done.", path)
+    return True
 
 
 def remove_file(path: Path) -> bool:

tests/unit/test_service.py

@@ -783,22 +783,56 @@ def test_write_to_file_success(self, mock_os, mock_open):
 
         mock_open.assert_called_with(path, "w", encoding="utf-8")
         mock_file.write.assert_called_with(content)
+        mock_os.chmod.assert_not_called()
 
-    @mock.patch("service.os.open", new_callable=mock.mock_open)
-    @mock.patch("service.os.fdopen", new_callable=mock.mock_open)
+    def test_write_file_already_exist_changes_permission(self):
+        path = pathlib.Path(self.temp_file.name)
+        prior_permission = path.stat().st_mode & 0o777
+        new_permission = 0o666
+        self.assertNotEqual(prior_permission, new_permission)
+
+        service.write_to_file(path, "", new_permission)
+
+        after_permission = path.stat().st_mode & 0o777
+        self.assertEqual(after_permission, new_permission)
+
+    def test_write_file_changes_content(self):
+        path = pathlib.Path(self.temp_file.name)
+        content_before = (
+            "port: 10200\nlevel: DEBUG\ncollect_timeout: 60\n\nenable_collectors:\n  \n  "
+            "- collector.poweredge_raid\n  \n  - collector.ipmi_dcmi\n  \n  "
+            "- collector.ipmi_sensor\n  \n  - collector.ipmi_sel\n\n  - collector.redfish\n  "
+            "\n\nredfish_host: 'https://10.229.2.8'\nredfish_username: 'root'\n"
+            "redfish_password: '<redacted>'\nredfish_client_timeout: '60'\n"
+        )
+        with open(path, "w", encoding="utf-8") as file:
+            file.write(content_before)
+
+        # simulate the redfish disable option
+        content_after = (
+            "port: 10200\nlevel: DEBUG\ncollect_timeout: 60\n\nenable_collectors:\n  \n"
+            "  - collector.poweredge_raid\n  \n  - collector.ipmi_dcmi\n  "
+            "\n  - collector.ipmi_sensor\n  \n  - collector.ipmi_sel\n"
+        )
+
+        service.write_to_file(path, content_after)
+
+        self.assertEqual(path.read_text(), content_after)
+
+    @mock.patch("builtins.open", new_callable=mock.mock_open)
     @mock.patch("service.os")
-    def test_write_to_file_with_mode_success(self, mock_os, mock_fdopen, mock_open):
+    def test_write_to_file_with_mode_success(self, mock_os, mock_open):
         path = pathlib.Path(self.temp_file.name)
         content = "Hello, world!"
 
-        mock_file = mock_fdopen.return_value.__enter__.return_value
+        mock_file = mock_open.return_value.__enter__.return_value
 
         result = service.write_to_file(path, content, mode=0o600)
         self.assertTrue(result)
 
-        mock_open.assert_called_with(path, mock_os.O_CREAT | mock_os.O_WRONLY, 0o600)
-        mock_fdopen.assert_called_with(mock_open.return_value, "w", encoding="utf-8")
+        mock_open.assert_called_with(path, "w", encoding="utf-8")
         mock_file.write.assert_called_with(content)
+        mock_os.chmod.assert_called_with(path, 0o600)
 
     @mock.patch("builtins.open", new_callable=mock.mock_open)
     def test_write_to_file_permission_error(self, mock_open):