From 3db8f96a1019e0245a6bd32b39a4ba6a19750de9 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Wed, 28 Aug 2024 11:27:28 -0400 Subject: [PATCH 1/5] Added initial redirect support for CPP SDK --- .gitignore | 6 +- include/cpprealm/networking/http.hpp | 10 +- src/cpprealm/analytics.cpp | 2 +- .../internal/networking/network_transport.cpp | 126 +++++++++++------- tests/admin_utils.cpp | 73 +++++++--- tests/sync/flexible_sync_tests.cpp | 57 +++++++- tests/sync/networking_tests.cpp | 17 +-- 7 files changed, 197 insertions(+), 94 deletions(-) diff --git a/.gitignore b/.gitignore index e7d5d1eb..b03bbd26 100644 --- a/.gitignore +++ b/.gitignore @@ -1,7 +1,10 @@ .DS_Store /.build +/build.debug /Packages /*.xcodeproj +.idea/ +/.vscode /.swiftpm xcuserdata/ DerivedData/ @@ -10,5 +13,4 @@ Package.resolved examples/*.pro.user docs/html docs/latex -.idea/ -realm-core/src/realm/parser/generated/ \ No newline at end of file +realm-core/src/realm/parser/generated/ diff --git a/include/cpprealm/networking/http.hpp b/include/cpprealm/networking/http.hpp index a01acb7d..fc0e6107 100644 --- a/include/cpprealm/networking/http.hpp +++ b/include/cpprealm/networking/http.hpp @@ -105,8 +105,8 @@ namespace realm::networking { // Interface for providing http transport struct http_transport_client { virtual ~http_transport_client() = default; - virtual void send_request_to_server(const request& request, - std::function&& completion) = 0; + virtual void send_request_to_server(::realm::networking::request &&request, + std::function &&completion) = 0; }; /// Produces a http transport client from the factory. @@ -115,7 +115,7 @@ namespace realm::networking { [[maybe_unused]] void set_http_client_factory(std::function()>&&); /// Built in HTTP transport client. - struct default_http_transport : public http_transport_client { + struct default_http_transport : public http_transport_client, public std::enable_shared_from_this { struct configuration { /** * Extra HTTP headers to be set on each request to Atlas Device Sync when using the internal HTTP client. @@ -150,8 +150,8 @@ namespace realm::networking { ~default_http_transport() = default; - void send_request_to_server(const ::realm::networking::request& request, - std::function&& completion); + void send_request_to_server(::realm::networking::request &&request, + std::function &&completion); protected: configuration m_configuration; diff --git a/src/cpprealm/analytics.cpp b/src/cpprealm/analytics.cpp index 1536449c..641a4dda 100644 --- a/src/cpprealm/analytics.cpp +++ b/src/cpprealm/analytics.cpp @@ -273,7 +273,7 @@ namespace realm { std::stringstream json_ss; json_ss << post_data; auto json_str = json_ss.str(); - auto transport = std::make_unique(); + auto transport = std::make_shared(); std::vector buffer; buffer.resize(5000); diff --git a/src/cpprealm/internal/networking/network_transport.cpp b/src/cpprealm/internal/networking/network_transport.cpp index 543d05a2..10782e9b 100644 --- a/src/cpprealm/internal/networking/network_transport.cpp +++ b/src/cpprealm/internal/networking/network_transport.cpp @@ -29,6 +29,8 @@ #include "realm/util/base64.hpp" #include "realm/util/uri.hpp" +#include +#include #include namespace realm::networking { @@ -124,8 +126,8 @@ namespace realm::networking { } } - void default_http_transport::send_request_to_server(const ::realm::networking::request& request, - std::function&& completion_block) { + void default_http_transport::send_request_to_server(::realm::networking::request &&request, + std::function &&completion_block) { const auto uri = realm::util::Uri(request.url); std::string userinfo, host, port; uri.get_auth(userinfo, host, port); @@ -170,7 +172,7 @@ namespace realm::networking { auto address = realm::sync::network::make_address(host, e); ep = realm::sync::network::Endpoint(address, stoi(port)); } else { - auto resolved = resolver.resolve(sync::network::Resolver::Query(host, is_localhost ? "9090" : "443")); + auto resolved = resolver.resolve(sync::network::Resolver::Query(host, port)); ep = *resolved.begin(); } } @@ -243,45 +245,45 @@ namespace realm::networking { socket.ssl_stream->set_logger(logger.get()); } - realm::sync::HTTPHeaders headers; + realm::sync::HTTPClient m_http_client = realm::sync::HTTPClient(socket, logger); + auto convert_method = [](::realm::networking::http_method method) { + switch (method) { + case ::realm::networking::http_method::get: + return realm::sync::HTTPMethod::Get; + case ::realm::networking::http_method::put: + return realm::sync::HTTPMethod::Put; + case ::realm::networking::http_method::post: + return realm::sync::HTTPMethod::Post; + case ::realm::networking::http_method::patch: + return realm::sync::HTTPMethod::Patch; + case ::realm::networking::http_method::del: + return realm::sync::HTTPMethod::Delete; + default: + REALM_UNREACHABLE(); + } + }; + + realm::sync::HTTPRequest http_req{ + convert_method(request.method), + {}, + request.url, + request.body.empty() ? std::nullopt : std::make_optional(request.body)}; for (auto& [k, v] : request.headers) { - headers[k] = v; + http_req.headers[k] = v; } - headers["Host"] = host; - headers["User-Agent"] = "Realm C++ SDK"; + http_req.headers["Host"] = host; + http_req.headers["User-Agent"] = "Realm C++ SDK"; if (!request.body.empty()) { - headers["Content-Length"] = util::to_string(request.body.size()); + http_req.headers["Content-Length"] = util::to_string(request.body.size()); } if (m_configuration.custom_http_headers) { for (auto& header : *m_configuration.custom_http_headers) { - headers.emplace(header); + http_req.headers.emplace(header); } } - realm::sync::HTTPClient m_http_client = realm::sync::HTTPClient(socket, logger); - realm::sync::HTTPMethod method; - switch (request.method) { - case ::realm::networking::http_method::get: - method = realm::sync::HTTPMethod::Get; - break; - case ::realm::networking::http_method::put: - method = realm::sync::HTTPMethod::Put; - break; - case ::realm::networking::http_method::post: - method = realm::sync::HTTPMethod::Post; - break; - case ::realm::networking::http_method::patch: - method = realm::sync::HTTPMethod::Patch; - break; - case ::realm::networking::http_method::del: - method = realm::sync::HTTPMethod::Delete; - break; - default: - REALM_UNREACHABLE(); - } - /* * Flow of events: * 1. hostname is resolved from DNS @@ -295,26 +297,50 @@ namespace realm::networking { service.post([&](realm::Status&&){ auto handler = [&](std::error_code ec) { if (ec.value() == 0) { - realm::sync::HTTPRequest req; - req.method = method; - req.headers = headers; - req.path = request.url; - req.body = request.body.empty() ? std::nullopt : std::optional(request.body); - - m_http_client.async_request(std::move(req), [cb = std::move(completion_block)](const realm::sync::HTTPResponse& r, const std::error_code&) { - ::realm::networking::response res; - res.body = r.body ? *r.body : ""; - for (auto& [k, v] : r.headers) { - res.headers[k] = v; - } - res.http_status_code = static_cast(r.status); - res.custom_status_code = 0; - cb(res); - }); + // Pass along the original request so it can be resent to the redirected location URL if needed + m_http_client.async_request(std::move(http_req), + [self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block)](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { + constexpr std::string_view location_header = "location"; + auto transport = self.lock(); + // If an error occurred or the transport has gone away, then send "operation aborted" to callback + if (ec || !transport) { + cb({0, util::error::operation_aborted, {}, {}, std::nullopt}); + return; + } + // Was a redirect response (301 or 308) received? + if (resp.status == realm::sync::HTTPStatus::PermanentRedirect || resp.status == realm::sync::HTTPStatus::MovedPermanently) { + // A possible future enhancement could be to cache the redirect URLs to prevent having + // to perform redirections every time. + std::string redirect_url; + // Grab the new location from the 'Location' header and retry the request + for (auto &[key, value]: resp.headers) { + if (key.size() == location_header.size() && + std::equal(key.begin(), key.end(), location_header.begin(), location_header.end(), [](char a, char b) { + return std::tolower(static_cast(a)) == std::tolower(static_cast(b)); + })) { + // Get the redirect URL path returned from the server and exit the loop + redirect_url = value; + break; + } + } + // If the redirect URL path wasn't found in headers, then send the response to the client... + if (!redirect_url.empty()) { + // Otherwise, resend the original request to the new redirect URL path + // Perform the entire operation again, since the remote host is likely changing and + // a newe socket will need to be opened, + orig_request.url = redirect_url; + return transport->send_request_to_server(std::move(orig_request), std::move(cb)); + } + } + ::realm::networking::response res{static_cast(resp.status), 0, {}, resp.body ? std::move(*resp.body) : "", std::nullopt}; + // Copy over all the headers + for (auto &[k, v]: resp.headers) { + res.headers[k] = v; + } + cb(res); + }); } else { - ::realm::networking::response response; - response.custom_status_code = util::error::operation_aborted; - completion_block(std::move(response)); + completion_block({0, util::error::operation_aborted, {}, {}, std::nullopt}); return; } }; diff --git a/tests/admin_utils.cpp b/tests/admin_utils.cpp index 5762b59b..9471721e 100644 --- a/tests/admin_utils.cpp +++ b/tests/admin_utils.cpp @@ -23,6 +23,8 @@ #include #include +#include + #include "admin_utils.hpp" #include "external/json/json.hpp" @@ -30,14 +32,14 @@ namespace Admin { Admin::Session *Admin::Session::instance = nullptr; std::mutex Admin::Session::mutex; - static app::Response do_http_request(app::Request &&request) { - networking::default_http_transport transport; + static app::Response do_http_request(const app::Request &request) { + auto transport = std::make_shared(); std::promise p; std::future f = p.get_future(); - transport.send_request_to_server(::realm::internal::networking::to_request(std::move(request)), - [&p](auto&& response) { - p.set_value(::realm::internal::networking::to_core_response(std::move(response))); - }); + transport->send_request_to_server(std::move(::realm::internal::networking::to_request(request)), + [&p](const ::realm::networking::response &response) { + p.set_value(::realm::internal::networking::to_core_response(response)); + }); return f.get(); } @@ -52,9 +54,12 @@ namespace Admin { {"Accept", "application/json"}}; request.body = body.str(); - auto result = do_http_request(std::move(request)); - if (result.http_status_code != 200) { - REALM_TERMINATE(util::format("Unable to authenticate at %1 with provider '%2': %3", baas_url, provider_type, result.body).c_str()); + auto result = Admin::do_http_request(request); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE(util::format("Unable to authenticate at %1: operation aborted", request.url).c_str()); + } + if (result.http_status_code < 200 && result.http_status_code >= 300) { + REALM_TERMINATE(util::format("Unable to authenticate at %1 with provider '%2'(%3): %4", request.url, provider_type, result.http_status_code, result.body).c_str()); } auto parsed_response = static_cast(bson::parse(result.body)); return {static_cast(parsed_response["access_token"]), static_cast(parsed_response["refresh_token"])}; @@ -69,10 +74,13 @@ namespace Admin { {"Content-Type", "application/json"}, {"apiKey", *m_baasaas_api_key}}; - auto result = Admin::do_http_request(std::move(request)); + auto result = Admin::do_http_request(request); - if (result.http_status_code != 200) { - REALM_TERMINATE("Unable to start container"); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE("Unable to start container: Operation aborted"); + } + if (result.http_status_code < 200 && result.http_status_code >= 300) { + REALM_TERMINATE(util::format("Unable to start container [%1]: %2", result.http_status_code, result.body).c_str()); } m_container_id = nlohmann::json::parse(result.body)["id"]; @@ -86,9 +94,12 @@ namespace Admin { {"Content-Type", "application/json"}, {"apiKey", *m_baasaas_api_key}}; - auto result = Admin::do_http_request(std::move(request)); - if (result.http_status_code != 200) { - REALM_TERMINATE("Unable to stop container"); + auto result = Admin::do_http_request(request); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE("Unable to stop container: Operation aborted"); + } + if (result.http_status_code < 200 && result.http_status_code >= 300) { + REALM_TERMINATE(util::format("Unable to stop container [%1]: %2", result.http_status_code, result.body).c_str()); } } @@ -113,7 +124,10 @@ namespace Admin { {"Content-Type", "application/json;charset=utf-8"}, {"apiKey", *m_baasaas_api_key}}; - auto result = Admin::do_http_request(std::move(request)); + auto result = Admin::do_http_request(request); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE("Error while waiting for container: Operation aborted"); + } auto json = nlohmann::json::parse(result.body); if (json.contains("httpUrl")) { url = json["httpUrl"]; @@ -130,7 +144,10 @@ namespace Admin { request.method = realm::app::HttpMethod::get; request.url = realm::util::format("%1/api/private/v1.0/version", *url); - auto result = Admin::do_http_request(std::move(request)); + auto result = Admin::do_http_request(request); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE("Error while waiting for container: Operation aborted"); + } if (result.http_status_code >= 200 && result.http_status_code < 300) { break; } @@ -161,8 +178,11 @@ namespace Admin { {"Content-Type", "application/json;charset=utf-8"}, {"Accept", "application/json"}}; request.body = std::move(body); - auto response = Admin::do_http_request(std::move(request)); + auto response = Admin::do_http_request(request); + if (response.custom_status_code == util::error::operation_aborted) { + throw std::runtime_error(util::format("An error occurred while calling %1: Operation Aborted", url)); + } if (response.http_status_code >= 400) { throw std::runtime_error(util::format("An error occurred while calling %1: %2", url, response.body)); } @@ -471,7 +491,13 @@ namespace Admin { request.headers = { {"Authorization", "Bearer " + tokens.first}}; - auto result = do_http_request(std::move(request)); + auto result = Admin::do_http_request(request); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE(util::format("An error occurred while requesting user profile: Operation Aborted").c_str()); + } + if (result.http_status_code < 200 && result.http_status_code >= 300) { + REALM_TERMINATE(util::format("Unable to request user profile [%1]: %2", result.http_status_code, result.body).c_str()); + } auto parsed_response = static_cast(bson::parse(result.body)); auto roles = static_cast(parsed_response["roles"]); auto group_id = static_cast(static_cast(roles[0])["group_id"]); @@ -529,9 +555,12 @@ namespace Admin { {"Authorization", util::format("Bearer %1", m_refresh_token)}}; request.body = body.str(); - auto result = do_http_request(std::move(request)); - if (result.http_status_code >= 400) { - REALM_TERMINATE("Unable to refresh access token"); + auto result = Admin::do_http_request(request); + if (result.custom_status_code == util::error::operation_aborted) { + REALM_TERMINATE(util::format("Unable to refresh access token: Operation Aborted").c_str()); + } + if (result.http_status_code < 200 && result.http_status_code >= 300) { + REALM_TERMINATE(util::format("Unable to refresh access token [%1]: %2", result.http_status_code, result.body).c_str()); } auto parsed_response = static_cast(bson::parse(result.body)); m_access_token = static_cast(parsed_response["access_token"]); diff --git a/tests/sync/flexible_sync_tests.cpp b/tests/sync/flexible_sync_tests.cpp index f1142527..89e69058 100644 --- a/tests/sync/flexible_sync_tests.cpp +++ b/tests/sync/flexible_sync_tests.cpp @@ -4,11 +4,12 @@ using namespace realm; -TEST_CASE("flexible_sync", "[sync]") { +TEST_CASE("flexible_sync", "[sync][flx]") { auto config = realm::App::configuration(); config.app_id = Admin::Session::shared().cached_app_id(); config.base_url = Admin::Session::shared().base_url(); auto app = realm::App(config); + app.get_sync_manager().set_log_level(logger::level::trace); SECTION("all") { auto user = app.login(realm::App::credentials::anonymous()).get(); auto flx_sync_config = user.flexible_sync_configuration(); @@ -181,7 +182,6 @@ TEST_CASE("flexible_sync", "[sync]") { CHECK(synced_realm.objects().size() == 2); } } - } template @@ -245,8 +245,7 @@ TEST_CASE("set collection sync", "[set]") { auto time = std::chrono::system_clock::now(); auto time2 = time + time.time_since_epoch(); test_set(&managed_obj.set_date_col, scenario, {time, time, time2, std::chrono::time_point()}); // here - test_set(&managed_obj.set_mixed_col, scenario, {realm::mixed((int64_t)42), realm::mixed((int64_t)42), realm::mixed("24"), realm::mixed(realm::uuid("18de7916-7f84-11ec-a8a3-0242ac120002"))}); - + test_set(&managed_obj.set_mixed_col, scenario, {realm::mixed((int64_t) 42), realm::mixed((int64_t) 42), realm::mixed("24"), realm::mixed(realm::uuid("18de7916-7f84-11ec-a8a3-0242ac120002"))}); realm.get_sync_session()->wait_for_upload_completion().get(); realm.get_sync_session()->wait_for_download_completion().get(); @@ -270,7 +269,7 @@ TEST_CASE("set collection sync", "[set]") { } } -TEST_CASE("pause_resume_sync", "[sync]") { +TEST_CASE("pause_resume_sync", "[sync][flx]") { auto config = realm::App::configuration(); config.app_id = Admin::Session::shared().cached_app_id(); config.base_url = Admin::Session::shared().base_url(); @@ -323,4 +322,50 @@ TEST_CASE("pause_resume_sync", "[sync]") { }); CHECK(synced_realm.get_sync_session()->state() == realm::sync_session::state::active); } -} \ No newline at end of file +} + +TEST_CASE("delete created sync objects", "[sync][flx]") { + auto config = realm::App::configuration(); + config.app_id = Admin::Session::shared().cached_app_id(); + config.base_url = Admin::Session::shared().base_url(); + auto app = realm::App(config); + + auto user = app.login(realm::App::credentials::anonymous()).get(); + auto flx_sync_config = user.flexible_sync_configuration(); + auto synced_realm = db(flx_sync_config); + auto update_success = synced_realm.subscriptions().update([](realm::mutable_sync_subscription_set &subs) { + subs.clear(); + }) + .get(); + CHECK(update_success == true); + update_success = synced_realm.subscriptions().update([](realm::mutable_sync_subscription_set &subs) { + subs.add("foo-strings"); + subs.add("foo-link"); + }) + .get(); + CHECK(update_success == true); + + CHECK(synced_realm.subscriptions().size() == 2); + synced_realm.get_sync_session()->wait_for_upload_completion().get(); + synced_realm.get_sync_session()->wait_for_download_completion().get(); + synced_realm.refresh(); + auto links = synced_realm.objects(); + // No links were created during the tests + CHECK(links.size() == 0); + auto objs = synced_realm.objects(); + CHECK(objs.size() > 0); + synced_realm.write([&synced_realm, &objs]() { + while (objs.size() > 0) { + auto obj = objs[0]; + synced_realm.remove(obj); + } + }); + synced_realm.get_sync_session()->wait_for_upload_completion().get(); + synced_realm.get_sync_session()->wait_for_download_completion().get(); + synced_realm.refresh(); + + links = synced_realm.objects(); + CHECK(objs.size() == 0); + objs = synced_realm.objects(); + CHECK(objs.size() == 0); +} diff --git a/tests/sync/networking_tests.cpp b/tests/sync/networking_tests.cpp index 82d2209b..0f62cd5f 100644 --- a/tests/sync/networking_tests.cpp +++ b/tests/sync/networking_tests.cpp @@ -60,16 +60,17 @@ TEST_CASE("custom transport to proxy", "[proxy]") { m_configuration = configuration; } - void send_request_to_server(const ::realm::networking::request& request, - std::function&& completion) override { - auto req_copy = request; - const std::string from = "https:"; - const std::string to = "http:"; - if (req_copy.url.find(from) == 0) { - req_copy.url.replace(0, from.length(), to); + void send_request_to_server(::realm::networking::request &&request, + std::function &&completion) override { + // We're already working with a copy of the original request that was created + // by `to_request()` by `core_http_transport_shim` + constexpr std::string_view from = "https:"; + constexpr std::string_view to = "http:"; + if (request.url.find(from) == 0) { + request.url.replace(0, from.length(), to); } m_called = true; - return ::realm::networking::default_http_transport::send_request_to_server(req_copy, std::move(completion)); + return ::realm::networking::default_http_transport::send_request_to_server(std::move(request), std::move(completion)); } bool was_called() const { From 8918f13cbdec2b682c551fd1b4441a7226a4210c Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Wed, 28 Aug 2024 12:11:35 -0400 Subject: [PATCH 2/5] Updated changelog --- CHANGELOG.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a45cdd11..fb80b395 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,15 @@ NEXT_RELEASE Release notes (YYYY-MM-DD) ### Enhancements * Add `realm::db_config::enable_forced_sync_history()` which allows you to open a synced Realm even if a sync configuration is not supplied. +* Added 301/308 http redirect response support to the default HTTP transport, since it has been + removed from Core and is the responsibility of the SDKs to handle the redirect operation. A + redirect should rarely be received from teh server, and typically happens after changing the + deployment server for the cloud app or the base URL is configured to connect to the wrong host. + If a custom HTTP transport is provided by the developer, it should also support handling + redirect responses with a status code of 301 or 308 and not change the request method from POST + to GET when resending the request. +Add `managed>::contains_key` for conveniently checking if a managed map + contains a given key. Use this method in the Type Safe Query API instead of `managed>::find`. ### Compatibility * Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. From 08490c0994fd16d4d7332f4235e40e5759acf465 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Wed, 28 Aug 2024 13:02:55 -0400 Subject: [PATCH 3/5] Added max redirect support; reverted debug output that was left on --- include/cpprealm/networking/http.hpp | 16 +++++- .../internal/networking/network_transport.cpp | 54 +++++++++++-------- tests/sync/flexible_sync_tests.cpp | 1 - 3 files changed, 47 insertions(+), 24 deletions(-) diff --git a/include/cpprealm/networking/http.hpp b/include/cpprealm/networking/http.hpp index fc0e6107..790bf6d5 100644 --- a/include/cpprealm/networking/http.hpp +++ b/include/cpprealm/networking/http.hpp @@ -143,6 +143,13 @@ namespace realm::networking { * is not set. */ std::function ssl_verify_callback; + + /** + * Maximum number of subsequent redirect responses from the server to prevent getting stuck + * in a redirect loop indefinitely. Set to 0 to disable redirect support or -1 to allow + * redirecting indefinitely. + */ + int max_redirect_count = 30; }; default_http_transport() = default; @@ -151,9 +158,16 @@ namespace realm::networking { ~default_http_transport() = default; void send_request_to_server(::realm::networking::request &&request, - std::function &&completion); + std::function &&completion) + { + send_request_to_server(std::move(request), std::move(completion), 0); + } protected: + void send_request_to_server(::realm::networking::request &&request, + std::function &&completion, + int redirect_count); + configuration m_configuration; }; } diff --git a/src/cpprealm/internal/networking/network_transport.cpp b/src/cpprealm/internal/networking/network_transport.cpp index 10782e9b..414afa54 100644 --- a/src/cpprealm/internal/networking/network_transport.cpp +++ b/src/cpprealm/internal/networking/network_transport.cpp @@ -127,7 +127,8 @@ namespace realm::networking { } void default_http_transport::send_request_to_server(::realm::networking::request &&request, - std::function &&completion_block) { + std::function &&completion_block, + int redirect_count) { const auto uri = realm::util::Uri(request.url); std::string userinfo, host, port; uri.get_auth(userinfo, host, port); @@ -299,7 +300,7 @@ namespace realm::networking { if (ec.value() == 0) { // Pass along the original request so it can be resent to the redirected location URL if needed m_http_client.async_request(std::move(http_req), - [self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block)](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { + [self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block), redirect_count](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { constexpr std::string_view location_header = "location"; auto transport = self.lock(); // If an error occurred or the transport has gone away, then send "operation aborted" to callback @@ -309,28 +310,37 @@ namespace realm::networking { } // Was a redirect response (301 or 308) received? if (resp.status == realm::sync::HTTPStatus::PermanentRedirect || resp.status == realm::sync::HTTPStatus::MovedPermanently) { - // A possible future enhancement could be to cache the redirect URLs to prevent having - // to perform redirections every time. - std::string redirect_url; - // Grab the new location from the 'Location' header and retry the request - for (auto &[key, value]: resp.headers) { - if (key.size() == location_header.size() && - std::equal(key.begin(), key.end(), location_header.begin(), location_header.end(), [](char a, char b) { - return std::tolower(static_cast(a)) == std::tolower(static_cast(b)); - })) { - // Get the redirect URL path returned from the server and exit the loop - redirect_url = value; - break; + auto max_redirects = transport->m_configuration.max_redirect_count; + // Are redirects still allowed to continue? + if (max_redirects < 0 || ++redirect_count < max_redirects) { + // A possible future enhancement could be to cache the redirect URLs to prevent having + // to perform redirections every time. + std::string redirect_url; + // Grab the new location from the 'Location' header and retry the request + for (auto &[key, value]: resp.headers) { + if (key.size() == location_header.size() && + std::equal(key.begin(), key.end(), location_header.begin(), location_header.end(), [](char a, char b) { + return std::tolower(static_cast(a)) == std::tolower(static_cast(b)); + })) { + // If the redirect URL path returned from the server was not empty, save it and exit the loop + if (!value.empty()) { + redirect_url = value; + break; + } + // Otherwise, keep looking, in case there's another 'location' entry + } + } + // If the redirect URL path wasn't found in headers, then send the response to the client... + if (!redirect_url.empty()) { + // Otherwise, resend the original request to the new redirect URL path + // Perform the entire operation again, since the remote host is likely changing and + // a newe socket will need to be opened, + orig_request.url = redirect_url; + return transport->send_request_to_server(std::move(orig_request), std::move(cb), redirect_count); } } - // If the redirect URL path wasn't found in headers, then send the response to the client... - if (!redirect_url.empty()) { - // Otherwise, resend the original request to the new redirect URL path - // Perform the entire operation again, since the remote host is likely changing and - // a newe socket will need to be opened, - orig_request.url = redirect_url; - return transport->send_request_to_server(std::move(orig_request), std::move(cb)); - } + // If redirects disabled, max redirect reached or location was missing from response, then pass the + // redirect response to the callback function } ::realm::networking::response res{static_cast(resp.status), 0, {}, resp.body ? std::move(*resp.body) : "", std::nullopt}; // Copy over all the headers diff --git a/tests/sync/flexible_sync_tests.cpp b/tests/sync/flexible_sync_tests.cpp index 89e69058..7e779687 100644 --- a/tests/sync/flexible_sync_tests.cpp +++ b/tests/sync/flexible_sync_tests.cpp @@ -9,7 +9,6 @@ TEST_CASE("flexible_sync", "[sync][flx]") { config.app_id = Admin::Session::shared().cached_app_id(); config.base_url = Admin::Session::shared().base_url(); auto app = realm::App(config); - app.get_sync_manager().set_log_level(logger::level::trace); SECTION("all") { auto user = app.login(realm::App::credentials::anonymous()).get(); auto flx_sync_config = user.flexible_sync_configuration(); From 011cc91bd9c7d16862618b2203ee769b09322d17 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Wed, 28 Aug 2024 13:03:56 -0400 Subject: [PATCH 4/5] Clang format --- include/cpprealm/networking/http.hpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/include/cpprealm/networking/http.hpp b/include/cpprealm/networking/http.hpp index 790bf6d5..4b6a26ec 100644 --- a/include/cpprealm/networking/http.hpp +++ b/include/cpprealm/networking/http.hpp @@ -158,8 +158,7 @@ namespace realm::networking { ~default_http_transport() = default; void send_request_to_server(::realm::networking::request &&request, - std::function &&completion) - { + std::function &&completion) { send_request_to_server(std::move(request), std::move(completion), 0); } From c85aaf49fb587b16a16be222b6ab09c68d0cfcb0 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Thu, 29 Aug 2024 17:25:05 -0400 Subject: [PATCH 5/5] Updates from review --- CHANGELOG.md | 10 ++-- include/cpprealm/networking/http.hpp | 2 +- .../internal/networking/network_transport.cpp | 49 +++++++------------ 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb80b395..e18f583f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,12 +9,12 @@ NEXT_RELEASE Release notes (YYYY-MM-DD) even if a sync configuration is not supplied. * Added 301/308 http redirect response support to the default HTTP transport, since it has been removed from Core and is the responsibility of the SDKs to handle the redirect operation. A - redirect should rarely be received from teh server, and typically happens after changing the - deployment server for the cloud app or the base URL is configured to connect to the wrong host. + redirect should rarely be received from the server, and typically happens after changing the + deployment model for the cloud app or the base URL is configured to connect to the wrong host. If a custom HTTP transport is provided by the developer, it should also support handling - redirect responses with a status code of 301 or 308 and not change the request method from POST - to GET when resending the request. -Add `managed>::contains_key` for conveniently checking if a managed map + redirect responses with a status code of 301 or 308, remove the Authorization header and + retain the original HTTP method when re-sending requests after a redirect. +* Add `managed>::contains_key` for conveniently checking if a managed map contains a given key. Use this method in the Type Safe Query API instead of `managed>::find`. ### Compatibility diff --git a/include/cpprealm/networking/http.hpp b/include/cpprealm/networking/http.hpp index 4b6a26ec..678817ad 100644 --- a/include/cpprealm/networking/http.hpp +++ b/include/cpprealm/networking/http.hpp @@ -115,7 +115,7 @@ namespace realm::networking { [[maybe_unused]] void set_http_client_factory(std::function()>&&); /// Built in HTTP transport client. - struct default_http_transport : public http_transport_client, public std::enable_shared_from_this { + struct default_http_transport : public http_transport_client { struct configuration { /** * Extra HTTP headers to be set on each request to Atlas Device Sync when using the internal HTTP client. diff --git a/src/cpprealm/internal/networking/network_transport.cpp b/src/cpprealm/internal/networking/network_transport.cpp index 414afa54..1abd2376 100644 --- a/src/cpprealm/internal/networking/network_transport.cpp +++ b/src/cpprealm/internal/networking/network_transport.cpp @@ -246,7 +246,7 @@ namespace realm::networking { socket.ssl_stream->set_logger(logger.get()); } - realm::sync::HTTPClient m_http_client = realm::sync::HTTPClient(socket, logger); + realm::sync::HTTPClient m_http_client(socket, logger); auto convert_method = [](::realm::networking::http_method method) { switch (method) { case ::realm::networking::http_method::get: @@ -300,53 +300,40 @@ namespace realm::networking { if (ec.value() == 0) { // Pass along the original request so it can be resent to the redirected location URL if needed m_http_client.async_request(std::move(http_req), - [self = weak_from_this(), orig_request = std::move(request), cb = std::move(completion_block), redirect_count](const realm::sync::HTTPResponse &resp, const std::error_code &ec) mutable { + [this, orig_request = std::move(request), cb = std::move(completion_block), redirect_count](realm::sync::HTTPResponse resp, std::error_code ec) mutable { constexpr std::string_view location_header = "location"; - auto transport = self.lock(); + constexpr std::string_view authorization_header = "authorization"; // If an error occurred or the transport has gone away, then send "operation aborted" to callback - if (ec || !transport) { + if (ec) { cb({0, util::error::operation_aborted, {}, {}, std::nullopt}); return; } // Was a redirect response (301 or 308) received? if (resp.status == realm::sync::HTTPStatus::PermanentRedirect || resp.status == realm::sync::HTTPStatus::MovedPermanently) { - auto max_redirects = transport->m_configuration.max_redirect_count; + auto max_redirects = m_configuration.max_redirect_count; // Are redirects still allowed to continue? if (max_redirects < 0 || ++redirect_count < max_redirects) { // A possible future enhancement could be to cache the redirect URLs to prevent having // to perform redirections every time. - std::string redirect_url; // Grab the new location from the 'Location' header and retry the request - for (auto &[key, value]: resp.headers) { - if (key.size() == location_header.size() && - std::equal(key.begin(), key.end(), location_header.begin(), location_header.end(), [](char a, char b) { - return std::tolower(static_cast(a)) == std::tolower(static_cast(b)); - })) { - // If the redirect URL path returned from the server was not empty, save it and exit the loop - if (!value.empty()) { - redirect_url = value; - break; + if (auto location = resp.headers.find(location_header); location != resp.headers.end()) { + if (!location->second.empty()) { + // Perform the entire operation again, since the remote host is likely changing and + // a new socket will need to be opened, + orig_request.url = location->second; + // Also remove the authorization header before forwarding the request to the new + // location, to prevent leaking the access token to an unauthorized server + if (auto authorization = resp.headers.find(authorization_header); authorization != resp.headers.end()) { + resp.headers.erase(authorization); } - // Otherwise, keep looking, in case there's another 'location' entry + return send_request_to_server(std::move(orig_request), std::move(cb), redirect_count); } } - // If the redirect URL path wasn't found in headers, then send the response to the client... - if (!redirect_url.empty()) { - // Otherwise, resend the original request to the new redirect URL path - // Perform the entire operation again, since the remote host is likely changing and - // a newe socket will need to be opened, - orig_request.url = redirect_url; - return transport->send_request_to_server(std::move(orig_request), std::move(cb), redirect_count); - } + // If redirects disabled, max redirect reached or location was missing from response, then pass the + // redirect response to the callback function } - // If redirects disabled, max redirect reached or location was missing from response, then pass the - // redirect response to the callback function - } - ::realm::networking::response res{static_cast(resp.status), 0, {}, resp.body ? std::move(*resp.body) : "", std::nullopt}; - // Copy over all the headers - for (auto &[k, v]: resp.headers) { - res.headers[k] = v; } + ::realm::networking::response res{static_cast(resp.status), 0, {resp.headers.begin(), resp.headers.end()}, resp.body ? std::move(*resp.body) : "", std::nullopt}; cb(res); }); } else {