From ec453528de087fb5a34ffc33ece8b1becb69a0d6 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Mon, 15 May 2023 12:51:30 -0400 Subject: [PATCH 01/10] Always refresh metadata on app login --- src/realm/object-store/sync/app.cpp | 59 ++++++++++++++++------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 76688329840..fc834d4157c 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -659,35 +659,40 @@ void App::log_in_with_credentials( BsonDocument body = credentials.serialize_as_bson(); attach_auth_options(body); - do_request({HttpMethod::post, route, m_request_timeout_ms, - get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, - [completion = std::move(completion), credentials, linking_user, - self = shared_from_this()](const Response& response) mutable { - if (auto error = AppUtils::check_for_errors(response)) { - self->log_error("App: log_in_with_credentials failed: %1 message: %2", - response.http_status_code, error->what()); - return completion(nullptr, std::move(error)); - } + // Always update the location on a login to keep the location metadata up to date in cases where + // the transport handles the redirection automatically and does not return the redirect response + // Since the user is logged out as part of a deployment model change, the user will need to log in + // again, but Core will not know that this was driven by an HTTP redirection. + update_metadata_and_resend( + {HttpMethod::post, route, m_request_timeout_ms, + get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, + [completion = std::move(completion), credentials, linking_user, + self = shared_from_this()](const Response& response) mutable { + if (auto error = AppUtils::check_for_errors(response)) { + self->log_error("App: log_in_with_credentials failed: %1 message: %2", response.http_status_code, + error->what()); + return completion(nullptr, std::move(error)); + } - std::shared_ptr sync_user = linking_user; - try { - auto json = parse(response.body); - if (linking_user) { - linking_user->update_access_token(get(json, "access_token")); - } - else { - sync_user = self->m_sync_manager->get_user( - get(json, "user_id"), get(json, "refresh_token"), - get(json, "access_token"), credentials.provider_as_string(), - get(json, "device_id")); - } - } - catch (const AppError& e) { - return completion(nullptr, e); - } + std::shared_ptr sync_user = linking_user; + try { + auto json = parse(response.body); + if (linking_user) { + linking_user->update_access_token(get(json, "access_token")); + } + else { + sync_user = self->m_sync_manager->get_user( + get(json, "user_id"), get(json, "refresh_token"), + get(json, "access_token"), credentials.provider_as_string(), + get(json, "device_id")); + } + } + catch (const AppError& e) { + return completion(nullptr, e); + } - self->get_profile(sync_user, std::move(completion)); - }); + self->get_profile(sync_user, std::move(completion)); + }); } void App::log_in_with_credentials( From 5d5ac4c1289caa3192f7d312fa36d0749255ce58 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Mon, 15 May 2023 13:08:49 -0400 Subject: [PATCH 02/10] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6fe42a72ac..ebb115f4975 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* Always update location metadata on user login to keep info up to date ([#6630](https://github.com/realm/realm-core/issues/6630), since v12.9.0) ### Breaking changes * `platform` and `cpu_arch` fields in the `device_info` structure in App::Config can no longer be provided by the SDK's, they are inferred by the library ([PR #6612](https://github.com/realm/realm-core/pull/6612)) From b07e3026c0bb92b2ba9be58e09e075814e5ac9d4 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Mon, 15 May 2023 20:20:57 -0400 Subject: [PATCH 03/10] Always update location when requested; fix c_api test --- src/realm/object-store/sync/app.cpp | 11 +++++----- src/realm/object-store/sync/sync_session.cpp | 16 ++++++++------- test/object-store/c_api/c_api.cpp | 21 ++++++++++++++------ 3 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index fc834d4157c..a55134f9d20 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -870,11 +870,7 @@ void App::init_app_metadata(UniqueFunction&)>&& co { std::string route; - if (!new_hostname && m_location_updated) { - // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided - return completion(util::none); // early return - } - else { + { std::lock_guard lock(*m_route_mutex); route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route()); } @@ -884,6 +880,8 @@ void App::init_app_metadata(UniqueFunction&)>&& co req.url = route; req.timeout_ms = m_request_timeout_ms; + log_debug("App: request location: %1", route); + m_config.transport->send_request_to_server(req, [self = shared_from_this(), completion = std::move(completion)](const Response& response) { // If the response contains an error, then pass it up @@ -1101,7 +1099,8 @@ void App::refresh_access_token(const std::shared_ptr& sync_user, bool route = util::format("%1/auth/session", m_base_route); } - log_debug("App: refresh_access_token: email: %1", sync_user->user_profile().email()); + log_debug("App: refresh_access_token: email: %1 %2", sync_user->user_profile().email(), + update_location ? "(updating location)" : ""); Request request{HttpMethod::post, std::move(route), m_request_timeout_ms, get_request_headers(sync_user, RequestTokenType::RefreshToken)}; diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp index f5610715929..1a339154065 100644 --- a/src/realm/object-store/sync/sync_session.cpp +++ b/src/realm/object-store/sync/sync_session.cpp @@ -763,13 +763,16 @@ void SyncSession::handle_error(sync::SessionErrorInfo error) } } else if (error_code.category() == sync::websocket::websocket_error_category()) { + using WebSocketError = sync::websocket::WebSocketError; + auto websocket_error = static_cast(error_code.value()); + // The server replies with '401: unauthorized' if the access token is invalid, expired, revoked, or the user // is disabled. In this scenario we attempt an automatic token refresh and if that succeeds continue as // normal. If the refresh request also fails with 401 then we need to stop retrying and pass along the error; // see handle_refresh(). - bool redirect_occurred = error_code == sync::websocket::WebSocketError::websocket_moved_permanently; - if (redirect_occurred || error_code == sync::websocket::WebSocketError::websocket_unauthorized || - error_code == sync::websocket::WebSocketError::websocket_abnormal_closure) { + bool redirect_occurred = websocket_error == WebSocketError::websocket_moved_permanently; + if (redirect_occurred || websocket_error == WebSocketError::websocket_unauthorized || + websocket_error == WebSocketError::websocket_abnormal_closure) { if (auto u = user()) { // If a redirection occurred, the location metadata will be updated before refreshing the access // token. @@ -780,14 +783,13 @@ void SyncSession::handle_error(sync::SessionErrorInfo error) // If the websocket was closed cleanly or if the socket disappeared, don't notify the user as an error // since the sync client will retry. - if (error_code == sync::websocket::WebSocketError::websocket_read_error || - error_code == sync::websocket::WebSocketError::websocket_write_error) { + if (websocket_error == WebSocketError::websocket_read_error || + websocket_error == WebSocketError::websocket_write_error) { return; } // Surface a simplified websocket error to the user. - auto simplified_error = sync::websocket::get_simplified_websocket_error( - static_cast(error_code.value())); + auto simplified_error = sync::websocket::get_simplified_websocket_error(websocket_error); std::error_code new_error_code(simplified_error, sync::websocket::websocket_error_category()); error = sync::SessionErrorInfo(new_error_code, error.message, error.try_again); } diff --git a/test/object-store/c_api/c_api.cpp b/test/object-store/c_api/c_api.cpp index e10e1c9cfb8..b6fa466ef6e 100644 --- a/test/object-store/c_api/c_api.cpp +++ b/test/object-store/c_api/c_api.cpp @@ -288,8 +288,9 @@ class CApiUnitTestTransport : public app::GenericNetworkTransport { std::string m_provider_type; public: - CApiUnitTestTransport(const std::string& provider_type = "anon-user") - : m_provider_type(provider_type) + CApiUnitTestTransport(const std::string& provider_type = {}, uint64_t request_timeout = 60000) + : m_provider_type(provider_type.empty() ? "anon-user" : provider_type) + , request_timeout(request_timeout) { profile_0 = nlohmann::json({{"name", "profile_0_name"}, {"first_name", "profile_0_first_name"}, @@ -302,6 +303,11 @@ class CApiUnitTestTransport : public app::GenericNetworkTransport { {"max_age", "profile_0_max_age"}}); } + explicit CApiUnitTestTransport(const uint64_t request_timeout) + : CApiUnitTestTransport({}, request_timeout) + { + } + void set_provider_type(const std::string& provider_type) { m_provider_type = provider_type; @@ -316,6 +322,7 @@ class CApiUnitTestTransport : public app::GenericNetworkTransport { const std::string identity_0_id = "eflkjf393flkj33fjf3"; const std::string identity_1_id = "aewfjklewfwoifejjef"; nlohmann::json profile_0; + uint64_t request_timeout; private: @@ -355,7 +362,7 @@ class CApiUnitTestTransport : public app::GenericNetworkTransport { {"coreVersion", REALM_VERSION_STRING}, {"bundleId", "some_bundle_id"}}}})); - CHECK(request.timeout_ms == 60000); + CHECK(request.timeout_ms == request_timeout); std::string response = nlohmann::json({{"access_token", access_token}, {"refresh_token", access_token}, @@ -614,7 +621,9 @@ TEST_CASE("C API (non-database)", "[c_api]") { #if REALM_ENABLE_AUTH_TESTS SECTION("realm_app_config_t") { - std::shared_ptr transport = std::make_shared(); + const uint64_t request_timeout = 2500; + std::shared_ptr transport = + std::make_shared(request_timeout); auto http_transport = realm_http_transport(transport); auto app_config = cptr(realm_app_config_new("app_id_123", &http_transport)); CHECK(app_config.get() != nullptr); @@ -630,8 +639,8 @@ TEST_CASE("C API (non-database)", "[c_api]") { realm_app_config_set_local_app_version(app_config.get(), "some_app_version"); CHECK(app_config->local_app_version == "some_app_version"); - realm_app_config_set_default_request_timeout(app_config.get(), 2500); - CHECK(app_config->default_request_timeout_ms == 2500); + realm_app_config_set_default_request_timeout(app_config.get(), request_timeout); + CHECK(app_config->default_request_timeout_ms == request_timeout); realm_app_config_set_platform_version(app_config.get(), "some_platform_version"); CHECK(app_config->device_info.platform_version == "some_platform_version"); From 067c9bd923cea65d084a0fac0dd4f10482ff2323 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Tue, 16 May 2023 11:29:43 -0400 Subject: [PATCH 04/10] Update test to properly evaluate websocket redirections; added one more test --- test/object-store/CMakeLists.txt | 3 ++ test/object-store/sync/app.cpp | 84 ++++++++++++++++++++++++-------- 2 files changed, 66 insertions(+), 21 deletions(-) diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt index 51725920af3..d4977596e49 100644 --- a/test/object-store/CMakeLists.txt +++ b/test/object-store/CMakeLists.txt @@ -119,9 +119,12 @@ if(REALM_TEST_LOGGING) ) if(REALM_TEST_LOGGING_LEVEL) + message(STATUS "Test logging level: ${REALM_TEST_LOGGING_LEVEL}") target_compile_definitions(ObjectStoreTests PRIVATE TEST_LOGGING_LEVEL=${REALM_TEST_LOGGING_LEVEL} ) + else() + message(STATUS "Test logging enabled") endif() endif() diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 154d69671b7..9c63df4f89a 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -2213,7 +2213,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { logger->trace("Received request[%1]: %2", request_count, request.url); if (request_count == 0) { // First request should be to location - REQUIRE(request.url.find_first_of("/location") != std::string::npos); + REQUIRE(request.url.find("/location") != std::string::npos); if (request.url.find("https://") != std::string::npos) { redirect_scheme = "https://"; } @@ -2361,10 +2361,10 @@ TEST_CASE("app: sync integration", "[sync][app]") { std::string websocket_url = "ws://some-websocket:9090"; std::string original_url; redir_transport->request_hook = [&](const Request& request) { - logger->trace("Received request[%1]: %2", request_count, request.url); + logger->trace("request.url (%1): %2", request_count, request.url); if (request_count == 0) { // First request should be to location - REQUIRE(request.url.find_first_of("/location") != std::string::npos); + REQUIRE(request.url.find("/location") != std::string::npos); if (request.url.find("https://") != std::string::npos) { original_scheme = "https://"; } @@ -2380,7 +2380,6 @@ TEST_CASE("app: sync integration", "[sync][app]") { logger->trace("original_url (%1): %2", request_count, original_url); } else if (request_count == 1) { - logger->trace("request.url (%1): %2", request_count, request.url); REQUIRE(!request.redirect_count); redir_transport->simulated_response = { 308, @@ -2389,7 +2388,6 @@ TEST_CASE("app: sync integration", "[sync][app]") { "Some body data"}; } else if (request_count == 2) { - logger->trace("request.url (%1): %2", request_count, request.url); REQUIRE(request.url.find("http://somehost:9090") != std::string::npos); REQUIRE(request.url.find("location") != std::string::npos); // app hostname will be updated via the metadata info @@ -2402,7 +2400,6 @@ TEST_CASE("app: sync integration", "[sync][app]") { original_url, websocket_url)}; } else { - logger->trace("request.url (%1): %2", request_count, request.url); REQUIRE(request.url.find(original_url) != std::string::npos); redir_transport->simulated_response.reset(); } @@ -2507,10 +2504,11 @@ TEST_CASE("app: sync integration", "[sync][app]") { }; int request_count = 0; redir_transport->request_hook = [&](const Request& request) { + logger->trace("request.url (%1): %2", request_count, request.url); if (request_count++ == 0) { - logger->trace("request.url (%1): %2", request_count, request.url); - // First request should be to location - REQUIRE(request.url.find_first_of("/location") != std::string::npos); + // First request should be a location request against the original URL + REQUIRE(request.url.find(original_host) != std::string::npos); + REQUIRE(request.url.find("/location") != std::string::npos); REQUIRE(request.redirect_count == 0); redir_transport->simulated_response = { static_cast(sync::HTTPStatus::PermanentRedirect), @@ -2518,8 +2516,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { {{"Location", redirect_url}, {"Content-Type", "application/json"}}, "Some body data"}; } - else if (request.url.find("location") != std::string::npos) { - logger->trace("request.url (%1): %2", request_count, request.url); + else if (request.url.find("/location") != std::string::npos) { redir_transport->simulated_response = { static_cast(sync::HTTPStatus::Ok), 0, @@ -2530,7 +2527,6 @@ TEST_CASE("app: sync integration", "[sync][app]") { redirect_host, redirect_scheme, websocket_scheme)}; } else { - logger->trace("request.url (%1): %2", request_count, request.url); redir_transport->simulated_response.reset(); } }; @@ -2561,10 +2557,10 @@ TEST_CASE("app: sync integration", "[sync][app]") { }; int request_count = 0; redir_transport->request_hook = [&](const Request& request) { + logger->trace("request.url (%1): %2", request_count, request.url); if (request_count++ == 0) { - logger->trace("request.url (%1): %2", request_count, request.url); - // First request should be to location - REQUIRE(request.url.find_first_of("/location") != std::string::npos); + // First request should be a location request against the original URL + REQUIRE(request.url.find(original_host) != std::string::npos); REQUIRE(request.redirect_count == 0); redir_transport->simulated_response = { static_cast(sync::HTTPStatus::MovedPermanently), @@ -2572,8 +2568,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { {{"Location", redirect_url}, {"Content-Type", "application/json"}}, "Some body data"}; } - else if (request.url.find("location") != std::string::npos) { - logger->trace("request.url (%1): %2", request_count, request.url); + else if (request.url.find("/location") != std::string::npos) { redir_transport->simulated_response = { static_cast(sync::HTTPStatus::Ok), 0, @@ -2584,14 +2579,12 @@ TEST_CASE("app: sync integration", "[sync][app]") { redirect_host, redirect_scheme, websocket_scheme)}; } else if (request.url.find("auth/session") != std::string::npos) { - logger->trace("request.url (%1): %2", request_count, request.url); redir_transport->simulated_response = {static_cast(sync::HTTPStatus::Unauthorized), 0, {{"Content-Type", "application/json"}}, ""}; } else { - logger->trace("request.url (%1): %2", request_count, request.url); redir_transport->simulated_response.reset(); } }; @@ -2600,9 +2593,58 @@ TEST_CASE("app: sync integration", "[sync][app]") { sync_session->resume(); REQUIRE(wait_for_download(*r)); std::unique_lock lk(logout_mutex); - REQUIRE(logout_cv.wait_for(lk, std::chrono::seconds(15), [&]() { + auto result = logout_cv.wait_for(lk, std::chrono::seconds(15), [&]() { return logged_out; - })); + }); + REQUIRE(result); + REQUIRE(!user1->is_logged_in()); + } + SECTION("Too many websocket redirects logs out user") { + auto sync_manager = test_session.app()->sync_manager(); + auto sync_session = sync_manager->get_existing_session(r->config().path); + sync_session->pause(); + + int connect_count = 0; + redir_provider->websocket_connect_func = [&connect_count](int& status_code, std::string& body) { + if (connect_count++ > 0) + return false; + + status_code = static_cast(sync::HTTPStatus::MovedPermanently); + body = ""; + return true; + }; + int request_count = 0; + int max_http_redirects = 20; // from app.cpp in object-store + redir_transport->request_hook = [&](const Request& request) { + logger->trace("request.url (%1): %2", request_count, request.url); + if (request_count++ == 0) { + // First request should be a location request against the original URL + REQUIRE(request.url.find(original_host) != std::string::npos); + REQUIRE(request.redirect_count == 0); + } + if (request.url.find("/location") != std::string::npos) { + // Keep returning the redirected response + REQUIRE(request.redirect_count < 20); + redir_transport->simulated_response = { + static_cast(sync::HTTPStatus::MovedPermanently), + 0, + {{"Location", redirect_url}, {"Content-Type", "application/json"}}, + "Some body data"}; + } + else { + // should not get any other types of requests during the test - the log out is local + REQUIRE(false); + } + }; + + SyncManager::OnlyForTesting::voluntary_disconnect_all_connections(*sync_manager); + sync_session->resume(); + REQUIRE(wait_for_download(*r)); + std::unique_lock lk(logout_mutex); + auto result = logout_cv.wait_for(lk, std::chrono::seconds(15), [&]() { + return logged_out; + }); + REQUIRE(result); REQUIRE(!user1->is_logged_in()); } } From c7c3fc1ba3ecf7b641af643d983c7cc872940f3c Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Tue, 16 May 2023 15:44:11 -0400 Subject: [PATCH 05/10] Updated changelog and fixed compile warning --- CHANGELOG.md | 8 ++++---- test/object-store/sync/app.cpp | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ebb115f4975..f94f04da853 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,13 +6,13 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* Always update location metadata on user login to keep info up to date ([#6630](https://github.com/realm/realm-core/issues/6630), since v12.9.0) +* Access token refresh for websockets was not updating the location metadata ([#6630](https://github.com/realm/realm-core/issues/6630), since v13.9.3) ### Breaking changes * `platform` and `cpu_arch` fields in the `device_info` structure in App::Config can no longer be provided by the SDK's, they are inferred by the library ([PR #6612](https://github.com/realm/realm-core/pull/6612)) * `bundle_id` is now a required field in the `device_info` structure in App::Config ([PR #6612](https://github.com/realm/realm-core/pull/6612)) -### Compatibility +### Compatibility * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5. ----------- @@ -32,7 +32,6 @@ * Fixed a fatal error (reported to the sync error handler) during client reset (or automatic PBS to FLX migration) if the reset has been triggered during an async open and the schema being applied has added new classes. ([#6601](https://github.com/realm/realm-core/issues/6601), since automatic client resets were introduced in v11.5.0) * Full text search would sometimes find words where the word only matches the beginning of the search token ([#6591](https://github.com/realm/realm-core/issues/6591), since v13.0.0) * Added missing includes of `` surfaced by gcc13 ([#6616](https://github.com/realm/realm-core/pull/6616)) -* Prevent crashing on Results.freeze if underlying object or table were removed by making Results.is_valid correctly report its state. ([#6401](https://github.com/realm/realm-core/issues/6401)) ### Compatibility * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5. @@ -43,7 +42,6 @@ * Add initial support for targeting WebAssembly with Emscripten ([PR #6263](https://github.com/realm/realm-core/pull/6263)). * Sync session multiplexing is now enabled by default. The method `SyncManager::enable_session_multiplexing()` has been renamed `SyncManager::set_session_multiplexing()`. (PR [#6557](https://github.com/realm/realm-core/pull/6557)) * Bump protocol to v9 to indicate client has fix for client reset error during async open ([#6609](https://github.com/realm/realm-core/issues/6609)) -* Fixed `Results::is_valid()` in order to return `false` if the results is bound to a deleted object or table. (PR [#6445](https://github.com/realm/realm-core/pull/6445)) ---------------------------------------------- @@ -56,6 +54,7 @@ ### Fixed * Exclusion of words in a full text search does not work ([#6512](https://github.com/realm/realm-core/issues/6512), since v13.0.0 ); +* Prevent crashing on Results.freeze if underlying object or table were removed by making Results.is_valid correctly report its state. ([#6401](https://github.com/realm/realm-core/issues/6401)) ### Breaking changes * Add `service_name` parameter to `realm_app_call_function` (PR [#6394](https://github.com/realm/realm-core/pull/6394)). @@ -69,6 +68,7 @@ * Reduce the memory footprint of an automatic (discard or recover) client reset when there are large incoming changes from the server. ([#6567](https://github.com/realm/realm-core/issues/6567)) * `get_committed_file_format_version()` safe access to mappings vector from multiple threads. * Add CI tests for Clang 16/Ubuntu 22.04 and update lint task ([PR #6563](https://github.com/realm/realm-core/pull/6563)) +* Fixed `Results::is_valid()` in order to return `false` if the results is bound to a deleted object or table. (PR [#6445](https://github.com/realm/realm-core/pull/6445)) ---------------------------------------------- diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 9c63df4f89a..e4b12999b2b 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -2614,7 +2614,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { return true; }; int request_count = 0; - int max_http_redirects = 20; // from app.cpp in object-store + const int max_http_redirects = 20; // from app.cpp in object-store redir_transport->request_hook = [&](const Request& request) { logger->trace("request.url (%1): %2", request_count, request.url); if (request_count++ == 0) { @@ -2624,7 +2624,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { } if (request.url.find("/location") != std::string::npos) { // Keep returning the redirected response - REQUIRE(request.redirect_count < 20); + REQUIRE(request.redirect_count < max_http_redirects); redir_transport->simulated_response = { static_cast(sync::HTTPStatus::MovedPermanently), 0, From a87056f29f0e7317cb68303d3a2f1ddf0f0aa743 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Thu, 18 May 2023 12:29:50 -0400 Subject: [PATCH 06/10] Added location checks back to test --- test/object-store/sync/app.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index e4b12999b2b..c7223609e99 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -2561,6 +2561,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { if (request_count++ == 0) { // First request should be a location request against the original URL REQUIRE(request.url.find(original_host) != std::string::npos); + REQUIRE(request.url.find("/location") != std::string::npos); REQUIRE(request.redirect_count == 0); redir_transport->simulated_response = { static_cast(sync::HTTPStatus::MovedPermanently), @@ -2620,6 +2621,7 @@ TEST_CASE("app: sync integration", "[sync][app]") { if (request_count++ == 0) { // First request should be a location request against the original URL REQUIRE(request.url.find(original_host) != std::string::npos); + REQUIRE(request.url.find("/location") != std::string::npos); REQUIRE(request.redirect_count == 0); } if (request.url.find("/location") != std::string::npos) { From afbd3128d6df3fe628c8a08474e77885095f6c59 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Thu, 18 May 2023 19:03:56 -0400 Subject: [PATCH 07/10] added mutex locking around location updated state and reworked requesting location update to use flag --- src/realm/object-store/sync/app.cpp | 112 +++++++++++++++++----------- src/realm/object-store/sync/app.hpp | 8 +- 2 files changed, 75 insertions(+), 45 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index a55134f9d20..8dba2624f29 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -301,9 +301,15 @@ void App::configure(const SyncClientConfig& sync_client_config) auto sync_route = make_sync_route(m_app_route); m_sync_manager->configure(shared_from_this(), sync_route, sync_client_config); if (auto metadata = m_sync_manager->app_metadata()) { - // If there is app metadata stored, then update the hostname/syncroute using that info + // If there is app metadata stored, then set up the initial hostname/syncroute + // using that info - it will be updated upon first request to the server update_hostname(metadata); } + { + std::lock_guard lock(*m_route_mutex); + // Always update the location after the app is configured/re-configured + m_location_updated = false; + } } bool App::init_logger() @@ -337,7 +343,7 @@ void App::log_error(const char* message, Params&&... params) std::string App::make_sync_route(const std::string& http_app_route) { - // change the scheme in the base url to ws from http to satisfy the sync client + // change the scheme in the base url from http to ws to satisfy the sync client URL auto sync_route = http_app_route + sync_path; size_t uri_scheme_start = sync_route.find("http"); if (uri_scheme_start == 0) @@ -659,11 +665,21 @@ void App::log_in_with_credentials( BsonDocument body = credentials.serialize_as_bson(); attach_auth_options(body); - // Always update the location on a login to keep the location metadata up to date in cases where - // the transport handles the redirection automatically and does not return the redirect response - // Since the user is logged out as part of a deployment model change, the user will need to log in - // again, but Core will not know that this was driven by an HTTP redirection. - update_metadata_and_resend( + // To ensure the location metadata is always kept up to date, requery the location info before + // logging in the user. Since some SDK network transports (e.g. for JS) automatically handle + // redirects, the location is not updated when a redirect response from the server is received. + // This is especially necessary when the deployment model is changed, where the redirect response + // provides information about the new location of the server and a location info update is + // triggered. If the App never receives a redirect response from the server (because it was + // automatically handled) after the deployment model was changed and the user was logged out, + // the HTTP and websocket URL values will never be updated with the new server location. + { + std::lock_guard lock(*m_route_mutex); + m_location_updated = false; + } + + // If m_location_updated is false, location metadata will be updated before sending the request + do_request( {HttpMethod::post, route, m_request_timeout_ms, get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, [completion = std::move(completion), credentials, linking_user, @@ -870,7 +886,11 @@ void App::init_app_metadata(UniqueFunction&)>&& co { std::string route; - { + if (!new_hostname && m_location_updated) { + // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided + return completion(util::none); // early return + } + else { std::lock_guard lock(*m_route_mutex); route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route()); } @@ -905,7 +925,10 @@ void App::init_app_metadata(UniqueFunction&)>&& co // No metadata in use, update the hostname and sync route directly self->update_hostname(hostname, ws_hostname); } - self->m_location_updated = true; + { + std::lock_guard lock(*self->m_route_mutex); + self->m_location_updated = true; + } } catch (const AppError&) { // Pass the response back to completion @@ -957,19 +980,25 @@ void App::do_request(Request&& request, UniqueFunction&& { request.timeout_ms = default_timeout_ms; - // Refresh the location metadata every time an app is created to ensure the http and - // websocket URL information is up to date. - if (m_location_updated) { - m_config.transport->send_request_to_server( - std::move(request), [self = shared_from_this(), completion = std::move(completion)]( - Request&& request, const Response& response) mutable { - self->handle_possible_redirect_response(std::move(request), response, std::move(completion)); - }); - return; // early return + // Refresh the location metadata every time an app is created (or when requested) to ensure the http + // and websocket URL information is up to date. + { + std::unique_lock lock(*m_route_mutex); + if (!m_location_updated) { + lock.unlock(); + // Location metadata has not yet been received after the App was created, update the location + // info and then send the request + update_metadata_and_resend(std::move(request), std::move(completion)); + return; // early return + } } - // if we do not have metadata yet, update the metadata and resend the request - update_metadata_and_resend(std::move(request), std::move(completion)); + // location info has already been received after App was created + m_config.transport->send_request_to_server( + std::move(request), [self = shared_from_this(), completion = std::move(completion)]( + Request&& request, const Response& response) mutable { + self->handle_possible_redirect_response(std::move(request), response, std::move(completion)); + }); } void App::handle_possible_redirect_response(Request&& request, const Response& response, @@ -1097,36 +1126,33 @@ void App::refresh_access_token(const std::shared_ptr& sync_user, bool { std::lock_guard lock(*m_route_mutex); route = util::format("%1/auth/session", m_base_route); + // If the location needs to be updated, reset the location updated flag + if (update_location) { + m_location_updated = false; + } } log_debug("App: refresh_access_token: email: %1 %2", sync_user->user_profile().email(), update_location ? "(updating location)" : ""); - Request request{HttpMethod::post, std::move(route), m_request_timeout_ms, - get_request_headers(sync_user, RequestTokenType::RefreshToken)}; - auto handler = [completion = std::move(completion), sync_user](const Response& response) { - if (auto error = AppUtils::check_for_errors(response)) { - return completion(std::move(error)); - } - - try { - auto json = parse(response.body); - sync_user->update_access_token(get(json, "access_token")); - } - catch (AppError& err) { - return completion(std::move(err)); - } + // If m_location_updated is false, location metadata will be updated before sending the request + do_request({HttpMethod::post, std::move(route), m_request_timeout_ms, + get_request_headers(sync_user, RequestTokenType::RefreshToken)}, + [completion = std::move(completion), sync_user](const Response& response) { + if (auto error = AppUtils::check_for_errors(response)) { + return completion(std::move(error)); + } - return completion(util::none); - }; + try { + auto json = parse(response.body); + sync_user->update_access_token(get(json, "access_token")); + } + catch (AppError& err) { + return completion(std::move(err)); + } - if (update_location) { - // If update_location, update the location metadata before sending the request - update_metadata_and_resend(std::move(request), std::move(handler)); - } - else { - do_request(std::move(request), std::move(handler)); - } + return completion(util::none); + }); } std::string App::function_call_url_path() const diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp index 6842cb861ae..52066779537 100644 --- a/src/realm/object-store/sync/app.hpp +++ b/src/realm/object-store/sync/app.hpp @@ -403,13 +403,17 @@ class App : public std::enable_shared_from_this, friend class OnlyForTesting; Config m_config; - mutable std::unique_ptr m_route_mutex = std::make_unique(); + + // mutable to allow locking for reads in const functions + // this is a shared pointer to support the App move constructor + mutable std::shared_ptr m_route_mutex = std::make_shared(); std::string m_base_url; std::string m_base_route; std::string m_app_route; std::string m_auth_route; - uint64_t m_request_timeout_ms; bool m_location_updated = false; + + uint64_t m_request_timeout_ms; std::shared_ptr m_sync_manager; std::shared_ptr m_logger_ptr; From 51f421e3651379a52cab047e01ab06cbeec1b6b9 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Thu, 18 May 2023 20:40:03 -0400 Subject: [PATCH 08/10] clang format and fix incorrect timeout value --- src/realm/object-store/sync/app.cpp | 58 ++++++++++++++--------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 8dba2624f29..fbaa59e0ae6 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -679,36 +679,35 @@ void App::log_in_with_credentials( } // If m_location_updated is false, location metadata will be updated before sending the request - do_request( - {HttpMethod::post, route, m_request_timeout_ms, - get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, - [completion = std::move(completion), credentials, linking_user, - self = shared_from_this()](const Response& response) mutable { - if (auto error = AppUtils::check_for_errors(response)) { - self->log_error("App: log_in_with_credentials failed: %1 message: %2", response.http_status_code, - error->what()); - return completion(nullptr, std::move(error)); - } + do_request({HttpMethod::post, route, m_request_timeout_ms, + get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, + [completion = std::move(completion), credentials, linking_user, + self = shared_from_this()](const Response& response) mutable { + if (auto error = AppUtils::check_for_errors(response)) { + self->log_error("App: log_in_with_credentials failed: %1 message: %2", + response.http_status_code, error->what()); + return completion(nullptr, std::move(error)); + } - std::shared_ptr sync_user = linking_user; - try { - auto json = parse(response.body); - if (linking_user) { - linking_user->update_access_token(get(json, "access_token")); - } - else { - sync_user = self->m_sync_manager->get_user( - get(json, "user_id"), get(json, "refresh_token"), - get(json, "access_token"), credentials.provider_as_string(), - get(json, "device_id")); - } - } - catch (const AppError& e) { - return completion(nullptr, e); - } + std::shared_ptr sync_user = linking_user; + try { + auto json = parse(response.body); + if (linking_user) { + linking_user->update_access_token(get(json, "access_token")); + } + else { + sync_user = self->m_sync_manager->get_user( + get(json, "user_id"), get(json, "refresh_token"), + get(json, "access_token"), credentials.provider_as_string(), + get(json, "device_id")); + } + } + catch (const AppError& e) { + return completion(nullptr, e); + } - self->get_profile(sync_user, std::move(completion)); - }); + self->get_profile(sync_user, std::move(completion)); + }); } void App::log_in_with_credentials( @@ -978,7 +977,8 @@ void App::update_metadata_and_resend(Request&& request, UniqueFunction&& completion) { - request.timeout_ms = default_timeout_ms; + // Make sure the timeout value is set to the configured request timeout value + request.timeout_ms = m_request_timeout_ms; // Refresh the location metadata every time an app is created (or when requested) to ensure the http // and websocket URL information is up to date. From bab73003b321ada046f01563316d06cb7fc64b5b Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Mon, 22 May 2023 15:59:58 -0400 Subject: [PATCH 09/10] Reworked update location logic a bit and removed unused function --- src/realm/object-store/sync/app.cpp | 121 ++++++++++++++-------------- src/realm/object-store/sync/app.hpp | 6 +- 2 files changed, 63 insertions(+), 64 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index fbaa59e0ae6..1a9b559cb62 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -673,41 +673,37 @@ void App::log_in_with_credentials( // triggered. If the App never receives a redirect response from the server (because it was // automatically handled) after the deployment model was changed and the user was logged out, // the HTTP and websocket URL values will never be updated with the new server location. - { - std::lock_guard lock(*m_route_mutex); - m_location_updated = false; - } - - // If m_location_updated is false, location metadata will be updated before sending the request - do_request({HttpMethod::post, route, m_request_timeout_ms, - get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, - [completion = std::move(completion), credentials, linking_user, - self = shared_from_this()](const Response& response) mutable { - if (auto error = AppUtils::check_for_errors(response)) { - self->log_error("App: log_in_with_credentials failed: %1 message: %2", - response.http_status_code, error->what()); - return completion(nullptr, std::move(error)); - } + do_request( + {HttpMethod::post, route, m_request_timeout_ms, + get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()}, + [completion = std::move(completion), credentials, linking_user, + self = shared_from_this()](const Response& response) mutable { + if (auto error = AppUtils::check_for_errors(response)) { + self->log_error("App: log_in_with_credentials failed: %1 message: %2", response.http_status_code, + error->what()); + return completion(nullptr, std::move(error)); + } - std::shared_ptr sync_user = linking_user; - try { - auto json = parse(response.body); - if (linking_user) { - linking_user->update_access_token(get(json, "access_token")); - } - else { - sync_user = self->m_sync_manager->get_user( - get(json, "user_id"), get(json, "refresh_token"), - get(json, "access_token"), credentials.provider_as_string(), - get(json, "device_id")); - } - } - catch (const AppError& e) { - return completion(nullptr, e); - } + std::shared_ptr sync_user = linking_user; + try { + auto json = parse(response.body); + if (linking_user) { + linking_user->update_access_token(get(json, "access_token")); + } + else { + sync_user = self->m_sync_manager->get_user( + get(json, "user_id"), get(json, "refresh_token"), + get(json, "access_token"), credentials.provider_as_string(), + get(json, "device_id")); + } + } + catch (const AppError& e) { + return completion(nullptr, e); + } - self->get_profile(sync_user, std::move(completion)); - }); + self->get_profile(sync_user, std::move(completion)); + }, + false); } void App::log_in_with_credentials( @@ -885,15 +881,16 @@ void App::init_app_metadata(UniqueFunction&)>&& co { std::string route; - if (!new_hostname && m_location_updated) { - // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided - return completion(util::none); // early return - } - else { + { std::lock_guard lock(*m_route_mutex); - route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route()); + if (!new_hostname && m_location_updated) { + // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided + return completion(util::none); // early return + } + else { + route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route()); + } } - Request req; req.method = HttpMethod::get; req.url = route; @@ -975,7 +972,7 @@ void App::update_metadata_and_resend(Request&& request, UniqueFunction&& completion) +void App::do_request(Request&& request, UniqueFunction&& completion, bool update_location) { // Make sure the timeout value is set to the configured request timeout value request.timeout_ms = m_request_timeout_ms; @@ -984,6 +981,10 @@ void App::do_request(Request&& request, UniqueFunction&& // and websocket URL information is up to date. { std::unique_lock lock(*m_route_mutex); + if (update_location) { + // Force the location to be updated before sending the request. + m_location_updated = false; + } if (!m_location_updated) { lock.unlock(); // Location metadata has not yet been received after the App was created, update the location @@ -1126,33 +1127,31 @@ void App::refresh_access_token(const std::shared_ptr& sync_user, bool { std::lock_guard lock(*m_route_mutex); route = util::format("%1/auth/session", m_base_route); - // If the location needs to be updated, reset the location updated flag - if (update_location) { - m_location_updated = false; - } } log_debug("App: refresh_access_token: email: %1 %2", sync_user->user_profile().email(), update_location ? "(updating location)" : ""); - // If m_location_updated is false, location metadata will be updated before sending the request - do_request({HttpMethod::post, std::move(route), m_request_timeout_ms, - get_request_headers(sync_user, RequestTokenType::RefreshToken)}, - [completion = std::move(completion), sync_user](const Response& response) { - if (auto error = AppUtils::check_for_errors(response)) { - return completion(std::move(error)); - } + // If update_location is set, force the location info to be updated before sending the request + do_request( + {HttpMethod::post, std::move(route), m_request_timeout_ms, + get_request_headers(sync_user, RequestTokenType::RefreshToken)}, + [completion = std::move(completion), sync_user](const Response& response) { + if (auto error = AppUtils::check_for_errors(response)) { + return completion(std::move(error)); + } - try { - auto json = parse(response.body); - sync_user->update_access_token(get(json, "access_token")); - } - catch (AppError& err) { - return completion(std::move(err)); - } + try { + auto json = parse(response.body); + sync_user->update_access_token(get(json, "access_token")); + } + catch (AppError& err) { + return completion(std::move(err)); + } - return completion(util::none); - }); + return completion(util::none); + }, + update_location); } std::string App::function_call_url_path() const diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp index 52066779537..5e1eb61aaa1 100644 --- a/src/realm/object-store/sync/app.hpp +++ b/src/realm/object-store/sync/app.hpp @@ -467,15 +467,15 @@ class App : public std::enable_shared_from_this, void update_metadata_and_resend(Request&& request, util::UniqueFunction&& completion, const util::Optional& new_hostname = util::none); - void basic_request(std::string&& route, std::string&& body, - util::UniqueFunction)>&& completion); void post(std::string&& route, util::UniqueFunction)>&& completion, const bson::BsonDocument& body); /// Performs a request to the Stitch server. This request does not contain authentication state. /// @param request The request to be performed /// @param completion Returns the response from the server - void do_request(Request&& request, util::UniqueFunction&& completion); + /// @param update_location Force the location metadata to be updated prior to sending the request + void do_request(Request&& request, util::UniqueFunction&& completion, + bool update_location = false); /// Check to see if hte response is a redirect and handle, otherwise pass the response to compleetion /// @param request The request to be performed (in case it needs to be sent again) From 9025c2eca04e5d8039f1f00d44d09ecaff8de376 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Mon, 22 May 2023 22:47:51 -0400 Subject: [PATCH 10/10] Free mutex before calling completion on early exit in init_app_metadata --- src/realm/object-store/sync/app.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 1a9b559cb62..6a6b2dd55ea 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -882,9 +882,11 @@ void App::init_app_metadata(UniqueFunction&)>&& co std::string route; { - std::lock_guard lock(*m_route_mutex); + std::unique_lock lock(*m_route_mutex); + // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided if (!new_hostname && m_location_updated) { - // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided + // Release the lock before calling the completion function + lock.unlock(); return completion(util::none); // early return } else {