user add/delete: add unit tests
This commit adds the test infrastructure such that we are able to
manipulate groups and users inside the unit test container.
To make it happen, the following changes are made:
1. executeCmd was moved to header
2. made the two userDelete and userAdd function virtual; this is needed
since on BMC the daemon is run as root by default; however on unit test
container, it's run as the current user on VMs. Thus in the unit test,
these two functions are overridden to dummy commands.
It can be shown that the unit test coverage increases a lot.
Tested: unit test pasesd
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I7be636d8acd487242831d1c74941e848e508b8f9
diff --git a/test/user_mgr_test.cpp b/test/user_mgr_test.cpp
index 588c2fa..16d6617 100644
--- a/test/user_mgr_test.cpp
+++ b/test/user_mgr_test.cpp
@@ -253,6 +253,12 @@
// Set config files to test files
pamPasswdConfigFile = tempPamConfigFile;
pamAuthConfigFile = tempPamConfigFile;
+
+ ON_CALL(*this, executeUserAdd).WillByDefault(testing::Return());
+
+ ON_CALL(*this, executeUserDelete).WillByDefault(testing::Return());
+
+ ON_CALL(*this, getIpmiUsersCount).WillByDefault(testing::Return(0));
}
~UserMgrInTest() override
@@ -260,6 +266,13 @@
EXPECT_NO_THROW(removeFile(tempPamConfigFile));
}
+ MOCK_METHOD(void, executeUserAdd, (const char*, const char*, bool, bool),
+ (override));
+
+ MOCK_METHOD(void, executeUserDelete, (const char*), (override));
+
+ MOCK_METHOD(size_t, getIpmiUsersCount, (), (override));
+
protected:
static sdbusplus::bus_t busInTest;
std::string tempPamConfigFile;
@@ -360,5 +373,54 @@
sdbusplus::xyz::openbmc_project::Common::Error::InvalidArgument);
}
+TEST_F(UserMgrInTest, UserAddNotRootFailedWithInternalFailure)
+{
+ EXPECT_THROW(
+ UserMgr::executeUserAdd("user0", "ipmi,ssh", true, true),
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+}
+
+TEST_F(UserMgrInTest, UserDeleteNotRootFailedWithInternalFailure)
+{
+ EXPECT_THROW(
+ UserMgr::executeUserDelete("user0"),
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+}
+
+TEST_F(UserMgrInTest,
+ ThrowForMaxGrpUserCountThrowsNoResourceWhenIpmiUserExceedLimit)
+{
+ EXPECT_CALL(*this, getIpmiUsersCount()).WillOnce(Return(ipmiMaxUsers));
+ EXPECT_THROW(
+ throwForMaxGrpUserCount({"ipmi"}),
+ sdbusplus::xyz::openbmc_project::User::Common::Error::NoResource);
+}
+
+TEST_F(UserMgrInTest, CreateUserThrowsInternalFailureWhenExecuteUserAddFails)
+{
+ EXPECT_CALL(*this, executeUserAdd)
+ .WillOnce(testing::Throw(
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure()));
+ EXPECT_THROW(
+ createUser("whatever", {"redfish"}, "", true),
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+}
+
+TEST_F(UserMgrInTest, DeleteUserThrowsInternalFailureWhenExecuteUserDeleteFails)
+{
+ std::string username = "user";
+ EXPECT_NO_THROW(
+ UserMgr::createUser(username, {"redfish", "ssh"}, "priv-user", true));
+ EXPECT_CALL(*this, executeUserDelete(testing::StrEq(username)))
+ .WillOnce(testing::Throw(
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure()))
+ .WillOnce(testing::DoDefault());
+
+ EXPECT_THROW(
+ deleteUser(username),
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+ EXPECT_NO_THROW(UserMgr::deleteUser(username));
+}
+
} // namespace user
} // namespace phosphor
diff --git a/user_mgr.cpp b/user_mgr.cpp
index f2c500c..37116f6 100644
--- a/user_mgr.cpp
+++ b/user_mgr.cpp
@@ -31,8 +31,6 @@
#include <unistd.h>
#include <boost/algorithm/string/split.hpp>
-#include <boost/process/child.hpp>
-#include <boost/process/io.hpp>
#include <phosphor-logging/elog-errors.hpp>
#include <phosphor-logging/elog.hpp>
#include <phosphor-logging/log.hpp>
@@ -55,10 +53,8 @@
{
static constexpr const char* passwdFileName = "/etc/passwd";
-static constexpr size_t ipmiMaxUsers = 15;
static constexpr size_t ipmiMaxUserNameLen = 16;
static constexpr size_t systemMaxUserNameLen = 30;
-static constexpr size_t maxSystemUsers = 30;
static constexpr const char* grpSsh = "ssh";
static constexpr uint8_t minPasswdLength = 8;
static constexpr int success = 0;
@@ -107,35 +103,6 @@
using Argument = xyz::openbmc_project::Common::InvalidArgument;
-template <typename... ArgTypes>
-static std::vector<std::string> executeCmd(const char* path,
- ArgTypes&&... tArgs)
-{
- std::vector<std::string> stdOutput;
- boost::process::ipstream stdOutStream;
- boost::process::child execProg(path, const_cast<char*>(tArgs)...,
- boost::process::std_out > stdOutStream);
- std::string stdOutLine;
-
- while (stdOutStream && std::getline(stdOutStream, stdOutLine) &&
- !stdOutLine.empty())
- {
- stdOutput.emplace_back(stdOutLine);
- }
-
- execProg.wait();
-
- int retCode = execProg.exit_code();
- if (retCode)
- {
- log<level::ERR>("Command execution failed", entry("PATH=%s", path),
- entry("RETURN_CODE=%d", retCode));
- elog<InternalFailure>();
- }
-
- return stdOutput;
-}
-
std::string getCSVFromVector(std::span<const std::string> vec)
{
if (vec.empty())
@@ -316,12 +283,7 @@
}
try
{
- // set EXPIRE_DATE to 0 to disable user, PAM takes 0 as expire on
- // 1970-01-01, that's an implementation-defined behavior
- executeCmd("/usr/sbin/useradd", userName.c_str(), "-G", groups.c_str(),
- "-m", "-N", "-s",
- (sshRequested ? "/bin/sh" : "/bin/nologin"), "-e",
- (enabled ? "" : "1970-01-01"));
+ executeUserAdd(userName.c_str(), groups.c_str(), sshRequested, enabled);
}
catch (const InternalFailure& e)
{
@@ -350,7 +312,7 @@
throwForUserDoesNotExist(userName);
try
{
- executeCmd("/usr/sbin/userdel", userName.c_str(), "-r");
+ executeUserDelete(userName.c_str());
}
catch (const InternalFailure& e)
{
@@ -890,6 +852,12 @@
return userList.size();
}
+size_t UserMgr::getNonIpmiUsersCount()
+{
+ std::vector<std::string> ipmiUsers = getUsersInGroup("ipmi");
+ return usersList.size() - ipmiUsers.size();
+}
+
bool UserMgr::isUserEnabled(const std::string& userName)
{
// All user management lock has to be based on /etc/shadow
@@ -1344,5 +1312,20 @@
this->emit_object_added();
}
+void UserMgr::executeUserAdd(const char* userName, const char* groups,
+ bool sshRequested, bool enabled)
+{
+ // set EXPIRE_DATE to 0 to disable user, PAM takes 0 as expire on
+ // 1970-01-01, that's an implementation-defined behavior
+ executeCmd("/usr/sbin/useradd", userName, "-G", groups, "-m", "-N", "-s",
+ (sshRequested ? "/bin/sh" : "/bin/nologin"), "-e",
+ (enabled ? "" : "1970-01-01"));
+}
+
+void UserMgr::executeUserDelete(const char* userName)
+{
+ executeCmd("/usr/sbin/userdel", userName, "-r");
+}
+
} // namespace user
} // namespace phosphor
diff --git a/user_mgr.hpp b/user_mgr.hpp
index d298bf0..c0ebe0b 100644
--- a/user_mgr.hpp
+++ b/user_mgr.hpp
@@ -16,8 +16,14 @@
#pragma once
#include "users.hpp"
+#include <boost/process/child.hpp>
+#include <boost/process/io.hpp>
+#include <phosphor-logging/elog-errors.hpp>
+#include <phosphor-logging/elog.hpp>
+#include <phosphor-logging/log.hpp>
#include <sdbusplus/bus.hpp>
#include <sdbusplus/server/object.hpp>
+#include <xyz/openbmc_project/Common/error.hpp>
#include <xyz/openbmc_project/User/AccountPolicy/server.hpp>
#include <xyz/openbmc_project/User/Manager/server.hpp>
@@ -32,6 +38,9 @@
namespace user
{
+inline constexpr size_t ipmiMaxUsers = 15;
+inline constexpr size_t maxSystemUsers = 30;
+
using UserMgrIface = sdbusplus::xyz::openbmc_project::User::server::Manager;
using UserSSHLists =
std::pair<std::vector<std::string>, std::vector<std::string>>;
@@ -67,8 +76,35 @@
bool removeStringFromCSV(std::string& csvStr, const std::string& delStr);
template <typename... ArgTypes>
-static std::vector<std::string> executeCmd(const char* path,
- ArgTypes&&... tArgs);
+std::vector<std::string> executeCmd(const char* path, ArgTypes&&... tArgs)
+{
+ std::vector<std::string> stdOutput;
+ boost::process::ipstream stdOutStream;
+ boost::process::child execProg(path, const_cast<char*>(tArgs)...,
+ boost::process::std_out > stdOutStream);
+ std::string stdOutLine;
+
+ while (stdOutStream && std::getline(stdOutStream, stdOutLine) &&
+ !stdOutLine.empty())
+ {
+ stdOutput.emplace_back(stdOutLine);
+ }
+
+ execProg.wait();
+
+ int retCode = execProg.exit_code();
+ if (retCode)
+ {
+ phosphor::logging::log<phosphor::logging::level::ERR>(
+ "Command execution failed",
+ phosphor::logging::entry("PATH=%s", path),
+ phosphor::logging::entry("RETURN_CODE=%d", retCode));
+ phosphor::logging::elog<
+ sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure>();
+ }
+
+ return stdOutput;
+}
/** @class UserMgr
* @brief Responsible for managing user accounts over the D-Bus interface.
@@ -197,6 +233,13 @@
**/
UserInfoMap getUserInfo(std::string userName) override;
+ /** @brief get IPMI user count
+ * method to get IPMI user count
+ *
+ * @return - returns user count
+ */
+ virtual size_t getIpmiUsersCount(void);
+
protected:
/** @brief get pam argument value
* method to get argument value from pam configuration
@@ -232,6 +275,8 @@
*/
bool isUserExist(const std::string& userName);
+ size_t getNonIpmiUsersCount();
+
/** @brief check user exists
* method to check whether user exist, and throw if not.
*
@@ -256,6 +301,18 @@
throwForUserNameConstraints(const std::string& userName,
const std::vector<std::string>& groupNames);
+ /** @brief check group user count
+ * method to check max group user count, and throw if limit reached
+ *
+ * @param[in] groupNames - group name
+ */
+ void throwForMaxGrpUserCount(const std::vector<std::string>& groupNames);
+
+ virtual void executeUserAdd(const char* userName, const char* groups,
+ bool sshRequested, bool enabled);
+
+ virtual void executeUserDelete(const char* userName);
+
private:
/** @brief sdbusplus handler */
sdbusplus::bus_t& bus;
@@ -291,13 +348,6 @@
*/
UserSSHLists getUserAndSshGrpList(void);
- /** @brief check group user count
- * method to check max group user count, and throw if limit reached
- *
- * @param[in] groupNames - group name
- */
- void throwForMaxGrpUserCount(const std::vector<std::string>& groupNames);
-
/** @brief check for valid privielge
* method to check valid privilege, and throw if invalid
*
@@ -326,13 +376,6 @@
*/
void initUserObjects(void);
- /** @brief get IPMI user count
- * method to get IPMI user count
- *
- * @return - returns user count
- */
- size_t getIpmiUsersCount(void);
-
/** @brief get service name
* method to get dbus service name
*