From 522377dcb85082da598403e104a44d621b4c2bb4 Mon Sep 17 00:00:00 2001
From: Jonathan Doman <jonathan.doman@intel.com>
Date: Fri, 18 Aug 2023 15:27:54 -0700
Subject: Refactor populateUserInfo

- No need to set error code in asyncResp since caller already does that.
  Then we can remove the asyncResp param altogether.
- Check if session is valid before unpacking properties to avoid
  unnecessary work.
- Use std::optional instead of pointers for slighter cleaner code.
- Enforce required properties for local users based on D-Bus interface
  documentation (UserGroups must be provided for local users).

Change-Id: I770d3556a0d62182b6abd72bfa3f8d62e2a105d1
Signed-off-by: Jonathan Doman <jonathan.doman@intel.com>
---
 include/dbus_privileges.hpp | 68 ++++++++++++++-------------------------------
 1 file changed, 21 insertions(+), 47 deletions(-)

(limited to 'include')

diff --git a/include/dbus_privileges.hpp b/include/dbus_privileges.hpp
index 16aae5e62a..6602a5c2fa 100644
--- a/include/dbus_privileges.hpp
+++ b/include/dbus_privileges.hpp
@@ -19,75 +19,49 @@ namespace crow
 // Populate session with user information.
 inline bool
     populateUserInfo(Request& req,
-                     const std::shared_ptr<bmcweb::AsyncResp>& asyncResp,
                      const dbus::utility::DBusPropertiesMap& userInfoMap)
 {
-    const std::string* userRolePtr = nullptr;
-    const bool* remoteUser = nullptr;
-    const bool* passwordExpired = nullptr;
-    const std::vector<std::string>* userGroups = nullptr;
+    if (req.session == nullptr)
+    {
+        return false;
+    }
+
+    std::string userRole;
+    bool remoteUser = false;
+    std::optional<bool> passwordExpired;
+    std::optional<std::vector<std::string>> userGroups;
 
     const bool success = sdbusplus::unpackPropertiesNoThrow(
         redfish::dbus_utils::UnpackErrorPrinter(), userInfoMap, "UserPrivilege",
-        userRolePtr, "RemoteUser", remoteUser, "UserPasswordExpired",
+        userRole, "RemoteUser", remoteUser, "UserPasswordExpired",
         passwordExpired, "UserGroups", userGroups);
 
     if (!success)
     {
         BMCWEB_LOG_ERROR("Failed to unpack user properties.");
-        asyncResp->res.result(
-            boost::beast::http::status::internal_server_error);
         return false;
     }
 
-    if (req.session == nullptr)
+    if (!remoteUser && (!passwordExpired || !userGroups))
     {
+        BMCWEB_LOG_ERROR(
+            "Missing UserPasswordExpired or UserGroups property for local user");
         return false;
     }
 
-    if (userRolePtr != nullptr)
-    {
-        req.session->userRole = *userRolePtr;
-        BMCWEB_LOG_DEBUG("userName = {} userRole = {}", req.session->username,
-                         *userRolePtr);
-    }
-
-    if (remoteUser == nullptr)
-    {
-        BMCWEB_LOG_ERROR("RemoteUser property missing or wrong type");
-        asyncResp->res.result(
-            boost::beast::http::status::internal_server_error);
-        return false;
-    }
-    bool expired = false;
-    if (passwordExpired == nullptr)
-    {
-        if (!*remoteUser)
-        {
-            BMCWEB_LOG_ERROR("UserPasswordExpired property is expected for"
-                             " local user but is missing or wrong type");
-            asyncResp->res.result(
-                boost::beast::http::status::internal_server_error);
-            return false;
-        }
-    }
-    else
-    {
-        expired = *passwordExpired;
-    }
+    req.session->userRole = userRole;
+    BMCWEB_LOG_DEBUG("userName = {} userRole = {}", req.session->username,
+                     userRole);
 
     // Set isConfigureSelfOnly based on D-Bus results.  This
     // ignores the results from both pamAuthenticateUser and the
     // value from any previous use of this session.
-    req.session->isConfigureSelfOnly = expired;
+    req.session->isConfigureSelfOnly = passwordExpired.value_or(false);
 
-    if (userGroups != nullptr)
-    {
-        req.session->userGroups = *userGroups;
-    }
-    else
+    req.session->userGroups.clear();
+    if (userGroups)
     {
-        req.session->userGroups.clear();
+        req.session->userGroups.swap(*userGroups);
     }
 
     return true;
@@ -147,7 +121,7 @@ void afterGetUserInfo(Request& req,
         return;
     }
 
-    if (!populateUserInfo(req, asyncResp, userInfoMap))
+    if (!populateUserInfo(req, userInfoMap))
     {
         BMCWEB_LOG_ERROR("Failed to populate user information");
         asyncResp->res.result(
-- 
cgit v1.2.3