commit 6b6f2d8057efa47aad03ee14b2cca9f2b83d7456
author Nan Zhou <nanzhoumails@gmail.com> Tue Oct 25 00:07:17 2022 +0000
committer Nan Zhou <nanzhoumails@gmail.com> Fri Nov 11 18:37:32 2022 +0000
tree a05b93159a7e03701f1e037f099bff3a4a83f1f5
parent 784aecd4d6c8ddf67060455b60e24d76287abdbe

userEnable: fix bug and add unit test

This commit adds unit tests for the |userEnable| function. To make it
happen, a new overload of |executeUserModify| is introduced. The idea
is the same as previous commits where we add sudo in unit tests.

Thanks to this unit test, this commit fixes an existing bug where the
corresponding user's |userEnabled| attribute isn't updated.

Tested: unit test passed

Coverage:
  lines......: 81.3% (1918 of 2359 lines)
  functions..: 93.9% (400 of 426 functions)
  branches...: 32.0% (3029 of 9469 branches)

Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I89752e5fcfc1aabb4090b0b2e8faf5f1b5ee5e76

test/user_mgr_test.cpp

diff --git a/test/user_mgr_test.cpp b/test/user_mgr_test.cpp
index 693f18c..f613f9e 100644
--- a/test/user_mgr_test.cpp
+++ b/test/user_mgr_test.cpp
@@ -264,6 +264,9 @@
 
         ON_CALL(*this, executeUserModify(testing::_, testing::_, testing::_))
             .WillByDefault(testing::Return());
+
+        ON_CALL(*this, executeUserModifyUserEnable)
+            .WillByDefault(testing::Return());
     }
 
     ~UserMgrInTest() override
@@ -284,6 +287,9 @@
     MOCK_METHOD(void, executeUserModify, (const char*, const char*, bool),
                 (override));
 
+    MOCK_METHOD(void, executeUserModifyUserEnable, (const char*, bool),
+                (override));
+
   protected:
     static sdbusplus::bus_t busInTest;
     std::string tempPamConfigFile;
@@ -671,5 +677,44 @@
     EXPECT_EQ(AccountPolicyIface::accountUnlockTimeout(), 3);
 }
 
+TEST_F(UserMgrInTest, UserEnableOnSuccess)
+{
+    std::string username = "user001";
+    EXPECT_NO_THROW(
+        UserMgr::createUser(username, {"redfish", "ssh"}, "priv-user", true));
+    UserInfoMap userInfo = getUserInfo(username);
+    EXPECT_EQ(std::get<UserEnabled>(userInfo["UserEnabled"]), true);
+
+    EXPECT_NO_THROW(userEnable(username, false));
+
+    userInfo = getUserInfo(username);
+    EXPECT_EQ(std::get<UserEnabled>(userInfo["UserEnabled"]), false);
+
+    EXPECT_NO_THROW(UserMgr::deleteUser(username));
+}
+
+TEST_F(UserMgrInTest, UserEnableThrowsInternalFailureIfExecuteUserModifyFail)
+{
+    std::string username = "user001";
+    EXPECT_NO_THROW(
+        UserMgr::createUser(username, {"redfish", "ssh"}, "priv-user", true));
+    UserInfoMap userInfo = getUserInfo(username);
+    EXPECT_EQ(std::get<UserEnabled>(userInfo["UserEnabled"]), true);
+
+    EXPECT_CALL(*this, executeUserModifyUserEnable(testing::StrEq(username),
+                                                   testing::Eq(false)))
+        .WillOnce(testing::Throw(
+            sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure()));
+    EXPECT_THROW(
+        userEnable(username, false),
+        sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure);
+
+    userInfo = getUserInfo(username);
+    // Stay unchanged
+    EXPECT_EQ(std::get<UserEnabled>(userInfo["UserEnabled"]), true);
+
+    EXPECT_NO_THROW(UserMgr::deleteUser(username));
+}
+
 } // namespace user
 } // namespace phosphor

user_mgr.cpp

diff --git a/user_mgr.cpp b/user_mgr.cpp
index 7b2b697..f7d5e8e 100644
--- a/user_mgr.cpp
+++ b/user_mgr.cpp
@@ -617,10 +617,7 @@
     throwForUserDoesNotExist(userName);
     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/usermod", userName.c_str(), "-e",
-                   (enabled ? "" : "1970-01-01"));
+        executeUserModifyUserEnable(userName.c_str(), enabled);
     }
     catch (const InternalFailure& e)
     {
@@ -631,6 +628,7 @@
     log<level::INFO>("User enabled/disabled state updated successfully",
                      entry("USER_NAME=%s", userName.c_str()),
                      entry("ENABLED=%d", enabled));
+    usersList[userName]->setUserEnabled(enabled);
     return;
 }
 
@@ -1346,5 +1344,13 @@
                (sshRequested ? "/bin/sh" : "/bin/nologin"));
 }
 
+void UserMgr::executeUserModifyUserEnable(const char* userName, 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/usermod", userName, "-e",
+               (enabled ? "" : "1970-01-01"));
+}
+
 } // namespace user
 } // namespace phosphor

user_mgr.hpp

diff --git a/user_mgr.hpp b/user_mgr.hpp
index 53097a8..3f0d59c 100644
--- a/user_mgr.hpp
+++ b/user_mgr.hpp
@@ -320,6 +320,9 @@
     virtual void executeUserModify(const char* userName, const char* newGroups,
                                    bool sshRequested);
 
+    virtual void executeUserModifyUserEnable(const char* userName,
+                                             bool enabled);
+
     /** @brief check for valid privielge
      *  method to check valid privilege, and throw if invalid
      *

users.cpp

diff --git a/users.cpp b/users.cpp
index 7957482..4340756 100644
--- a/users.cpp
+++ b/users.cpp
@@ -143,6 +143,11 @@
     return UsersIface::userEnabled();
 }
 
+void Users::setUserEnabled(bool value)
+{
+    UsersIface::userEnabled(value);
+}
+
 /** @brief update user enabled state
  *
  *  @param[in] value - bool value

users.hpp

diff --git a/users.hpp b/users.hpp
index 3541625..0f3065e 100644
--- a/users.hpp
+++ b/users.hpp
@@ -97,6 +97,8 @@
      */
     bool userEnabled(void) const override;
 
+    void setUserEnabled(bool value);
+
     /** @brief update user enabled state
      *
      *  @param[in] value - bool value