Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept access token from Authorization header #1720

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+149 −27
Open

Accept access token from Authorization header #1720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 32 additions & 3 deletions src/engine/Server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,27 @@
httpServer.run();
}

std::optional<std::string> 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, "\"."));
Comment on lines +174 to +176
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dislike responding with the access token, but did it to stay consistent with the rest of the code. Sensitive data should imo not be echoed.

}
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) {
Expand All @@ -172,7 +193,8 @@
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
Expand All @@ -186,9 +208,14 @@
parsedRequest.parameters_.erase(paramName);
}
};
auto extractAccessTokenFromRequest = [&parsedRequest, &request]() {
parsedRequest.accessToken_ =
extractAccessToken(request, parsedRequest.parameters_);
};

if (request.method() == http::verb::get) {
setOperationIfSpecifiedInParams.template operator()<Query>("query");
extractAccessTokenFromRequest();
if (parsedRequest.parameters_.contains("update")) {
throw std::runtime_error("SPARQL Update is not allowed as GET request.");
}
Expand Down Expand Up @@ -258,15 +285,18 @@
}
setOperationIfSpecifiedInParams.template operator()<Query>("query");
setOperationIfSpecifiedInParams.template operator()<Update>("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(
Expand Down Expand Up @@ -353,8 +383,7 @@
// 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_);

Check warning on line 386 in src/engine/Server.cpp

View check run for this annotation

Codecov / codecov/patch

src/engine/Server.cpp#L386 

Added line #L386 was not covered by tests
auto requireValidAccessToken = [&accessTokenOk](
const std::string& actionName) {
if (!accessTokenOk) {
Expand Down
7 changes: 7 additions & 0 deletions src/engine/Server.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<std::string> 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`.
Expand Down
2 changes: 2 additions & 0 deletions src/util/http/UrlParser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> accessToken_;
ParamValueMap parameters_;
std::variant<sparqlOperation::Query, sparqlOperation::Update,
sparqlOperation::None>
Expand Down
132 changes: 108 additions & 24 deletions test/ServerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,14 @@ using namespace ad_utility::url_parser::sparqlOperation;

namespace {
auto ParsedRequestIs = [](const std::string& path,
const std::optional<std::string>& accessToken,
const ParamValueMap& parameters,
const std::variant<Query, Update, None>& operation)
-> testing::Matcher<const ParsedRequest> {
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_,
Expand Down Expand Up @@ -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")),
Expand All @@ -92,18 +97,20 @@ 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,
"query=SELECT%20%2A%20WHERE%20%7B%7D&default-graph-uri=https%3A%2F%"
"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 {}"}));
Expand All @@ -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(
Expand All @@ -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<qup42>: there could be some more here, but i'll wait until #1668
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

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) {
Expand Down Expand Up @@ -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<qup42>: replace once #1668 is merged
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO

auto makeRequest =
[](const http::verb method = http::verb::get,
const std::string& target = "/",
const ad_utility::HashMap<http::field, std::string>& headers = {},
const std::optional<std::string>& body = std::nullopt) {
// version 11 stands for HTTP/1.1
auto req = http::request<http::string_body>{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\"."));
}
Loading