Address some review comments

include/multipass/utils/permission_utils.h

@@ -27,19 +27,18 @@
 
 namespace multipass
 {
+namespace fs = std::filesystem;
 
 class PermissionUtils : public Singleton<PermissionUtils>
 {
 public:
-    using Path = std::filesystem::path;
-
     PermissionUtils(const PrivatePass&) noexcept;
 
-    virtual void set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const;
-    virtual void take_ownership(const Path& path) const;
+    virtual void set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const;
+    virtual void take_ownership(const fs::path& path) const;
 
     // sets owner to root and sets permissions such that only owner has access.
-    virtual void restrict_permissions(const Path& path) const;
+    virtual void restrict_permissions(const fs::path& path) const;
 };
 } // namespace multipass
 

src/daemon/default_vm_image_vault.cpp

@@ -670,8 +670,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image,
                                                     const ProgressMonitor& monitor,
                                                     const mp::Path& dest_dir)
 {
-    MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner);
-    MP_PERMISSIONS.take_ownership(dest_dir.toStdU16String());
+    MP_UTILS.make_dir(dest_dir);
 
     QFileInfo file_info{source_image.image_path};
     const auto image_name = file_info.fileName().remove(".xz");
@@ -682,8 +681,7 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image,
 
 mp::VMImage mp::DefaultVMImageVault::image_instance_from(const VMImage& prepared_image, const mp::Path& dest_dir)
 {
-    MP_UTILS.make_dir(dest_dir, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner);
-    MP_PERMISSIONS.take_ownership(dest_dir.toStdU16String());
+    MP_UTILS.make_dir(dest_dir);
 
     return {mp::vault::copy(prepared_image.image_path, dest_dir),
             prepared_image.id,

src/iso/cloud_init_iso.cpp

@@ -538,8 +538,6 @@ void mp::CloudInitIso::write_to(const Path& path)
         throw std::runtime_error{fmt::format(
             "Failed to open file for writing during cloud-init generation: {}; path: {}", f.errorString(), path)};
 
-    MP_PERMISSIONS.restrict_permissions(path.toStdU16String());
-
     const uint32_t num_reserved_bytes = 32768u;
     const uint32_t num_reserved_blocks = num_blocks(num_reserved_bytes);
     f.seek(num_reserved_bytes);

src/platform/backends/lxd/lxd_vm_image_vault.cpp

@@ -94,8 +94,6 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr
         mp::vault::delete_file(original_image_path);
     }
 
-    MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String());
-
     return new_image_path;
 }
 

src/utils/permission_utils.cpp

@@ -23,19 +23,17 @@
 namespace mp = multipass;
 namespace fs = mp::fs;
 
-using Path = mp::PermissionUtils::Path;
-
 namespace
 {
-void set_single_permissions(const Path& path, const QFileDevice::Permissions& permissions)
+void set_single_permissions(const fs::path& path, const QFileDevice::Permissions& permissions)
 {
     QString qpath = QString::fromUtf8(path.u8string());
 
     if (!MP_PLATFORM.set_permissions(qpath, permissions))
         throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string()));
 }
 
-void set_single_owner(const Path& path)
+void set_single_owner(const fs::path& path)
 {
     QString qpath = QString::fromUtf8(path.u8string());
 
@@ -44,7 +42,7 @@ void set_single_owner(const Path& path)
 }
 
 // only exists because MP_FILEOPS doesn't overload the throwing variaions of std::filesystem functions
-void throw_if_error(const Path& path, const std::error_code& ec)
+void throw_if_error(const fs::path& path, const std::error_code& ec)
 {
     if (ec)
         throw std::system_error(
@@ -54,7 +52,7 @@ void throw_if_error(const Path& path, const std::error_code& ec)
 
 // recursively iterates over all files in folder and applies a function that takes a path
 template <class Func>
-void apply_on_files(const Path& path, Func&& func)
+void apply_on_files(const fs::path& path, Func&& func)
 {
     std::error_code ec{};
     if (!MP_FILEOPS.exists(path, ec) || ec)
@@ -83,22 +81,21 @@ void apply_on_files(const Path& path, Func&& func)
 }
 } // namespace
 
-mp::PermissionUtils::PermissionUtils(const Singleton<PermissionUtils>::PrivatePass& pass) noexcept
-    : Singleton<PermissionUtils>::Singleton{pass}
+mp::PermissionUtils::PermissionUtils(const PrivatePass& pass) noexcept : Singleton{pass}
 {
 }
 
-void mp::PermissionUtils::set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const
+void mp::PermissionUtils::set_permissions(const fs::path& path, const QFileDevice::Permissions& permissions) const
 {
     apply_on_files(path, [&](const fs::path& apply_path) { set_single_permissions(apply_path, permissions); });
 }
 
-void mp::PermissionUtils::take_ownership(const Path& path) const
+void mp::PermissionUtils::take_ownership(const fs::path& path) const
 {
     apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path); });
 }
 
