user_channel: use hardcoded user manager service name
Current user list fetching scheme relies on this D-Bus idiom:
1. Register InterfaceAdded/Removed signals on /xyz/openbmc_project/user
2. Call GetManagedObjects on /xyz/openbmc_project/user
However, taking ObjectMapper into account, this scheme becomes:
1. Register InterfaceAdded/Removed signals on /xyz/openbmc_project/user
2. Call GetObject on /xyz/openbmc_project/user (to get service name)
3. Call GetManagedObjects on /xyz/openbmc_project/user
If step 2 of the latter scheme fails because ObjectMapper has not
finished introspecting yet, we basically lose the initial user list.
This is what happened in our system.
Do we expect some arbitrary services to be able to provide user
management apart from xyz.openbmc_project.User.Manager? According to
OpenBMC User Management design docs [1], user management is documented
to be handled by one common service. bmcweb is also using the hardcoded
user manager name [2]. I also saw Ed had some comments regarding
some usage of ObjectMapper on discord [3].
[1] https://github.com/openbmc/docs/blob/9168592c88de40c802823ba9ed6267f1b54a4002/architecture/user-management.md
[2] https://github.com/openbmc/bmcweb/blob/1940677a35870b51eaffc50406609124668743d0/redfish-core/lib/roles.hpp#L130-L133
[3] https://discord.com/channels/775381525260664832/867820390406422538/1343372176547643432
This removes the dependency on ObjectMapper inside user management code
and uses the user manager service name directly.
Tested: IPMI user management features work normally
Change-Id: Id0000000d2a84234fc6c61044d7a55e48561b36f
Signed-off-by: Khang D Nguyen <khangng@os.amperecomputing.com>
diff --git a/user_channel/user_mgmt.cpp b/user_channel/user_mgmt.cpp
index bf11462..c041332 100644
--- a/user_channel/user_mgmt.cpp
+++ b/user_channel/user_mgmt.cpp
@@ -57,15 +57,6 @@
static constexpr const char* intfAddedSignal = "InterfacesAdded";
static constexpr const char* intfRemovedSignal = "InterfacesRemoved";
-// Object Mapper related
-static constexpr const char* objMapperService =
- "xyz.openbmc_project.ObjectMapper";
-static constexpr const char* objMapperPath =
- "/xyz/openbmc_project/object_mapper";
-static constexpr const char* objMapperInterface =
- "xyz.openbmc_project.ObjectMapper";
-static constexpr const char* getObjectMethod = "GetObject";
-
static constexpr const char* ipmiUserMutex = "ipmi_usr_mutex";
static constexpr const char* ipmiMutexCleanupLockFile =
"/var/run/ipmi/ipmi_usr_mutex_cleanup";
@@ -77,6 +68,8 @@
static constexpr size_t privMask = 0xF;
// User manager related
+static constexpr const char* userMgrService =
+ "xyz.openbmc_project.User.Manager";
static constexpr const char* userMgrObjBasePath = "/xyz/openbmc_project/user";
static constexpr const char* userObjBasePath = "/xyz/openbmc_project/user";
static constexpr const char* userMgrInterface =
@@ -126,30 +119,6 @@
std::unique_ptr<sdbusplus::bus::match_t> userPropertiesSignal
__attribute__((init_priority(101)));
-// TODO: Below code can be removed once it is moved to common layer libmiscutil
-std::string getUserService(sdbusplus::bus_t& bus, const std::string& intf,
- const std::string& path)
-{
- auto mapperCall = bus.new_method_call(objMapperService, objMapperPath,
- objMapperInterface, getObjectMethod);
-
- mapperCall.append(path);
- mapperCall.append(std::vector<std::string>({intf}));
-
- auto mapperResponseMsg = bus.call(mapperCall);
-
- std::map<std::string, std::vector<std::string>> mapperResponse;
- mapperResponseMsg.read(mapperResponse);
-
- if (mapperResponse.begin() == mapperResponse.end())
- {
- throw sdbusplus::exception::SdBusError(
- -EIO, "ERROR in reading the mapper response");
- }
-
- return mapperResponse.begin()->first;
-}
-
void setDbusProperty(sdbusplus::bus_t& bus, const std::string& service,
const std::string& objPath, const std::string& interface,
const std::string& property,
@@ -173,25 +142,6 @@
}
}
-std::string getUserServiceName()
-{
- static sdbusplus::bus_t bus(ipmid_get_sd_bus_connection());
- static std::string userMgmtService;
- if (userMgmtService.empty())
- {
- try
- {
- userMgmtService =
- ipmi::getUserService(bus, userMgrInterface, userMgrObjBasePath);
- }
- catch (const sdbusplus::exception_t& e)
- {
- userMgmtService.clear();
- }
- }
- return userMgmtService;
-}
-
UserAccess& getUserAccessObject()
{
static UserAccess userAccess;
@@ -401,7 +351,7 @@
try
{
auto method = bus.new_method_call(
- getUserServiceName().c_str(), msg.get_path(),
+ userMgrService, msg.get_path(),
dBusPropertiesInterface, getAllPropertiesMethod);
method.append(usersInterface);
auto reply = bus.call(method);
@@ -574,9 +524,9 @@
std::map<DbusUserObjPath, DbusUserObjValue> properties;
try
{
- auto method = bus.new_method_call(getUserServiceName().c_str(),
- userMgrObjBasePath, dBusObjManager,
- getManagedObjectsMethod);
+ auto method =
+ bus.new_method_call(userMgrService, userMgrObjBasePath,
+ dBusObjManager, getManagedObjectsMethod);
auto reply = bus.call(method);
reply.read(properties);
}
@@ -814,7 +764,7 @@
sdbusplus::message::object_path tempUserPath(userObjBasePath);
tempUserPath /= userName;
std::string userPath(tempUserPath);
- setDbusProperty(bus, getUserServiceName(), userPath, usersInterface,
+ setDbusProperty(bus, userMgrService, userPath, usersInterface,
userEnabledProperty, enabledState);
userInfo->userEnabled = enabledState;
try
@@ -915,7 +865,7 @@
sdbusplus::message::object_path tempUserPath(userObjBasePath);
tempUserPath /= userName;
std::string userPath(tempUserPath);
- setDbusProperty(bus, getUserServiceName(), userPath, usersInterface,
+ setDbusProperty(bus, userMgrService, userPath, usersInterface,
userPrivProperty, priv);
}
userInfo->userPrivAccess[chNum].privilege = privAccess.privilege;
@@ -1026,9 +976,9 @@
std::string userPath(tempUserPath);
try
{
- auto method = bus.new_method_call(
- getUserServiceName().c_str(), userPath.c_str(),
- deleteUserInterface, deleteUserMethod);
+ auto method =
+ bus.new_method_call(userMgrService, userPath.c_str(),
+ deleteUserInterface, deleteUserMethod);
auto reply = bus.call(method);
}
catch (const sdbusplus::exception_t& e)
@@ -1048,9 +998,9 @@
return ccUnspecifiedError;
}
// Create new user
- auto method = bus.new_method_call(
- getUserServiceName().c_str(), userMgrObjBasePath,
- userMgrInterface, createUserMethod);
+ auto method =
+ bus.new_method_call(userMgrService, userMgrObjBasePath,
+ userMgrInterface, createUserMethod);
method.append(userName.c_str(), availableGroups,
ipmiPrivIndex[PRIVILEGE_USER], false);
auto reply = bus.call(method);
@@ -1077,9 +1027,9 @@
try
{
// User rename
- auto method = bus.new_method_call(
- getUserServiceName().c_str(), userMgrObjBasePath,
- userMgrInterface, renameUserMethod);
+ auto method =
+ bus.new_method_call(userMgrService, userMgrObjBasePath,
+ userMgrInterface, renameUserMethod);
method.append(oldUser.c_str(), userName.c_str());
auto reply = bus.call(method);
}
@@ -1523,9 +1473,9 @@
std::map<std::string, PrivAndGroupType> properties;
try
{
- auto method = bus.new_method_call(
- getUserServiceName().c_str(), userMgrObjBasePath,
- dBusPropertiesInterface, getAllPropertiesMethod);
+ auto method = bus.new_method_call(userMgrService, userMgrObjBasePath,
+ dBusPropertiesInterface,
+ getAllPropertiesMethod);
method.append(userMgrInterface);
auto reply = bus.call(method);
@@ -1676,9 +1626,9 @@
std::map<DbusUserObjPath, DbusUserObjValue> managedObjs;
try
{
- auto method = bus.new_method_call(getUserServiceName().c_str(),
- userMgrObjBasePath, dBusObjManager,
- getManagedObjectsMethod);
+ auto method =
+ bus.new_method_call(userMgrService, userMgrObjBasePath,
+ dBusObjManager, getManagedObjectsMethod);
auto reply = bus.call(method);
reply.read(managedObjs);
}