From 08f596c2eba4320d41aa5f812f0151d2071bd7d3 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 10 Oct 2018 20:57:29 -0400 Subject: telemetry_json: Remove unnecessary includes Removes unused includes. Also rectifies a missing include. --- src/web_service/telemetry_json.cpp | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/web_service/telemetry_json.cpp') diff --git a/src/web_service/telemetry_json.cpp b/src/web_service/telemetry_json.cpp index 033ea1ea4f..e77950aa3b 100644 --- a/src/web_service/telemetry_json.cpp +++ b/src/web_service/telemetry_json.cpp @@ -2,8 +2,6 @@ // Licensed under GPLv2 or any later version // Refer to the license.txt file included. -#include -#include "common/assert.h" #include "common/detached_tasks.h" #include "web_service/telemetry_json.h" #include "web_service/web_backend.h" -- cgit v1.2.3 From ef83cc5f94ee4ee4f0268aef7939860905591b56 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 10 Oct 2018 20:59:25 -0400 Subject: telemetry_json: Take std::string parameters by value Taking them by const reference isn't advisable here, because it means the std::move calls were doing nothing and we were always copying the std::string instances. --- src/web_service/telemetry_json.cpp | 3 +-- src/web_service/telemetry_json.h | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) (limited to 'src/web_service/telemetry_json.cpp') diff --git a/src/web_service/telemetry_json.cpp b/src/web_service/telemetry_json.cpp index e77950aa3b..b24e806a89 100644 --- a/src/web_service/telemetry_json.cpp +++ b/src/web_service/telemetry_json.cpp @@ -8,8 +8,7 @@ namespace WebService { -TelemetryJson::TelemetryJson(const std::string& host, const std::string& username, - const std::string& token) +TelemetryJson::TelemetryJson(std::string host, std::string username, std::string token) : host(std::move(host)), username(std::move(username)), token(std::move(token)) {} TelemetryJson::~TelemetryJson() = default; diff --git a/src/web_service/telemetry_json.h b/src/web_service/telemetry_json.h index 72627a4167..92ead9856c 100644 --- a/src/web_service/telemetry_json.h +++ b/src/web_service/telemetry_json.h @@ -18,7 +18,7 @@ namespace WebService { */ class TelemetryJson : public Telemetry::VisitorInterface { public: - TelemetryJson(const std::string& host, const std::string& username, const std::string& token); + TelemetryJson(std::string host, std::string username, std::string token); ~TelemetryJson(); void Visit(const Telemetry::Field& field) override; -- cgit v1.2.3 From 1d3abb95da13c095ac10a42940e9c295aaab39eb Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 10 Oct 2018 21:04:19 -0400 Subject: telemetry_json: Use the PImpl idiom to avoid unnecessary dependency exposure Users of the web_service library shouldn't need to care about an external library like json.h. However, given it's exposed in our interface, this requires that other libraries publicly link in the JSON library. We can do better. By using the PImpl idiom, we can hide this dependency in the cpp file and remove the need to link that library in altogether. --- src/web_service/telemetry_json.cpp | 86 +++++++++++++++++++++++--------------- src/web_service/telemetry_json.h | 18 +------- 2 files changed, 55 insertions(+), 49 deletions(-) (limited to 'src/web_service/telemetry_json.cpp') diff --git a/src/web_service/telemetry_json.cpp b/src/web_service/telemetry_json.cpp index b24e806a89..3c5590054b 100644 --- a/src/web_service/telemetry_json.cpp +++ b/src/web_service/telemetry_json.cpp @@ -2,93 +2,113 @@ // Licensed under GPLv2 or any later version // Refer to the license.txt file included. +#include #include "common/detached_tasks.h" #include "web_service/telemetry_json.h" #include "web_service/web_backend.h" namespace WebService { -TelemetryJson::TelemetryJson(std::string host, std::string username, std::string token) - : host(std::move(host)), username(std::move(username)), token(std::move(token)) {} -TelemetryJson::~TelemetryJson() = default; +struct TelemetryJson::Impl { + Impl(std::string host, std::string username, std::string token) + : host{std::move(host)}, username{std::move(username)}, token{std::move(token)} {} -template -void TelemetryJson::Serialize(Telemetry::FieldType type, const std::string& name, T value) { - sections[static_cast(type)][name] = value; -} + nlohmann::json& TopSection() { + return sections[static_cast(Telemetry::FieldType::None)]; + } -void TelemetryJson::SerializeSection(Telemetry::FieldType type, const std::string& name) { - TopSection()[name] = sections[static_cast(type)]; -} + const nlohmann::json& TopSection() const { + return sections[static_cast(Telemetry::FieldType::None)]; + } + + template + void Serialize(Telemetry::FieldType type, const std::string& name, T value) { + sections[static_cast(type)][name] = value; + } + + void SerializeSection(Telemetry::FieldType type, const std::string& name) { + TopSection()[name] = sections[static_cast(type)]; + } + + nlohmann::json output; + std::array sections; + std::string host; + std::string username; + std::string token; +}; + +TelemetryJson::TelemetryJson(std::string host, std::string username, std::string token) + : impl{std::make_unique(std::move(host), std::move(username), std::move(token))} {} +TelemetryJson::~TelemetryJson() = default; void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue()); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), std::string(field.GetValue())); + impl->Serialize(field.GetType(), field.GetName(), std::string(field.GetValue())); } void TelemetryJson::Visit(const Telemetry::Field& field) { - Serialize(field.GetType(), field.GetName(), field.GetValue().count()); + impl->Serialize(field.GetType(), field.GetName(), field.GetValue().count()); } void TelemetryJson::Complete() { - SerializeSection(Telemetry::FieldType::App, "App"); - SerializeSection(Telemetry::FieldType::Session, "Session"); - SerializeSection(Telemetry::FieldType::Performance, "Performance"); - SerializeSection(Telemetry::FieldType::UserFeedback, "UserFeedback"); - SerializeSection(Telemetry::FieldType::UserConfig, "UserConfig"); - SerializeSection(Telemetry::FieldType::UserSystem, "UserSystem"); - - auto content = TopSection().dump(); + impl->SerializeSection(Telemetry::FieldType::App, "App"); + impl->SerializeSection(Telemetry::FieldType::Session, "Session"); + impl->SerializeSection(Telemetry::FieldType::Performance, "Performance"); + impl->SerializeSection(Telemetry::FieldType::UserFeedback, "UserFeedback"); + impl->SerializeSection(Telemetry::FieldType::UserConfig, "UserConfig"); + impl->SerializeSection(Telemetry::FieldType::UserSystem, "UserSystem"); + + auto content = impl->TopSection().dump(); // Send the telemetry async but don't handle the errors since they were written to the log Common::DetachedTasks::AddTask( - [host{this->host}, username{this->username}, token{this->token}, content]() { + [host{impl->host}, username{impl->username}, token{impl->token}, content]() { Client{host, username, token}.PostJson("/telemetry", content, true); }); } diff --git a/src/web_service/telemetry_json.h b/src/web_service/telemetry_json.h index 9a2aaa2c81..93371414ae 100644 --- a/src/web_service/telemetry_json.h +++ b/src/web_service/telemetry_json.h @@ -4,10 +4,8 @@ #pragma once -#include #include #include -#include #include "common/telemetry.h" namespace WebService { @@ -39,20 +37,8 @@ public: void Complete() override; private: - nlohmann::json& TopSection() { - return sections[static_cast(Telemetry::FieldType::None)]; - } - - template - void Serialize(Telemetry::FieldType type, const std::string& name, T value); - - void SerializeSection(Telemetry::FieldType type, const std::string& name); - - nlohmann::json output; - std::array sections; - std::string host; - std::string username; - std::string token; + struct Impl; + std::unique_ptr impl; }; } // namespace WebService -- cgit v1.2.3 From 2619abec47d2e579a42b0ee299611f8e4479d5f6 Mon Sep 17 00:00:00 2001 From: Lioncash Date: Wed, 10 Oct 2018 21:23:41 -0400 Subject: web_backend: Make Client use the PImpl idiom Like with TelemetryJson, we can make the implementation details private and avoid the need to expose httplib to external libraries that need to use the Client class. --- src/common/web_result.h | 1 + src/web_service/telemetry_json.cpp | 1 + src/web_service/verify_login.cpp | 1 + src/web_service/web_backend.cpp | 235 ++++++++++++++++++++++--------------- src/web_service/web_backend.h | 58 ++------- 5 files changed, 154 insertions(+), 142 deletions(-) (limited to 'src/web_service/telemetry_json.cpp') diff --git a/src/common/web_result.h b/src/common/web_result.h index 9699266740..8bfa2141d8 100644 --- a/src/common/web_result.h +++ b/src/common/web_result.h @@ -5,6 +5,7 @@ #pragma once #include +#include "common/common_types.h" namespace Common { struct WebResult { diff --git a/src/web_service/telemetry_json.cpp b/src/web_service/telemetry_json.cpp index 3c5590054b..0a8f2bd9e1 100644 --- a/src/web_service/telemetry_json.cpp +++ b/src/web_service/telemetry_json.cpp @@ -4,6 +4,7 @@ #include #include "common/detached_tasks.h" +#include "common/web_result.h" #include "web_service/telemetry_json.h" #include "web_service/web_backend.h" diff --git a/src/web_service/verify_login.cpp b/src/web_service/verify_login.cpp index 124aa3863e..ca4b43b938 100644 --- a/src/web_service/verify_login.cpp +++ b/src/web_service/verify_login.cpp @@ -3,6 +3,7 @@ // Refer to the license.txt file included. #include +#include "common/web_result.h" #include "web_service/verify_login.h" #include "web_service/web_backend.h" diff --git a/src/web_service/web_backend.cpp b/src/web_service/web_backend.cpp index 787b0fbcbd..b7737b6156 100644 --- a/src/web_service/web_backend.cpp +++ b/src/web_service/web_backend.cpp @@ -3,9 +3,11 @@ // Refer to the license.txt file included. #include +#include #include -#include #include +#include +#include "common/common_types.h" #include "common/logging/log.h" #include "common/web_result.h" #include "core/settings.h" @@ -20,99 +22,132 @@ constexpr u32 HTTPS_PORT = 443; constexpr u32 TIMEOUT_SECONDS = 30; -Client::JWTCache Client::jwt_cache{}; +struct Client::Impl { + Impl(std::string host, std::string username, std::string token) + : host{std::move(host)}, username{std::move(username)}, token{std::move(token)} { + std::lock_guard lock(jwt_cache.mutex); + if (this->username == jwt_cache.username && this->token == jwt_cache.token) { + jwt = jwt_cache.jwt; + } + } + + /// A generic function handles POST, GET and DELETE request together + Common::WebResult GenericJson(const std::string& method, const std::string& path, + const std::string& data, bool allow_anonymous) { + if (jwt.empty()) { + UpdateJWT(); + } + + if (jwt.empty() && !allow_anonymous) { + LOG_ERROR(WebService, "Credentials must be provided for authenticated requests"); + return Common::WebResult{Common::WebResult::Code::CredentialsMissing, + "Credentials needed"}; + } + + auto result = GenericJson(method, path, data, jwt); + if (result.result_string == "401") { + // Try again with new JWT + UpdateJWT(); + result = GenericJson(method, path, data, jwt); + } -Client::Client(const std::string& host, const std::string& username, const std::string& token) - : host(host), username(username), token(token) { - std::lock_guard lock(jwt_cache.mutex); - if (username == jwt_cache.username && token == jwt_cache.token) { - jwt = jwt_cache.jwt; + return result; } -} -Common::WebResult Client::GenericJson(const std::string& method, const std::string& path, - const std::string& data, const std::string& jwt, - const std::string& username, const std::string& token) { - if (cli == nullptr) { - auto parsedUrl = LUrlParser::clParseURL::ParseURL(host); - int port; - if (parsedUrl.m_Scheme == "http") { - if (!parsedUrl.GetPort(&port)) { - port = HTTP_PORT; - } - cli = - std::make_unique(parsedUrl.m_Host.c_str(), port, TIMEOUT_SECONDS); - } else if (parsedUrl.m_Scheme == "https") { - if (!parsedUrl.GetPort(&port)) { - port = HTTPS_PORT; + /** + * A generic function with explicit authentication method specified + * JWT is used if the jwt parameter is not empty + * username + token is used if jwt is empty but username and token are not empty + * anonymous if all of jwt, username and token are empty + */ + Common::WebResult GenericJson(const std::string& method, const std::string& path, + const std::string& data, const std::string& jwt = "", + const std::string& username = "", const std::string& token = "") { + if (cli == nullptr) { + auto parsedUrl = LUrlParser::clParseURL::ParseURL(host); + int port; + if (parsedUrl.m_Scheme == "http") { + if (!parsedUrl.GetPort(&port)) { + port = HTTP_PORT; + } + cli = std::make_unique(parsedUrl.m_Host.c_str(), port, + TIMEOUT_SECONDS); + } else if (parsedUrl.m_Scheme == "https") { + if (!parsedUrl.GetPort(&port)) { + port = HTTPS_PORT; + } + cli = std::make_unique(parsedUrl.m_Host.c_str(), port, + TIMEOUT_SECONDS); + } else { + LOG_ERROR(WebService, "Bad URL scheme {}", parsedUrl.m_Scheme); + return Common::WebResult{Common::WebResult::Code::InvalidURL, "Bad URL scheme"}; } - cli = std::make_unique(parsedUrl.m_Host.c_str(), port, - TIMEOUT_SECONDS); - } else { - LOG_ERROR(WebService, "Bad URL scheme {}", parsedUrl.m_Scheme); - return Common::WebResult{Common::WebResult::Code::InvalidURL, "Bad URL scheme"}; } - } - if (cli == nullptr) { - LOG_ERROR(WebService, "Invalid URL {}", host + path); - return Common::WebResult{Common::WebResult::Code::InvalidURL, "Invalid URL"}; - } + if (cli == nullptr) { + LOG_ERROR(WebService, "Invalid URL {}", host + path); + return Common::WebResult{Common::WebResult::Code::InvalidURL, "Invalid URL"}; + } - httplib::Headers params; - if (!jwt.empty()) { - params = { - {std::string("Authorization"), fmt::format("Bearer {}", jwt)}, - }; - } else if (!username.empty()) { - params = { - {std::string("x-username"), username}, - {std::string("x-token"), token}, + httplib::Headers params; + if (!jwt.empty()) { + params = { + {std::string("Authorization"), fmt::format("Bearer {}", jwt)}, + }; + } else if (!username.empty()) { + params = { + {std::string("x-username"), username}, + {std::string("x-token"), token}, + }; + } + + params.emplace(std::string("api-version"), + std::string(API_VERSION.begin(), API_VERSION.end())); + if (method != "GET") { + params.emplace(std::string("Content-Type"), std::string("application/json")); }; - } - params.emplace(std::string("api-version"), std::string(API_VERSION.begin(), API_VERSION.end())); - if (method != "GET") { - params.emplace(std::string("Content-Type"), std::string("application/json")); - }; + httplib::Request request; + request.method = method; + request.path = path; + request.headers = params; + request.body = data; - httplib::Request request; - request.method = method; - request.path = path; - request.headers = params; - request.body = data; + httplib::Response response; - httplib::Response response; + if (!cli->send(request, response)) { + LOG_ERROR(WebService, "{} to {} returned null", method, host + path); + return Common::WebResult{Common::WebResult::Code::LibError, "Null response"}; + } - if (!cli->send(request, response)) { - LOG_ERROR(WebService, "{} to {} returned null", method, host + path); - return Common::WebResult{Common::WebResult::Code::LibError, "Null response"}; - } + if (response.status >= 400) { + LOG_ERROR(WebService, "{} to {} returned error status code: {}", method, host + path, + response.status); + return Common::WebResult{Common::WebResult::Code::HttpError, + std::to_string(response.status)}; + } - if (response.status >= 400) { - LOG_ERROR(WebService, "{} to {} returned error status code: {}", method, host + path, - response.status); - return Common::WebResult{Common::WebResult::Code::HttpError, - std::to_string(response.status)}; - } + auto content_type = response.headers.find("content-type"); - auto content_type = response.headers.find("content-type"); + if (content_type == response.headers.end()) { + LOG_ERROR(WebService, "{} to {} returned no content", method, host + path); + return Common::WebResult{Common::WebResult::Code::WrongContent, ""}; + } - if (content_type == response.headers.end()) { - LOG_ERROR(WebService, "{} to {} returned no content", method, host + path); - return Common::WebResult{Common::WebResult::Code::WrongContent, ""}; + if (content_type->second.find("application/json") == std::string::npos && + content_type->second.find("text/html; charset=utf-8") == std::string::npos) { + LOG_ERROR(WebService, "{} to {} returned wrong content: {}", method, host + path, + content_type->second); + return Common::WebResult{Common::WebResult::Code::WrongContent, "Wrong content"}; + } + return Common::WebResult{Common::WebResult::Code::Success, "", response.body}; } - if (content_type->second.find("application/json") == std::string::npos && - content_type->second.find("text/html; charset=utf-8") == std::string::npos) { - LOG_ERROR(WebService, "{} to {} returned wrong content: {}", method, host + path, - content_type->second); - return Common::WebResult{Common::WebResult::Code::WrongContent, "Wrong content"}; - } - return Common::WebResult{Common::WebResult::Code::Success, "", response.body}; -} + // Retrieve a new JWT from given username and token + void UpdateJWT() { + if (username.empty() || token.empty()) { + return; + } -void Client::UpdateJWT() { - if (!username.empty() && !token.empty()) { auto result = GenericJson("POST", "/jwt/internal", "", "", username, token); if (result.result_code != Common::WebResult::Code::Success) { LOG_ERROR(WebService, "UpdateJWT failed"); @@ -123,27 +158,39 @@ void Client::UpdateJWT() { jwt_cache.jwt = jwt = result.returned_data; } } -} -Common::WebResult Client::GenericJson(const std::string& method, const std::string& path, - const std::string& data, bool allow_anonymous) { - if (jwt.empty()) { - UpdateJWT(); - } + std::string host; + std::string username; + std::string token; + std::string jwt; + std::unique_ptr cli; + + struct JWTCache { + std::mutex mutex; + std::string username; + std::string token; + std::string jwt; + }; + static inline JWTCache jwt_cache; +}; - if (jwt.empty() && !allow_anonymous) { - LOG_ERROR(WebService, "Credentials must be provided for authenticated requests"); - return Common::WebResult{Common::WebResult::Code::CredentialsMissing, "Credentials needed"}; - } +Client::Client(std::string host, std::string username, std::string token) + : impl{std::make_unique(std::move(host), std::move(username), std::move(token))} {} - auto result = GenericJson(method, path, data, jwt); - if (result.result_string == "401") { - // Try again with new JWT - UpdateJWT(); - result = GenericJson(method, path, data, jwt); - } +Client::~Client() = default; + +Common::WebResult Client::PostJson(const std::string& path, const std::string& data, + bool allow_anonymous) { + return impl->GenericJson("POST", path, data, allow_anonymous); +} + +Common::WebResult Client::GetJson(const std::string& path, bool allow_anonymous) { + return impl->GenericJson("GET", path, "", allow_anonymous); +} - return result; +Common::WebResult Client::DeleteJson(const std::string& path, const std::string& data, + bool allow_anonymous) { + return impl->GenericJson("DELETE", path, data, allow_anonymous); } } // namespace WebService diff --git a/src/web_service/web_backend.h b/src/web_service/web_backend.h index d75fbcc155..c637e09df3 100644 --- a/src/web_service/web_backend.h +++ b/src/web_service/web_backend.h @@ -4,23 +4,19 @@ #pragma once -#include -#include +#include #include -#include -#include -#include "common/common_types.h" -#include "common/web_result.h" -namespace httplib { -class Client; +namespace Common { +struct WebResult; } namespace WebService { class Client { public: - Client(const std::string& host, const std::string& username, const std::string& token); + Client(std::string host, std::string username, std::string token); + ~Client(); /** * Posts JSON to the specified path. @@ -30,9 +26,7 @@ public: * @return the result of the request. */ Common::WebResult PostJson(const std::string& path, const std::string& data, - bool allow_anonymous) { - return GenericJson("POST", path, data, allow_anonymous); - } + bool allow_anonymous); /** * Gets JSON from the specified path. @@ -40,9 +34,7 @@ public: * @param allow_anonymous If true, allow anonymous unauthenticated requests. * @return the result of the request. */ - Common::WebResult GetJson(const std::string& path, bool allow_anonymous) { - return GenericJson("GET", path, "", allow_anonymous); - } + Common::WebResult GetJson(const std::string& path, bool allow_anonymous); /** * Deletes JSON to the specified path. @@ -52,41 +44,11 @@ public: * @return the result of the request. */ Common::WebResult DeleteJson(const std::string& path, const std::string& data, - bool allow_anonymous) { - return GenericJson("DELETE", path, data, allow_anonymous); - } + bool allow_anonymous); private: - /// A generic function handles POST, GET and DELETE request together - Common::WebResult GenericJson(const std::string& method, const std::string& path, - const std::string& data, bool allow_anonymous); - - /** - * A generic function with explicit authentication method specified - * JWT is used if the jwt parameter is not empty - * username + token is used if jwt is empty but username and token are not empty - * anonymous if all of jwt, username and token are empty - */ - Common::WebResult GenericJson(const std::string& method, const std::string& path, - const std::string& data, const std::string& jwt = "", - const std::string& username = "", const std::string& token = ""); - - // Retrieve a new JWT from given username and token - void UpdateJWT(); - - std::string host; - std::string username; - std::string token; - std::string jwt; - std::unique_ptr cli; - - struct JWTCache { - std::mutex mutex; - std::string username; - std::string token; - std::string jwt; - }; - static JWTCache jwt_cache; + struct Impl; + std::unique_ptr impl; }; } // namespace WebService -- cgit v1.2.3