-void mp::PermissionUtils::restrict_permissions(const Path& path) const
+void mp::PermissionUtils::restrict_permissions(const fs::path& path) const
 {
     apply_on_files(path, [&](const fs::path& apply_path) {
         set_single_owner(apply_path);

src/utils/vm_image_vault_utils.cpp

@@ -48,8 +48,6 @@ QString mp::vault::copy(const QString& file_name, const QDir& output_dir)
     auto new_path = output_dir.filePath(source_name);
     QFile::copy(file_name, new_path);
 
-    MP_PERMISSIONS.restrict_permissions(new_path.toStdU16String());
-
     return new_path;
 }
 
@@ -95,8 +93,6 @@ QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressM
 
     xz_decoder.decode_to(new_image_path, monitor);
 
-    MP_PERMISSIONS.restrict_permissions(new_image_path.toStdU16String());
-
     mp::vault::delete_file(image_path);
 
     return new_image_path;

tests/lxd/test_lxd_image_vault.cpp

@@ -22,7 +22,6 @@
 #include "tests/common.h"
 #include "tests/mock_image_host.h"
 #include "tests/mock_logger.h"
-#include "tests/mock_permission_utils.h"
 #include "tests/mock_process_factory.h"
 #include "tests/stub_url_downloader.h"
 #include "tests/temp_dir.h"
@@ -61,9 +60,6 @@ struct LXDImageVault : public Test
 
     mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject();
     std::unique_ptr<NiceMock<mpt::MockNetworkAccessManager>> mock_network_access_manager;
-    const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection =
-        mpt::MockPermissionUtils::inject<NiceMock>();
-    mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first;
     std::vector<mp::VMImageHost*> hosts;
     NiceMock<mpt::MockImageHost> host;
     QUrl base_url{"unix:///[email protected]"};

tests/mock_permission_utils.h

@@ -35,10 +35,10 @@ class MockPermissionUtils : public PermissionUtils
 
     MOCK_METHOD(void,
                 set_permissions,
-                (const Path& path, const QFileDevice::Permissions& permissions),
+                (const fs::path& path, const QFileDevice::Permissions& permissions),
                 (const, override));
-    MOCK_METHOD(void, take_ownership, (const Path& path), (const, override));
-    MOCK_METHOD(void, restrict_permissions, (const Path& path), (const, override));
+    MOCK_METHOD(void, take_ownership, (const fs::path& path), (const, override));
+    MOCK_METHOD(void, restrict_permissions, (const fs::path& path), (const, override));
 
     MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils);
 };

tests/test_base_virtual_machine_factory.cpp

@@ -17,7 +17,6 @@
 
 #include "common.h"
 #include "mock_logger.h"
-#include "mock_permission_utils.h"
 #include "mock_platform.h"
 #include "stub_ssh_key_provider.h"
 #include "stub_url_downloader.h"
@@ -124,10 +123,6 @@ TEST_F(BaseFactory, networks_throws)
 // at this time.  Instead, just make sure an ISO image is created and has the expected path.
 TEST_F(BaseFactory, creates_cloud_init_iso_image)
 {
-    auto [mock_permission_utils, permission_utils_guard] = mpt::MockPermissionUtils::inject();
-
-    EXPECT_CALL(*mock_permission_utils, restrict_permissions(_));
-
     MockBaseFactory factory;
     const std::string name{"foo"};
     const YAML::Node metadata{YAML::Load({fmt::format("name: {}", name)})}, vendor_data{metadata}, user_data{metadata},

tests/test_cloud_init_iso.cpp

@@ -17,7 +17,6 @@
 
 #include "common.h"
 #include "mock_file_ops.h"
-#include "mock_permission_utils.h"
 #include "temp_dir.h"
 
 #include <multipass/cloud_init_iso.h>
@@ -76,9 +75,6 @@ struct CloudInitIso : public Test
     mpt::TempDir temp_dir;
     QString iso_path;
     std::filesystem::path std_iso_path;
-
-    const mpt::MockPermissionUtils::GuardedMock attr{mpt::MockPermissionUtils::inject<NiceMock>()};
-    mpt::MockPermissionUtils& mock_permission_utils = *attr.first;
 };
 
 TEST_F(CloudInitIso, check_contains_false)
