diff --git a/CHANGELOG.md b/CHANGELOG.md index 37243983f11..98dff016c77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* None. +* 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 * None. diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 76688329840..6a6b2dd55ea 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,35 +665,45 @@ 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)); - } + // 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. + 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( @@ -865,20 +881,25 @@ void App::init_app_metadata(UniqueFunction&)>&& co { std::string route; - if (!new_hostname && m_location_updated) { + { + 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 - 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) { + // Release the lock before calling the completion function + lock.unlock(); + 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; 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 @@ -902,7 +923,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 @@ -950,23 +974,34 @@ void App::update_metadata_and_resend(Request&& request, UniqueFunction&& completion) +void App::do_request(Request&& request, UniqueFunction&& completion, bool update_location) { - 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 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 (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 + // 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, @@ -1096,33 +1131,29 @@ 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)}; - 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 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)); + } - 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); + }, + 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 6842cb861ae..5e1eb61aaa1 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; @@ -463,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) 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/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/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"); diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 154d69671b7..c7223609e99 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,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::MovedPermanently), @@ -2572,8 +2569,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 +2580,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 +2594,59 @@ 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; + 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) { + // 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) { + // Keep returning the redirected response + REQUIRE(request.redirect_count < max_http_redirects); + 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()); } }