From d3e8eac4c6119c5f27ad9bbf59cbb22e5371ff56 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Mon, 20 Jan 2025 13:53:46 +0100 Subject: [PATCH 01/16] accept access token from header --- src/engine/Server.cpp | 35 +++++++++- src/engine/Server.h | 7 ++ src/util/http/UrlParser.h | 2 + test/ServerTest.cpp | 132 +++++++++++++++++++++++++++++++------- 4 files changed, 149 insertions(+), 27 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 08fc6f9607..5614572b32 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -164,6 +164,27 @@ void Server::run(const string& indexBaseName, bool useText, bool usePatterns, httpServer.run(); } +std::optional Server::extractAccessToken( + const ad_utility::httpUtils::HttpRequest auto& request, + const ad_utility::url_parser::ParamValueMap& params) { + if (request.find(http::field::authorization) != request.end()) { + string_view authorization = request[http::field::authorization]; + const std::string prefix = "Bearer "; + if (!authorization.starts_with(prefix)) { + throw std::runtime_error(absl::StrCat( + "Authorization header must start with \"Bearer \". Got: \"", + authorization, "\".")); + } + authorization.remove_prefix(prefix.length()); + return std::string(authorization); + } + if (params.contains("access-token")) { + return ad_utility::url_parser::getParameterCheckAtMostOnce(params, + "access-token"); + } + return std::nullopt; +} + // _____________________________________________________________________________ ad_utility::url_parser::ParsedRequest Server::parseHttpRequest( const ad_utility::httpUtils::HttpRequest auto& request) { @@ -172,7 +193,8 @@ ad_utility::url_parser::ParsedRequest Server::parseHttpRequest( using namespace ad_utility::url_parser::sparqlOperation; auto parsedUrl = ad_utility::url_parser::parseRequestTarget(request.target()); ad_utility::url_parser::ParsedRequest parsedRequest{ - std::move(parsedUrl.path_), std::move(parsedUrl.parameters_), None{}}; + std::move(parsedUrl.path_), std::nullopt, + std::move(parsedUrl.parameters_), None{}}; // Some valid requests (e.g. QLever's custom commands like retrieving index // statistics) don't have a query. So an empty operation is not necessarily an @@ -186,9 +208,14 @@ ad_utility::url_parser::ParsedRequest Server::parseHttpRequest( parsedRequest.parameters_.erase(paramName); } }; + auto extractAccessTokenFromRequest = [&parsedRequest, &request]() { + parsedRequest.accessToken_ = + extractAccessToken(request, parsedRequest.parameters_); + }; if (request.method() == http::verb::get) { setOperationIfSpecifiedInParams.template operator()("query"); + extractAccessTokenFromRequest(); if (parsedRequest.parameters_.contains("update")) { throw std::runtime_error("SPARQL Update is not allowed as GET request."); } @@ -258,15 +285,18 @@ ad_utility::url_parser::ParsedRequest Server::parseHttpRequest( } setOperationIfSpecifiedInParams.template operator()("query"); setOperationIfSpecifiedInParams.template operator()("update"); + extractAccessTokenFromRequest(); return parsedRequest; } if (contentType.starts_with(contentTypeSparqlQuery)) { parsedRequest.operation_ = Query{request.body()}; + extractAccessTokenFromRequest(); return parsedRequest; } if (contentType.starts_with(contentTypeSparqlUpdate)) { parsedRequest.operation_ = Update{request.body()}; + extractAccessTokenFromRequest(); return parsedRequest; } throw std::runtime_error(absl::StrCat( @@ -353,8 +383,7 @@ Awaitable Server::process( // Check the access token. If an access token is provided and the check fails, // throw an exception and do not process any part of the query (even if the // processing had been allowed without access token). - bool accessTokenOk = - checkAccessToken(checkParameter("access-token", std::nullopt)); + bool accessTokenOk = checkAccessToken(parsedHttpRequest.accessToken_); auto requireValidAccessToken = [&accessTokenOk]( const std::string& actionName) { if (!accessTokenOk) { diff --git a/src/engine/Server.h b/src/engine/Server.h index 3ccc070cb7..15c37a85fd 100644 --- a/src/engine/Server.h +++ b/src/engine/Server.h @@ -34,6 +34,7 @@ class Server { FRIEND_TEST(ServerTest, parseHttpRequest); FRIEND_TEST(ServerTest, getQueryId); FRIEND_TEST(ServerTest, createMessageSender); + FRIEND_TEST(ServerTest, extractAccessToken); public: explicit Server(unsigned short port, size_t numThreads, @@ -114,6 +115,12 @@ class Server { static ad_utility::url_parser::ParsedRequest parseHttpRequest( const ad_utility::httpUtils::HttpRequest auto& request); + /// Extract the Access token for that request from the `Authorization` header + /// or the URL query parameters. + static std::optional extractAccessToken( + const ad_utility::httpUtils::HttpRequest auto& request, + const ad_utility::url_parser::ParamValueMap& params); + /// Handle a single HTTP request. Check whether a file request or a query was /// sent, and dispatch to functions handling these cases. This function /// requires the constraints for the `HttpHandler` in `HttpServer.h`. diff --git a/src/util/http/UrlParser.h b/src/util/http/UrlParser.h index 33ebc86b1d..80525e8f35 100644 --- a/src/util/http/UrlParser.h +++ b/src/util/http/UrlParser.h @@ -62,10 +62,12 @@ struct None { // Representation of parsed HTTP request. // - `path_` is the URL path +// - `accessToken_` is the access token for that request // - `parameters_` is a hashmap of the parameters // - `operation_` the operation that should be performed struct ParsedRequest { std::string path_; + std::optional accessToken_; ParamValueMap parameters_; std::variant diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index 292d77f12b..685e85c7e3 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -17,11 +17,14 @@ using namespace ad_utility::url_parser::sparqlOperation; namespace { auto ParsedRequestIs = [](const std::string& path, + const std::optional& accessToken, const ParamValueMap& parameters, const std::variant& operation) -> testing::Matcher { return testing::AllOf( AD_FIELD(ad_utility::url_parser::ParsedRequest, path_, testing::Eq(path)), + AD_FIELD(ad_utility::url_parser::ParsedRequest, accessToken_, + testing::Eq(accessToken)), AD_FIELD(ad_utility::url_parser::ParsedRequest, parameters_, testing::ContainerEq(parameters)), AD_FIELD(ad_utility::url_parser::ParsedRequest, operation_, @@ -55,19 +58,21 @@ TEST(ServerTest, parseHttpRequest) { "application/x-www-form-urlencoded;charset=UTF-8"; const std::string QUERY = "application/sparql-query"; const std::string UPDATE = "application/sparql-update"; - EXPECT_THAT(parse(MakeGetRequest("/")), ParsedRequestIs("/", {}, None{})); + EXPECT_THAT(parse(MakeGetRequest("/")), + ParsedRequestIs("/", std::nullopt, {}, None{})); EXPECT_THAT(parse(MakeGetRequest("/ping")), - ParsedRequestIs("/ping", {}, None{})); + ParsedRequestIs("/ping", std::nullopt, {}, None{})); EXPECT_THAT(parse(MakeGetRequest("/?cmd=stats")), - ParsedRequestIs("/", {{"cmd", {"stats"}}}, None{})); + ParsedRequestIs("/", std::nullopt, {{"cmd", {"stats"}}}, None{})); EXPECT_THAT(parse(MakeGetRequest( "/?query=SELECT+%2A%20WHERE%20%7B%7D&action=csv_export")), - ParsedRequestIs("/", {{"action", {"csv_export"}}}, + ParsedRequestIs("/", std::nullopt, {{"action", {"csv_export"}}}, Query{"SELECT * WHERE {}"})); EXPECT_THAT( parse(MakePostRequest("/", URLENCODED, "query=SELECT+%2A%20WHERE%20%7B%7D&send=100")), - ParsedRequestIs("/", {{"send", {"100"}}}, Query{"SELECT * WHERE {}"})); + ParsedRequestIs("/", std::nullopt, {{"send", {"100"}}}, + Query{"SELECT * WHERE {}"})); AD_EXPECT_THROW_WITH_MESSAGE( parse(MakePostRequest("/", URLENCODED, "ääär y=SELECT+%2A%20WHERE%20%7B%7D&send=100")), @@ -92,10 +97,12 @@ TEST(ServerTest, parseHttpRequest) { EXPECT_THAT( parse(MakePostRequest("/", "application/x-www-form-urlencoded", "query=SELECT%20%2A%20WHERE%20%7B%7D&send=100")), - ParsedRequestIs("/", {{"send", {"100"}}}, Query{"SELECT * WHERE {}"})); - EXPECT_THAT(parse(MakePostRequest("/", URLENCODED, - "query=SELECT%20%2A%20WHERE%20%7B%7D")), - ParsedRequestIs("/", {}, Query{"SELECT * WHERE {}"})); + ParsedRequestIs("/", std::nullopt, {{"send", {"100"}}}, + Query{"SELECT * WHERE {}"})); + EXPECT_THAT( + parse(MakePostRequest("/", URLENCODED, + "query=SELECT%20%2A%20WHERE%20%7B%7D")), + ParsedRequestIs("/", std::nullopt, {}, Query{"SELECT * WHERE {}"})); EXPECT_THAT( parse(MakePostRequest( "/", URLENCODED, @@ -103,7 +110,7 @@ TEST(ServerTest, parseHttpRequest) { "2Fw3.org%2Fdefault&named-graph-uri=https%3A%2F%2Fw3.org%2F1&named-" "graph-uri=https%3A%2F%2Fw3.org%2F2")), ParsedRequestIs( - "/", + "/", std::nullopt, {{"default-graph-uri", {"https://w3.org/default"}}, {"named-graph-uri", {"https://w3.org/1", "https://w3.org/2"}}}, Query{"SELECT * WHERE {}"})); @@ -112,13 +119,15 @@ TEST(ServerTest, parseHttpRequest) { "query=SELECT%20%2A%20WHERE%20%7B%7D")), testing::StrEq("URL-encoded POST requests must not contain query " "parameters in the URL.")); - EXPECT_THAT(parse(MakePostRequest("/", URLENCODED, "cmd=clear-cache")), - ParsedRequestIs("/", {{"cmd", {"clear-cache"}}}, None{})); - EXPECT_THAT(parse(MakePostRequest("/", QUERY, "SELECT * WHERE {}")), - ParsedRequestIs("/", {}, Query{"SELECT * WHERE {}"})); EXPECT_THAT( - parse(MakePostRequest("/?send=100", QUERY, "SELECT * WHERE {}")), - ParsedRequestIs("/", {{"send", {"100"}}}, Query{"SELECT * WHERE {}"})); + parse(MakePostRequest("/", URLENCODED, "cmd=clear-cache")), + ParsedRequestIs("/", std::nullopt, {{"cmd", {"clear-cache"}}}, None{})); + EXPECT_THAT( + parse(MakePostRequest("/", QUERY, "SELECT * WHERE {}")), + ParsedRequestIs("/", std::nullopt, {}, Query{"SELECT * WHERE {}"})); + EXPECT_THAT(parse(MakePostRequest("/?send=100", QUERY, "SELECT * WHERE {}")), + ParsedRequestIs("/", std::nullopt, {{"send", {"100"}}}, + Query{"SELECT * WHERE {}"})); AD_EXPECT_THROW_WITH_MESSAGE( parse(MakeBasicRequest(http::verb::patch, "/")), testing::StrEq( @@ -132,14 +141,35 @@ TEST(ServerTest, parseHttpRequest) { AD_EXPECT_THROW_WITH_MESSAGE( parse(MakeGetRequest("/?update=DELETE%20%2A%20WHERE%20%7B%7D")), testing::StrEq("SPARQL Update is not allowed as GET request.")); - EXPECT_THAT(parse(MakePostRequest("/", UPDATE, "DELETE * WHERE {}")), - ParsedRequestIs("/", {}, Update{"DELETE * WHERE {}"})); - EXPECT_THAT(parse(MakePostRequest("/", URLENCODED, - "update=DELETE%20%2A%20WHERE%20%7B%7D")), - ParsedRequestIs("/", {}, Update{"DELETE * WHERE {}"})); - EXPECT_THAT(parse(MakePostRequest("/", URLENCODED, - "update=DELETE+%2A+WHERE%20%7B%7D")), - ParsedRequestIs("/", {}, Update{"DELETE * WHERE {}"})); + EXPECT_THAT( + parse(MakePostRequest("/", UPDATE, "DELETE * WHERE {}")), + ParsedRequestIs("/", std::nullopt, {}, Update{"DELETE * WHERE {}"})); + EXPECT_THAT( + parse(MakePostRequest("/", URLENCODED, + "update=DELETE%20%2A%20WHERE%20%7B%7D")), + ParsedRequestIs("/", std::nullopt, {}, Update{"DELETE * WHERE {}"})); + EXPECT_THAT( + parse( + MakePostRequest("/", URLENCODED, "update=DELETE+%2A+WHERE%20%7B%7D")), + ParsedRequestIs("/", std::nullopt, {}, Update{"DELETE * WHERE {}"})); + // TODO: there could be some more here, but i'll wait until #1668 + EXPECT_THAT( + parse(MakeGetRequest("/?query=a&access-token=foo")), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, Query{"a"})); + EXPECT_THAT( + parse(MakePostRequest("/", URLENCODED, + "update=DELETE%20WHERE%20%7B%7D&access-token=foo")), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + Update{"DELETE WHERE {}"})); + EXPECT_THAT(parse(MakePostRequest( + "/", URLENCODED, + "query=SELECT%20%2A%20WHERE%20%7B%7D&access-token=foo")), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + Query{"SELECT * WHERE {}"})); + EXPECT_THAT( + parse(MakePostRequest("/?access-token=foo", UPDATE, "DELETE * WHERE {}")), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + Update{"DELETE * WHERE {}"})); } TEST(ServerTest, checkParameter) { @@ -284,3 +314,57 @@ TEST(ServerTest, createMessageSender) { "SELECT * WHERE { ?a ?b ?c }"), testing::HasSubstr("Assertion `queryHubLock` failed.")); } + +TEST(ServerTest, extractAccessToken) { + auto extract = [](const ad_utility::httpUtils::HttpRequest auto& request) { + auto parsedUrl = parseRequestTarget(request.target()); + return Server::extractAccessToken(request, parsedUrl.parameters_); + }; + // TODO: replace once #1668 is merged + auto makeRequest = + [](const http::verb method = http::verb::get, + const std::string& target = "/", + const ad_utility::HashMap& headers = {}, + const std::optional& body = std::nullopt) { + // version 11 stands for HTTP/1.1 + auto req = http::request{method, target, 11}; + for (const auto& [key, value] : headers) { + req.set(key, value); + } + if (body.has_value()) { + req.body() = body.value(); + req.prepare_payload(); + } + return req; + }; + EXPECT_THAT(extract(MakeGetRequest("/")), testing::Eq(std::nullopt)); + EXPECT_THAT(extract(MakeGetRequest("/?access-token=foo")), + testing::Optional(testing::Eq("foo"))); + EXPECT_THAT( + extract(makeRequest(http::verb::get, "/", + {{http::field::authorization, "Bearer foo"}})), + testing::Optional(testing::Eq("foo"))); + // The header takes precedence over the query parameter. + EXPECT_THAT( + extract(makeRequest(http::verb::get, "/?access-token=bar", + {{http::field::authorization, "Bearer foo"}})), + testing::Optional(testing::Eq("foo"))); + AD_EXPECT_THROW_WITH_MESSAGE( + extract(makeRequest(http::verb::get, "/", + {{http::field::authorization, "foo"}})), + testing::HasSubstr( + "Authorization header must start with \"Bearer \". Got: \"foo\".")); + EXPECT_THAT(extract(MakePostRequest("/", "text/turtle", "")), + testing::Eq(std::nullopt)); + EXPECT_THAT(extract(MakePostRequest("/?access-token=foo", "text/turtle", "")), + testing::Optional(testing::Eq("foo"))); + EXPECT_THAT( + extract(makeRequest(http::verb::post, "/?access-token=bar", + {{http::field::authorization, "Bearer foo"}})), + testing::Optional(testing::Eq("foo"))); + AD_EXPECT_THROW_WITH_MESSAGE( + extract(makeRequest(http::verb::post, "/?access-token=bar", + {{http::field::authorization, "foo"}})), + testing::HasSubstr( + "Authorization header must start with \"Bearer \". Got: \"foo\".")); +} From 676d679407cb29f841eb5ea5aba342d35e9eb649 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Wed, 22 Jan 2025 12:40:54 +0100 Subject: [PATCH 02/16] don't echo the request access token --- src/engine/Server.cpp | 10 ++++------ test/ServerTest.cpp | 4 ++-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 0d01817a5a..a50afebd3b 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -172,9 +172,8 @@ std::optional Server::extractAccessToken( string_view authorization = request[http::field::authorization]; const std::string prefix = "Bearer "; if (!authorization.starts_with(prefix)) { - throw std::runtime_error(absl::StrCat( - "Authorization header must start with \"Bearer \". Got: \"", - authorization, "\".")); + throw std::runtime_error( + absl::StrCat("Authorization header doesn't start with \"Bearer \".")); } authorization.remove_prefix(prefix.length()); return std::string(authorization); @@ -1128,8 +1127,7 @@ bool Server::checkAccessToken( if (!accessToken) { return false; } - auto accessTokenProvidedMsg = absl::StrCat( - "Access token \"access-token=", accessToken.value(), "\" provided"); + auto accessTokenProvidedMsg = absl::StrCat("Access token was provided"); auto requestIgnoredMsg = ", request is ignored"; if (accessToken_.empty()) { throw std::runtime_error(absl::StrCat( @@ -1138,7 +1136,7 @@ bool Server::checkAccessToken( } else if (!ad_utility::constantTimeEquals(accessToken.value(), accessToken_)) { throw std::runtime_error(absl::StrCat( - accessTokenProvidedMsg, " but not correct", requestIgnoredMsg)); + accessTokenProvidedMsg, " but it was invalid", requestIgnoredMsg)); } else { LOG(DEBUG) << accessTokenProvidedMsg << " and correct" << std::endl; return true; diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index e3111e52ca..e8a179248e 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -314,7 +314,7 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "foo"}})), testing::HasSubstr( - "Authorization header must start with \"Bearer \". Got: \"foo\".")); + "Authorization header doesn't start with \"Bearer \".")); EXPECT_THAT(extract(makePostRequest("/", "text/turtle", "")), testing::Eq(std::nullopt)); EXPECT_THAT(extract(makePostRequest("/?access-token=foo", "text/turtle", "")), @@ -327,5 +327,5 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "foo"}})), testing::HasSubstr( - "Authorization header must start with \"Bearer \". Got: \"foo\".")); + "Authorization header doesn't start with \"Bearer \".")); } From e27398ce4bc653518ded989dabfc74dcc5c63f39 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Wed, 22 Jan 2025 12:54:22 +0100 Subject: [PATCH 03/16] no precedence in the ways to specify access token --- src/engine/Server.cpp | 19 +++++++++++++++---- test/ServerTest.cpp | 13 ++++++++----- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index a50afebd3b..d2e113187e 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -168,6 +168,8 @@ void Server::run(const string& indexBaseName, bool useText, bool usePatterns, std::optional Server::extractAccessToken( const ad_utility::httpUtils::HttpRequest auto& request, const ad_utility::url_parser::ParamValueMap& params) { + std::optional tokenFromAuthorizationHeader; + std::optional tokenFromParameter; if (request.find(http::field::authorization) != request.end()) { string_view authorization = request[http::field::authorization]; const std::string prefix = "Bearer "; @@ -176,13 +178,22 @@ std::optional Server::extractAccessToken( absl::StrCat("Authorization header doesn't start with \"Bearer \".")); } authorization.remove_prefix(prefix.length()); - return std::string(authorization); + tokenFromAuthorizationHeader = std::string(authorization); } if (params.contains("access-token")) { - return ad_utility::url_parser::getParameterCheckAtMostOnce(params, - "access-token"); + tokenFromParameter = ad_utility::url_parser::getParameterCheckAtMostOnce( + params, "access-token"); } - return std::nullopt; + // If both are specified, they must be equal. This way there is no hidden + // precedence. + if (tokenFromAuthorizationHeader && tokenFromParameter && + tokenFromAuthorizationHeader != tokenFromParameter) { + throw std::runtime_error( + "Access token is specified both in the `Authorization` Header and the " + "parameters, but they aren't the same."); + } + return tokenFromAuthorizationHeader ? std::move(tokenFromAuthorizationHeader) + : std::move(tokenFromParameter); } // _____________________________________________________________________________ diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index e8a179248e..51610e01d8 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -305,11 +305,12 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "Bearer foo"}})), testing::Optional(testing::Eq("foo"))); - // The header takes precedence over the query parameter. - EXPECT_THAT( + AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), - testing::Optional(testing::Eq("foo"))); + testing::HasSubstr( + "Access token is specified both in the `Authorization` Header and " + "the parameters, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "foo"}})), @@ -319,10 +320,12 @@ TEST(ServerTest, extractAccessToken) { testing::Eq(std::nullopt)); EXPECT_THAT(extract(makePostRequest("/?access-token=foo", "text/turtle", "")), testing::Optional(testing::Eq("foo"))); - EXPECT_THAT( + AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), - testing::Optional(testing::Eq("foo"))); + testing::HasSubstr( + "Access token is specified both in the `Authorization` Header and " + "the parameters, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "foo"}})), From 07c2769eac2056c2487e1f9776abecc5025e2116 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Wed, 22 Jan 2025 14:42:52 +0100 Subject: [PATCH 04/16] more extensive tests --- src/engine/Server.cpp | 2 ++ test/ServerTest.cpp | 54 ++++++++++++++++++++++++------------------- 2 files changed, 32 insertions(+), 24 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index d2e113187e..fc0e738c7e 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -296,6 +296,8 @@ ad_utility::url_parser::ParsedRequest Server::parseHttpRequest( } setOperationIfSpecifiedInParams.template operator()("query"); setOperationIfSpecifiedInParams.template operator()("update"); + // We parse the access token from the url-encoded parameters in the body. + // The URL parameters must be empty for URL-encoded POST (see above). extractAccessTokenFromRequest(); return parsedRequest; diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index 51610e01d8..2523a7fa78 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -136,24 +136,43 @@ TEST(ServerTest, parseHttpRequest) { parse( makePostRequest("/", URLENCODED, "update=DELETE+%2A+WHERE%20%7B%7D")), ParsedRequestIs("/", std::nullopt, {}, Update{"DELETE * WHERE {}"})); - // TODO: there could be some more here, but i'll wait until #1668 EXPECT_THAT( parse(makeGetRequest("/?query=a&access-token=foo")), ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, Query{"a"})); + EXPECT_THAT( + parse(makeRequest(http::verb::get, "/?query=a", + {{http::field::authorization, {"Bearer bar"}}})), + ParsedRequestIs("/", "bar", {}, Query{"a"})); + AD_EXPECT_THROW_WITH_MESSAGE( + parse(makeRequest(http::verb::get, "/?query=a&access-token=foo", + {{http::field::authorization, {"Bearer bar"}}})), + testing::HasSubstr( + "Access token is specified both in the `Authorization` Header and " + "the parameters, but they aren't the same.")); EXPECT_THAT( parse(makePostRequest("/", URLENCODED, - "update=DELETE%20WHERE%20%7B%7D&access-token=foo")), - ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + "update=DELETE%20WHERE%20%7B%7D&access-token=baz")), + ParsedRequestIs("/", "baz", {{"access-token", {"baz"}}}, Update{"DELETE WHERE {}"})); + EXPECT_THAT(parse(makeRequest(http::verb::post, "/", + {{http::field::content_type, {URLENCODED}}, + {http::field::authorization, {"Bearer bar"}}}, + "update=DELETE%20WHERE%20%7B%7D")), + ParsedRequestIs("/", "bar", {{"access-token", {"bar"}}}, + Update{"DELETE WHERE {}"})); EXPECT_THAT(parse(makePostRequest( "/", URLENCODED, - "query=SELECT%20%2A%20WHERE%20%7B%7D&access-token=foo")), - ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + "query=SELECT%20%2A%20WHERE%20%7B%7D&access-token=baz")), + ParsedRequestIs("/", "baz", {{"access-token", {"baz"}}}, Query{"SELECT * WHERE {}"})); EXPECT_THAT( - parse(makePostRequest("/?access-token=foo", UPDATE, "DELETE * WHERE {}")), + parse(makePostRequest("/?access-token=foo", UPDATE, "DELETE WHERE {}")), ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, - Update{"DELETE * WHERE {}"})); + Update{"DELETE WHERE {}"})); + EXPECT_THAT( + parse(makePostRequest("/?access-token=foo", QUERY, "SELECT * WHERE {}")), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + Update{"SELECT * WHERE {}"})); } TEST(ServerTest, determineResultPinning) { @@ -281,23 +300,6 @@ TEST(ServerTest, extractAccessToken) { auto parsedUrl = parseRequestTarget(request.target()); return Server::extractAccessToken(request, parsedUrl.parameters_); }; - // TODO: replace once #1668 is merged - auto makeRequest = - [](const http::verb method = http::verb::get, - const std::string& target = "/", - const ad_utility::HashMap& headers = {}, - const std::optional& body = std::nullopt) { - // version 11 stands for HTTP/1.1 - auto req = http::request{method, target, 11}; - for (const auto& [key, value] : headers) { - req.set(key, value); - } - if (body.has_value()) { - req.body() = body.value(); - req.prepare_payload(); - } - return req; - }; EXPECT_THAT(extract(makeGetRequest("/")), testing::Eq(std::nullopt)); EXPECT_THAT(extract(makeGetRequest("/?access-token=foo")), testing::Optional(testing::Eq("foo"))); @@ -305,6 +307,10 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "Bearer foo"}})), testing::Optional(testing::Eq("foo"))); + EXPECT_THAT( + extract(makeRequest(http::verb::get, "/?access-token=foo", + {{http::field::authorization, "Bearer foo"}})), + testing::Optional(testing::Eq("foo"))); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), From 4b5194f58d81bd7a7a155304c9effe3fea57ac3b Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Wed, 22 Jan 2025 15:50:30 +0100 Subject: [PATCH 05/16] most extensive tests --- test/ServerTest.cpp | 138 ++++++++++++++++++++++++--------- test/util/HttpRequestHelpers.h | 3 +- 2 files changed, 103 insertions(+), 38 deletions(-) diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index 2523a7fa78..aee17aec57 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -136,43 +136,107 @@ TEST(ServerTest, parseHttpRequest) { parse( makePostRequest("/", URLENCODED, "update=DELETE+%2A+WHERE%20%7B%7D")), ParsedRequestIs("/", std::nullopt, {}, Update{"DELETE * WHERE {}"})); - EXPECT_THAT( - parse(makeGetRequest("/?query=a&access-token=foo")), - ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, Query{"a"})); - EXPECT_THAT( - parse(makeRequest(http::verb::get, "/?query=a", - {{http::field::authorization, {"Bearer bar"}}})), - ParsedRequestIs("/", "bar", {}, Query{"a"})); - AD_EXPECT_THROW_WITH_MESSAGE( - parse(makeRequest(http::verb::get, "/?query=a&access-token=foo", - {{http::field::authorization, {"Bearer bar"}}})), - testing::HasSubstr( - "Access token is specified both in the `Authorization` Header and " - "the parameters, but they aren't the same.")); - EXPECT_THAT( - parse(makePostRequest("/", URLENCODED, - "update=DELETE%20WHERE%20%7B%7D&access-token=baz")), - ParsedRequestIs("/", "baz", {{"access-token", {"baz"}}}, - Update{"DELETE WHERE {}"})); - EXPECT_THAT(parse(makeRequest(http::verb::post, "/", - {{http::field::content_type, {URLENCODED}}, - {http::field::authorization, {"Bearer bar"}}}, - "update=DELETE%20WHERE%20%7B%7D")), - ParsedRequestIs("/", "bar", {{"access-token", {"bar"}}}, - Update{"DELETE WHERE {}"})); - EXPECT_THAT(parse(makePostRequest( - "/", URLENCODED, - "query=SELECT%20%2A%20WHERE%20%7B%7D&access-token=baz")), - ParsedRequestIs("/", "baz", {{"access-token", {"baz"}}}, - Query{"SELECT * WHERE {}"})); - EXPECT_THAT( - parse(makePostRequest("/?access-token=foo", UPDATE, "DELETE WHERE {}")), - ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, - Update{"DELETE WHERE {}"})); - EXPECT_THAT( - parse(makePostRequest("/?access-token=foo", QUERY, "SELECT * WHERE {}")), - ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, - Update{"SELECT * WHERE {}"})); + auto testAccessTokenCombinations = + [&](const http::verb& method, std::string_view pathBase, + const std::variant& expectedOperation, + const ad_utility::HashMap& headers = {}, + const std::optional& body = std::nullopt, + ad_utility::source_location l = + ad_utility::source_location::current()) { + auto t = generateLocationTrace(l); + // Test the cases: + // 1. No Access token + // 2. Access token in query + // 3. Access token in Authorization header + // 4. Different Access tokens + // 5. Same Access token + boost::urls::url pathWithAccessToken{pathBase}; + pathWithAccessToken.params().append({"access-token", "foo"}); + ad_utility::HashMap + headersWithDifferentAccessToken{headers}; + headersWithDifferentAccessToken.insert( + {http::field::authorization, "Bearer bar"}); + ad_utility::HashMap + headersWithSameAccessToken{headers}; + headersWithSameAccessToken.insert( + {http::field::authorization, "Bearer foo"}); + EXPECT_THAT(parse(makeRequest(method, pathBase, headers, body)), + ParsedRequestIs("/", std::nullopt, {}, expectedOperation)); + EXPECT_THAT(parse(makeRequest(method, pathWithAccessToken.buffer(), + headers, body)), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + expectedOperation)); + EXPECT_THAT(parse(makeRequest(method, pathBase, + headersWithDifferentAccessToken, body)), + ParsedRequestIs("/", "bar", {}, expectedOperation)); + EXPECT_THAT(parse(makeRequest(method, pathWithAccessToken.buffer(), + headersWithSameAccessToken, body)), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + expectedOperation)); + AD_EXPECT_THROW_WITH_MESSAGE( + parse(makeRequest(method, pathWithAccessToken.buffer(), + headersWithDifferentAccessToken, body)), + testing::HasSubstr("Access token is specified both in the " + "`Authorization` Header and " + "the parameters, but they aren't the same.")); + }; + testAccessTokenCombinations(http::verb::get, "/?query=a", Query{"a"}); + testAccessTokenCombinations(http::verb::post, "/", Query{"a"}, + {{http::field::content_type, QUERY}}, "a"); + testAccessTokenCombinations(http::verb::post, "/", Update{"a"}, + {{http::field::content_type, UPDATE}}, "a"); + auto testAccessTokenCombinationsUrlEncoded = + [&](const std::string& bodyBase, + const std::variant& expectedOperation, + ad_utility::source_location l = + ad_utility::source_location::current()) { + auto t = generateLocationTrace(l); + // Test the cases: + // 1. No Access token + // 2. Access token in query + // 3. Access token in Authorization header + // 4. Different Access tokens + // 5. Same Access token + boost::urls::url paramsWithAccessToken{absl::StrCat("/?", bodyBase)}; + paramsWithAccessToken.params().append({"access-token", "foo"}); + std::string bodyWithAccessToken{ + paramsWithAccessToken.encoded_params().buffer()}; + ad_utility::HashMap headers{ + {http::field::content_type, {URLENCODED}}}; + ad_utility::HashMap + headersWithDifferentAccessToken{ + {http::field::content_type, {URLENCODED}}, + {http::field::authorization, "Bearer bar"}}; + ad_utility::HashMap + headersWithSameAccessToken{ + {http::field::content_type, {URLENCODED}}, + {http::field::authorization, "Bearer foo"}}; + EXPECT_THAT( + parse(makeRequest(http::verb::post, "/", headers, bodyBase)), + ParsedRequestIs("/", std::nullopt, {}, expectedOperation)); + EXPECT_THAT(parse(makeRequest(http::verb::post, "/", headers, + bodyWithAccessToken)), + ParsedRequestIs("/", "foo", {{"access-token", {"foo"}}}, + expectedOperation)); + EXPECT_THAT( + parse(makeRequest(http::verb::post, "/", + headersWithDifferentAccessToken, bodyBase)), + ParsedRequestIs("/", "bar", {}, expectedOperation)); + EXPECT_THAT(parse(makeRequest(http::verb::post, "/", + headersWithSameAccessToken, bodyBase)), + ParsedRequestIs("/", "foo", {}, expectedOperation)); + AD_EXPECT_THROW_WITH_MESSAGE( + parse(makeRequest(http::verb::post, "/", + headersWithDifferentAccessToken, + bodyWithAccessToken)), + testing::HasSubstr("Access token is specified both in the " + "`Authorization` Header and " + "the parameters, but they aren't the same.")); + }; + testAccessTokenCombinationsUrlEncoded("query=SELECT%20%2A%20WHERE%20%7B%7D", + Query{"SELECT * WHERE {}"}); + testAccessTokenCombinationsUrlEncoded("update=DELETE%20WHERE%20%7B%7D", + Update{"DELETE WHERE {}"}); } TEST(ServerTest, determineResultPinning) { diff --git a/test/util/HttpRequestHelpers.h b/test/util/HttpRequestHelpers.h index 4505e828db..cb57ead971 100644 --- a/test/util/HttpRequestHelpers.h +++ b/test/util/HttpRequestHelpers.h @@ -14,7 +14,8 @@ namespace http = boost::beast::http; // Construct a boost::beast request with the HTTP method, target path, headers // and body. inline auto makeRequest( - const http::verb method = http::verb::get, const std::string& target = "/", + const http::verb method = http::verb::get, + const std::string_view target = "/", const ad_utility::HashMap& headers = {}, const std::optional& body = std::nullopt) { // version 11 stands for HTTP/1.1 From e15048e1a6f15570b5c4afdaf24686d78beae0d4 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Fri, 24 Jan 2025 10:47:53 +0100 Subject: [PATCH 06/16] code review --- src/engine/Server.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index e02e84c4a5..cbf622a2f0 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -191,7 +191,7 @@ std::optional Server::extractAccessToken( tokenFromAuthorizationHeader != tokenFromParameter) { throw std::runtime_error( "Access token is specified both in the `Authorization` Header and the " - "parameters, but they aren't the same."); + "`access-token` parameter, but they aren't the same."); } return tokenFromAuthorizationHeader ? std::move(tokenFromAuthorizationHeader) : std::move(tokenFromParameter); @@ -1201,8 +1201,8 @@ bool Server::checkAccessToken( if (!accessToken) { return false; } - auto accessTokenProvidedMsg = absl::StrCat("Access token was provided"); - auto requestIgnoredMsg = ", request is ignored"; + const auto accessTokenProvidedMsg = "Access token was provided"; + const auto requestIgnoredMsg = ", request is ignored"; if (accessToken_.empty()) { throw std::runtime_error(absl::StrCat( accessTokenProvidedMsg, From 07a815fc4efc90fefbe36e6d14a04a0cbd7c98a8 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Fri, 24 Jan 2025 13:14:59 +0100 Subject: [PATCH 07/16] fix tests --- test/ServerTest.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index fcb0e12f36..791cd4d3e3 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -179,8 +179,8 @@ TEST(ServerTest, parseHttpRequest) { parse(makeRequest(method, pathWithAccessToken.buffer(), headersWithDifferentAccessToken, body)), testing::HasSubstr("Access token is specified both in the " - "`Authorization` Header and " - "the parameters, but they aren't the same.")); + "`Authorization` Header and the `access-token` " + "parameter, but they aren't the same.")); }; testAccessTokenCombinations(http::verb::get, "/?query=a", Query{"a"}); testAccessTokenCombinations(http::verb::post, "/", Query{"a"}, @@ -232,8 +232,8 @@ TEST(ServerTest, parseHttpRequest) { headersWithDifferentAccessToken, bodyWithAccessToken)), testing::HasSubstr("Access token is specified both in the " - "`Authorization` Header and " - "the parameters, but they aren't the same.")); + "`Authorization` Header and the `access-token` " + "parameter, but they aren't the same.")); }; testAccessTokenCombinationsUrlEncoded("query=SELECT%20%2A%20WHERE%20%7B%7D", Query{"SELECT * WHERE {}"}); @@ -429,7 +429,7 @@ TEST(ServerTest, extractAccessToken) { {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( "Access token is specified both in the `Authorization` Header and " - "the parameters, but they aren't the same.")); + "the `access-token` parameter, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "foo"}})), @@ -444,7 +444,7 @@ TEST(ServerTest, extractAccessToken) { {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( "Access token is specified both in the `Authorization` Header and " - "the parameters, but they aren't the same.")); + "the `access-token` parameter, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "foo"}})), From 9671ffdf2441281101206f396b8828734f7c9812 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Fri, 24 Jan 2025 14:44:50 +0100 Subject: [PATCH 08/16] code review --- src/engine/Server.cpp | 8 +++++--- test/ServerTest.cpp | 34 ++++++++++++++++++---------------- 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index cbf622a2f0..3670a83c62 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -173,10 +173,11 @@ std::optional Server::extractAccessToken( std::optional tokenFromParameter; if (request.find(http::field::authorization) != request.end()) { string_view authorization = request[http::field::authorization]; + // TODO: bearer is wrong, probably `Basic`. const std::string prefix = "Bearer "; if (!authorization.starts_with(prefix)) { - throw std::runtime_error( - absl::StrCat("Authorization header doesn't start with \"Bearer \".")); + throw std::runtime_error(absl::StrCat( + "Authorization header doesn't start with \"", prefix, "\".")); } authorization.remove_prefix(prefix.length()); tokenFromAuthorizationHeader = std::string(authorization); @@ -190,7 +191,8 @@ std::optional Server::extractAccessToken( if (tokenFromAuthorizationHeader && tokenFromParameter && tokenFromAuthorizationHeader != tokenFromParameter) { throw std::runtime_error( - "Access token is specified both in the `Authorization` Header and the " + "Access token is specified both in the `Authorization` header and by " + "the " "`access-token` parameter, but they aren't the same."); } return tokenFromAuthorizationHeader ? std::move(tokenFromAuthorizationHeader) diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index 791cd4d3e3..c3e5adaf64 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -147,11 +147,11 @@ TEST(ServerTest, parseHttpRequest) { ad_utility::source_location::current()) { auto t = generateLocationTrace(l); // Test the cases: - // 1. No Access token + // 1. No access token // 2. Access token in query - // 3. Access token in Authorization header - // 4. Different Access tokens - // 5. Same Access token + // 3. Access token in `Authorization` header + // 4. Different access tokens + // 5. Same access token boost::urls::url pathWithAccessToken{pathBase}; pathWithAccessToken.params().append({"access-token", "foo"}); ad_utility::HashMap @@ -178,9 +178,10 @@ TEST(ServerTest, parseHttpRequest) { AD_EXPECT_THROW_WITH_MESSAGE( parse(makeRequest(method, pathWithAccessToken.buffer(), headersWithDifferentAccessToken, body)), - testing::HasSubstr("Access token is specified both in the " - "`Authorization` Header and the `access-token` " - "parameter, but they aren't the same.")); + testing::HasSubstr( + "Access token is specified both in the " + "`Authorization` header and by the `access-token` " + "parameter, but they aren't the same.")); }; testAccessTokenCombinations(http::verb::get, "/?query=a", Query{"a"}); testAccessTokenCombinations(http::verb::post, "/", Query{"a"}, @@ -194,11 +195,11 @@ TEST(ServerTest, parseHttpRequest) { ad_utility::source_location::current()) { auto t = generateLocationTrace(l); // Test the cases: - // 1. No Access token + // 1. No access token // 2. Access token in query - // 3. Access token in Authorization header - // 4. Different Access tokens - // 5. Same Access token + // 3. Access token in `Authorization` header + // 4. Different access tokens + // 5. Same access token boost::urls::url paramsWithAccessToken{absl::StrCat("/?", bodyBase)}; paramsWithAccessToken.params().append({"access-token", "foo"}); std::string bodyWithAccessToken{ @@ -231,9 +232,10 @@ TEST(ServerTest, parseHttpRequest) { parse(makeRequest(http::verb::post, "/", headersWithDifferentAccessToken, bodyWithAccessToken)), - testing::HasSubstr("Access token is specified both in the " - "`Authorization` Header and the `access-token` " - "parameter, but they aren't the same.")); + testing::HasSubstr( + "Access token is specified both in the " + "`Authorization` header and by the `access-token` " + "parameter, but they aren't the same.")); }; testAccessTokenCombinationsUrlEncoded("query=SELECT%20%2A%20WHERE%20%7B%7D", Query{"SELECT * WHERE {}"}); @@ -428,7 +430,7 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( - "Access token is specified both in the `Authorization` Header and " + "Access token is specified both in the `Authorization` header and by" "the `access-token` parameter, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/", @@ -443,7 +445,7 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( - "Access token is specified both in the `Authorization` Header and " + "Access token is specified both in the `Authorization` header and by" "the `access-token` parameter, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", From dbdb5ca7b336e7b24e6f69d3b797da117dbd2529 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Fri, 24 Jan 2025 14:55:49 +0100 Subject: [PATCH 09/16] smaller changes in 1-on-1 review --- src/engine/Server.cpp | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 3670a83c62..48b9284e90 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -405,8 +405,7 @@ Awaitable Server::process( if (!accessTokenOk) { throw std::runtime_error(absl::StrCat( actionName, - " requires a valid access token. No valid access token is present.", - "Processing of request aborted.")); + " requires a valid access token but no access token was provided")); } }; @@ -1204,15 +1203,14 @@ bool Server::checkAccessToken( return false; } const auto accessTokenProvidedMsg = "Access token was provided"; - const auto requestIgnoredMsg = ", request is ignored"; if (accessToken_.empty()) { - throw std::runtime_error(absl::StrCat( - accessTokenProvidedMsg, - " but server was started without --access-token", requestIgnoredMsg)); + throw std::runtime_error( + absl::StrCat(accessTokenProvidedMsg, + " but server was started without --access-token")); } else if (!ad_utility::constantTimeEquals(accessToken.value(), accessToken_)) { - throw std::runtime_error(absl::StrCat( - accessTokenProvidedMsg, " but it was invalid", requestIgnoredMsg)); + throw std::runtime_error( + absl::StrCat(accessTokenProvidedMsg, " but it was invalid")); } else { LOG(DEBUG) << accessTokenProvidedMsg << " and correct" << std::endl; return true; From cd20b81df888b6805340401edfef28c0e22abd53 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Fri, 24 Jan 2025 15:40:40 +0100 Subject: [PATCH 10/16] missed a space --- test/ServerTest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index c3e5adaf64..059de69e2f 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -430,7 +430,7 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( - "Access token is specified both in the `Authorization` header and by" + "Access token is specified both in the `Authorization` header and by " "the `access-token` parameter, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/", @@ -445,7 +445,7 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( - "Access token is specified both in the `Authorization` header and by" + "Access token is specified both in the `Authorization` header and by " "the `access-token` parameter, but they aren't the same.")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", From ca3c05c3fe30c5186cf6bb2a2c085984125254c6 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Mon, 27 Jan 2025 10:35:11 +0100 Subject: [PATCH 11/16] replace bearer authentication with basic --- src/engine/Server.cpp | 35 ++++++++++++++++++++++--------- src/engine/Server.h | 6 ++++++ test/ServerTest.cpp | 48 ++++++++++++++++++++++++++++++++++--------- 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 48b9284e90..771b221fb6 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -12,6 +12,7 @@ #include #include "GraphStoreProtocol.h" +#include "absl/strings/escaping.h" #include "engine/ExecuteUpdate.h" #include "engine/ExportQueryExecutionTrees.h" #include "engine/QueryPlanner.h" @@ -166,6 +167,26 @@ void Server::run(const string& indexBaseName, bool useText, bool usePatterns, httpServer.run(); } +std::pair Server::decodeBasicAuthorization( + std::string_view authorizationHeader) { + const std::string prefix = "Basic "; + if (!authorizationHeader.starts_with(prefix)) { + throw std::runtime_error(absl::StrCat( + "Authorization header doesn't start with \"", prefix, "\".")); + } + authorizationHeader.remove_prefix(prefix.length()); + std::string decoded; + if (!absl::Base64Unescape(authorizationHeader, &decoded)) { + throw std::runtime_error("Failed to decode the Authorization header."); + } + // Everything after the first `:` is the password. (See RFC 7617 Sec. 2) + const auto separatorPos = decoded.find(":"); + if (separatorPos == string::npos) { + throw std::runtime_error("Failed to decode the Authorization header."); + } + return {decoded.substr(0, separatorPos), decoded.substr(separatorPos + 1)}; +} + std::optional Server::extractAccessToken( const ad_utility::httpUtils::HttpRequest auto& request, const ad_utility::url_parser::ParamValueMap& params) { @@ -173,14 +194,9 @@ std::optional Server::extractAccessToken( std::optional tokenFromParameter; if (request.find(http::field::authorization) != request.end()) { string_view authorization = request[http::field::authorization]; - // TODO: bearer is wrong, probably `Basic`. - const std::string prefix = "Bearer "; - if (!authorization.starts_with(prefix)) { - throw std::runtime_error(absl::StrCat( - "Authorization header doesn't start with \"", prefix, "\".")); - } - authorization.remove_prefix(prefix.length()); - tokenFromAuthorizationHeader = std::string(authorization); + auto [username, password] = decodeBasicAuthorization(authorization); + // TODO: what to do with the username? + tokenFromAuthorizationHeader = std::move(password); } if (params.contains("access-token")) { tokenFromParameter = ad_utility::url_parser::getParameterCheckAtMostOnce( @@ -192,8 +208,7 @@ std::optional Server::extractAccessToken( tokenFromAuthorizationHeader != tokenFromParameter) { throw std::runtime_error( "Access token is specified both in the `Authorization` header and by " - "the " - "`access-token` parameter, but they aren't the same."); + "the `access-token` parameter, but they aren't the same."); } return tokenFromAuthorizationHeader ? std::move(tokenFromAuthorizationHeader) : std::move(tokenFromParameter); diff --git a/src/engine/Server.h b/src/engine/Server.h index e286e15b02..b58caa8de1 100644 --- a/src/engine/Server.h +++ b/src/engine/Server.h @@ -35,6 +35,7 @@ class Server { FRIEND_TEST(ServerTest, parseHttpRequest); FRIEND_TEST(ServerTest, getQueryId); FRIEND_TEST(ServerTest, createMessageSender); + FRIEND_TEST(ServerTest, decodeBasicAuthorization); FRIEND_TEST(ServerTest, extractAccessToken); public: @@ -116,6 +117,11 @@ class Server { static ad_utility::url_parser::ParsedRequest parseHttpRequest( const ad_utility::httpUtils::HttpRequest auto& request); + /// Extract the username and password from an authorization header with the + /// `Basic` scheme. + static std::pair decodeBasicAuthorization( + std::string_view authorizationHeader); + /// Extract the Access token for that request from the `Authorization` header /// or the URL query parameters. static std::optional extractAccessToken( diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index 059de69e2f..5a37163f96 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -157,11 +157,11 @@ TEST(ServerTest, parseHttpRequest) { ad_utility::HashMap headersWithDifferentAccessToken{headers}; headersWithDifferentAccessToken.insert( - {http::field::authorization, "Bearer bar"}); + {http::field::authorization, "Basic OmJhcg=="}); ad_utility::HashMap headersWithSameAccessToken{headers}; headersWithSameAccessToken.insert( - {http::field::authorization, "Bearer foo"}); + {http::field::authorization, "Basic OmZvbw=="}); EXPECT_THAT(parse(makeRequest(method, pathBase, headers, body)), ParsedRequestIs("/", std::nullopt, {}, expectedOperation)); EXPECT_THAT(parse(makeRequest(method, pathWithAccessToken.buffer(), @@ -209,11 +209,11 @@ TEST(ServerTest, parseHttpRequest) { ad_utility::HashMap headersWithDifferentAccessToken{ {http::field::content_type, {URLENCODED}}, - {http::field::authorization, "Bearer bar"}}; + {http::field::authorization, "Basic OmJhcg=="}}; ad_utility::HashMap headersWithSameAccessToken{ {http::field::content_type, {URLENCODED}}, - {http::field::authorization, "Bearer foo"}}; + {http::field::authorization, "Basic OmZvbw=="}}; EXPECT_THAT( parse(makeRequest(http::verb::post, "/", headers, bodyBase)), ParsedRequestIs("/", std::nullopt, {}, expectedOperation)); @@ -410,6 +410,34 @@ TEST(ServerTest, createResponseMetadata) { EXPECT_THAT(metadata["located-triples"], testing::Eq(locatedTriplesJson)); } +TEST(ServerTest, decodeBasicAuthorization) { + const auto decode = Server::decodeBasicAuthorization; + const std::string wrongBeginning = + "Authorization header doesn't start with \"Basic \"."; + const std::string failedToDecode = + "Failed to decode the Authorization header."; + AD_EXPECT_THROW_WITH_MESSAGE(decode(""), testing::HasSubstr(wrongBeginning)); + AD_EXPECT_THROW_WITH_MESSAGE(decode("Bearer foo"), + testing::HasSubstr(wrongBeginning)); + AD_EXPECT_THROW_WITH_MESSAGE(decode("Bearer"), + testing::HasSubstr(wrongBeginning)); + // Not base64 + AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic foo"), + testing::HasSubstr(failedToDecode)); + // Stripped 1 padding `=` at the end + AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic OmZvbw="), + testing::HasSubstr(failedToDecode)); + // No colon + AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic Zm9vYmFy"), + testing::HasSubstr(failedToDecode)); + EXPECT_THAT(decode("Basic YTpi"), testing::Eq(std::pair("a", "b"))); + EXPECT_THAT(decode("Basic OmZvbzpiYXI="), + testing::Eq(std::pair("", "foo:bar"))); + EXPECT_THAT(decode("Basic e31fJSQ6YWJjZGVmZw=="), + testing::Eq(std::pair("{}_%$", "abcdefg"))); + EXPECT_THAT(decode("Basic OmZvbw=="), testing::Eq(std::pair("", "foo"))); +} + TEST(ServerTest, extractAccessToken) { auto extract = [](const ad_utility::httpUtils::HttpRequest auto& request) { auto parsedUrl = parseRequestTarget(request.target()); @@ -420,15 +448,15 @@ TEST(ServerTest, extractAccessToken) { testing::Optional(testing::Eq("foo"))); EXPECT_THAT( extract(makeRequest(http::verb::get, "/", - {{http::field::authorization, "Bearer foo"}})), + {{http::field::authorization, "Basic OmZvbw=="}})), testing::Optional(testing::Eq("foo"))); EXPECT_THAT( extract(makeRequest(http::verb::get, "/?access-token=foo", - {{http::field::authorization, "Bearer foo"}})), + {{http::field::authorization, "Basic OmZvbw=="}})), testing::Optional(testing::Eq("foo"))); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/?access-token=bar", - {{http::field::authorization, "Bearer foo"}})), + {{http::field::authorization, "Basic OmZvbw=="}})), testing::HasSubstr( "Access token is specified both in the `Authorization` header and by " "the `access-token` parameter, but they aren't the same.")); @@ -436,14 +464,14 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "foo"}})), testing::HasSubstr( - "Authorization header doesn't start with \"Bearer \".")); + "Authorization header doesn't start with \"Basic \".")); EXPECT_THAT(extract(makePostRequest("/", "text/turtle", "")), testing::Eq(std::nullopt)); EXPECT_THAT(extract(makePostRequest("/?access-token=foo", "text/turtle", "")), testing::Optional(testing::Eq("foo"))); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", - {{http::field::authorization, "Bearer foo"}})), + {{http::field::authorization, "Basic OmZvbw=="}})), testing::HasSubstr( "Access token is specified both in the `Authorization` header and by " "the `access-token` parameter, but they aren't the same.")); @@ -451,5 +479,5 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "foo"}})), testing::HasSubstr( - "Authorization header doesn't start with \"Bearer \".")); + "Authorization header doesn't start with \"Basic \".")); } From b3ac863d0768266ef701bb961b68825f2d853834 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Mon, 27 Jan 2025 11:27:46 +0100 Subject: [PATCH 12/16] fix gcc incompatibility --- test/ServerTest.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index 5a37163f96..e1ac8199be 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -430,12 +430,14 @@ TEST(ServerTest, decodeBasicAuthorization) { // No colon AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic Zm9vYmFy"), testing::HasSubstr(failedToDecode)); - EXPECT_THAT(decode("Basic YTpi"), testing::Eq(std::pair("a", "b"))); + EXPECT_THAT(decode("Basic YTpi"), + testing::Pair(testing::Eq("a"), testing::Eq("b"))); EXPECT_THAT(decode("Basic OmZvbzpiYXI="), - testing::Eq(std::pair("", "foo:bar"))); + testing::Pair(testing::Eq(""), testing::Eq("foo:bar"))); EXPECT_THAT(decode("Basic e31fJSQ6YWJjZGVmZw=="), - testing::Eq(std::pair("{}_%$", "abcdefg"))); - EXPECT_THAT(decode("Basic OmZvbw=="), testing::Eq(std::pair("", "foo"))); + testing::Pair(testing::Eq("{}_%$"), testing::Eq("abcdefg"))); + EXPECT_THAT(decode("Basic OmZvbw=="), + testing::Pair(testing::Eq(""), testing::Eq("foo"))); } TEST(ServerTest, extractAccessToken) { From 44e4e8feb55d2bb4006d75f0d27623e0e12f3175 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Wed, 29 Jan 2025 13:55:27 +0100 Subject: [PATCH 13/16] Revert to Bearer Reverts ca3c05c3fe30c5186cf6bb2a2c085984125254c6 --- src/engine/Server.cpp | 31 ++++++--------------------- src/engine/Server.h | 6 ------ test/ServerTest.cpp | 50 +++++++++---------------------------------- 3 files changed, 17 insertions(+), 70 deletions(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 4c3fd2f8a6..9d58498033 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -12,7 +12,6 @@ #include #include "GraphStoreProtocol.h" -#include "absl/strings/escaping.h" #include "engine/ExecuteUpdate.h" #include "engine/ExportQueryExecutionTrees.h" #include "engine/QueryPlanner.h" @@ -170,26 +169,6 @@ void Server::run(const string& indexBaseName, bool useText, bool usePatterns, httpServer.run(); } -std::pair Server::decodeBasicAuthorization( - std::string_view authorizationHeader) { - const std::string prefix = "Basic "; - if (!authorizationHeader.starts_with(prefix)) { - throw std::runtime_error(absl::StrCat( - "Authorization header doesn't start with \"", prefix, "\".")); - } - authorizationHeader.remove_prefix(prefix.length()); - std::string decoded; - if (!absl::Base64Unescape(authorizationHeader, &decoded)) { - throw std::runtime_error("Failed to decode the Authorization header."); - } - // Everything after the first `:` is the password. (See RFC 7617 Sec. 2) - const auto separatorPos = decoded.find(":"); - if (separatorPos == string::npos) { - throw std::runtime_error("Failed to decode the Authorization header."); - } - return {decoded.substr(0, separatorPos), decoded.substr(separatorPos + 1)}; -} - std::optional Server::extractAccessToken( const ad_utility::httpUtils::HttpRequest auto& request, const ad_utility::url_parser::ParamValueMap& params) { @@ -197,9 +176,13 @@ std::optional Server::extractAccessToken( std::optional tokenFromParameter; if (request.find(http::field::authorization) != request.end()) { string_view authorization = request[http::field::authorization]; - auto [username, password] = decodeBasicAuthorization(authorization); - // TODO: what to do with the username? - tokenFromAuthorizationHeader = std::move(password); + const std::string prefix = "Bearer "; + if (!authorization.starts_with(prefix)) { + throw std::runtime_error(absl::StrCat( + "Authorization header doesn't start with \"", prefix, "\".")); + } + authorization.remove_prefix(prefix.length()); + tokenFromAuthorizationHeader = std::string(authorization); } if (params.contains("access-token")) { tokenFromParameter = ad_utility::url_parser::getParameterCheckAtMostOnce( diff --git a/src/engine/Server.h b/src/engine/Server.h index 60d59caa2b..9558f8743e 100644 --- a/src/engine/Server.h +++ b/src/engine/Server.h @@ -35,7 +35,6 @@ class Server { FRIEND_TEST(ServerTest, parseHttpRequest); FRIEND_TEST(ServerTest, getQueryId); FRIEND_TEST(ServerTest, createMessageSender); - FRIEND_TEST(ServerTest, decodeBasicAuthorization); FRIEND_TEST(ServerTest, extractAccessToken); public: @@ -117,11 +116,6 @@ class Server { static ad_utility::url_parser::ParsedRequest parseHttpRequest( const ad_utility::httpUtils::HttpRequest auto& request); - /// Extract the username and password from an authorization header with the - /// `Basic` scheme. - static std::pair decodeBasicAuthorization( - std::string_view authorizationHeader); - /// Extract the Access token for that request from the `Authorization` header /// or the URL query parameters. static std::optional extractAccessToken( diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index e2f11d87c7..fad6af96a6 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -231,11 +231,11 @@ TEST(ServerTest, parseHttpRequest) { ad_utility::HashMap headersWithDifferentAccessToken{headers}; headersWithDifferentAccessToken.insert( - {http::field::authorization, "Basic OmJhcg=="}); + {http::field::authorization, "Bearer bar"}); ad_utility::HashMap headersWithSameAccessToken{headers}; headersWithSameAccessToken.insert( - {http::field::authorization, "Basic OmZvbw=="}); + {http::field::authorization, "Bearer foo"}); EXPECT_THAT(parse(makeRequest(method, pathBase, headers, body)), ParsedRequestIs("/", std::nullopt, {}, expectedOperation)); EXPECT_THAT(parse(makeRequest(method, pathWithAccessToken.buffer(), @@ -283,11 +283,11 @@ TEST(ServerTest, parseHttpRequest) { ad_utility::HashMap headersWithDifferentAccessToken{ {http::field::content_type, {URLENCODED}}, - {http::field::authorization, "Basic OmJhcg=="}}; + {http::field::authorization, "Bearer bar"}}; ad_utility::HashMap headersWithSameAccessToken{ {http::field::content_type, {URLENCODED}}, - {http::field::authorization, "Basic OmZvbw=="}}; + {http::field::authorization, "Bearer foo"}}; EXPECT_THAT( parse(makeRequest(http::verb::post, "/", headers, bodyBase)), ParsedRequestIs("/", std::nullopt, {}, expectedOperation)); @@ -484,36 +484,6 @@ TEST(ServerTest, createResponseMetadata) { EXPECT_THAT(metadata["located-triples"], testing::Eq(locatedTriplesJson)); } -TEST(ServerTest, decodeBasicAuthorization) { - const auto decode = Server::decodeBasicAuthorization; - const std::string wrongBeginning = - "Authorization header doesn't start with \"Basic \"."; - const std::string failedToDecode = - "Failed to decode the Authorization header."; - AD_EXPECT_THROW_WITH_MESSAGE(decode(""), testing::HasSubstr(wrongBeginning)); - AD_EXPECT_THROW_WITH_MESSAGE(decode("Bearer foo"), - testing::HasSubstr(wrongBeginning)); - AD_EXPECT_THROW_WITH_MESSAGE(decode("Bearer"), - testing::HasSubstr(wrongBeginning)); - // Not base64 - AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic foo"), - testing::HasSubstr(failedToDecode)); - // Stripped 1 padding `=` at the end - AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic OmZvbw="), - testing::HasSubstr(failedToDecode)); - // No colon - AD_EXPECT_THROW_WITH_MESSAGE(decode("Basic Zm9vYmFy"), - testing::HasSubstr(failedToDecode)); - EXPECT_THAT(decode("Basic YTpi"), - testing::Pair(testing::Eq("a"), testing::Eq("b"))); - EXPECT_THAT(decode("Basic OmZvbzpiYXI="), - testing::Pair(testing::Eq(""), testing::Eq("foo:bar"))); - EXPECT_THAT(decode("Basic e31fJSQ6YWJjZGVmZw=="), - testing::Pair(testing::Eq("{}_%$"), testing::Eq("abcdefg"))); - EXPECT_THAT(decode("Basic OmZvbw=="), - testing::Pair(testing::Eq(""), testing::Eq("foo"))); -} - TEST(ServerTest, extractAccessToken) { auto extract = [](const ad_utility::httpUtils::HttpRequest auto& request) { auto parsedUrl = parseRequestTarget(request.target()); @@ -524,15 +494,15 @@ TEST(ServerTest, extractAccessToken) { testing::Optional(testing::Eq("foo"))); EXPECT_THAT( extract(makeRequest(http::verb::get, "/", - {{http::field::authorization, "Basic OmZvbw=="}})), + {{http::field::authorization, "Bearer foo"}})), testing::Optional(testing::Eq("foo"))); EXPECT_THAT( extract(makeRequest(http::verb::get, "/?access-token=foo", - {{http::field::authorization, "Basic OmZvbw=="}})), + {{http::field::authorization, "Bearer foo"}})), testing::Optional(testing::Eq("foo"))); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/?access-token=bar", - {{http::field::authorization, "Basic OmZvbw=="}})), + {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( "Access token is specified both in the `Authorization` header and by " "the `access-token` parameter, but they aren't the same.")); @@ -540,14 +510,14 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "foo"}})), testing::HasSubstr( - "Authorization header doesn't start with \"Basic \".")); + "Authorization header doesn't start with \"Bearer \".")); EXPECT_THAT(extract(makePostRequest("/", "text/turtle", "")), testing::Eq(std::nullopt)); EXPECT_THAT(extract(makePostRequest("/?access-token=foo", "text/turtle", "")), testing::Optional(testing::Eq("foo"))); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", - {{http::field::authorization, "Basic OmZvbw=="}})), + {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( "Access token is specified both in the `Authorization` header and by " "the `access-token` parameter, but they aren't the same.")); @@ -555,5 +525,5 @@ TEST(ServerTest, extractAccessToken) { extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "foo"}})), testing::HasSubstr( - "Authorization header doesn't start with \"Basic \".")); + "Authorization header doesn't start with \"Bearer \".")); } From a8b052f876b07ff4818d9b56e3cd7f07aaac50af Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Thu, 30 Jan 2025 20:33:08 +0100 Subject: [PATCH 14/16] small changes --- src/engine/Server.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/Server.cpp b/src/engine/Server.cpp index 9d58498033..4f7fc97a4e 100644 --- a/src/engine/Server.cpp +++ b/src/engine/Server.cpp @@ -194,7 +194,7 @@ std::optional Server::extractAccessToken( tokenFromAuthorizationHeader != tokenFromParameter) { throw std::runtime_error( "Access token is specified both in the `Authorization` header and by " - "the `access-token` parameter, but they aren't the same."); + "the `access-token` parameter, but they are not the same"); } return tokenFromAuthorizationHeader ? std::move(tokenFromAuthorizationHeader) : std::move(tokenFromParameter); From 23d5eb74a1a0e8fd800da62997879df8e1fcf787 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Thu, 30 Jan 2025 20:35:53 +0100 Subject: [PATCH 15/16] upgrade upload-artifact action --- .github/workflows/sparql-conformance.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/sparql-conformance.yml b/.github/workflows/sparql-conformance.yml index 38152b5c79..3e4bdfd63d 100644 --- a/.github/workflows/sparql-conformance.yml +++ b/.github/workflows/sparql-conformance.yml @@ -80,7 +80,7 @@ jobs: echo ${{github.event.pull_request.head.sha}} > ./conformance-report/sha mv ${{ github.workspace}}/qlever-test-suite/results/${{ github.sha }}.json.bz2 conformance-report/${{ github.event.pull_request.head.sha }}.json.bz2 - name: Upload coverage artifact - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: conformance-report path: conformance-report/ \ No newline at end of file From 8928c4df06229ea4d61c8062d59e8d7cdab79153 Mon Sep 17 00:00:00 2001 From: Julian Mundhahs Date: Thu, 30 Jan 2025 21:16:34 +0100 Subject: [PATCH 16/16] adjust tests --- test/ServerTest.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/ServerTest.cpp b/test/ServerTest.cpp index fad6af96a6..0319b138d5 100644 --- a/test/ServerTest.cpp +++ b/test/ServerTest.cpp @@ -255,7 +255,7 @@ TEST(ServerTest, parseHttpRequest) { testing::HasSubstr( "Access token is specified both in the " "`Authorization` header and by the `access-token` " - "parameter, but they aren't the same.")); + "parameter, but they are not the same")); }; testAccessTokenCombinations(http::verb::get, "/?query=a", Query{"a", {}}); testAccessTokenCombinations(http::verb::post, "/", Query{"a", {}}, @@ -309,7 +309,7 @@ TEST(ServerTest, parseHttpRequest) { testing::HasSubstr( "Access token is specified both in the " "`Authorization` header and by the `access-token` " - "parameter, but they aren't the same.")); + "parameter, but they are not the same")); }; testAccessTokenCombinationsUrlEncoded("query=SELECT%20%2A%20WHERE%20%7B%7D", Query{"SELECT * WHERE {}", {}}); @@ -505,7 +505,7 @@ TEST(ServerTest, extractAccessToken) { {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( "Access token is specified both in the `Authorization` header and by " - "the `access-token` parameter, but they aren't the same.")); + "the `access-token` parameter, but they are not the same")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::get, "/", {{http::field::authorization, "foo"}})), @@ -520,7 +520,7 @@ TEST(ServerTest, extractAccessToken) { {{http::field::authorization, "Bearer foo"}})), testing::HasSubstr( "Access token is specified both in the `Authorization` header and by " - "the `access-token` parameter, but they aren't the same.")); + "the `access-token` parameter, but they are not the same")); AD_EXPECT_THROW_WITH_MESSAGE( extract(makeRequest(http::verb::post, "/?access-token=bar", {{http::field::authorization, "foo"}})),