@@ -140,8 +136,6 @@ TEST_F(CloudInitIso, check_index_operator_found_key)
 
 TEST_F(CloudInitIso, creates_iso_file)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso iso;
     iso.add_file("test", "test data");
     iso.write_to(iso_path);
@@ -153,8 +147,6 @@ TEST_F(CloudInitIso, creates_iso_file)
 
 TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.write_to(iso_path);
 
@@ -168,8 +160,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_open_file)
 
 TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.write_to(iso_path);
 
@@ -188,8 +178,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_single_bytes)
 
 TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descriptor)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.write_to(iso_path);
 
@@ -213,8 +201,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_it_has_Joliet_volume_descrip
 
 TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.write_to(iso_path);
 
@@ -240,8 +226,6 @@ TEST_F(CloudInitIso, reads_iso_file_Joliet_volume_descriptor_malformed)
 
 TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.write_to(iso_path);
 
@@ -263,8 +247,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_array)
 
 TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.write_to(iso_path);
 
@@ -292,8 +274,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_check_root_dir_record_data)
 
 TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     // At least one actual file entry is need to reach the read_bytes_to_vec call
     original_iso.add_file("test1", "test data1");
@@ -319,8 +299,6 @@ TEST_F(CloudInitIso, reads_iso_file_failed_to_read_vec)
 
 TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     // At least one actual file entry is need to reach the convert_u16_name_back call
     original_iso.add_file("test1", "test data1");
@@ -356,8 +334,6 @@ TEST_F(CloudInitIso, reads_iso_file_encoded_file_name_is_not_even_length)
 
 TEST_F(CloudInitIso, reads_iso_file_with_random_string_data)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
 
     original_iso.add_file("test1", "test data1");
@@ -395,8 +371,6 @@ timezone: Europe/Amsterdam
     content: "multipass/version/1.14.0-dev.1209+g5b2c7f7d # written by Multipass\nmultipass/driver/qemu-8.0.4 # written by Multipass\nmultipass/host/ubuntu-23.10 # written by Multipass\nmultipass/alias/default # written by Multipass\n"
 )";
 
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
 
     original_iso.add_file("meta-data", default_meta_data_content);
@@ -412,8 +386,6 @@ timezone: Europe/Amsterdam
 
 TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2);
-
     mp::CloudInitIso original_iso;
 
     original_iso.add_file("meta-data", default_meta_data_content);
@@ -440,8 +412,6 @@ TEST_F(CloudInitIso, updateCloudInitWithNewNonEmptyExtraInterfaces)
 
 TEST_F(CloudInitIso, updateCloudInitWithNewEmptyExtraInterfaces)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2);
-
     mp::CloudInitIso original_iso;
     original_iso.add_file("meta-data", default_meta_data_content);
     original_iso.add_file("network-config", "dummy_data");
@@ -465,8 +435,6 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces)
     const std::string src_network_config_data_content =
         fmt::format(network_config_data_content_template, "00:00:00:00:00:00", "00:00:00:00:00:01");
 
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2);
-
     mp::CloudInitIso original_iso;
     original_iso.add_file("meta-data", src_meta_data_content);
     original_iso.add_file("network-config", src_network_config_data_content);
@@ -492,8 +460,6 @@ TEST_F(CloudInitIso, updateCloneCloudInitSrcFileWithExtraInterfaces)
 
 TEST_F(CloudInitIso, addExtraInterfaceToCloudInit)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(std_iso_path)).Times(2);
-
     mp::CloudInitIso original_iso;
     original_iso.add_file("meta-data", default_meta_data_content);
     original_iso.write_to(iso_path);
@@ -505,8 +471,6 @@ TEST_F(CloudInitIso, addExtraInterfaceToCloudInit)
 
 TEST_F(CloudInitIso, getInstanceIdFromCloudInit)
 {
-    EXPECT_CALL(mock_permission_utils, restrict_permissions(_));
-
     mp::CloudInitIso original_iso;
     original_iso.add_file("meta-data", default_meta_data_content);
     original_iso.write_to(iso_path);