summaryrefslogtreecommitdiff
path: root/test
AgeCommit message (Collapse)AuthorFilesLines
2023-12-05Unit test ConnectionEd Tanous1-0/+85
Boost asio provides a test stream object that we can use to begin unit testing the connection object. This patchset uses it to re-enable some simple http1.1 tests. There's some features that have snuck into the connection class that aren't compatible with a stream (like ip address getting), so unfortunately we do need the connection class to be aware if it's in test mode, but that tradeoff seems worthwhile. Tested: Unit test pass. Change-Id: Id8b1f8866582b58502dbafe6139f841bf64b8ef3 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-10-31Move to file_body in boostEd Tanous1-14/+14
As is, it reads the whole file into memory before sending it. While fairly fast for the user, this wastes ram, and makes bmcweb less useful on less capable systems. This patch enables using the boost::beast::http::file_body type, which has more efficient serialization semantics than using a std::string. To do this, it adds a openFile() handler to http::Response, which can be used to properly open a file. Once the file is opened, the existing string body is ignored, and the file payload is sent instead. openFile() also returns success or failure, to allow users to properly handle 404s and other errors. To prove that it works, I moved over every instance of direct use of the body() method over to using this, including the webasset handler. The webasset handler specifically should help with system load when doing an initial page load of the webui. Tested: Redfish service validator passes. Change-Id: Ic7ea9ffefdbc81eb985de7edc0fac114822994ad Signed-off-by: Ed Tanous <ed@tanous.net>
2023-10-05Update to boost 1.83.0Ed Tanous1-1/+1
In boost 1.83.0, the boost::url maintainers deprecated the header only usage of the library without warning. A discussion with the maintainers[1] made it clear that they removed the abiliy on purpose, and they're not going to add it back or add a deprecation strategy (they did say they would update the documentation to actually match the intent), and that from here on in we should be using the cmake boost project to pull in the non-header-only boost libraries we use (which at this point is ONLY boost url). This commit updates to remove the usage of boost::urls::result typedef, which was deprecated in this release (which causes a compile error) and moves it to boost::system::result. In addition, it updates our meson files to pull in the boost project as a cmake dependency. [1] https://cpplang.slack.com/archives/C01JR6C9C4U/p1696441238739129 Tested: Not yet. Change-Id: Ia7adfc0348588915440687c3ab83a1de3e6b845a Signed-off-by: Ed Tanous <edtanous@google.com>
2023-09-25Clean up vm CredentialPipeEd Tanous1-0/+41
This code is needlessly complicated for what it does. Even with the intent, which is secure buffer cleanup, it's trivial to encase all this into a single class that accepts the strings by rvalue reference, then cleans them up afterward. Doing this also cleans up a potential lifetime problem, where if the unix socket returned immediately, it would've invalidated the buffers that were being sent. It also moves to async_write, instead of async_write_some. The former could in theory fail if the socket blocks (unlikely in this scenario) but it's good to handle anyway. Tested: Need some help here. There's no backend for this, so we might just have to rely on inspection. Change-Id: I9032d458f8eb7a0689bee575aae611641bacee26 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-09-08Simplify datetime parsingEd Tanous1-3/+2
This code as it stands pulls in the full datetime library from boost, including io, and a bunch of timezone code. The bmc doesn't make use of any of this, so we can rely on a much simplified version. Unfortunately for us, gcc still doesn't implement the c++20 std::chrono::parse[1]. There is a reference library available from [2] that backports the parse function to compilers that don't yet support it, and is the basis for the libc++ version. This commit opts to copy in the header as-written, under the assumption that we will never need to pull in new versions of this library, and will move to the std ersion as soon as it's available in the next gcc version. This commit simplifies things down to improve compile times and binary size. It saves ~22KB of compressed binary size, or about 3%. Tested: Unit tests pass. Pretty good coverage. [1] https://en.cppreference.com/w/cpp/chrono/parse [2] https://github.com/HowardHinnant/date/blob/master/include/date/date.h Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I706b91cc3d9df3f32068125bc47ff0c374eb8d87
2023-08-23Move http client to URLEd Tanous1-45/+0
Type safety is a good thing. In: https://gerrit.openbmc.org/c/openbmc/bmcweb/+/65606 It was found that splitting out the URI into encoded pieces in the early phase removed some information we needed, namely whether or not a URI was ipv6. This commit changes http client such that it passes all the information through, with the correct type, rather than passing in hostname, port, path, and ssl separately. Opportunistically, because a number of log lines are changing, this uses the opportunity to remove a number of calls to std::to_string, and rely on std::format instead. Now that we no longer use custom URI splitting code, the ValidateAndSplitUrl() method can be removed, given that our validation now happens in the URI class. Tested: Aggregation works properly when satellite URIs are queried. Change-Id: I9f605863179af54c5af2719bc5ce9d29cbfffab7 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-08-07Fix bugprone-unchecked-optional-access findingsEd Tanous2-13/+46
Clang-tidy has the aforementioned check, which shows a few places in the core where we ignored the required optional checks. Fix all uses. Note, we cannot enable the check that this time because of some weird code in health.hpp that crashes tidy[1]. That will need to be a future improvement. There are tests that call something like ASSERT(optional) EXPECT(optional->foo()) While this isn't an actual violation, clang-tidy doesn't seem to be smart enough to deal with it, so add some explicit checks. [1] https://github.com/llvm/llvm-project/issues/55530 Tested: Redfish service validator passes. Change-Id: Ied579cd0b957efc81aff5d5d1091a740a7a2d7e3 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-07-14AsyncResolve cleanups and error handlingEd Tanous1-0/+25
The Async DBus resolver really has nothing to do with crow, which is our core http library namespace and has some opportunistic cleanups that can be done. This commit moves it into the bmcweb namespace (unimportantly) and breaks out one of the larger functions such that it can be unit tested, and unit tests it. Tested: Unit tests pass. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ie3cfbb0ef81a027a1ad42358c04967a517471117
2023-07-12Use openssl random number generatorEd Tanous1-0/+23
We already have a generator class. We should use it. Wrap this into a function that can be unit tested, and add unit tests. Note, some files also needed to change name, because random.hpp conflicts with the built in random, and causes circular build problems. This commit changes it to ossl_random. Tested: Unit tests pass. Now has coverage. Redfish service validator passes. Change-Id: I5f8eee1af5f4843a352c6fd0e26d67fd3320ef53 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-06-28Remove the black_magic namespaceEd Tanous2-2/+2
The black_magic namespace has been eradicated of what most would call "black magic" and while there's some non-trivial stuff in there, it's far from the most complicated part of this stack. This commit takes the two remaining things in the black_magic namespace, namely the parameter tagging functionality, and moves them into the utility namespace. Tested: Redfish service validator passes Change-Id: I9e2686fff5ef498cafc4cb83d4d808ea849f7737 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-06-21json utils: add getEstimatedJsonSizeNan Zhou1-0/+54
Add a utility function which estimates the size of the JSON tree. It is used in the children change to limit the reponse size of expand query. Tested: 1. unit test passed; 2. tested on hardware, the following are real sizes and estimation ``` Real payload size, Estimation, query 15.69 KB, 10.21 KB, redfish/v1?$expand=.($levels=1) 95.76 KB, 62.11 KB, redfish/v1?$expand=.($levels=2) 117.14 KB, 72.71 KB, redfish/v1?$expand=.($levels=3) 127.65 KB, 77.64 KB, redfish/v1?$expand=.($levels=4) ``` Signed-off-by: Nan Zhou <nanzhoumails@gmail.com> Change-Id: Iae26d6732a6ec63ecc59eacf657b4bf33c07c046
2023-06-12query: Fix default expand level with delegatedWilly Tu1-28/+65
With delegate expand, the default expand level is -= `queryCapabilities.canDelegateExpandLevel`. This creates an overlap of expand process between delegate expand vs. default expand. With query.expandLevel = 2 -> query.expandLevel = 1 and delegated.expandLevel = 1. Both delegated and default expand will try to only expand of level one instead of level 2 for the default. The code in https://github.com/openbmc/bmcweb/blob/479e899d5f57a67647f83b7f615d2c8565290bcf/redfish-core/include/utils/query_param.hpp#L583-L597 stated that the level with "@odata.id" + other property is treated as a seperate level. So with `query.expandLevel = 1` it just loop through the id that was already expanded and is noop. Tested: Before: /redfish/v1/Chassis/BMC/Sensors?$expand=.($levels=2) returns the same result as level=1. Needs level=3 to expand to the next level. The RelatedItem in here doesn't get expanded with level=2. ``` wget -qO- 'http://localhost:80/redfish/v1/Chassis/BMC/Sensors?$expand=.($levels=1)' ... { "@odata.id": "/redfish/v1/Chassis/BMC/Sensors/temperature_DIMMXX", "@odata.type": "#Sensor.v1_2_0.Sensor", "Id": "temperature_DIMMXX", "Name": "DIMMXX", "Reading": 30.0, "ReadingRangeMax": 127.0, "ReadingRangeMin": -128.0, "ReadingType": "Temperature", "ReadingUnits": "Cel", "RelatedItem": [ { "@odata.id": "/redfish/v1/Systems/system/Memory/dimmXX" } ], "Status": { "Health": "OK", "State": "Enabled" }, "Thresholds": { "LowerCaution": { "Reading": null }, "LowerCritical": { "Reading": null }, "UpperCaution": { "Reading": 93.0 }, "UpperCritical": { "Reading": 95.0 } } } ], "Members@odata.count": 242, "Name": "Sensors" } ``` After: level=2 was able to expand to the next level. Change-Id: I542177a94a33f8df7afbb68837f3a53b86140c86 Signed-off-by: Willy Tu <wltu@google.com>
2023-06-09Fix hack on Set-CookieEd Tanous1-1/+1
This is one that I couldn't figure out for a while. Turns out that fields has both a set() and an insert() method. Whereas set() replaces, insert() appends, which is what we want in this case. This allows us to call the actual methods several times, instead of essentially string injecting our own code, which should make it clearer. At the same time, there was one unit test that was structured such that it was using addHeader to clear a header, so this commit adds an explicit "clearHeader()" method, so we can be explicit. Tested: Logging into the webui in chrome (which uses POST /login) shows: 401 with no cookie header if the incorrect password is used 200 with 2 Set-Cookie headers set: Set-Cookie: SESSION=<session tag>; SameSite=Strict; Secure; HttpOnly Set-Cookie: XSRF-TOKEN=<token tag>; SameSite=Strict; Secure Change-Id: I9b87a48ea6ba892fc08e66940563dea86edb9a65 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-06-08hex_units: Fix Werror=conversionWilly Tu1-2/+2
Convert all types to uint8_t to not hit the conversion warning. Change-Id: Ia535ca0a2f4045cbde06a2f8f8eaad9570a0f4a5 Signed-off-by: Willy Tu <wltu@google.com> Signed-off-by: Ed Tanous <edtanous@google.com>
2023-05-26json utility: add sortNan Zhou1-0/+53
This commit adds a utility function |sortJsonArrayByKey|. It can sort an json array by value of a given key of each element. Use cases includes: 1. sort the MemberCollection by @odata.id Tested: 1. unit test passed; Signed-off-by: Nan Zhou <nanzhoumails@gmail.com> Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Idc175fab3af5c6102a5a3439b712b659ecb76468
2023-05-25Fix some includesEd Tanous2-2/+4
System includes should be included with <>, in-tree includes should be included with "". This was found manually, with the help of the following grep statement[1]. git grep -o -h "#include .*" | sort | uniq Tested: Code compiles Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I1a6b2a5ba35ccbbb61c67b7c4b036a2d7b3a36a3
2023-05-20Added new pre-defined usergroup called hostconsoleNinad Palsule1-0/+9
The new pre-defined usergroup named "hostconsole" is added to differentiate access between host console and manager console. The only users allowed to interact with host console are part of the "hostconsole" group and they are in an administrator role. Note: The changes are spread across multiple repositories listed under "Related commits:" The bmcweb changes to incorporate new group are as follows: - The new user is added in the hostconsole group only if it has an administrative role. - The ssh usergroup is only translated to ManagerConsole redfish group and hostconsole usergroup is translated to HostConsole redfish group. - The following changes are made to check the privileges for host console access - The new OEM privilege "OpenBMCHostConsole" added for host console access. This privilege is not shared externally hence it is not documented. - Updated obmc_console BMCWEB_ROUTE to use the new privilege. - Router functions now save user role and user groups in the session - getUserPrivileges() function now takes session reference instead of user role. This function now also checks for the user group "hostconsole" and add the new privilege if user is member of this group. - Updated all callers of the getUserPrivileges to pass session reference. - Added test to validate that new privilege is set correctly. Tested: Loaded code on the system and validated that; - New user gets added in hostconsole group. NOTE: Prior to this commit all groups are assigned to new user. This drop does not change that behavior. - Access from the web gui is only available for users in hostconsole group. Used IBM internal simulator called simics to test this. This simulator allows accessing openbmc from GUI. - Checked the role collection and there is no change. $ curl -k -H "X-Auth-Token: $TOKEN" -X GET \ https://${bmc}/redfish/v1/AccountService/Roles $ curl -k -H "X-Auth-Token: $TOKEN" -X GET \ https://${bmc}/redfish/v1/AccountService/Roles/Administrator $ curl -k -H "X-Auth-Token: $TOKEN" -X GET \ https://${bmc}/redfish/v1/AccountService/Roles/ReadOnly $ curl -k -H "X-Auth-Token: $TOKEN" -X GET \ https://${bmc}/redfish/v1/AccountService/Roles/Operator - HostConsole is in AccountType when hostconsole group is present in UserGroups D-Bus property $ id user99 uid=1006(user99) gid=100(users) groups=1000(priv-admin),1005(web),\ 1006(redfish),1013(hostconsole),100(users) $ curl -k https://${bmc}/redfish/v1/AccountService/Accounts/user99 { "@odata.id": "/redfish/v1/AccountService/Accounts/user99", "@odata.type": "#ManagerAccount.v1_4_0.ManagerAccount", "AccountTypes": [ "HostConsole", "Redfish", "WebUI", "ManagerConsole" ], "Description": "User Account", "Enabled": true, "Id": "user99", "Links": { "Role": { "@odata.id": "/redfish/v1/AccountService/Roles/Administrator" } }, "Locked": false, "Locked@Redfish.AllowableValues": [ "false" ], "Name": "User Account", "Password": null, "PasswordChangeRequired": false, "RoleId": "Administrator", "UserName": "user99" - The hostconsole group is not present for readonly or operator users and also made sure that console access is not provided. This testing is done one the system and console access was tried by modifying the https://github.com/openbmc/bmcweb/blob/master/scripts/websocket_test.py + curl -k https://${bmc}/redfish/v1/AccountService/Accounts/user99 { "@odata.id": "/redfish/v1/AccountService/Accounts/user99", "@odata.type": "#ManagerAccount.v1_4_0.ManagerAccount", "AccountTypes": [ "IPMI", "Redfish", "WebUI", "ManagerConsole" ], "Description": "User Account", "Enabled": true, "Id": "user99", "Links": { "Role": { "@odata.id": "/redfish/v1/AccountService/Roles/ReadOnly" } }, "Locked": false, "Locked@Redfish.AllowableValues": [ "false" ], "Name": "User Account", "Password": null, "PasswordChangeRequired": false, "RoleId": "ReadOnly", "UserName": "user99" [INFO "http_connection.hpp":209] Request: 0x150ac38 HTTP/1.1 GET /console0 ::ffff:x.x.xx.xxx [DEBUG "routing.hpp":1265] Matched rule (upgrade) '/console0' 1 / 2 [DEBUG "routing.hpp":1084] userName = user99 userRole = priv-user [DEBUG "routing.hpp":1123] IsUserPrivileged: group=ipmi [DEBUG "routing.hpp":1123] IsUserPrivileged: group=redfish [DEBUG "routing.hpp":1123] IsUserPrivileged: group=ssh [DEBUG "routing.hpp":1123] IsUserPrivileged: group=web [DEBUG "routing.hpp":93] checkPrivileges: BASE USER: Login [DEBUG "routing.hpp":93] checkPrivileges: BASE USER: ConfigureSelf [DEBUG "routing.hpp":113] checkPrivileges: OEM REQUIRED: OpenBMCHostConsole [ERROR "routing.hpp":1192] Insufficient Privilege + curl -k https://${bmc}/redfish/v1/AccountService/Accounts/user99 { "@odata.id": "/redfish/v1/AccountService/Accounts/user99", "@odata.type": "#ManagerAccount.v1_4_0.ManagerAccount", "AccountTypes": [ "IPMI", "Redfish", "WebUI", "ManagerConsole" ], "Description": "User Account", "Enabled": true, "Id": "user99", "Links": { "Role": { "@odata.id": "/redfish/v1/AccountService/Roles/Operator" } }, "Locked": false, "Locked@Redfish.AllowableValues": [ "false" ], "Name": "User Account", "Password": null, "PasswordChangeRequired": false, "RoleId": "Operator", "UserName": "user99" [INFO "http_connection.hpp":209] Request: 0x21c7c38 HTTP/1.1 GET /console0 ::ffff:x.x.xx.xxx [DEBUG "routing.hpp":1265] Matched rule (upgrade) '/console0' 1 / 2 [DEBUG "routing.hpp":1084] userName = user99 userRole = priv-operator [DEBUG "routing.hpp":1123] IsUserPrivileged: group=ipmi [DEBUG "routing.hpp":1123] IsUserPrivileged: group=redfish [DEBUG "routing.hpp":1123] IsUserPrivileged: group=ssh [DEBUG "routing.hpp":1123] IsUserPrivileged: group=web [DEBUG "routing.hpp":93] checkPrivileges: BASE USER: Login [DEBUG "routing.hpp":93] checkPrivileges: BASE USER: ConfigureComponents [DEBUG "routing.hpp":93] checkPrivileges: BASE USER: ConfigureSelf [DEBUG "routing.hpp":113] checkPrivileges: OEM REQUIRED: OpenBMCHostConsole [ERROR "routing.hpp":1192] Insufficient Privilege Related commits: NOTE: docs, openbmc, obmc-console changes are already merged. bmcweb and phosphor-user-manager will be merged together. docs: https://gerrit.openbmc.org/c/openbmc/docs/+/60968 phosphor-user-manager: https://gerrit.openbmc.org/c/openbmc/phosphor-user-manager/+/61583 openbmc: https://gerrit.openbmc.org/c/openbmc/openbmc/+/61582 obmc-console: https://gerrit.openbmc.org/c/openbmc/obmc-console/+/61581 bmcweb: https://gerrit.openbmc.org/c/openbmc/bmcweb/+/61580 Change-Id: Ia5a33dafc9a76444e6a8e74e752f0f90cb0a31c8 Signed-off-by: Ninad Palsule <ninadpalsule@us.ibm.com>
2023-05-19Write test capable of catching regressionsEd Tanous1-13/+37
This is a pretty simple test, but should be able to catch the regression injected in the previous commit. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I67de097059a6e0dd8d2c02c1aa6c69954a6d7be3
2023-05-18Add a unit test for dumpHtmlEd Tanous1-0/+41
We have no unit tests for this. This isn't very extensive, but we should have at least one. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I212ee528b354f2ed47f88076be009fd6e16fb760
2023-05-16Boost::urls::formatEd Tanous1-34/+1
Boost 1.82 dropped a lovely new toy, boost::urls::format, which is a lot like our urlFromPieces method, but better in that it makes the resulting uris more readable, and allows doing things like fragments in a single line instead of multiple. We should prefer it in some cases. Tested: Redfish service validator passes. Spot checks of URLs work as expected. Unit tests pass. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ia7b38f0a95771c862507e7d5b4aa68aa1c98403c
2023-05-12Update RedfishVersion to 1.17.0Ed Tanous1-1/+1
The latest version of the specification is 1.17.0, and arguably, we should be updating this every time we pull in any new feature, but that hasn't happened. So far as I'm aware, there are no tools that actually look at this parameter to make branching decisions in the client about supported features, so the likelihood this has impact is basically nil. Tested: GET /redfish/v1 returns RedfishVersion of 1.17.0 Redfish service validator passes. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I055b6010329599b7b39c587fa85faf51a38c9b57
2023-05-12ibm-locks: shut up clang-tidy by disabling codePatrick Williams1-1/+4
There is code in ibm/locks that has had clang-tidy warnings disabled for a while due to multiple safety and endianness issues. The code has not been fixed in a while and with clang-16 it is unable to be exempted further. Disable it until someone who cares can fix this in the proper way. ``` ../include/ibm/locks.hpp:522:14: error: 'p' is an unsafe pointer used for buffer access [-Werror,-Wunsafe-buffer-usage] uint8_t* p = reinterpret_cast<uint8_t*>(&resourceId1); ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../include/ibm/locks.hpp:527:25: note: used in buffer access here uint8_t pPosition = p[position]; ^ ../include/ibm/locks.hpp:524:14: error: 'q' is an unsafe pointer used for buffer access [-Werror,-Wunsafe-buffer-usage] uint8_t* q = reinterpret_cast<uint8_t*>(&resourceId2); ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../include/ibm/locks.hpp:529:25: note: used in buffer access here uint8_t qPosition = q[position]; ``` Signed-off-by: Patrick Williams <patrick@stwcx.xyz> Change-Id: I8a7fcbed1099419ad1715c86ffcbfef20820251e
2023-05-12use emplace where appropriate per clang-tidyPatrick Williams1-2/+2
The clang-tidy warning 'modernize-use-emplace' correctly flags a few places where emplace should be used over push. Signed-off-by: Patrick Williams <patrick@stwcx.xyz> Change-Id: I6ca79285a87d6927e718345dc8dce0387e6b1eda
2023-05-12Don't rely on implicit operator comparisonEd Tanous1-1/+2
Json has a bunch of implicit overloaded operators for comparing vectors/arrays/maps with their json counterparts. Unfortunately, when used in unit tests, these cause warnings in clang, so update the code to use the ElementsAre check from gmock. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I658557cb59d568fd50cf6f3bef73d6f90b5c56cf
2023-05-11clang-format: copy latest and re-formatPatrick Williams9-21/+20
clang-format-16 has some backwards incompatible changes that require additional settings for best compatibility and re-running the formatter. Copy the latest .clang-format from the docs repository and reformat the repository. Change-Id: I75f89d2959b0f1338c20d72ad669fbdc1d720835 Signed-off-by: Patrick Williams <patrick@stwcx.xyz>
2023-05-02Aggregation: Process subordinate top collectionsCarson Labrado1-0/+157
Adds a function to process responses from URIs that are uptree from a top level collection. A follow-up patch will hook this into the aggregation code to allow adding links to top level collections which are only supported by satellite BMCs. Adds test cases to validate this function is working correctly. Tested: New test cases pass Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: I7f0fd6c3955398e2fde136c1d3b37a6bf4bf06b9
2023-04-27Remove number support from the routerEd Tanous2-18/+19
The router historically came from crow. Crow supported wildcards of <int>, <float>, and <double>. bmcweb doesn't use them, nor should it in basically any case, as we now have explicit 404 handling. This commit removes them. This amounts to about -450 lines of code, but it's some of the scarier code we have, some of it existing in the namespace "black_magic". Reducing the brain debt for people working in this subsystem seems worthwhile. There is no case in the future where we would use integer based url parameters. Tested: Redfish service validator passes. Should be good enough coverage for a code removal. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I34add8df7d3486952474ca7ec3dc6be990c50ed0
2023-04-07Aggregation: Check for subordinate collectionCarson Labrado1-0/+129
Adds a search function which is able to determine if a passed URI is a top level collection, is uptree from a top level collection, or both. The type being searched for depends on a second argument passed to the function. Each of these searches are used to add links to top level collections which are only supported by a satellite BMC. They all use similar steps so rolling them into a single function cuts down on redundant code. Adds test cases to verify the implementation is correct. Tested: New test cases pass Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: I72ae7442d5f314656b57a73aee544bca516fa7c2
2023-03-28Aggregation: Prefix fix HttpHeaders propertyCarson Labrado1-0/+33
The "HttpHeaders" property in a response is an array of HTTP headers. We perform prefix fixing on the "Location" header from responses so we should also fix any "Location" headers which are contained by "HttpHeaders" in an aggregated response. This requires special handling since each header is represented as a single string in the response. Added testcase for HttpHeaders property Tested: All unit tests pass Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: I3040c4ea52b2bebcb6e206bb50585c6a75538f0a
2023-03-14Only parse to the depth requestedEd Tanous1-16/+162
There are cases in aggregation where an expand parameter might get forwarded to a client. Because our previous expand algorithm assumed that any endpoint within bmcweb would only produce "depth=1" responses, it was reasonable to assume that the pre-response could not contain expanded content. Aggregated resources can't make that assumption. This commit attempts to pass through depth through the request, to ensure that we only expand the level that the user requested, and not any level returned by the request. This is done by using the existence of the resource identifer "@odata.id" to indicate each level in an expanded response. This should be fine since the Redfish spec requires that property to exist. Added unit tests to cover aggregation scenarios. Modified existing $expand tests to comply with the resource identifier dependency. Tested: New unit tests pass Queried '/redfish/v1/Systems?$expand=.($levels=2)' on an aggregated system whose satellite BMC supported $expand. The overall response was correctly expanded for both resources on the aggregating BMC as well as on the satellite BMC. Expanding the satellite resources did not require sending multiple queries to the satellite. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I20ba60ee39bac11ffb3fe1768cec6299cf9ee13e Signed-off-by: Carson Labrado <clabrado@google.com>
2023-03-11Remove body member from RequestEd Tanous1-12/+8
Per cpp core guidelines, these should be methods. Tested: on last patchset of the series. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ib16479db9d2b68da68e7ad6e825c7e205c64f1de
2023-03-02Add ManagerProvidingService implementationEd Tanous1-2/+5
This property was added in Redfish 2022.3 to allow clients to determine which manager is hosting the ServiceRoot, such that they can find uptime statistics, and other metrics from that resource, without needing to attach them directly to serviceroot. Tested: Redfish service validator passes. GET /redfish/v1/Managers/bmc returns the expected response. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: If2b78528d1499fbdae46120e1a1792ecf7ceb1d3
2023-02-28Move getMessage and getMessageFromRegistry to cpp and add testSui Chen1-0/+32
This change moves getMessage and getMessageFromRegistry to a .cpp file so that they can be easily tested. Tested: Unit test passes Signed-off-by: Sui Chen <suichen@google.com> Change-Id: Ia9fc91e5a47036198bf013ff3ea21ea9f6d5259a
2023-02-24Pass string views by valueEd Tanous1-3/+3
string_view should always be passed by value; This commit is a sed replace of the code to make all string_views pass by value, per general coding guidelines[1]. [1] https://quuxplusone.github.io/blog/2021/11/09/pass-string-view-by-value/ Tested: Code compiles. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I55b342a29a0fbfce0a4ed9ea63db6014d03b134c
2023-02-23Update most resources to use urlFromPiecesWilly Tu1-0/+18
Only id in event_service and account_service have not been updated due to the risk of it breaking the username/id. It will require further testing to verify. Use urlFromPieces wherever that is needed to insert a variable in the URI. Don't use urlFromPieces when it is hardcoded values. This allow us to control all resource URIs that is dynamically added and to sync with the current recommanded method for `@odata.id`. The goal is to have a common place to manage the url created from dbus-paths in order to manage/update it easily when needed. Tested: RedfishValidtor Passed for all resource including the sensors with the fragments. Change-Id: I95cdfaaee58fc7f21c95f5944e1e5c813b3215f2 Signed-off-by: Willy Tu <wltu@google.com> Signed-off-by: Ed Tanous <edtanous@google.com>
2023-02-22Aggregation: Fix cppcheck errorsCarson Labrado1-1/+1
Corrections style complaints in the aggregator code. Tested: Jenkins output did not show any style complaints Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: I87426fcf2a48448a62152e0ad4a6c3aa54a7fc45
2023-02-21Disable token compress in strEd Tanous1-2/+23
There are certain cases where we use this split function, and we expect tokens to be read out. For example: /xyz/openbmc_project/sensors/unit/name Should split into a "" in the first position. This use case is not common, and a quick grep shows only two places in the code expect this behavior. Boost::split has this behavior already, which is what this function is emulating. While we could fix these, in the end they should be following the rules outlined in COMMON_ERRORS.md, which disallow this kind of parsing completely. Tested: New unit tests passing. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Iec3dcbf2b495b2b3b4ed419172c4133b16f7c65d
2023-02-17Add option for validating content-type headerEd Tanous1-2/+5
For systems implementing to the OWASP security guidelines[1] (of which all should ideally) we should be checking the content-type header all times that we parse a request as JSON. This commit adds an option for parsing content-type, and sets a default of "must get content-type". Ideally this would not be a breaking change, but given the number of guides and scripts that omit the content type, it seems worthwhile to add a trapdoor, such that people can opt into their own model on how they would like to see this checking work. Tested: ``` curl --insecure -H "Content-Type: application/json" -X POST -D headers.txt https://${bmc}/redfish/v1/SessionService/Sessions -d '{"UserName":"root", "Password":"0penBmc"}' ``` Succeeds. Removing Content-Type argument causes bmc to return Base.1.13.0.UnrecognizedRequestBody. [1] cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html Change-Id: Iaa47dd563b40036ff2fc2cacb70d941fd8853038 Signed-off-by: Ed Tanous <edtanous@google.com>
2023-02-17Implement alternative to on boost::splitEd Tanous1-0/+31
boost::split has a documented false-positive in clang-tidy. While normally we'd handle this with NOLINTNEXTLINE, this doesn't appear to work in all cases. Unclear why, but seems to be due to some of our lambda callback complexity. Each of these uses is a case where we should be using a more specific check, rather than split, but for the moment, this is the best we have. Tested: clang-tidy passes. [1] https://github.com/llvm/llvm-project/issues/40486 Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I144c6610cb740287b7225e2be03b4142a64f9563
2023-02-13Fix a couple #includesEd Tanous1-0/+1
In the continual quest to get tidy passing when run in isolation, fix some more includes. This includes removing a circular #include to app.hpp. We don't use app.hpp in these files, which is why our code compiles but having this include it here causes a few circular dependencies app.hpp -> http_server.hpp -> persistent_data.hpp -> app.hpp. app.hpp -> http_server.hpp -> authentication.hpp -> app.hpp. This confuses clang when run on header files directly. Fix a couple more includes at the same time. Tested: Code compiles Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ib62d78b49c7e38ef7061c9fbbf6b3d463f11917d
2023-02-11Aggregation: Fix up aggregated response header URIsKhang Kieu1-0/+38
The aggregator did not propagate header's fields from aggregated responses. This change will take into account of response code other than 200, which will modify a field called "Location". The Location field in the response's header will point to where the response data can be read from. This "Location" field in response Header will now contain the correct URI with the prefix appended. We will also copy over other Header Values to aggregated response. These header values include "Content-Type", "Allow", "Retry-After", and also the response's body Added some test cases for the above fixes. Tested: Unit Tests pass. Queries reponse that returns other result than 200 that has Location field and the response received is as expected. Signed-off-by: Khang Kieu <khangk@google.com> Change-Id: I77c7dae32a103fbec3015fe14b51a3ed0022143e
2023-02-03Aggregation: Better handle dropped requestsCarson Labrado1-0/+206
It's possible for HTTP client's request buffer to become full (especially when $expand is used). Instead of ignoring the requests we should provide a 429 Too Many Requests response for the provided callback to process. The aggregator's response handling also needs to account for this possibility so that it only completely overwrites the asyncResp object when it receives a response from a satellite. Also added more test cases for the response processing functions. Tested: Unit tests passed Flooded aggregator with requests for satellite resources. Requests began returning 429 Too Many Requests errors after the request buffer became full. Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: Ib052dc0454d759de7fae761977ca26d6b8d208e5
2023-01-23Break out set time function and unit test itEd Tanous1-0/+32
This function is something that's easily unit tested. Do it. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I8d664c77ec4b3a9886128597449c5f9c041b86b3
2023-01-19Aggregation: Fix and forward all responsesCarson Labrado1-1/+106
We only attempt prefix matching when we receive a 200 response. For the retry policy we consider 2XX and 404 to be valid codes. Instead we should forward all responses to the client and let them decide what action they want to take. As part of that we should always attempt to do prefix fixing on the response. Also fixes an oversight where we attempt to do prefix fixing on "OriginOfCondition" properties. That property is only a URI when it is an Action parameter in a SubmitTestEvent request. It is an object when it appears as a response property. Adds test cases for the above fixes. Tested: Tests pass. Queries to top level collections and aggregated URIs still return expected results with added prefixes. Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: Ic76324ceab618160061be5f3c687af20a857fa25
2023-01-17Add unit tests for ipv4VerifyIpAndGetBitcountEd Tanous1-1/+89
Per the title, add unit tests for this function. Tested: Unit tests pass Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: Ifdd9c314d2fa62ae3fa3b8f8150fcdd224a7eb03
2023-01-14LogService: Use DeleteAll DBus method in clearDumpClaire Weinan1-91/+0
Update the clearDump() implementation to call the DeleteAll D-Bus method instead of iterating through D-Bus objects representing individual log entries and calling the Delete D-Bus method on each one. (It's more efficient for phosphor-debug-collector to iterate through entries in its DeleteAll method handler than for bmcweb to iterate through them.) It seems like clearDump() wasn't originally implemented using DeleteAll because dumps of various types were under the same D-Bus path namespace at the time and there wasn't a way to selectively clear dumps of only a specific type. The commit at [1] put different dump types under different path namespaces (enabling us to now use DeleteAll). Now clients should see a bit of performance improvement when running the ClearLog action on dump LogServices, due to the reduced number of D-Bus method calls needed to execute ClearLog. Also updated getDumpServiceInfo() to populate the ClearLog action for dump LogServices based on whether their dump manager object implements xyz.openbmc_project.Collection.DeleteAll. Tested: Cleared the fault log containing 100 entries. Ran with the time command several times before and after the change: ``` time curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Managers/bmc/LogServices/FaultLog/Actions/LogService.ClearLog ``` Before the change, "real" time reported was ~1.2s. After the change, "real" time reported was ~0.4s. Forced creation of dump entries and then ran Redfish ClearLog action on each dump type: ``` curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Managers/bmc/LogServices/Dump/Actions/LogService.ClearLog curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Managers/bmc/LogServices/FaultLog/Actions/LogService.ClearLog curl -k -H "X-Auth-Token: $token" -X POST http://${bmc}/redfish/v1/Systems/system/LogServices/Dump/Actions/LogService.ClearLog ``` Then verified that there were no dump LogService entries afterwards: ``` curl -k -H "X-Auth-Token: $token" -X GET http://${bmc}/redfish/v1/Managers/bmc/LogServices/Dump/Entries curl -k -H "X-Auth-Token: $token" -X GET http://${bmc}/redfish/v1/Managers/bmc/LogServices/FaultLog/Entries curl -k -H "X-Auth-Token: $token" -X GET http://${bmc}/redfish/v1/Systems/system/LogServices/Dump/Entries ``` Also verified that the corresponding D-Bus objects were gone from the D-Bus tree after running ClearLog on each dump type: Before ClearLog: busctl tree xyz.openbmc_project.Dump.Manager `-/xyz `-/xyz/openbmc_project `-/xyz/openbmc_project/dump |-/xyz/openbmc_project/dump/bmc | `-/xyz/openbmc_project/dump/bmc/entry | `-/xyz/openbmc_project/dump/bmc/entry/101 |-/xyz/openbmc_project/dump/faultlog | `-/xyz/openbmc_project/dump/faultlog/entry | |-/xyz/openbmc_project/dump/faultlog/entry/11 | |-/xyz/openbmc_project/dump/faultlog/entry/12 | |-/xyz/openbmc_project/dump/faultlog/entry/13 | |-/xyz/openbmc_project/dump/faultlog/entry/14 | |-/xyz/openbmc_project/dump/faultlog/entry/15 | |-/xyz/openbmc_project/dump/faultlog/entry/16 | |-/xyz/openbmc_project/dump/faultlog/entry/17 | |-/xyz/openbmc_project/dump/faultlog/entry/18 | |-/xyz/openbmc_project/dump/faultlog/entry/19 | `-/xyz/openbmc_project/dump/faultlog/entry/20 |-/xyz/openbmc_project/dump/internal | `-/xyz/openbmc_project/dump/internal/manager `-/xyz/openbmc_project/dump/system `-/xyz/openbmc_project/dump/system/entry |-/xyz/openbmc_project/dump/system/entry/3 `-/xyz/openbmc_project/dump/system/entry/4 After ClearLog: busctl tree xyz.openbmc_project.Dump.Manager `-/xyz `-/xyz/openbmc_project `-/xyz/openbmc_project/dump |-/xyz/openbmc_project/dump/bmc |-/xyz/openbmc_project/dump/faultlog |-/xyz/openbmc_project/dump/internal | `-/xyz/openbmc_project/dump/internal/manager `-/xyz/openbmc_project/dump/system Confirmed that ClearLog action is listed for the following LogServices: /redfish/v1/Managers/bmc/LogServices/Dump /redfish/v1/Managers/bmc/LogServices/FaultLog /redfish/v1/Systems/system/LogServices/Dump Then ran "systemctl stop xyz.openbmc_project.Dump.Manager" (which removes dump manager objects including their xyz.openbmc_project.Collection.DeleteAll interface) and saw that the ClearLog action was no longer listed. Also locally built a version of phosphor-debug-collecor with the interface xyz.openbmc_project.Collection.DeleteAll removed from dump managers and ran it and saw that the ClearLog action wasn't listed. Redfish Service Validator passed on the following URIs (with service xyz.openbmc_project.Dump.Manager running): /redfish/v1/Managers/bmc/LogServices/Dump /redfish/v1/Managers/bmc/LogServices/FaultLog /redfish/v1/Systems/system/LogServices/Dump Note: Most dump LogService unit tests were removed in this patchset since this patchset adds a D-Bus call to getDumpServiceInfo(), and we haven't decided how to mock D-Bus calls for unit testing yet. [1] https://github.com/openbmc/phosphor-debug-collector/commit/fef66a951fe6fe283515480b2c493dfdc2275a95 Signed-off-by: Claire Weinan <cweinan@google.com> Change-Id: Ic5f8f9e3528f521887766d8710bd77f969d8236a
2023-01-12Aggregation: Improve prefix fixup matchingCarson Labrado1-0/+98
Utilize the new array of top level collection URIs to determine if a given URI in the response needs to have the aggregation prefix added. This removes the need to check for specific collections like /redfish/v1/UpdateService/FirmwareInventory which do not fit the generic format of /redfish/v1/<collection>. Future patches will use this same approach to improve the logic for initially determining if and how a request should be aggregated. This patch also adds a series of unit tests for the function responsible for adding a prefix to a given URI. Cases covered include valid URIs that involve a selection of aggregated resources, top level collection URIs, other invalid URIs, and URIs with a trailing "/". Tested: Unit tests pass. Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: I676983d3c77ae3126c04e9f57ad8698c51df2675
2023-01-03Fix sensor overrideEd Tanous1-0/+36
c1d019a6056a2a0ef50e577b3139ab5a8dc49355 Sensor Optimization Recently changed the way Ids were calculated in the sensor subsystem. Unfortunately, it wasn't clear to the author that this would effect the sensor override system, which relies on matching up a member ID with a dbus path, and was broken by this change. This commit breaks out the code to calculate the type and name from a given URI segment into a helper method. Tested: Inspection only. Very few systems support this feature. Code appears more correct than previously, which is known broken, so the lack of testing here seems reasonable. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I9aa8099a947a36b5ce914bc07ae60f1ebf0d209b
2022-12-15Prepare for boost::url upgradeEd Tanous1-5/+5
The new boost URL now interops properly with std::string_view, which is great, and cleans up a bunch of mediocre code to convert one to another. It has also been pulled into boost-proper, so we no longer need a boost-url dependency that's separate. Unfortunately, boost url makes these improvements by changing boost::string_view for boost::urls::const_string, which causes us to have some compile errors on the missing type. The bulk of these changes fall into a couple categories, and have to be executed in one commit. string() is replaced with buffer() on the url and url_view types boost::string_view is replaced by std::string_view for many times, in many cases removing a temporary that we had in the code previously. Tested: Code compiles with boost 1.81.0 beta. Redfish service validator passes. Pretty good unit test coverage for URL-specific use cases. Signed-off-by: Ed Tanous <edtanous@google.com> Change-Id: I8d3dc89b53d1cc390887fe53605d4867f75f76fd
2022-12-08Aggregation: Detect and fix all URI propertiesCarson Labrado1-0/+41
There are a number of properties of Type "string (uri)" for which we do not currently support adding prefixes. This patch adds support for all existing URI properties which are missed by the existing implementation. This change will be needed by future patches which will expand aggregation support to all top level collections defined by the schema. Those collections that are not currently supported include properties whose URIs should be fixed, but would be missed by the existing implementation. Tested: New unit test passes. URI properties are still handled correctly. ```shell curl localhost/redfish/v1/Chassis/5B247A_<chassisID> { "@odata.id": "/redfish/v1/Chassis/5B247A_<chassisID>", "@odata.type": "#Chassis.v1_16_0.Chassis", "Actions": { "#Chassis.Reset": { "@Redfish.ActionInfo": "/redfish/v1/Chassis/5B247A_<chassisID>/ResetActionInfo", "target": "/redfish/v1/Chassis/5B247A_<chassisID>/Actions/Chassis.Reset" } }, ... } ``` Signed-off-by: Carson Labrado <clabrado@google.com> Change-Id: I3b3e06ee3191564d266598f7bc9f1641e6fcb333
generated by cgit v1.2.3 (git 2.39.0) at 2024-06-14 11:59:55 +0300