From c85aaf49fb587b16a16be222b6ab09c68d0cfcb0 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Thu, 29 Aug 2024 17:25:05 -0400 Subject: [PATCH] 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 {