diff --git a/include/multipass/platform.h b/include/multipass/platform.h index 81ef91f701..35430674c4 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -60,7 +60,7 @@ class Platform : public Singleton 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 index 6bb35a56bd..5241fec24c 100644 --- a/include/multipass/utils/permission_utils.h +++ b/include/multipass/utils/permission_utils.h @@ -36,7 +36,7 @@ class PermissionUtils : public Singleton 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 index 1ef792e154..ca07a8b87c 100644 --- 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 index 31004e9ff2..9ee79cb457 100644 --- 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 index 0d630a19d5..e0973c9a27 100644 --- 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 index 62367a7281..eb99a5c88d 100644 --- 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 index ae132b93ee..cd8374f1d6 100644 --- a/tests/test_permission_utils.cpp +++ b/tests/test_permission_utils.cpp @@ -15,8 +15,6 @@ * */ -#include - #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,9 +198,9 @@ 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())))); } @@ -217,7 +208,7 @@ TEST_F(TestPermissionUtilsBadDir, take_ownership_throws_on_broken_iterator) 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,