From aa346355f3bfb7911d9be7783099f631272d5480 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 23 Aug 2024 00:11:17 -0400 Subject: [PATCH 1/7] Removed redirect tests --- test/object-store/sync/app.cpp | 548 +++++++++++---------------------- 1 file changed, 181 insertions(+), 367 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 06d6fa56a0..b782a17d52 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -665,39 +665,6 @@ TEST_CASE("app: verify app utils helpers", "[sync][app][local]") { CHECK(!AppUtils::is_success_status_code(300)); CHECK(!AppUtils::is_success_status_code(99999)); } - - SECTION("is_redirect_status_code") { - // Only MovedPermanently(301) and PermanentRedirect(308) return true - CHECK(AppUtils::is_redirect_status_code(301)); - CHECK(AppUtils::is_redirect_status_code(308)); - CHECK(!AppUtils::is_redirect_status_code(0)); - CHECK(!AppUtils::is_redirect_status_code(200)); - CHECK(!AppUtils::is_redirect_status_code(300)); - CHECK(!AppUtils::is_redirect_status_code(403)); - CHECK(!AppUtils::is_redirect_status_code(99999)); - } - - SECTION("extract_redir_location") { - auto comp = AppUtils::extract_redir_location( - {{"Content-Type", "application/json"}, {"Location", "http://redirect.host"}}); - CHECK(comp == "http://redirect.host"); - comp = AppUtils::extract_redir_location({{"location", "http://redirect.host"}}); - CHECK(comp == "http://redirect.host"); - comp = AppUtils::extract_redir_location({{"LoCaTiOn", "http://redirect.host/"}}); - CHECK(comp == "http://redirect.host/"); - comp = AppUtils::extract_redir_location({{"LOCATION", "http://redirect.host/includes/path"}}); - CHECK(comp == "http://redirect.host/includes/path"); - comp = AppUtils::extract_redir_location({{"Content-Type", "application/json"}}); - CHECK(!comp); - comp = AppUtils::extract_redir_location({{"some-location", "http://redirect.host"}}); - CHECK(!comp); - comp = AppUtils::extract_redir_location({{"location", ""}}); - CHECK(!comp); - comp = AppUtils::extract_redir_location({}); - CHECK(!comp); - comp = AppUtils::extract_redir_location({{"location", "bad-server-url"}}); - CHECK(!comp); - } } // MARK: - Login with Credentials Tests @@ -2751,7 +2718,6 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") { } else { INFO(request.url); - // any later requests (eg. redirect) won't have a current user REQUIRE(!user); } // simulate the server denying the refresh @@ -3386,104 +3352,6 @@ TEST_CASE("app: redirect handling", "[sync][pbs][app]") { auto app = oas.app(); const auto partition = random_string(100); - SECTION("invalid redirect response reports and error") { - int request_count = 0; - - // This will fail due to no Location header - transport->request_hook = [&](const Request& request) -> std::optional { - logger->trace("request.url (%1): %2", request_count, request.url); - REQUIRE(request_count++ == 0); - return Response{301, 0, {{"Content-Type", "application/json"}}, "Some body data"}; - }; - app->provider_client().register_email( - creds.email, creds.password, [&](util::Optional error) { - REQUIRE(error); - REQUIRE(error->is_client_error()); - REQUIRE(error->code() == ErrorCodes::ClientRedirectError); - REQUIRE(error->reason() == "Redirect response missing location header"); - }); - - // This will fail due to empty Location header - transport->request_hook = [&](const Request& request) -> std::optional { - logger->trace("request.url (%1): %2", request_count, request.url); - REQUIRE(request_count++ == 1); - return Response{301, 0, {{"Location", ""}, {"Content-Type", "application/json"}}, "Some body data"}; - }; - - app->provider_client().register_email( - creds.email, creds.password, [&](util::Optional error) { - REQUIRE(error); - REQUIRE(error->is_client_error()); - REQUIRE(error->code() == ErrorCodes::ClientRedirectError); - REQUIRE(error->reason() == "Redirect response missing location header"); - }); - } - - SECTION("valid redirect response") { - int request_count = 0; - const std::string second_host = "http://second.invalid:9091"; - const std::string third_host = "http://third.invalid:9092"; - - transport->request_hook = [&](const Request& request) -> std::optional { - logger->trace("Received request[%1]: %2", request_count, request.url); - switch (request_count++) { - case 0: - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - REQUIRE_THAT(request.url, ContainsSubstring(*oas_config.base_url)); - return Response{301, 0, {{"Location", second_host}, {"Content-Type", "application/json"}}, ""}; - - case 1: - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - REQUIRE_THAT(request.url, ContainsSubstring(second_host)); - return Response{301, 0, {{"Location", third_host}, {"Content-Type", "application/json"}}, ""}; - - case 2: - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - REQUIRE_THAT(request.url, ContainsSubstring(third_host)); - return Response{301, 0, {{"Location", second_host}, {"Content-Type", "application/json"}}, ""}; - - case 3: - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - REQUIRE_THAT(request.url, ContainsSubstring(second_host)); - return std::nullopt; - - default: - // some.fake.url is the location reported by UnitTestTransport - REQUIRE_THAT(request.url, ContainsSubstring("https://some.fake.url")); - return std::nullopt; - } - }; - - // This will be successful after a couple of retries due to the redirect response - app->provider_client().register_email( - creds.email, creds.password, [&](util::Optional error) { - REQUIRE(!error); - }); - } - - SECTION("too many redirects eventually reports an error") { - int request_count = 0; - transport->request_hook = [&](const Request& request) -> std::optional { - logger->trace("request.url (%1): %2", request_count, request.url); - REQUIRE(request_count < 21); - ++request_count; - return Response{request_count % 2 == 1 ? 308 : 301, - 0, - {{"Location", "http://somehost:9090"}, {"Content-Type", "application/json"}}, - "Some body data"}; - }; - - app->log_in_with_credentials(app::AppCredentials::username_password(creds.email, creds.password), - [&](std::shared_ptr user, util::Optional error) { - REQUIRE(!user); - REQUIRE(error); - REQUIRE(error->is_client_error()); - REQUIRE(error->code() == ErrorCodes::ClientTooManyRedirects); - REQUIRE(error->reason() == "number of redirections exceeded 20"); - }); - REQUIRE(request_count == 21); - } - SECTION("server in maintenance reports error") { transport->request_hook = [&](const Request&) -> std::optional { nlohmann::json maintenance_error = {{"error_code", "MaintenanceInProgress"}, @@ -3674,57 +3542,25 @@ TEST_CASE("app: redirect handling", "[sync][pbs][app]") { REQUIRE(result); REQUIRE_FALSE(realm_config.sync_config->user->is_logged_in()); } - - SECTION("too many websocket redirects logs out user") { - int request_count = 0; - const int max_http_redirects = 20; // from app.cpp in object-store - transport->request_hook = [&](const Request& request) -> std::optional { - logger->trace("request.url (%1): %2", request_count, request.url); - - // The test should never request anything other than /location - // even though the user is set to the logged-out state as trying - // to log out on the server needs to go through /location first too - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - REQUIRE(request_count <= max_http_redirects); - - // First request should be a location request against the original URL - // and rest should use the redirect url - if (request_count++ == 0) { - REQUIRE_THAT(request.url, ContainsSubstring("some.fake.url")); - } - else { - REQUIRE_THAT(request.url, ContainsSubstring("asdf.invalid")); - } - // Keep returning the redirected response - return Response{static_cast(sync::HTTPStatus::MovedPermanently), - 0, - {{"Location", "http://asdf.invalid"}}, - ""}; - }; - - 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_FALSE(realm_config.sync_config->user->is_logged_in()); - } } } TEST_CASE("app: base_url", "[sync][app][base_url]") { struct BaseUrlTransport : UnitTestTransport { std::string expected_url; - std::optional redirect_url; + std::string location_url; + std::string location_wsurl; bool location_requested = false; bool location_returns_error = false; - void reset(std::string_view expect_url, std::optional redir_url = std::nullopt) + void reset(std::string expect_url, std::optional url = std::nullopt, + std::optional wsurl = std::nullopt) { - expected_url = std::string(expect_url); - redirect_url = redir_url; + expected_url = expect_url; + REALM_ASSERT(!expected_url.empty()); + location_url = url.value_or(expect_url); + REALM_ASSERT(!location_url.empty()); + location_wsurl = wsurl.value_or(App::create_ws_host_url(location_url)); location_requested = false; location_returns_error = false; } @@ -3740,37 +3576,37 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { completion(app::Response{static_cast(sync::HTTPStatus::NotFound), 0, {}, "404 not found"}); return; } - if (redirect_url) { - // Update the expected url to be the redirect url - expected_url = std::string(*redirect_url); - redirect_url.reset(); - - completion(app::Response{static_cast(sync::HTTPStatus::PermanentRedirect), - 0, - {{"location", expected_url}}, - "308 permanent redirect"}); - return; - } - auto ws_url = App::create_ws_host_url(expected_url); completion( app::Response{static_cast(sync::HTTPStatus::Ok), 0, {}, util::format("{\"deployment_model\":\"GLOBAL\",\"location\":\"US-VA\",\"hostname\":" "\"%1\",\"ws_hostname\":\"%2\"}", - expected_url, ws_url)}); + location_url, location_wsurl)}); return; } - + if (location_requested) { + CHECK_THAT(request.url, ContainsSubstring(location_url)); + } + else { + CHECK_THAT(request.url, ContainsSubstring(expected_url)); + } UnitTestTransport::send_request_to_server(request, std::move(completion)); } }; auto logger = util::Logger::get_default_logger(); - - auto redir_transport = std::make_shared(); + std::string default_base_url = std::string(App::default_base_url()); + std::string default_base_wsurl = App::create_ws_host_url(App::default_base_url()); + std::string test_base_url = "https://base.someurl.fake"; + std::string test_base_wsurl = "wss://base.someurl.fake"; + std::string test_location_url = "https://loc.someurl.fake"; + std::string test_location_wsurl = "wss://loc.someurl.fake"; + std::string test_location_wsurl2 = "wss://ws.loc.someurl.fake"; + + auto location_transport = std::make_shared(); auto get_config_with_base_url = [&](std::optional base_url = std::nullopt) { - OfflineAppSession::Config config(redir_transport); + OfflineAppSession::Config config(location_transport); config.base_url = base_url; return config; }; @@ -3819,189 +3655,157 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { SECTION("Test app config baseurl") { { // First time through, base_url is empty; https://services.cloud.mongodb.com is expected - redir_transport->reset(App::default_base_url()); + location_transport->reset(std::string(App::default_base_url())); auto config = get_config_with_base_url(); OfflineAppSession oas(config); auto app = oas.app(); // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); + CHECK(!location_transport->location_requested); // Initial hostname and ws hostname use base url, but aren't used until location is updated CHECK(app->get_host_url() == App::default_base_url()); CHECK(app->get_ws_host_url() == App::create_ws_host_url(App::default_base_url())); oas.make_user(); - CHECK(redir_transport->location_requested); + CHECK(location_transport->location_requested); CHECK(app->get_base_url() == App::default_base_url()); CHECK(app->get_host_url() == App::default_base_url()); CHECK(app->get_ws_host_url() == App::create_ws_host_url(App::default_base_url())); } { - // Second time through, base_url is set to https://alternate.someurl.fake is expected - redir_transport->reset("https://alternate.someurl.fake"); - auto config = get_config_with_base_url("https://alternate.someurl.fake"); + // Base_url is set to test_base_url and test_location_url is expected after + // location request + location_transport->reset(test_base_url, test_location_url); + auto config = get_config_with_base_url(test_base_url); OfflineAppSession oas(config); auto app = oas.app(); // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); + CHECK(!location_transport->location_requested); // Initial hostname and ws hostname use base url, but aren't used until location is updated - CHECK(app->get_host_url() == "https://alternate.someurl.fake"); - CHECK(app->get_ws_host_url() == "wss://alternate.someurl.fake"); + CHECK(app->get_host_url() == test_base_url); + CHECK(app->get_ws_host_url() == test_base_wsurl); oas.make_user(); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == "https://alternate.someurl.fake"); - CHECK(app->get_host_url() == "https://alternate.someurl.fake"); - CHECK(app->get_ws_host_url() == "wss://alternate.someurl.fake"); + CHECK(location_transport->location_requested); + CHECK(app->get_base_url() == test_base_url); + CHECK(app->get_host_url() == test_location_url); + CHECK(app->get_ws_host_url() == test_location_wsurl); } { // Third time through, base_url is not set, expect https://services.cloud.mongodb.com, // since metadata is no longer used - std::string expected_url = std::string(App::default_base_url()); - std::string expected_wsurl = App::create_ws_host_url(App::default_base_url()); - redir_transport->reset(expected_url); + location_transport->reset(default_base_url); auto config = get_config_with_base_url(); OfflineAppSession oas(config); auto app = oas.app(); // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); - // Initial hostname and ws hostname use base url, but aren't used until location is updated - CHECK(app->get_host_url() == expected_url); - CHECK(app->get_ws_host_url() == expected_wsurl); - - oas.make_user(); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == expected_url); - CHECK(app->get_host_url() == expected_url); - CHECK(app->get_ws_host_url() == expected_wsurl); - } - { - // Fourth time through, base_url is set to https://some-other.someurl.fake, with a redirect - redir_transport->reset("https://some-other.someurl.fake", "http://redirect.someurl.fake"); - auto config = get_config_with_base_url("https://some-other.someurl.fake"); - OfflineAppSession oas(config); - auto app = oas.app(); - - // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); + CHECK(!location_transport->location_requested); // Initial hostname and ws hostname use base url, but aren't used until location is updated - CHECK(app->get_host_url() == "https://some-other.someurl.fake"); - CHECK(app->get_ws_host_url() == "wss://some-other.someurl.fake"); + CHECK(app->get_host_url() == default_base_url); + CHECK(app->get_ws_host_url() == default_base_wsurl); oas.make_user(); - CHECK(redir_transport->location_requested); - // Base URL is still set to the original value - CHECK(app->get_base_url() == "https://some-other.someurl.fake"); - // Hostname and ws hostname use the redirect URL values - CHECK(app->get_host_url() == "http://redirect.someurl.fake"); - CHECK(app->get_ws_host_url() == "ws://redirect.someurl.fake"); + CHECK(location_transport->location_requested); + CHECK(app->get_base_url() == default_base_url); + CHECK(app->get_host_url() == default_base_url); + CHECK(app->get_ws_host_url() == default_base_wsurl); } } - SECTION("Test update_baseurl") { - redir_transport->reset("https://alternate.someurl.fake"); - auto config = get_config_with_base_url("https://alternate.someurl.fake"); + SECTION("Test update_baseurl after first request") { + bool error_occurred = GENERATE(true, false); + + location_transport->reset(test_base_url, test_location_url); + auto config = get_config_with_base_url(test_base_url); OfflineAppSession oas(config); auto app = oas.app(); // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); + CHECK(!location_transport->location_requested); + // Perform an operation prior to updating the base URL oas.make_user(); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == "https://alternate.someurl.fake"); - CHECK(app->get_host_url() == "https://alternate.someurl.fake"); - CHECK(app->get_ws_host_url() == "wss://alternate.someurl.fake"); + CHECK(location_transport->location_requested); + CHECK(app->get_base_url() == test_base_url); + CHECK(app->get_host_url() == test_location_url); + CHECK(app->get_ws_host_url() == test_location_wsurl); - redir_transport->reset(App::default_base_url()); + location_transport->reset(default_base_url); + location_transport->location_returns_error = error_occurred; // Revert the base URL to the default URL value using the empty string - app->update_base_url("", [](util::Optional error) { - CHECK(!error); - }); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == App::default_base_url()); - CHECK(app->get_host_url() == App::default_base_url()); - CHECK(app->get_ws_host_url() == App::create_ws_host_url(App::default_base_url())); - oas.make_user(); + app->update_base_url("", [error_occurred](util::Optional error) { + CHECK(error.has_value() == error_occurred); + }); + CHECK(location_transport->location_requested); + if (error_occurred) { + // Not updated due to the error + CHECK(app->get_base_url() == test_base_url); + CHECK(app->get_host_url() == test_location_url); + CHECK(app->get_ws_host_url() == test_location_wsurl); + } + else { + // updated successfully + CHECK(app->get_base_url() == default_base_url); + CHECK(app->get_host_url() == default_base_url); + CHECK(app->get_ws_host_url() == default_base_wsurl); + oas.make_user(); // try another operation + } } - SECTION("Test update_baseurl with redirect") { - redir_transport->reset("https://alternate.someurl.fake"); - auto config = get_config_with_base_url("https://alternate.someurl.fake"); - OfflineAppSession oas(config); - auto app = oas.app(); - - // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); - - oas.make_user(); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == "https://alternate.someurl.fake"); - CHECK(app->get_host_url() == "https://alternate.someurl.fake"); - CHECK(app->get_ws_host_url() == "wss://alternate.someurl.fake"); - - redir_transport->reset("http://some-other.someurl.fake", "https://redirect.otherurl.fake"); - - app->update_base_url("http://some-other.someurl.fake", [](util::Optional error) { - CHECK(!error); - }); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == "http://some-other.someurl.fake"); - CHECK(app->get_host_url() == "https://redirect.otherurl.fake"); - CHECK(app->get_ws_host_url() == "wss://redirect.otherurl.fake"); - // Expected URL is still "https://redirect.otherurl.fake" after redirect - oas.make_user(); - } + SECTION("Test update_baseurl before first request") { + bool error_occurred = GENERATE(true, false); - SECTION("Test update_baseurl returns error") { - redir_transport->reset("http://alternate.someurl.fake"); - auto config = get_config_with_base_url("http://alternate.someurl.fake"); + location_transport->reset(default_base_url, test_location_url, test_location_wsurl2); + location_transport->location_returns_error = error_occurred; + auto config = get_config_with_base_url(test_base_url); OfflineAppSession oas(config); auto app = oas.app(); - // Location is not requested until first app services request - CHECK(!redir_transport->location_requested); - - oas.make_user(); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == "http://alternate.someurl.fake"); - CHECK(app->get_host_url() == "http://alternate.someurl.fake"); - CHECK(app->get_ws_host_url() == "ws://alternate.someurl.fake"); + // Check updating the base URL before an initial app_services request. + CHECK(!location_transport->location_requested); - redir_transport->reset("https://some-other.someurl.fake"); - redir_transport->location_returns_error = true; - - app->update_base_url("https://some-other.someurl.fake", [](util::Optional error) { - CHECK(error); - }); - CHECK(redir_transport->location_requested); - // Verify original url values are still being used - CHECK(app->get_base_url() == "http://alternate.someurl.fake"); - CHECK(app->get_host_url() == "http://alternate.someurl.fake"); - CHECK(app->get_ws_host_url() == "ws://alternate.someurl.fake"); + // Revert the base URL to the default URL value using the empty string + app->update_base_url("", [error_occurred](util::Optional error) { + CHECK(error.has_value() == error_occurred); + }); + CHECK(location_transport->location_requested); + if (error_occurred) { + // Not updated due to the error + CHECK(app->get_base_url() == test_base_url); + CHECK(app->get_host_url() == test_base_url); + CHECK(app->get_ws_host_url() == test_base_wsurl); + } + else { + // updated successfully + CHECK(app->get_base_url() == default_base_url); + CHECK(app->get_host_url() == test_location_url); + CHECK(app->get_ws_host_url() == test_location_wsurl2); + oas.make_user(); // try another operation + } } // Verify new sync session updates location when created with cached user SECTION("Verify new sync session updates location") { bool use_ssl = GENERATE(true, false); - std::string initial_host = "alternate.someurl.fake"; - unsigned initial_port = use_ssl ? 443 : 80; - std::string expected_host = "redirect.someurl.fake"; - unsigned expected_port = 8081; - std::string init_url = util::format("http%1://%2", use_ssl ? "s" : "", initial_host); - std::string init_wsurl = util::format("ws%1://%2", use_ssl ? "s" : "", initial_host); - std::string redir_url = util::format("http%1://%2:%3", use_ssl ? "s" : "", expected_host, expected_port); - std::string redir_wsurl = util::format("ws%1://%2:%3", use_ssl ? "s" : "", expected_host, expected_port); + std::string base_host = "base.url.fake"; + std::string location_host = "alternate.url.fake"; + std::string new_location_host = "new.url.fake"; + unsigned location_port = use_ssl ? 443 : 80; + std::string sync_base_url = util::format("http://%1", base_host); + std::string sync_location_url = util::format("http%1://%2", use_ssl ? "s" : "", location_host); + std::string sync_location_wsurl = util::format("ws%1://%2", use_ssl ? "s" : "", location_host); + std::string new_location_url = util::format("http%1://%2", use_ssl ? "s" : "", new_location_host); + std::string new_location_wsurl = util::format("ws%1://%2", use_ssl ? "s" : "", new_location_host); auto socket_provider = std::make_shared(logger, "some user agent"); socket_provider->websocket_connect_func = []() -> std::optional { return SocketProviderError(sync::websocket::WebSocketError::websocket_connection_failed, "404 not found"); }; - auto config = get_config_with_base_url(init_url); + auto config = get_config_with_base_url(sync_base_url); config.metadata_mode = AppConfig::MetadataMode::NoEncryption; config.socket_provider = socket_provider; config.storage_path = util::make_temp_dir(); @@ -4009,24 +3813,24 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { // Log in to get a cached user { - redir_transport->reset(init_url); + location_transport->reset(sync_base_url, sync_location_url, sync_location_wsurl); OfflineAppSession oas(config); auto app = oas.app(); { + CHECK_FALSE(location_transport->location_requested); auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(init_url))); + CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(sync_base_url))); CHECK_FALSE(verified); } oas.make_user(); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == init_url); - CHECK(app->get_host_url() == init_url); - CHECK(app->get_ws_host_url() == init_wsurl); + CHECK(location_transport->location_requested); + CHECK(app->get_base_url() == sync_base_url); + CHECK(app->get_host_url() == sync_location_url); + CHECK(app->get_ws_host_url() == sync_location_wsurl); auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(init_url))); - CHECK_THAT(sync_route, ContainsSubstring(init_wsurl)); + CHECK_THAT(sync_route, ContainsSubstring(sync_location_wsurl)); CHECK(verified); } @@ -4034,37 +3838,42 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { config.delete_storage = true; // Recreate the app using the cached user and start a sync session, which will is set to fail on connect SECTION("Sync Session fails on connect after updating location") { - enum class TestState { start, session_started }; + enum class TestState { start, first_attempt, second_attempt, complete }; TestingStateMachine state(TestState::start); - redir_transport->reset(init_url, redir_url); + location_transport->reset(sync_base_url, new_location_url, new_location_wsurl); + // Reuse the config so the app uses the cached user OfflineAppSession oas(config); auto app = oas.app(); + REQUIRE(app->current_user()); - // Verify the default sync route, which has not been verified + // Verify the initial sync route, since the location hasn't been queried + // and the location is not "verified", the sync route host is based off + // the value provided in the AppConfig::base_url value { auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(init_url))); + CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(sync_base_url))); CHECK_FALSE(verified); } - REQUIRE(app->current_user()); - std::atomic connect_attempts = 0; socket_provider->endpoint_verify_func = [&](const sync::WebSocketEndpoint& ep) { - // First connection attempt is to the originally specified endpoint. Since - // it hasn't been verified, we swallow the error and do a location update, - // which will then try to connect to the redir target - auto attempt = connect_attempts++; - if (attempt == 0) { - CHECK(ep.address == initial_host); - CHECK(ep.port == initial_port); - CHECK(ep.is_ssl == use_ssl); - } - else { - CHECK(ep.address == expected_host); - CHECK(ep.port == expected_port); - CHECK(ep.is_ssl == use_ssl); - } + state.transition_with([&](TestState cur_state) -> std::optional { + if (cur_state == TestState::start) { + // First time through is using the original base URL + CHECK(ep.address == base_host); + CHECK(ep.port == 80); + CHECK(ep.is_ssl == false); + return TestState::first_attempt; + } + else if (cur_state == TestState::first_attempt) { + // Second time through is using the values from location endpoint + CHECK(ep.address == new_location_host); + CHECK(ep.port == location_port); + CHECK(ep.is_ssl == use_ssl); + return TestState::second_attempt; + } + return std::nullopt; + }); }; RealmConfig r_config; @@ -4076,42 +3885,47 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { CHECK(!error.status.is_ok()); CHECK(error.status.code() == ErrorCodes::SyncConnectFailed); CHECK(!error.is_fatal); - state.transition_to(TestState::session_started); + state.transition_with([&](TestState cur_state) -> std::optional { + CHECK(cur_state == TestState::second_attempt); + return TestState::complete; + }); }; auto realm = Realm::get_shared_realm(r_config); - state.wait_for(TestState::session_started); + state.wait_for(TestState::complete); - CHECK(redir_transport->location_requested); - CHECK(app->get_base_url() == init_url); - CHECK(app->get_host_url() == redir_url); - CHECK(app->get_ws_host_url() == redir_wsurl); + CHECK(location_transport->location_requested); + CHECK(app->get_base_url() == sync_base_url); + CHECK(app->get_host_url() == new_location_url); + CHECK(app->get_ws_host_url() == new_location_wsurl); auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(redir_wsurl)); + CHECK_THAT(sync_route, ContainsSubstring(new_location_wsurl)); CHECK(verified); } - SECTION("Sync Session retries after initial location failure") { enum class TestState { start, location_failed, session_started }; TestingStateMachine state(TestState::start); const int retry_count = GENERATE(1, 3); - redir_transport->reset(init_url); - redir_transport->location_returns_error = true; + location_transport->reset(sync_base_url, new_location_url, new_location_wsurl); + location_transport->location_returns_error = true; + // Reuse the config so the app uses the cached user OfflineAppSession oas(config); auto app = oas.app(); REQUIRE(app->current_user()); - // Verify the default sync route, which has not been verified + // Verify the initial sync route, since the location hasn't been queried + // and the location is not "verified", the sync route host is based off + // the value provided in the AppConfig::base_url value { auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(init_url))); + CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(sync_base_url))); CHECK_FALSE(verified); } socket_provider->endpoint_verify_func = [&](const sync::WebSocketEndpoint& ep) { - CHECK(ep.address == initial_host); - CHECK(ep.port == initial_port); - CHECK(ep.is_ssl == use_ssl); + CHECK(ep.address == base_host); + CHECK(ep.port == 80); + CHECK(ep.is_ssl == false); }; socket_provider->websocket_connect_func = [&, request_count = @@ -4120,33 +3934,33 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { // First connection attempt is to the unverified initial URL // since we have a valid access token but have never successfully // connected. This failing will trigger a location update. - CHECK_FALSE(redir_transport->location_requested); + CHECK_FALSE(location_transport->location_requested); } else { // All attempts after the first should have requested location - CHECK(redir_transport->location_requested); - redir_transport->location_requested = false; + CHECK(location_transport->location_requested); + location_transport->location_requested = false; } // Until we allow a location request to succeed we should keep // getting the original unverified route - if (redir_transport->location_returns_error) { - CHECK(app->get_base_url() == init_url); - CHECK(app->get_host_url() == init_url); - CHECK(app->get_ws_host_url() == init_wsurl); + if (location_transport->location_returns_error) { + CHECK(app->get_base_url() == sync_base_url); + CHECK(app->get_host_url() == sync_base_url); + CHECK(app->get_ws_host_url() == app::App::create_ws_host_url(sync_base_url)); { auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(init_url))); + CHECK_THAT(sync_route, ContainsSubstring(app::App::create_ws_host_url(sync_base_url))); CHECK_FALSE(verified); } } // After the chosen number of attempts let the location request succeed if (request_count++ >= retry_count) { - redir_transport->reset(init_url, redir_url); + location_transport->reset(sync_base_url, new_location_url, new_location_wsurl); socket_provider->endpoint_verify_func = [&](const sync::WebSocketEndpoint& ep) { - CHECK(ep.address == expected_host); - CHECK(ep.port == expected_port); + CHECK(ep.address == new_location_host); + CHECK(ep.port == location_port); CHECK(ep.is_ssl == use_ssl); state.transition_to(TestState::location_failed); }; @@ -4177,11 +3991,11 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { auto realm = Realm::get_shared_realm(r_config); state.wait_for(TestState::session_started); - CHECK(app->get_base_url() == init_url); - CHECK(app->get_host_url() == redir_url); - CHECK(app->get_ws_host_url() == redir_wsurl); + CHECK(app->get_base_url() == sync_base_url); + CHECK(app->get_host_url() == new_location_url); + CHECK(app->get_ws_host_url() == new_location_wsurl); auto [sync_route, verified] = app->sync_manager()->sync_route(); - CHECK_THAT(sync_route, ContainsSubstring(redir_wsurl)); + CHECK_THAT(sync_route, ContainsSubstring(new_location_wsurl)); CHECK(verified); } } From fd52bb1400508c28a2924078dd4c3a6de5de24da Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 23 Aug 2024 09:12:05 -0400 Subject: [PATCH 2/7] Removed one more location redirect test case --- test/object-store/sync/app.cpp | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index b782a17d52..a50edecf09 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3456,22 +3456,8 @@ TEST_CASE("app: redirect handling", "[sync][pbs][app]") { int request_count = 0; transport->request_hook = [&](const Request& request) -> std::optional { logger->trace("request.url (%1): %2", request_count, request.url); - ++request_count; - - // First request should be a location request against the original URL - if (request_count == 1) { + if (request.url.find("/location") != std::string::npos) { REQUIRE_THAT(request.url, ContainsSubstring("some.fake.url")); - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - return Response{static_cast(sync::HTTPStatus::PermanentRedirect), - 0, - {{"Location", "http://asdf.invalid"}}, - ""}; - } - - // Second request should be a location request against the new URL - if (request_count == 2) { - REQUIRE_THAT(request.url, ContainsSubstring("/location")); - REQUIRE_THAT(request.url, ContainsSubstring("asdf.invalid")); return Response{200, 0, {}, @@ -3489,7 +3475,6 @@ TEST_CASE("app: redirect handling", "[sync][pbs][app]") { sync_session->resume(); REQUIRE(!wait_for_download(*r)); - REQUIRE(request_count > 1); REQUIRE(realm_config.sync_config->user->is_logged_in()); // Verify session is using the updated server url from the redirect From 3a9c6632e280789921e7567ce040194164e554fb Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 23 Aug 2024 09:19:54 -0400 Subject: [PATCH 3/7] Removed 301/308 redirection support from App --- src/realm/object-store/sync/app.cpp | 150 +++++++--------------- src/realm/object-store/sync/app.hpp | 20 +-- src/realm/object-store/sync/app_utils.cpp | 22 ---- src/realm/object-store/sync/app_utils.hpp | 2 - 4 files changed, 51 insertions(+), 143 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index 9306807912..e66e0ee208 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -189,7 +189,6 @@ constexpr static std::string_view s_sync_path = "/realm-sync"; constexpr static uint64_t s_default_timeout_ms = 60000; constexpr static std::string_view s_username_password_provider_key = "local-userpass"; constexpr static std::string_view s_user_api_key_provider_key_path = "api_keys"; -constexpr static int s_max_http_redirects = 20; static util::FlatMap> s_apps_cache; // app_id -> base_url -> app std::mutex s_apps_mutex; } // anonymous namespace @@ -978,18 +977,17 @@ void App::delete_user(const std::shared_ptr& user, UniqueFunctionuser_id(); - user->detach_and_tear_down(); - m_metadata_store->delete_user(*m_file_manager, user_id); - emit_change_to_subscribers(); - } - completion(std::move(error)); - }); + do_authenticated_request(HttpMethod::del, url_for_path("/auth/delete"), "", user, RequestTokenType::AccessToken, + [completion = std::move(completion), user, this](const Response& response) { + auto error = AppUtils::check_for_errors(response); + if (!error) { + auto user_id = user->user_id(); + user->detach_and_tear_down(); + m_metadata_store->delete_user(*m_file_manager, user_id); + emit_change_to_subscribers(); + } + completion(std::move(error)); + }); } void App::link_user(const std::shared_ptr& user, const AppCredentials& credentials, @@ -1054,34 +1052,32 @@ std::string App::get_app_route(const Optional& hostname) const } void App::request_location(UniqueFunction)>&& completion, - std::optional&& new_hostname, std::optional&& redir_location, - int redirect_count) -{ - // Request the new location information at the new base url hostname; or redir response location if a redirect - // occurred during the initial location request. redirect_count is used to track the number of sequential - // redirect responses received during the location update and return an error if this count exceeds - // max_http_redirects. If neither new_hostname nor redir_location is provided, the current value of m_base_url - // will be used. - std::string app_route; - std::string base_url; + std::optional&& new_hostname) +{ + // Request the new location information the original configured base_url or the new_hostname + // if the base_url is being updated. If a new_hostname has not been provided and the location + // has already been requested, this function does nothing. + std::string app_route; // The app_route for the server to query the location + std::string base_url; // The configured base_url hostname used for querying the location { util::CheckedUniqueLock lock(m_route_mutex); // Skip if the location info has already been initialized and a new hostname is not provided - if (!new_hostname && !redir_location && m_location_updated) { + if (!new_hostname && m_location_updated) { // Release the lock before calling the completion function lock.unlock(); completion(util::none); return; } - base_url = new_hostname.value_or(m_base_url); - // If this is for a redirect after querying new_hostname, then use the redirect location - if (redir_location) - app_route = get_app_route(redir_location); - // If this is querying the new_hostname, then use that location - else if (new_hostname) + // If this is querying the new_hostname, then use that to query the location + if (new_hostname) { + base_url = *new_hostname; app_route = get_app_route(new_hostname); - else + } + // Otherwise, use the current hostname + else { app_route = get_app_route(); + base_url = m_base_url; + } REALM_ASSERT(!app_route.empty()); } @@ -1093,46 +1089,15 @@ void App::request_location(UniqueFunction)>&& compl log_debug("App: request location: %1", req.url); m_config.transport->send_request_to_server(req, [self = shared_from_this(), completion = std::move(completion), - base_url = std::move(base_url), - redirect_count](const Response& response) mutable { - // Check to see if a redirect occurred - if (AppUtils::is_redirect_status_code(response.http_status_code)) { - // Make sure we don't do too many redirects (max_http_redirects (20) is an arbitrary number) - if (redirect_count >= s_max_http_redirects) { - completion(AppError{ErrorCodes::ClientTooManyRedirects, - util::format("number of redirections exceeded %1", s_max_http_redirects), - {}, - response.http_status_code}); - return; - } - // Handle the redirect response when requesting the location - extract the - // new location header field and resend the request. - auto redir_location = AppUtils::extract_redir_location(response.headers); - if (!redir_location) { - // Location not found in the response, pass error response up the chain - completion(AppError{ErrorCodes::ClientRedirectError, - "Redirect response missing location header", - {}, - response.http_status_code}); - return; - } - // try to request the location info at the new location in the redirect response - // retry_count is passed in to track the number of subsequent redirection attempts - self->request_location(std::move(completion), std::move(base_url), std::move(redir_location), - redirect_count + 1); - return; - } - + base_url = std::move(base_url)](const Response& response) { // Location request was successful - update the location info - auto update_response = self->update_location(response, base_url); - if (update_response) { - self->log_error("App: request location failed (%1%2): %3", update_response->code_string(), - update_response->additional_status_code - ? util::format(" %1", *update_response->additional_status_code) - : "", - update_response->reason()); + auto error = self->update_location(response, base_url); + if (error) { + self->log_error("App: request location failed (%1%2): %3", error->code_string(), + error->additional_status_code ? util::format(" %1", *error->additional_status_code) : "", + error->reason()); } - completion(update_response); + completion(error); }); } @@ -1169,8 +1134,7 @@ std::optional App::update_location(const Response& response, const std return util::none; } -void App::update_location_and_resend(std::unique_ptr&& request, IntermediateCompletion&& completion, - Optional&& redir_location) +void App::update_location_and_resend(std::unique_ptr&& request, IntermediateCompletion&& completion) { // Update the location information if a redirect response was received or m_location_updated == false // and then send the request to the server with request.url updated to the new AppServices hostname. @@ -1192,13 +1156,13 @@ void App::update_location_and_resend(std::unique_ptr&& request, Interme // Retry the original request with the updated url auto& request_ref = *request; self->m_config.transport->send_request_to_server( - request_ref, [self = std::move(self), completion = std::move(completion), - request = std::move(request)](const Response& response) mutable { - self->check_for_redirect_response(std::move(request), response, std::move(completion)); + request_ref, + [completion = std::move(completion), request = std::move(request)](const Response& response) mutable { + completion(std::move(request), response); }); }, // The base_url is not changing for this request - util::none, std::move(redir_location)); + util::none); } void App::post(std::string&& route, UniqueFunction)>&& completion, const BsonDocument& body) @@ -1213,7 +1177,11 @@ void App::post(std::string&& route, UniqueFunction)>&& c void App::do_request(std::unique_ptr&& request, IntermediateCompletion&& completion, bool update_location) { // Verify the request URL to make sure it is valid - util::Uri::parse(request->url); + if (auto valid_url = util::Uri::try_parse(request->url); !valid_url.is_ok()) { + completion(std::move(request), AppUtils::make_apperror_response( + AppError{valid_url.get_status().code(), valid_url.get_status().reason()})); + return; + } // Refresh the location info when app is created or when requested (e.g. after a websocket redirect) // to ensure the http and websocket URL information is up to date. @@ -1235,36 +1203,12 @@ void App::do_request(std::unique_ptr&& request, IntermediateCompletion& // If location info has already been updated, then send the request directly auto& request_ref = *request; m_config.transport->send_request_to_server( - request_ref, [self = shared_from_this(), completion = std::move(completion), - request = std::move(request)](const Response& response) mutable { - self->check_for_redirect_response(std::move(request), response, std::move(completion)); + request_ref, + [completion = std::move(completion), request = std::move(request)](const Response& response) mutable { + completion(std::move(request), response); }); } -void App::check_for_redirect_response(std::unique_ptr&& request, const Response& response, - IntermediateCompletion&& completion) -{ - // If this isn't a redirect response, then we're done - if (!AppUtils::is_redirect_status_code(response.http_status_code)) { - return completion(std::move(request), response); - } - - // Handle a redirect response when sending the original request - extract the location - // header field and resend the request. - auto redir_location = AppUtils::extract_redir_location(response.headers); - if (!redir_location) { - // Location not found in the response, pass error response up the chain - return completion(std::move(request), - AppUtils::make_clienterror_response(ErrorCodes::ClientRedirectError, - "Redirect response missing location header", - response.http_status_code)); - } - - // Request the location info at the new location - once this is complete, the original - // request will be sent to the new server - update_location_and_resend(std::move(request), std::move(completion), std::move(redir_location)); -} - void App::do_authenticated_request(HttpMethod method, std::string&& route, std::string&& body, const std::shared_ptr& user, RequestTokenType token_type, util::UniqueFunction&& completion) diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp index 3fe735ca64..ddee73aecc 100644 --- a/src/realm/object-store/sync/app.hpp +++ b/src/realm/object-store/sync/app.hpp @@ -525,12 +525,8 @@ class App : public std::enable_shared_from_this, /// a new hostname is provided, the app metadata will be refreshed using the new hostname. /// @param completion The callback that will be called with the error on failure or empty on success /// @param new_hostname The (Original) new hostname to request the location from - /// @param redir_location The location provided by the last redirect response when querying location - /// @param redirect_count The current number of redirects that have occurred in a row - void request_location(util::UniqueFunction)>&& completion, - std::optional&& new_hostname = std::nullopt, - std::optional&& redir_location = std::nullopt, int redirect_count = 0) - REQUIRES(!m_route_mutex); + void request_location(util::UniqueFunction)>&& completion, + std::optional&& new_hostname) REQUIRES(!m_route_mutex); /// Update the location metadata from the location response /// @param response The response returned from the location request @@ -542,9 +538,8 @@ class App : public std::enable_shared_from_this, /// Update the app metadata and resend the request with the updated metadata /// @param request The original request object that needs to be sent after the update /// @param completion The original completion object that will be called with the response to the request - /// @param new_hostname If provided, the metadata will be requested from this hostname - void update_location_and_resend(std::unique_ptr&& request, IntermediateCompletion&& completion, - std::optional&& new_hostname = util::none) REQUIRES(!m_route_mutex); + void update_location_and_resend(std::unique_ptr&& request, IntermediateCompletion&& completion) + REQUIRES(!m_route_mutex); void post(std::string&& route, util::UniqueFunction)>&& completion, const bson::BsonDocument& body) REQUIRES(!m_route_mutex); @@ -559,13 +554,6 @@ class App : public std::enable_shared_from_this, std::unique_ptr make_request(HttpMethod method, std::string&& url, const std::shared_ptr& user, RequestTokenType, std::string&& body) const; - /// Process the redirect response received from the last request that was sent to the server - /// @param request The request to be performed (in case it needs to be sent again) - /// @param response The response from the send_request_to_server operation - /// @param completion Returns the response from the server if not a redirect - void check_for_redirect_response(std::unique_ptr&& request, const Response& response, - IntermediateCompletion&& completion) REQUIRES(!m_route_mutex); - void do_authenticated_request(HttpMethod, std::string&& route, std::string&& body, const std::shared_ptr& user, RequestTokenType, util::UniqueFunction&&) override REQUIRES(!m_route_mutex); diff --git a/src/realm/object-store/sync/app_utils.cpp b/src/realm/object-store/sync/app_utils.cpp index 2226496ea8..82e1074699 100644 --- a/src/realm/object-store/sync/app_utils.cpp +++ b/src/realm/object-store/sync/app_utils.cpp @@ -50,28 +50,6 @@ bool AppUtils::is_success_status_code(int status_code) return status_code == 0 || (status_code < 300 && status_code >= 200); } -bool AppUtils::is_redirect_status_code(int status_code) -{ - using namespace realm::sync; - // If the response contains a redirection, then return true - if (auto code = HTTPStatus(status_code); - code == HTTPStatus::MovedPermanently || code == HTTPStatus::PermanentRedirect) { - return true; - } - return false; -} - -std::optional AppUtils::extract_redir_location(const std::map& headers) -{ - // Look for case insensitive redirect "location" in headers - auto location = AppUtils::find_header("location", headers); - if (location && !location->second.empty() && util::Uri::try_parse(location->second).is_ok()) { - // If the location is valid, return it wholesale (e.g., it could include a path for API proxies) - return location->second; - } - return std::nullopt; -} - // Create a Response object with the given client error, message and optional http status code Response AppUtils::make_clienterror_response(ErrorCodes::Error code, const std::string_view message, std::optional http_status) diff --git a/src/realm/object-store/sync/app_utils.hpp b/src/realm/object-store/sync/app_utils.hpp index 2f6d1e4f66..371bbb3e45 100644 --- a/src/realm/object-store/sync/app_utils.hpp +++ b/src/realm/object-store/sync/app_utils.hpp @@ -38,8 +38,6 @@ class AppUtils { static const std::pair* find_header(const std::string& key_name, const std::map& search_map); static bool is_success_status_code(int status_code); - static bool is_redirect_status_code(int status_code); - static std::optional extract_redir_location(const std::map& headers); }; } // namespace realm::app From 519a71e80eb76cc7c0ccbf29be5ea962acaaa60b Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 23 Aug 2024 09:28:01 -0400 Subject: [PATCH 4/7] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81045741c7..203316df91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ * Fix crash during client app shutdown when Logger log level is set higher than Info. ([#7969](https://github.com/realm/realm-core/issues/7969), since v13.23.3) ### Breaking changes -* None. +* Removed http 301/308 redirection support from app services operations provided by App. It is assumed that the SDK's http implementation will handle http redirects instead. ([PR #7996](https://github.com/realm/realm-core/pull/7996)) ### Compatibility * Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. If you want to upgrade from an earlier file format version you will have to use RealmCore v13.x.y or earlier. From 8a753e5a2a8eecd50e6f52593013d3fb8e68711b Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Tue, 27 Aug 2024 17:36:22 -0400 Subject: [PATCH 5/7] Updates from review --- src/realm/object-store/sync/app.cpp | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index e66e0ee208..f1b7350fed 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -1176,6 +1176,11 @@ void App::post(std::string&& route, UniqueFunction)>&& c void App::do_request(std::unique_ptr&& request, IntermediateCompletion&& completion, bool update_location) { + // NOTE: Since the calls to `send_request_to_server()` or `upldate_location_and_resend()` do not + // capture a shared_ptr to App as part of their callback, any function that calls `do_request()` + // needs to capture the App as `self = shared_from_this()` to ensure the lifetime of the App + // object is extended until the callback is called after the operation is complete. + // Verify the request URL to make sure it is valid if (auto valid_url = util::Uri::try_parse(request->url); !valid_url.is_ok()) { completion(std::move(request), AppUtils::make_apperror_response( @@ -1258,10 +1263,10 @@ void App::handle_auth_failure(const AppError& error, std::unique_ptr&& // Reissue the request with the new access token request->headers = get_request_headers(user, RequestTokenType::AccessToken); - self->do_request(std::move(request), - [completion = std::move(completion)](auto&&, auto& response) { - completion(response); - }); + self->do_request(std::move(request), [self = self, completion = std::move(completion)]( + auto&&, auto& response) { + completion(response); + }); }); } From 5728e1f88942133c3a77edbd186bc5d670a737fc Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 30 Aug 2024 10:53:47 -0400 Subject: [PATCH 6/7] Updated changelog after release --- CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da755c937e..6cc80c4033 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ * None. ### Breaking changes -* None. +* Removed http 301/308 redirection support from app services operations provided by App. It is assumed that the SDK's http implementation will handle http redirects instead. ([PR #7996](https://github.com/realm/realm-core/pull/7996)) ### Compatibility * Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. If you want to upgrade from an earlier file format version you will have to use RealmCore v13.x.y or earlier. @@ -35,7 +35,7 @@ * Swift API misuse within a callback from core would result in an internal unreachable error rather than the exception being propagated properly ([#7836](https://github.com/realm/realm-core/issues/7836)). ### Breaking changes -* Removed http 301/308 redirection support from app services operations provided by App. It is assumed that the SDK's http implementation will handle http redirects instead. ([PR #7996](https://github.com/realm/realm-core/pull/7996)) +* None. ### Compatibility * Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. If you want to upgrade from an earlier file format version you will have to use RealmCore v13.x.y or earlier. From 3c6229ac63c840eb6b4f12779dae68dc2e9b9477 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker Date: Fri, 30 Aug 2024 10:57:38 -0400 Subject: [PATCH 7/7] Fixed misspelling and updated comment a bit --- src/realm/object-store/sync/app.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index f1b7350fed..6f21614415 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -1176,10 +1176,11 @@ void App::post(std::string&& route, UniqueFunction)>&& c void App::do_request(std::unique_ptr&& request, IntermediateCompletion&& completion, bool update_location) { - // NOTE: Since the calls to `send_request_to_server()` or `upldate_location_and_resend()` do not + // NOTE: Since the calls to `send_request_to_server()` or `update_location_and_resend()` do not // capture a shared_ptr to App as part of their callback, any function that calls `do_request()` - // needs to capture the App as `self = shared_from_this()` to ensure the lifetime of the App - // object is extended until the callback is called after the operation is complete. + // or `do_authenticated_request()` needs to capture the App as `self = shared_from_this()` for + // the completion callback to ensure the lifetime of the App object is extended until the + // callback is called after the operation is complete. // Verify the request URL to make sure it is valid if (auto valid_url = util::Uri::try_parse(request->url); !valid_url.is_ok()) {