From f365910caf848c4cbc77f2177e66d1f2498c457d Mon Sep 17 00:00:00 2001 From: Gunnar Mills Date: Wed, 29 Jan 2020 16:23:28 -0600 Subject: Update ManagerAccountCollection Privilege For the ManagerAccountCollection resource, /redfish/v1/AccountService/Accounts/, allow a "ConfigureSelf" user to GET but only return the user's account if the user does not have ConfigureUsers privilege. Took this code from other places in account_service. https://github.com/openbmc/bmcweb/blob/61dbeef97168db1a1f7a351c5f95e09afd361e48/redfish-core/lib/account_service.hpp#L1528 There was some question whether all accounts should be returned, Redfish clarified that only the user's account should be returned without ConfigureUsers privilege. https://redfishforum.com/thread/281/manageraccountcollection-change-allows-account-enumeration "we assumed that the Login privilege would only pertain to the current account and not allow viewing of other accounts" This fixes 2 Redfish validator errors if running the validator as a Readonly or Operator role. "ERROR - Accounts: GET of resource at URI /redfish/v1/AccountService/Accounts returned HTTP 403. Check URI." "ERROR - /redfish/v1/AccountService/Accounts: URI could not be acquired: 403" This was changed in Redfish 2019.3, redfish issue 1914 explains more. Tested: Ran the validator as operator role and admin role. No errors. As root: curl -k https://${bmc}/redfish/v1/AccountService/Accounts/ { "@odata.id": "/redfish/v1/AccountService/Accounts", "@odata.type": "#ManagerAccountCollection.ManagerAccountCollection", "Description": "BMC User Accounts", "Members": [ { "@odata.id": "/redfish/v1/AccountService/Accounts/readonly" }, { "@odata.id": "/redfish/v1/AccountService/Accounts/operator" }, { "@odata.id": "/redfish/v1/AccountService/Accounts/JimHalpert" }, { "@odata.id": "/redfish/v1/AccountService/Accounts/root" } ], "Members@odata.count": 4, As Operator: curl -k https://${bmc}/redfish/v1/AccountService/Accounts/ { "@odata.id": "/redfish/v1/AccountService/Accounts", "@odata.type": "#ManagerAccountCollection.ManagerAccountCollection", "Description": "BMC User Accounts", "Members": [ { "@odata.id": "/redfish/v1/AccountService/Accounts/operator" } ], "Members@odata.count": 1, Change-Id: I0694011ed3c4ecd3ea0c386fc24d086be39ac804 Signed-off-by: Gunnar Mills --- redfish-core/lib/account_service.hpp | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) (limited to 'redfish-core/lib/account_service.hpp') diff --git a/redfish-core/lib/account_service.hpp b/redfish-core/lib/account_service.hpp index 7028260327..9f989911c9 100644 --- a/redfish-core/lib/account_service.hpp +++ b/redfish-core/lib/account_service.hpp @@ -1323,8 +1323,14 @@ class AccountsCollection : public Node Node(app, "/redfish/v1/AccountService/Accounts/") { entityPrivileges = { + // According to the PrivilegeRegistry, GET should actually be + // "Login". A "Login" only privilege would return an empty "Members" + // list. Not going to worry about this since none of the defined + // roles are just "Login". E.g. Readonly is {"Login", + // "ConfigureSelf"}. In the rare event anyone defines a role that + // has Login but not ConfigureSelf, implement this. {boost::beast::http::verb::get, - {{"ConfigureUsers"}, {"ConfigureManager"}}}, + {{"ConfigureUsers"}, {"ConfigureSelf"}}}, {boost::beast::http::verb::head, {{"Login"}}}, {boost::beast::http::verb::patch, {{"ConfigureUsers"}}}, {boost::beast::http::verb::put, {{"ConfigureUsers"}}}, @@ -1344,8 +1350,8 @@ class AccountsCollection : public Node {"Description", "BMC User Accounts"}}; crow::connections::systemBus->async_method_call( - [asyncResp](const boost::system::error_code ec, - const ManagedObjectType& users) { + [asyncResp, &req, this](const boost::system::error_code ec, + const ManagedObjectType& users) { if (ec) { messages::internalError(asyncResp->res); @@ -1356,7 +1362,6 @@ class AccountsCollection : public Node asyncResp->res.jsonValue["Members"]; memberArray = nlohmann::json::array(); - asyncResp->res.jsonValue["Members@odata.count"] = users.size(); for (auto& user : users) { const std::string& path = @@ -1370,10 +1375,22 @@ class AccountsCollection : public Node { lastIndex += 1; } - memberArray.push_back( - {{"@odata.id", "/redfish/v1/AccountService/Accounts/" + - path.substr(lastIndex)}}); + + // As clarified by Redfish here: + // https://redfishforum.com/thread/281/manageraccountcollection-change-allows-account-enumeration + // Users without ConfigureUsers, only see their own account. + // Users with ConfigureUsers, see all accounts. + if (req.session->username == path.substr(lastIndex) || + isAllowedWithoutConfigureSelf(req)) + { + memberArray.push_back( + {{"@odata.id", + "/redfish/v1/AccountService/Accounts/" + + path.substr(lastIndex)}}); + } } + asyncResp->res.jsonValue["Members@odata.count"] = + memberArray.size(); }, "xyz.openbmc_project.User.Manager", "/xyz/openbmc_project/user", "org.freedesktop.DBus.ObjectManager", "GetManagedObjects"); -- cgit v1.2.3