[permissions] Remove root option from take ownership

canonical · Dec 4, 2024 · 3af663c · 3af663c
1 parent f16c020
commit 3af663c
Show file tree

Hide file tree

Showing 7 changed files with 28 additions and 42 deletions.
diff --git a/include/multipass/platform.h b/include/multipass/platform.h
@@ -60,7 +60,7 @@ class Platform : public Singleton<Platform>
     virtual int chown(const char* path, unsigned int uid, unsigned int gid) const;
     virtual int chmod(const char* path, unsigned int mode) const;
     virtual bool set_permissions(const multipass::Path& path, const QFileDevice::Permissions permissions) const;
-    virtual bool take_ownership(const multipass::Path& path, bool root = true) const;
+    virtual bool take_ownership(const multipass::Path& path) const;
     virtual bool link(const char* target, const char* link) const;
     virtual bool symlink(const char* target, const char* link, bool is_dir) const;
     virtual int utime(const char* path, int atime, int mtime) const;

diff --git a/include/multipass/utils/permission_utils.h b/include/multipass/utils/permission_utils.h
@@ -36,7 +36,7 @@ class PermissionUtils : public Singleton<PermissionUtils>
     PermissionUtils(const PrivatePass&) noexcept;
 
     virtual void set_permissions(const Path& path, const QFileDevice::Permissions& permissions) const;
-    virtual void take_ownership(const Path& path, bool root = true) const;
+    virtual void take_ownership(const Path& path) const;
 
     // sets owner to root and sets permissions such that only owner has access.
     virtual void restrict_permissions(const Path& path) const;

diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp
@@ -68,14 +68,9 @@ bool mp::platform::Platform::set_permissions(const mp::Path& path, const QFileDe
     return QFile::setPermissions(path, permissions);
 }
 
-bool mp::platform::Platform::take_ownership(const mp::Path& path, bool root) const
+bool mp::platform::Platform::take_ownership(const mp::Path& path) const
 {
-    if (root)
-    {
-        return this->chown(path.toStdString().c_str(), 0, 0) == 0;
-    }
-
-    throw std::logic_error("NYI");
+    return this->chown(path.toStdString().c_str(), 0, 0) == 0;
 }
 
 bool mp::platform::Platform::symlink(const char* target, const char* link, bool is_dir) const

diff --git a/src/utils/permission_utils.cpp b/src/utils/permission_utils.cpp
@@ -35,11 +35,11 @@ void set_single_permissions(const Path& path, const QFileDevice::Permissions& pe
         throw std::runtime_error(fmt::format("Cannot set permissions for '{}'", path.string()));
 }
 
-void set_single_owner(const Path& path, bool root)
+void set_single_owner(const Path& path)
 {
     QString qpath = QString::fromUtf8(path.u8string());
 
-    if (!MP_PLATFORM.take_ownership(qpath, root))
+    if (!MP_PLATFORM.take_ownership(qpath))
         throw std::runtime_error(fmt::format("Cannot set owner for '{}'", path.string()));
 }
 
@@ -93,15 +93,15 @@ void mp::PermissionUtils::set_permissions(const Path& path, const QFileDevice::P
     apply_on_files(path, [&](const fs::path& apply_path) { set_single_permissions(apply_path, permissions); });
 }
 
-void mp::PermissionUtils::take_ownership(const Path& path, bool root) const
+void mp::PermissionUtils::take_ownership(const Path& path) const
 {
-    apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path, root); });
+    apply_on_files(path, [&](const fs::path& apply_path) { set_single_owner(apply_path); });
 }
 
 void mp::PermissionUtils::restrict_permissions(const Path& path) const
 {
     apply_on_files(path, [&](const fs::path& apply_path) {
-        set_single_owner(apply_path, true);
-        set_single_permissions(apply_path, QFile::ReadOwner | QFile::WriteOwner);
+        set_single_owner(apply_path);
+        set_single_permissions(apply_path, QFile::ReadOwner | QFile::WriteOwner | QFile::ExeOwner);
     });
 }
diff --git a/tests/mock_permission_utils.h b/tests/mock_permission_utils.h
@@ -37,7 +37,7 @@ class MockPermissionUtils : public PermissionUtils
                 set_permissions,
                 (const Path& path, const QFileDevice::Permissions& permissions),
                 (const, override));
-    MOCK_METHOD(void, take_ownership, (const Path& path, bool root), (const, override));
+    MOCK_METHOD(void, take_ownership, (const Path& path), (const, override));
     MOCK_METHOD(void, restrict_permissions, (const Path& path), (const, override));
 
     MP_MOCK_SINGLETON_BOILERPLATE(MockPermissionUtils, PermissionUtils);

diff --git a/tests/mock_platform.h b/tests/mock_platform.h
@@ -46,7 +46,7 @@ class MockPlatform : public platform::Platform
                 set_permissions,
                 (const multipass::Path& path, const QFileDevice::Permissions),
                 (const, override));
