From 10cb44f3d47622faf795f8f09baef389cb0a6714 Mon Sep 17 00:00:00 2001 From: Ed Tanous Date: Thu, 11 Apr 2024 13:05:20 -0700 Subject: Stage 2 refactor LDAP parameters ReadJsonObject isn't required for cases where we don't have a list of structures, and ideally we should consolidate all fixed readJson calls in one place (and not have multi-depth readJson calls). This commit moves all the calls up, and consolidates all the LDAP patch params into a single struct that can be moved between the layers, rather than having the parameters individually. Tested: Does LDAP work anymore? Could use some help if anyone has test scripts, otherwise code compiles and this is inspection only, but similar to other mechanical changes we've made recently Change-Id: I77c0a8b97d4783fdca875c86d7dace122a0a55d7 Signed-off-by: Ed Tanous --- redfish-core/lib/account_service.hpp | 163 ++++++++++++++++++----------------- 1 file changed, 83 insertions(+), 80 deletions(-) diff --git a/redfish-core/lib/account_service.hpp b/redfish-core/lib/account_service.hpp index dc393ae883..7df5d83ab2 100644 --- a/redfish-core/lib/account_service.hpp +++ b/redfish-core/lib/account_service.hpp @@ -908,7 +908,22 @@ inline void * @param serverType Type of LDAP server(openLDAP/ActiveDirectory) */ -inline void handleLDAPPatch(nlohmann::json::object_t& input, +struct LdapPatchParams +{ + std::optional authType; + std::optional> serviceAddressList; + std::optional serviceEnabled; + std::optional> baseDNList; + std::optional userNameAttribute; + std::optional groupsAttribute; + std::optional userName; + std::optional password; + std::optional< + std::vector>> + remoteRoleMapData; +}; + +inline void handleLDAPPatch(LdapPatchParams&& input, const std::shared_ptr& asyncResp, const std::string& serverType) { @@ -923,78 +938,53 @@ inline void handleLDAPPatch(nlohmann::json::object_t& input, } else { + BMCWEB_LOG_ERROR("serverType wasn't AD or LDAP but was {}????", + serverType); return; } - std::optional authType; - std::optional> serviceAddressList; - std::optional serviceEnabled; - std::optional> baseDNList; - std::optional userNameAttribute; - std::optional groupsAttribute; - std::optional userName; - std::optional password; - std::optional< - std::vector>> - remoteRoleMapData; - // clang-format off - if (!json_util::readJsonObject(input, asyncResp->res, - "Authentication/AuthenticationType", authType, - "Authentication/Username", userName, - "Authentication/Password", password, - "LDAPService/SearchSettings/BaseDistinguishedNames", baseDNList, - "LDAPService/SearchSettings/UsernameAttribute", userNameAttribute, - "LDAPService/SearchSettings/GroupsAttribute", groupsAttribute, - "ServiceAddresses", serviceAddressList, - "ServiceEnabled", serviceEnabled, - "RemoteRoleMapping", remoteRoleMapData)) - { - return; - } - // clang-format on - - if (authType && *authType != "UsernameAndPassword") + if (input.authType && *input.authType != "UsernameAndPassword") { - messages::propertyValueNotInList(asyncResp->res, *authType, + messages::propertyValueNotInList(asyncResp->res, *input.authType, "AuthenticationType"); return; } - if (serviceAddressList) + if (input.serviceAddressList) { - if (serviceAddressList->empty()) + if (input.serviceAddressList->empty()) { messages::propertyValueNotInList( - asyncResp->res, *serviceAddressList, "ServiceAddress"); + asyncResp->res, *input.serviceAddressList, "ServiceAddress"); return; } } - if (baseDNList) + if (input.baseDNList) { - if (baseDNList->empty()) + if (input.baseDNList->empty()) { - messages::propertyValueNotInList(asyncResp->res, *baseDNList, + messages::propertyValueNotInList(asyncResp->res, *input.baseDNList, "BaseDistinguishedNames"); return; } } // nothing to update, then return - if (!userName && !password && !serviceAddressList && !baseDNList && - !userNameAttribute && !groupsAttribute && !serviceEnabled && - !remoteRoleMapData) + if (!input.userName && !input.password && !input.serviceAddressList && + !input.baseDNList && !input.userNameAttribute && + !input.groupsAttribute && !input.serviceEnabled && + !input.remoteRoleMapData) { return; } // Get the existing resource first then keep modifying // whenever any property gets updated. - getLDAPConfigData( - serverType, - [asyncResp, userName, password, baseDNList, userNameAttribute, - groupsAttribute, serviceAddressList, serviceEnabled, dbusObjectPath, - remoteRoleMapData](bool success, const LDAPConfigData& confData, - const std::string& serverT) mutable { + getLDAPConfigData(serverType, + [asyncResp, input = std::move(input), + dbusObjectPath = std::move(dbusObjectPath)]( + bool success, const LDAPConfigData& confData, + const std::string& serverT) mutable { if (!success) { messages::internalError(asyncResp->res); @@ -1008,43 +998,46 @@ inline void handleLDAPPatch(nlohmann::json::object_t& input, handleServiceEnablePatch(false, asyncResp, serverT, dbusObjectPath); } - if (serviceAddressList) + if (input.serviceAddressList) { - handleServiceAddressPatch(*serviceAddressList, asyncResp, serverT, - dbusObjectPath); + handleServiceAddressPatch(*input.serviceAddressList, asyncResp, + serverT, dbusObjectPath); } - if (userName) + if (input.userName) { - handleUserNamePatch(*userName, asyncResp, serverT, dbusObjectPath); + handleUserNamePatch(*input.userName, asyncResp, serverT, + dbusObjectPath); } - if (password) + if (input.password) { - handlePasswordPatch(*password, asyncResp, serverT, dbusObjectPath); + handlePasswordPatch(*input.password, asyncResp, serverT, + dbusObjectPath); } - if (baseDNList) + if (input.baseDNList) { - handleBaseDNPatch(*baseDNList, asyncResp, serverT, dbusObjectPath); + handleBaseDNPatch(*input.baseDNList, asyncResp, serverT, + dbusObjectPath); } - if (userNameAttribute) + if (input.userNameAttribute) { - handleUserNameAttrPatch(*userNameAttribute, asyncResp, serverT, - dbusObjectPath); + handleUserNameAttrPatch(*input.userNameAttribute, asyncResp, + serverT, dbusObjectPath); } - if (groupsAttribute) + if (input.groupsAttribute) { - handleGroupNameAttrPatch(*groupsAttribute, asyncResp, serverT, + handleGroupNameAttrPatch(*input.groupsAttribute, asyncResp, serverT, dbusObjectPath); } - if (serviceEnabled) + if (input.serviceEnabled) { // if user has given the value as true then enable // the service. if user has given false then no-op // as service is already stopped. - if (*serviceEnabled) + if (*input.serviceEnabled) { - handleServiceEnablePatch(*serviceEnabled, asyncResp, serverT, - dbusObjectPath); + handleServiceEnablePatch(*input.serviceEnabled, asyncResp, + serverT, dbusObjectPath); } } else @@ -1056,10 +1049,10 @@ inline void handleLDAPPatch(nlohmann::json::object_t& input, serverT, dbusObjectPath); } - if (remoteRoleMapData) + if (input.remoteRoleMapData) { handleRoleMapPatch(asyncResp, confData.groupRoleList, serverT, - *remoteRoleMapData); + *input.remoteRoleMapData); } }); } @@ -1304,23 +1297,39 @@ inline void handleAccountServicePatch( std::optional lockoutThreshold; std::optional minPasswordLength; std::optional maxPasswordLength; - std::optional ldapObject; - std::optional activeDirectoryObject; + LdapPatchParams ldapObject; + LdapPatchParams activeDirectoryObject; AuthMethods auth; // clang-format off if (!json_util::readJsonPatch( req, asyncResp->res, "AccountLockoutDuration", unlockTimeout, "AccountLockoutThreshold", lockoutThreshold, + "ActiveDirectory/Authentication/AuthenticationType", activeDirectoryObject.authType, + "ActiveDirectory/Authentication/Password", activeDirectoryObject.password, + "ActiveDirectory/Authentication/Username", activeDirectoryObject.userName, + "ActiveDirectory/LDAPService/SearchSettings/BaseDistinguishedNames", activeDirectoryObject.baseDNList, + "ActiveDirectory/LDAPService/SearchSettings/GroupsAttribute", activeDirectoryObject.groupsAttribute, + "ActiveDirectory/LDAPService/SearchSettings/UsernameAttribute", activeDirectoryObject.userNameAttribute, + "ActiveDirectory/RemoteRoleMapping", activeDirectoryObject.remoteRoleMapData, + "ActiveDirectory/ServiceAddresses", activeDirectoryObject.serviceAddressList, + "ActiveDirectory/ServiceEnabled", activeDirectoryObject.serviceEnabled, + "LDAP/Authentication/AuthenticationType", ldapObject.authType, + "LDAP/Authentication/Password", ldapObject.password, + "LDAP/Authentication/Username", ldapObject.userName, + "LDAP/LDAPService/SearchSettings/BaseDistinguishedNames", ldapObject.baseDNList, + "LDAP/LDAPService/SearchSettings/GroupsAttribute", ldapObject.groupsAttribute, + "LDAP/LDAPService/SearchSettings/UsernameAttribute", ldapObject.userNameAttribute, + "LDAP/RemoteRoleMapping", ldapObject.remoteRoleMapData, + "LDAP/ServiceAddresses", ldapObject.serviceAddressList, + "LDAP/ServiceEnabled", ldapObject.serviceEnabled, "MaxPasswordLength", maxPasswordLength, "MinPasswordLength", minPasswordLength, - "LDAP", ldapObject, - "ActiveDirectory", activeDirectoryObject, "Oem/OpenBMC/AuthMethods/BasicAuth", auth.basicAuth, "Oem/OpenBMC/AuthMethods/Cookie", auth.cookie, "Oem/OpenBMC/AuthMethods/SessionToken", auth.sessionToken, - "Oem/OpenBMC/AuthMethods/XToken", auth.xToken, - "Oem/OpenBMC/AuthMethods/TLS", auth.tls)) + "Oem/OpenBMC/AuthMethods/TLS", auth.tls, + "Oem/OpenBMC/AuthMethods/XToken", auth.xToken)) { return; } @@ -1340,18 +1349,12 @@ inline void handleAccountServicePatch( messages::propertyNotWritable(asyncResp->res, "MaxPasswordLength"); } - if (ldapObject) - { - handleLDAPPatch(*ldapObject, asyncResp, "LDAP"); - } + handleLDAPPatch(std::move(activeDirectoryObject), asyncResp, + "ActiveDirectory"); + handleLDAPPatch(std::move(ldapObject), asyncResp, "LDAP"); handleAuthMethodsPatch(asyncResp, auth); - if (activeDirectoryObject) - { - handleLDAPPatch(*activeDirectoryObject, asyncResp, "ActiveDirectory"); - } - if (unlockTimeout) { setDbusProperty( -- cgit v1.2.3