From 23cbc170913f9dcf4d05c64b0b60622429d934f7 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Wed, 13 Nov 2024 17:35:01 -0500 Subject: [PATCH] [platform] Remove chmod from platform.h --- include/multipass/platform.h | 3 +-- src/platform/platform_unix.cpp | 13 ++++--------- tests/mock_platform.h | 2 +- tests/unix/test_platform_unix.cpp | 23 +++++++++++++---------- 4 files changed, 19 insertions(+), 22 deletions(-) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index a5c153d9de..02ab2a58c1 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -58,8 +58,7 @@ class Platform : public Singleton virtual bool is_remote_supported(const std::string& remote) const; virtual bool is_backend_supported(const QString& backend) const; // temporary (?) 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 set_permissions(const multipass::Path& path, const QFileDevice::Permissions permissions) 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/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 9e00b6aaf3..2a5c659642 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -58,12 +58,7 @@ int mp::platform::Platform::chown(const char* path, unsigned int uid, unsigned i return ::lchown(path, uid, gid); } -int mp::platform::Platform::chmod(const char* path, unsigned int mode) const -{ - return ::chmod(path, mode); -} - -bool mp::platform::Platform::set_permissions(const mp::Path path, const QFileDevice::Permissions permissions) const +bool mp::platform::Platform::set_permissions(const mp::Path& path, const QFileDevice::Permissions permissions) const { return QFile::setPermissions(path, permissions); } @@ -108,7 +103,7 @@ void mp::platform::Platform::set_server_socket_restrictions(const std::string& s return; int gid{0}; - int mode{S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP}; + auto mode{QFileDevice::ReadUser | QFileDevice::WriteUser | QFileDevice::ReadGroup | QFileDevice::WriteGroup}; if (restricted) { @@ -124,14 +119,14 @@ void mp::platform::Platform::set_server_socket_restrictions(const std::string& s } else { - mode |= S_IROTH | S_IWOTH; + mode |= QFileDevice::ReadOther | QFileDevice::WriteOther; } const auto socket_path = tokens[1]; if (chown(socket_path.c_str(), 0, gid) == -1) throw std::runtime_error(fmt::format("Could not set ownership of the multipass socket: {}", strerror(errno))); - if (chmod(socket_path.c_str(), mode) == -1) + if (!set_permissions(socket_path.c_str(), mode)) throw std::runtime_error( fmt::format("Could not set permissions for the multipass socket: {}", strerror(errno))); } diff --git a/tests/mock_platform.h b/tests/mock_platform.h index 7f3c27fe5b..30baa9fe92 100644 --- a/tests/mock_platform.h +++ b/tests/mock_platform.h @@ -40,7 +40,7 @@ class MockPlatform : public platform::Platform MOCK_METHOD(bool, is_remote_supported, (const std::string&), (const, override)); MOCK_METHOD(bool, is_backend_supported, (const QString&), (const, override)); MOCK_METHOD(bool, is_alias_supported, (const std::string&, const std::string&), (const, override)); - MOCK_METHOD(int, chmod, (const char*, unsigned int), (const, override)); + MOCK_METHOD(bool, set_permissions, (const multipass::Path&, const QFileDevice::Permissions), (const, override)); MOCK_METHOD(int, chown, (const char*, unsigned int, unsigned int), (const, override)); MOCK_METHOD(bool, link, (const char*, const char*), (const, override)); MOCK_METHOD(bool, symlink, (const char*, const char*, bool), (const, override)); diff --git a/tests/unix/test_platform_unix.cpp b/tests/unix/test_platform_unix.cpp index 02c129e33d..e5cf16a5a2 100644 --- a/tests/unix/test_platform_unix.cpp +++ b/tests/unix/test_platform_unix.cpp @@ -36,6 +36,11 @@ namespace struct TestPlatformUnix : public Test { mpt::TempFile file; + + static constexpr auto restricted_permissions{QFileDevice::ReadUser | QFileDevice::WriteUser | + QFileDevice::ReadGroup | QFileDevice::WriteGroup}; + static constexpr auto relaxed_permissions{restricted_permissions | QFileDevice::ReadOther | + QFileDevice::WriteOther}; }; } // namespace @@ -44,8 +49,7 @@ TEST_F(TestPlatformUnix, setServerSocketRestrictionsNotRestrictedIsCorrect) auto [mock_platform, guard] = mpt::MockPlatform::inject(); EXPECT_CALL(*mock_platform, chown(_, 0, 0)).WillOnce(Return(0)); - EXPECT_CALL(*mock_platform, chmod(_, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)) - .WillOnce(Return(0)); + EXPECT_CALL(*mock_platform, set_permissions(_, relaxed_permissions)).WillOnce(Return(true)); EXPECT_NO_THROW(MP_PLATFORM.Platform::set_server_socket_restrictions(fmt::format("unix:{}", file.name()), false)); } @@ -58,7 +62,7 @@ TEST_F(TestPlatformUnix, setServerSocketRestrictionsRestrictedIsCorrect) group.gr_gid = gid; EXPECT_CALL(*mock_platform, chown(_, 0, gid)).WillOnce(Return(0)); - EXPECT_CALL(*mock_platform, chmod(_, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP)).WillOnce(Return(0)); + EXPECT_CALL(*mock_platform, set_permissions(_, restricted_permissions)).WillOnce(Return(true)); REPLACE(getgrnam, [&group](auto) { return &group; }); @@ -77,7 +81,7 @@ TEST_F(TestPlatformUnix, setServerSocketRestrictionsNonUnixTypeReturns) auto [mock_platform, guard] = mpt::MockPlatform::inject(); EXPECT_CALL(*mock_platform, chown).Times(0); - EXPECT_CALL(*mock_platform, chmod).Times(0); + EXPECT_CALL(*mock_platform, set_permissions).Times(0); EXPECT_NO_THROW(MP_PLATFORM.Platform::set_server_socket_restrictions(fmt::format("dns:{}", file.name()), false)); } @@ -102,11 +106,10 @@ TEST_F(TestPlatformUnix, setServerSocketRestrictionsChmodFailsThrows) auto [mock_platform, guard] = mpt::MockPlatform::inject(); EXPECT_CALL(*mock_platform, chown(_, 0, 0)).WillOnce(Return(0)); - EXPECT_CALL(*mock_platform, chmod(_, S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH | S_IWOTH)) - .WillOnce([](auto...) { - errno = EPERM; - return -1; - }); + EXPECT_CALL(*mock_platform, set_permissions(_, relaxed_permissions)).WillOnce([](auto...) { + errno = EPERM; + return false; + }); MP_EXPECT_THROW_THAT( MP_PLATFORM.Platform::set_server_socket_restrictions(fmt::format("unix:{}", file.name()), false), @@ -119,7 +122,7 @@ TEST_F(TestPlatformUnix, chmodSetsFileModsAndReturns) auto perms = QFileInfo(file.name()).permissions(); ASSERT_EQ(perms, QFileDevice::ReadOwner | QFileDevice::WriteOwner | QFileDevice::ReadUser | QFileDevice::WriteUser); - EXPECT_EQ(MP_PLATFORM.chmod(file.name().toStdString().c_str(), S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP), 0); + EXPECT_EQ(MP_PLATFORM.set_permissions(file.name(), restricted_permissions), true); perms = QFileInfo(file.name()).permissions();