-    MOCK_METHOD(bool, take_ownership, (const multipass::Path& path, bool root), (const, override));
+    MOCK_METHOD(bool, take_ownership, (const multipass::Path& path), (const, override));
     MOCK_METHOD(bool, link, (const char*, const char*), (const, override));
     MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override));
     MOCK_METHOD(int, utime, (const char*, int, int), (const, override));

diff --git a/tests/test_permission_utils.cpp b/tests/test_permission_utils.cpp
@@ -15,8 +15,6 @@
  *
  */
 
-#include <Poco/Path.h>
-
 #include "mock_file_ops.h"
 #include "mock_permission_utils.h"
 #include "mock_platform.h"
@@ -38,8 +36,8 @@ struct TestPermissionUtils : public Test
     const mp::PermissionUtils::Path test_path{"test_path"};
     const QString test_qpath{QString::fromUtf8(test_path.u8string())};
     static constexpr QFileDevice::Permissions test_permissions{QFileDevice::Permission::ReadOwner};
-    static constexpr QFileDevice::Permissions restricted_permissions{QFileDevice::Permission::ReadOwner |
-                                                                     QFileDevice::Permission::WriteOwner};
+    static constexpr QFileDevice::Permissions restricted_permissions{
+        QFileDevice::Permission::ReadOwner | QFileDevice::Permission::WriteOwner | QFile::ExeOwner};
 };
 
 struct TestPermissionUtilsNoFile : public TestPermissionUtils
@@ -98,21 +96,14 @@ TEST_F(TestPermissionUtilsFile, set_permissions_throws_on_failure)
 
 TEST_F(TestPermissionUtilsFile, take_ownership_takes_ownership)
 {
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true));
-
-    MP_PERMISSIONS.take_ownership(test_path, true);
-}
-
-TEST_F(TestPermissionUtilsFile, take_ownership_takes_user_ownership)
-{
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true));
 
-    MP_PERMISSIONS.take_ownership(test_path, false);
+    MP_PERMISSIONS.take_ownership(test_path);
 }
 
 TEST_F(TestPermissionUtilsFile, take_ownership_throws_on_failure)
 {
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, _)).WillOnce(Return(false));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(false));
 
     MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path),
                          std::runtime_error,
@@ -121,7 +112,7 @@ TEST_F(TestPermissionUtilsFile, take_ownership_throws_on_failure)
 
 TEST_F(TestPermissionUtilsFile, restrict_permissions_restricts_permissions)
 {
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true));
     EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true));
 
     MP_PERMISSIONS.restrict_permissions(test_path);
@@ -165,18 +156,18 @@ TEST_F(TestPermissionUtilsDir, set_permissions_iterates_dir)
 
 TEST_F(TestPermissionUtilsDir, take_ownership_iterates_dir)
 {
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true));
-    EXPECT_CALL(mock_platform, take_ownership(qpath1, false)).WillOnce(Return(true));
-    EXPECT_CALL(mock_platform, take_ownership(qpath2, false)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(qpath1)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(qpath2)).WillOnce(Return(true));
 
-    MP_PERMISSIONS.take_ownership(test_path, false);
+    MP_PERMISSIONS.take_ownership(test_path);
 }
 
 TEST_F(TestPermissionUtilsDir, restrict_permissions_iterates_dir)
 {
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true));
-    EXPECT_CALL(mock_platform, take_ownership(qpath1, true)).WillOnce(Return(true));
-    EXPECT_CALL(mock_platform, take_ownership(qpath2, true)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(qpath1)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(qpath2)).WillOnce(Return(true));
 
     EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true));
     EXPECT_CALL(mock_platform, set_permissions(qpath1, restricted_permissions)).WillOnce(Return(true));
@@ -207,17 +198,17 @@ TEST_F(TestPermissionUtilsBadDir, set_permissions_throws_on_broken_iterator)
 
 TEST_F(TestPermissionUtilsBadDir, take_ownership_throws_on_broken_iterator)
 {
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, false)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true));
 
-    MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path, false),
+    MP_EXPECT_THROW_THAT(MP_PERMISSIONS.take_ownership(test_path),
                          std::runtime_error,
                          mpt::match_what(AllOf(HasSubstr("Cannot iterate"), HasSubstr(test_path.string()))));
 }
 
 TEST_F(TestPermissionUtilsBadDir, restrict_permissions_throws_on_broken_iterator)
 {
     EXPECT_CALL(mock_platform, set_permissions(test_qpath, restricted_permissions)).WillOnce(Return(true));
-    EXPECT_CALL(mock_platform, take_ownership(test_qpath, true)).WillOnce(Return(true));
+    EXPECT_CALL(mock_platform, take_ownership(test_qpath)).WillOnce(Return(true));
 
     MP_EXPECT_THROW_THAT(MP_PERMISSIONS.restrict_permissions(test_path),
                          std::runtime_error,