From aa346355f3bfb7911d9be7783099f631272d5480 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 23 Aug 2024 00:11:17 -0400 Subject: [PATCH 01/22] 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<Response> { - 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<app::App::UsernamePasswordProviderClient>().register_email( - creds.email, creds.password, [&](util::Optional<app::AppError> 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<Response> { - 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<app::App::UsernamePasswordProviderClient>().register_email( - creds.email, creds.password, [&](util::Optional<app::AppError> 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<Response> { - 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<app::App::UsernamePasswordProviderClient>().register_email( - creds.email, creds.password, [&](util::Optional<app::AppError> error) { - REQUIRE(!error); - }); - } - - SECTION("too many redirects eventually reports an error") { - int request_count = 0; - transport->request_hook = [&](const Request& request) -> std::optional<Response> { - 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<realm::SyncUser> user, util::Optional<app::AppError> 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<Response> { 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<Response> { - 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<int>(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<std::string_view> 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<std::string_view> redir_url = std::nullopt) + void reset(std::string expect_url, std::optional<std::string> url = std::nullopt, + std::optional<std::string> 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<int>(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<int>(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<int>(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<BaseUrlTransport>(); + 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<BaseUrlTransport>(); auto get_config_with_base_url = [&](std::optional<std::string> 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<app::AppError> 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<app::AppError> 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<app::AppError> 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<app::AppError> 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<app::AppError> 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<HookedSocketProvider>(logger, "some user agent"); socket_provider->websocket_connect_func = []() -> std::optional<SocketProviderError> { 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<TestState> 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<int> 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<TestState> { + 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<TestState> { + 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<TestState> 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 <michael.wilkersonbarker@mongodb.com> Date: Fri, 23 Aug 2024 09:12:05 -0400 Subject: [PATCH 02/22] 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<Response> { 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<int>(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 <michael.wilkersonbarker@mongodb.com> Date: Fri, 23 Aug 2024 09:19:54 -0400 Subject: [PATCH 03/22] 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<std::string, util::FlatMap<std::string, SharedApp>> 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>& user, UniqueFunction<void(Opt } } - do_authenticated_request( - HttpMethod::del, url_for_path("/auth/delete"), "", user, RequestTokenType::AccessToken, - [self = shared_from_this(), 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)); - }); + 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>& user, const AppCredentials& credentials, @@ -1054,34 +1052,32 @@ std::string App::get_app_route(const Optional<std::string>& hostname) const } void App::request_location(UniqueFunction<void(std::optional<AppError>)>&& completion, - std::optional<std::string>&& new_hostname, std::optional<std::string>&& 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<std::string>&& 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<void(std::optional<AppError>)>&& 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<AppError> App::update_location(const Response& response, const std return util::none; } -void App::update_location_and_resend(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion, - Optional<std::string>&& redir_location) +void App::update_location_and_resend(std::unique_ptr<Request>&& 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>&& 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<void(Optional<AppError>)>&& completion, const BsonDocument& body) @@ -1213,7 +1177,11 @@ void App::post(std::string&& route, UniqueFunction<void(Optional<AppError>)>&& c void App::do_request(std::unique_ptr<Request>&& 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>&& 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>&& 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>& user, RequestTokenType token_type, util::UniqueFunction<void(const Response&)>&& 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<App>, /// 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<void(std::optional<AppError>)>&& completion, - std::optional<std::string>&& new_hostname = std::nullopt, - std::optional<std::string>&& redir_location = std::nullopt, int redirect_count = 0) - REQUIRES(!m_route_mutex); + void request_location(util::UniqueFunction<void(std::optional<app::AppError>)>&& completion, + std::optional<std::string>&& 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<App>, /// 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>&& request, IntermediateCompletion&& completion, - std::optional<std::string>&& new_hostname = util::none) REQUIRES(!m_route_mutex); + void update_location_and_resend(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion) + REQUIRES(!m_route_mutex); void post(std::string&& route, util::UniqueFunction<void(std::optional<AppError>)>&& completion, const bson::BsonDocument& body) REQUIRES(!m_route_mutex); @@ -559,13 +554,6 @@ class App : public std::enable_shared_from_this<App>, std::unique_ptr<Request> make_request(HttpMethod method, std::string&& url, const std::shared_ptr<User>& 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>&& 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>& user, RequestTokenType, util::UniqueFunction<void(const Response&)>&&) 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<std::string> AppUtils::extract_redir_location(const std::map<std::string, std::string>& 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<int> 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<const std::string, std::string>* find_header(const std::string& key_name, const std::map<std::string, std::string>& search_map); static bool is_success_status_code(int status_code); - static bool is_redirect_status_code(int status_code); - static std::optional<std::string> extract_redir_location(const std::map<std::string, std::string>& headers); }; } // namespace realm::app From 519a71e80eb76cc7c0ccbf29be5ea962acaaa60b Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 23 Aug 2024 09:28:01 -0400 Subject: [PATCH 04/22] 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 <michael.wilkersonbarker@mongodb.com> Date: Tue, 27 Aug 2024 17:36:22 -0400 Subject: [PATCH 05/22] 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<void(Optional<AppError>)>&& c void App::do_request(std::unique_ptr<Request>&& 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<Request>&& // 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 aba04c7add465000b996ef40c015619c37228d97 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Wed, 28 Aug 2024 16:35:36 -0400 Subject: [PATCH 06/22] Updated redirect server to support App request redirects and created redir_server cmd line tool --- test/object-store/CMakeLists.txt | 4 + test/object-store/tools/CMakeLists.txt | 15 ++++ test/object-store/tools/redir_server.cpp | 89 +++++++++++++++++++ .../util/sync/redirect_server.hpp | 68 ++++++++++---- 4 files changed, 161 insertions(+), 15 deletions(-) create mode 100644 test/object-store/tools/CMakeLists.txt create mode 100644 test/object-store/tools/redir_server.cpp diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt index b39deae4ae..84fb1d9a97 100644 --- a/test/object-store/CMakeLists.txt +++ b/test/object-store/CMakeLists.txt @@ -116,6 +116,10 @@ create_coverage_target(generate-coverage ObjectStoreTests) # add_bundled_test(ObjectStoreTests) add_labeled_test(ObjectStoreTests-local objstore-local ObjectStoreTests "~[baas]") +if(REALM_ENABLE_SYNC) + add_subdirectory(tools) +endif() + # Baas server and custom tests are only allowed when REALM_ENABLE_AUTH_TESTS is set if(REALM_ENABLE_SYNC) if(REALM_ENABLE_AUTH_TESTS) diff --git a/test/object-store/tools/CMakeLists.txt b/test/object-store/tools/CMakeLists.txt new file mode 100644 index 0000000000..1781c0db71 --- /dev/null +++ b/test/object-store/tools/CMakeLists.txt @@ -0,0 +1,15 @@ +if(REALM_ENABLE_SYNC) + add_executable(redir_server + redir_server.cpp + ../util/sync/redirect_server.hpp + ) + target_compile_definitions(redir_server PRIVATE + REALM_ENABLE_AUTH_TESTS=1 + ) + target_include_directories(redir_server PRIVATE + ${JSON_INCLUDE_DIR} + ${CMAKE_CURRENT_SOURCE_DIR}/.. + ${CMAKE_CURRENT_SOURCE_DIR}/../.. + ) + target_link_libraries(redir_server PUBLIC ObjectStore TestUtil) +endif() diff --git a/test/object-store/tools/redir_server.cpp b/test/object-store/tools/redir_server.cpp new file mode 100644 index 0000000000..e6a2d5de39 --- /dev/null +++ b/test/object-store/tools/redir_server.cpp @@ -0,0 +1,89 @@ +#include <util/sync/redirect_server.hpp> +#include <realm/util/future.hpp> +#include <realm/util/logger.hpp> + +#include <future> +#include <iostream> +#include <memory> +#include <signal.h> + +using namespace realm; + +static std::shared_ptr<std::promise<void>> s_program_promise; +static std::shared_ptr<util::Logger> s_logger; + +static void signalHandler(int signum) +{ + std::cerr << "Interrupt signal (" << signum << ") received.\n"; + + // Only complete promise one time + if (s_program_promise) { + s_program_promise->set_value(); + s_program_promise.reset(); + } + + exit(signum); +} + +static void print_usage(std::string_view executable, std::optional<std::string> error = std::nullopt) +{ + if (error) { + std::cerr << executable << " failed: " << *error << std::endl; + } + std::cout << "usage: " << executable << " [-h|--help] [REDIRECT_URL [LISTEN_PORT]]\n" << std::endl; + exit(error ? 1 : 0); +} + +int main(int argc, char* argv[]) +{ + // redir_server [-h|--help] [REDIRECT_URL [LISTEN_PORT]] + int port = 0; + std::string redir_url = "http://localhost:9090"; + + // Parse the argument list + constexpr int MAX_ARGS = 2; + + int arg_cnt = std::min(argc - 1, MAX_ARGS); + std::string args[arg_cnt]; + for (int i = 0; i < arg_cnt; i++) { + args[i] = argv[i + 1]; + if (args[i] == "-h" || args[i] == "--help") { + print_usage(argv[0]); + } + } + if (arg_cnt > 0) { + redir_url = args[0]; + if (redir_url.empty()) { + print_usage(argv[0], "REDIRECT_URL argument was empty"); + } + } + if (arg_cnt > 1) { + port = atoi(args[1].c_str()); + if (port <= 0 || port > 0xFFFF) { + print_usage(argv[0], util::format("invalid LISTEN_PORT value: %1", port)); + } + } + s_logger = std::make_shared<util::StderrLogger>(util::Logger::Level::debug); + + // register signal SIGINT, SIGTERM, and signal handler + signal(SIGINT, signalHandler); + signal(SIGTERM, signalHandler); + + s_program_promise = std::make_shared<std::promise<void>>(); + auto future = s_program_promise->get_future(); + + try { + auto server = sync::RedirectingHttpServer(redir_url, port, s_logger); + std::cout << "=====================================================\n"; + std::cout << "* Server listen port: " << server.base_url() << "\n"; + std::cout << "* Redirect base url: " << redir_url << "\n"; + std::cout << "=====================================================\n" << std::endl; + future.get(); + } + catch (std::exception e) { + std::cerr << "Error running server: " << e.what() << std::endl; + return 1; + } + + return 0; +} diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index 6c3adbccf6..77ab546466 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -24,18 +24,25 @@ #include <realm/sync/network/network.hpp> #include <realm/sync/network/websocket.hpp> #include <realm/util/future.hpp> +#include <realm/util/random.hpp> #include <realm/util/uri.hpp> #include <external/json/json.hpp> -#include <catch2/catch_all.hpp> - #include <thread> namespace realm::sync { class RedirectingHttpServer { public: + // Allow the redirecting server to choose the listen port RedirectingHttpServer(std::string redirect_to_base_url, std::shared_ptr<util::Logger> logger) + : RedirectingHttpServer(std::move(redirect_to_base_url), 0, std::move(logger)) + { + } + + // Specify the listen port to use + RedirectingHttpServer(std::string redirect_to_base_url, uint16_t listen_port, + std::shared_ptr<util::Logger> logger) : m_redirect_to_base_url(std::move(redirect_to_base_url)) , m_logger(std::make_shared<util::PrefixLogger>("HTTP Redirector ", std::move(logger))) , m_acceptor(m_service) @@ -43,9 +50,15 @@ class RedirectingHttpServer { m_service.run_until_stopped(); }) { - m_acceptor.open(m_endpoint.protocol()); - m_acceptor.bind(m_endpoint); - m_endpoint = m_acceptor.local_endpoint(); + network::Endpoint ep; + if (listen_port != 0) { + // If a port is provided, then configure the endpoint with this port num + ep = network::Endpoint(network::make_address("0.0.0.0"), listen_port); + } + m_acceptor.open(ep.protocol()); + m_acceptor.bind(ep); + ep = m_acceptor.local_endpoint(); + m_listen_port = ep.port(); m_acceptor.listen(); m_service.post([this](Status status) { REALM_ASSERT(status.is_ok()); @@ -62,7 +75,7 @@ class RedirectingHttpServer { std::string base_url() const { - return util::format("http://localhost:%1", m_endpoint.port()); + return util::format("http://localhost:%1", m_listen_port); } private: @@ -97,11 +110,11 @@ class RedirectingHttpServer { struct Conn : public util::RefCountBase, websocket::Config { Conn(network::Service& service, const std::shared_ptr<util::Logger>& logger) - : random(Catch::getSeed()) - , logger(logger) + : logger(logger) , socket(service) , http_server(socket, logger) { + util::seed_prng_nondeterministically(random); // Throws } // Implement the websocket::Config interface @@ -174,15 +187,22 @@ class RedirectingHttpServer { }; void send_simple_response(util::bind_ptr<Conn> conn, HTTPStatus status, std::string reason, std::string body) + { + send_http_response(conn, status, std::move(reason), {}, std::move(body)); + } + + void send_http_response(util::bind_ptr<Conn> conn, HTTPStatus status, std::string reason, HTTPHeaders headers, + std::string body) { m_logger->debug("sending http response %1: %2 \"%3\"", status, reason, body); HTTPResponse resp; resp.status = status; resp.reason = std::move(reason); + resp.headers = std::move(headers); resp.body = std::move(body); conn->http_server.async_send_response(resp, [this, conn](std::error_code ec) { if (ec && ec != util::error::operation_aborted) { - m_logger->warn("Error sending response: %1", ec); + m_logger->warn("Error sending response: [%1]: %2", ec, ec.message()); } }); } @@ -208,7 +228,7 @@ class RedirectingHttpServer { conn->http_server.async_send_response(*maybe_resp, [this, conn](std::error_code ec) { if (ec) { if (ec != util::error::operation_aborted) { - m_logger->warn("Error sending websocket HTTP upgrade response: %1", ec); + m_logger->warn("Error sending websocket HTTP upgrade response: [%1]: %2", ec, ec.message()); } return; } @@ -233,18 +253,21 @@ class RedirectingHttpServer { } do_accept(); if (ec) { - m_logger->error("Error accepting new connection in: %1", ec); + m_logger->error("Error accepting new connection to %1 [%2]: %3", base_url(), ec, ec.message()); return; } conn->http_server.async_receive_request([this, conn](HTTPRequest req, std::error_code ec) { + static bool use_301 = false; // send 301 on first redirect response if (ec) { if (ec != util::error::operation_aborted) { - m_logger->error("Error receiving HTTP request to redirect: %1", ec); + m_logger->error("Error receiving HTTP request to redirect [%1]: %2", ec, ec.message()); } return; } + m_logger->debug("Received request: %1", req.path); + if (req.path.find("/location") != std::string::npos) { std::string_view base_url(m_redirect_to_base_url); auto scheme = base_url.find("://"); @@ -255,7 +278,8 @@ class RedirectingHttpServer { {"ws_hostname", ws_url}}; auto body_str = body.dump(); - send_simple_response(conn, HTTPStatus::Ok, "Okay", std::move(body_str)); + send_http_response(conn, HTTPStatus::Ok, "Okay", {{"Content-Type", "application/json"}}, + std::move(body_str)); return; } @@ -264,16 +288,30 @@ class RedirectingHttpServer { return; } - send_simple_response(conn, HTTPStatus::NotFound, "Not found", {}); + // Send redirect response for appservices calls + // Starts with 'http' and contains api path + if (req.path.find("/api/client/v2.0/") == 0) { + use_301 = !use_301; + auto status = use_301 ? HTTPStatus::MovedPermanently : HTTPStatus::PermanentRedirect; + auto reason = use_301 ? "Moved Permanently" : "Permanent Redirect"; + auto location = m_redirect_to_base_url + req.path; + auto redirect = util::format("<html><body><p>%1 to <a href=\"%2\">%2</a></p></body></html>", + status, location); + send_http_response(conn, status, reason, {{"location", location}}, redirect); + return; + } + + send_simple_response(conn, HTTPStatus::NotFound, "Not found", + util::format("Not found: %1", req.path)); }); }); } const std::string m_redirect_to_base_url; + uint16_t m_listen_port; const std::shared_ptr<util::Logger> m_logger; network::Service m_service; network::Acceptor m_acceptor; - network::Endpoint m_endpoint; std::thread m_server_thread; }; From e0a971d76a10247a0ae79eab9283d1b5364215df Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Thu, 29 Aug 2024 14:59:25 -0400 Subject: [PATCH 07/22] Added test to verify http redirects using CURL to handle the redirections --- src/realm/object-store/sync/app.cpp | 24 +- src/realm/object-store/sync/app.hpp | 12 + test/object-store/sync/app.cpp | 253 ++++++++++++++++++ test/object-store/tools/redir_server.cpp | 76 ++++-- .../object-store/util/sync/baas_admin_api.cpp | 17 +- .../object-store/util/sync/baas_admin_api.hpp | 4 + .../util/sync/redirect_server.hpp | 121 ++++++++- 7 files changed, 469 insertions(+), 38 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index f1b7350fed..b39fcf3589 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -358,6 +358,7 @@ std::string App::get_ws_host_url() std::string App::make_sync_route(Optional<std::string> ws_host_url) { + // If not providing a new ws_host_url, then use the App's current ws_host_url return util::format("%1%2%3/%4%5", ws_host_url.value_or(m_ws_host_url), s_base_path, s_app_path, m_config.app_id, s_sync_path); } @@ -853,7 +854,8 @@ void App::log_in_with_credentials(const AppCredentials& credentials, const std:: completion(user, error); }); }, - false); + // Update location to make sure we're talking to the latest server before logging in + true); } void App::log_in_with_credentials( @@ -1024,6 +1026,26 @@ std::shared_ptr<User> App::create_fake_user_for_testing(const std::string& user_ return user; } +void App::reset_location_for_testing() +{ + util::CheckedLockGuard guard(m_route_mutex); + m_location_updated = false; + configure_route(m_base_url, ""); +} + +bool App::set_location_for_testing(const std::string& base_url, const std::string& hostname, + const std::string& ws_hostname, std::optional<std::string> sync_route) +{ + // Simulate a location response from the server to update the location info + nlohmann::json body{ + {"deployment_model", "GLOBAL"}, {"location", "US-VA"}, {"hostname", hostname}, {"ws_hostname", ws_hostname}}; + if (sync_route) { + body["sync_route"] = *sync_route; + } + Response response{static_cast<int>(sync::HTTPStatus::Ok), 0, {{"Content-Type", "application/json"}}, body.dump()}; + auto result = update_location(response, base_url); + return !result.has_value(); +} void App::refresh_custom_data(const std::shared_ptr<User>& user, UniqueFunction<void(Optional<AppError>)>&& completion) diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp index ddee73aecc..9046e1b70a 100644 --- a/src/realm/object-store/sync/app.hpp +++ b/src/realm/object-store/sync/app.hpp @@ -192,6 +192,18 @@ class App : public std::enable_shared_from_this<App>, std::shared_ptr<User> create_fake_user_for_testing(const std::string& user_id, const std::string& access_token, const std::string& refresh_token) REQUIRES(!m_user_mutex); + // For testing only + // Reset the location_updated flag so the next App request will request the location again + void reset_location_for_testing() REQUIRES(!m_route_mutex); + + // For testing only + // Manually set the base_url and the location and ws_hostname as if it was returned from a + // successful request of the server's location endpoint. This also sets the location_updated + // flag to true so the location will not be requested again. + bool set_location_for_testing(const std::string& base_url, const std::string& hostname, + const std::string& ws_hostname, + std::optional<std::string> sync_route = std::nullopt) REQUIRES(!m_route_mutex); + // MARK: - Provider Clients /// A struct representing a user API key as returned by the App server. diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index a50edecf09..684e7f7f07 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3248,6 +3248,259 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") { } } +TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { + auto logger = util::Logger::get_default_logger(); + auto app_session = get_runtime_app_session(); + + // Skip this test if not using the redirect server + if (!get_redirector()) + return; + + util::ScopeExit cleanup([&]() noexcept { + if (auto& director = get_redirector(); director) { + // Reset the redirector state when the test exits + director->reset_state(); + } + }); + + std::mutex counter_mutex; + int error_count = 0; + int location_count = 0; + int redirect_count = 0; + int wsredirect_count = 0; + using RedirectEvent = sync::RedirectingHttpServer::Event; + get_redirector()->set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { + std::lock_guard lk(counter_mutex); + switch (event) { + case RedirectEvent::location: + location_count++; + logger->debug("Redirector event: location - count: %1", location_count); + return; + case RedirectEvent::redirect: + redirect_count++; + logger->debug("Redirector event: redirect - count: %1", redirect_count); + return; + case RedirectEvent::ws_redirect: + wsredirect_count++; + logger->debug("Redirector event: ws_redirect - count: %1", wsredirect_count); + return; + case RedirectEvent::error: + error_count++; + logger->error("Redirect server received error: %1", message.value_or("unknown error")); + return; + } + }); + + struct CounterValue { + bool greater_than; + int value = 0; + CounterValue(bool greater_than, int value) + : greater_than(greater_than) + , value(value) + { + } + CounterValue(int value) + : CounterValue(false, value) + { + } + }; + + auto check_value = [](int value, const CounterValue& check, std::string_view name) { + INFO(util::format("Checking '%1' counter value", name)); + if (check.greater_than) + REQUIRE(value > check.value); + else + REQUIRE(value == check.value); + }; + + auto reset_counters = [&] { + std::lock_guard lk(counter_mutex); + error_count = 0; + location_count = 0; + redirect_count = 0; + wsredirect_count = 0; + }; + + auto check_counters = [&](CounterValue locations, CounterValue redirects, CounterValue wsredirects, + CounterValue errors) { + std::lock_guard lk(counter_mutex); + check_value(location_count, locations, "locations"); + check_value(redirect_count, redirects, "redirects"); + check_value(wsredirect_count, wsredirects, "ws redirects"); + check_value(error_count, errors, "errors"); + }; + + TestAppSession session(app_session); + auto app = session.app(); + + // Make sure the location response points to the actual server + get_redirector()->force_http_redirect(false); + get_redirector()->force_websocket_redirect(false); + // We should have already requested the location when the user was logged in + // when the session was created. + auto user1 = app->current_user(); + REQUIRE(user1); + // Expected location requested 1 time for the original location request, + // all others 0 since location request prior to login hits actual server + check_counters({1}, {0}, {0}, {0}); + REQUIRE(app->get_base_url() == get_redirector()->base_url()); + REQUIRE(app->get_host_url() == get_redirector()->location_hostname()); + + // Switch the location to use the redirector's address for http requests which will + // return redirect responses to redirect the request to the actual server + get_redirector()->force_http_redirect(true); + get_redirector()->force_websocket_redirect(false); + reset_counters(); + // Reset the location flag and the cached location info so the app will request + // the location from the original base URL again upon the next appservices request. + app->reset_location_for_testing(); + // Email registration should complete successfully + AutoVerifiedEmailCredentials creds; + { + auto pf = util::make_promise_future<void>(); + app->provider_client<app::App::UsernamePasswordProviderClient>().register_email( + creds.email, creds.password, + [promise = util::CopyablePromiseHolder<void>(std::move(pf.promise))]( + util::Optional<app::AppError> error) mutable { + if (error) { + promise.get_promise().set_error(error->to_status()); + return; + } + promise.get_promise().emplace_value(); + }); + REQUIRE(pf.future.get_no_throw().is_ok()); + } + // Login should fail since the profile() command does not complete successfully due + // to the authorization headers being stripped from the redirected request + REQUIRE_FALSE(session.log_in_user(creds).is_ok()); + // User was originally logged in, but logged out when profile request failed and + // app's current user was not updated + auto user2 = app->current_user(); + REQUIRE(user2->is_logged_in()); + REQUIRE(user1 == user2); + // Expected location requested 2 times, and at least 1 redirects, all others 0 + check_counters({2}, {true, 1}, {0}, {0}); + REQUIRE(app->get_base_url() == get_redirector()->base_url()); + REQUIRE(app->get_host_url() == get_redirector()->base_url()); + + // Revert the location to point to the actual server's address so the login + // will complete successfully. + get_redirector()->force_http_redirect(false); + get_redirector()->force_websocket_redirect(false); + reset_counters(); + // Log in will refresh the location prior to performing the login + auto result = session.log_in_user(creds); + REQUIRE(result.is_ok()); + // Since the log in completed successfully, app's current user was updated to + // the new user. + auto user3 = result.get_value(); + REQUIRE(user3); + REQUIRE(user3->is_logged_in()); + REQUIRE(user3 == app->current_user()); + REQUIRE(user3 != user2); + // Expected location requested 1 time for location prior to login, all others 0 + check_counters({1}, {0}, {0}, {0}); + REQUIRE(app->get_base_url() == get_redirector()->base_url()); + REQUIRE(app->get_host_url() == get_redirector()->server_url()); + + + auto get_dogs = [](SharedRealm r) -> Results { + wait_for_upload(*r, std::chrono::seconds(10)); + wait_for_download(*r, std::chrono::seconds(10)); + return Results(r, r->read_group().get_table("class_Dog")); + }; + + auto create_one_dog = [](SharedRealm r) { + r->begin_transaction(); + CppContext c; + Object::create(c, r, "Dog", + std::any(AnyDict{{"_id", std::any(ObjectId::gen())}, + {"breed", std::string("bulldog")}, + {"name", std::string("fido")}}), + CreatePolicy::ForceCreate); + r->commit_transaction(); + }; + + const auto schema = get_default_schema(); + const auto partition = random_string(100); + // First websocket connection is not using redirection. Should connect + // directly to the actual server + { + reset_counters(); + SyncTestFile config(user1, partition, schema); + auto r = Realm::get_shared_realm(config); + REQUIRE(get_dogs(r).size() == 0); + create_one_dog(r); + REQUIRE(get_dogs(r).size() == 1); + // The redirect server is not expected to be used... + check_counters({0}, {0}, {0}, {0}); + } + // Switch the location to use the redirector's address for websocket requests which will + // return the 4003 redirect close code, forcing app to update the location and refresh + // the access token. + get_redirector()->force_websocket_redirect(true); + // Since app uses the hostname value returned from the last location response to create + // the server URL for requesting the location, the first location request (due to the + // location_updated flag being reset) needs to return the redirect server for both + // hostname and ws_hostname. When the location is requested a second time due to the + // login request, the location response should include the actual server for the + // hostname (so the login is successful) and the redirect server for the ws_hostname + // so the websocket initially connects to the redirect server. + { + auto& redirector = get_redirector(); + redirector->force_http_redirect(true); + redirector->set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { + std::lock_guard lk(counter_mutex); + switch (event) { + case RedirectEvent::location: + location_count++; + logger->debug("Redirector event: location - count: %1", location_count); + if (location_count == 1) + // No longer sending redirect server as location hostname value + redirector->force_http_redirect(false); + return; + case RedirectEvent::redirect: + redirect_count++; + logger->debug("Redirector event: redirect - count: %1", redirect_count); + return; + case RedirectEvent::ws_redirect: + wsredirect_count++; + logger->debug("Redirector event: ws_redirect - count: %1", wsredirect_count); + return; + case RedirectEvent::error: + error_count++; + logger->error("Redirect server received error: %1", message.value_or("unknown error")); + return; + } + }); + } + + { + reset_counters(); + // Reset the location flag and the cached location info so the app will request + // the location from the original base URL again upon the next appservices request. + app->reset_location_for_testing(); + // Create a new user and log in to update the location info + // and start with a new realm + auto result = session.create_user_and_log_in(); + REQUIRE(result.is_ok()); + // The location should have been requested twice; once since the location_updated + // flag was reset and a second time for the login request. One redirect occurred + // for the register_email request, since that was sent to the redirect server. + check_counters({2}, {1}, {0}, {0}); + reset_counters(); + SyncTestFile config(app->current_user(), partition, schema); + auto r = Realm::get_shared_realm(config); + Results dogs = get_dogs(r); + REQUIRE(dogs.size() == 1); + REQUIRE(dogs.get(0).get<String>("breed") == "bulldog"); + REQUIRE(dogs.get(0).get<String>("name") == "fido"); + // The location should have been requested again and the websocket should have + // been hit, which sent the redirect close code. + check_counters({0}, {0}, {1}, {0}); + } +} + TEST_CASE("app: sync logs contain baas coid", "[sync][app][baas]") { class InMemoryLogger : public util::Logger { public: diff --git a/test/object-store/tools/redir_server.cpp b/test/object-store/tools/redir_server.cpp index e6a2d5de39..591ef42b8d 100644 --- a/test/object-store/tools/redir_server.cpp +++ b/test/object-store/tools/redir_server.cpp @@ -30,7 +30,9 @@ static void print_usage(std::string_view executable, std::optional<std::string> if (error) { std::cerr << executable << " failed: " << *error << std::endl; } - std::cout << "usage: " << executable << " [-h|--help] [REDIRECT_URL [LISTEN_PORT]]\n" << std::endl; + std::cout << "usage: " << executable + << " [-h|--help] [-r|--http-redirect] [-w|--ws-redirect] [REDIRECT_URL [LISTEN_PORT]]\n" + << std::endl; exit(error ? 1 : 0); } @@ -38,29 +40,44 @@ int main(int argc, char* argv[]) { // redir_server [-h|--help] [REDIRECT_URL [LISTEN_PORT]] int port = 0; - std::string redir_url = "http://localhost:9090"; - - // Parse the argument list - constexpr int MAX_ARGS = 2; - - int arg_cnt = std::min(argc - 1, MAX_ARGS); - std::string args[arg_cnt]; - for (int i = 0; i < arg_cnt; i++) { - args[i] = argv[i + 1]; - if (args[i] == "-h" || args[i] == "--help") { - print_usage(argv[0]); - } - } - if (arg_cnt > 0) { - redir_url = args[0]; - if (redir_url.empty()) { - print_usage(argv[0], "REDIRECT_URL argument was empty"); + constexpr std::string_view default_url = "http://localhost:9090"; + bool http_redirect = false; + bool websocket_redirect = false; + std::string redir_url; + int arg_stg_cnt = 0; + for (int i = 1; i < argc; i++) { + std::string arg = argv[i]; + if (arg[0] == '-') { + if (arg == "-h" || arg == "--help") { + print_usage(argv[0]); + } + else if (arg == "-r" || arg == "--http-redirect") { + http_redirect = true; + } + else if (arg == "-w" || arg == "--ws-redirect") { + websocket_redirect = true; + } + else { + print_usage(argv[0], util::format("invalid argument: %1", argv)); + } } - } - if (arg_cnt > 1) { - port = atoi(args[1].c_str()); - if (port <= 0 || port > 0xFFFF) { - print_usage(argv[0], util::format("invalid LISTEN_PORT value: %1", port)); + else { + if (arg_stg_cnt == 0) { + redir_url = arg; + if (redir_url.empty()) { + print_usage(argv[0], "REDIRECT_URL cannot be empty"); + } + } + if (arg_stg_cnt == 1) { + port = atoi(argv[i]); + if (port <= 0 || port > 0xFFFF) { + print_usage(argv[0], util::format("invalid LISTEN_PORT value: %1", port)); + } + } + if (arg_stg_cnt > 1) { + print_usage(argv[0], util::format("invalid argument: %1", arg)); + } + arg_stg_cnt++; } } s_logger = std::make_shared<util::StderrLogger>(util::Logger::Level::debug); @@ -71,12 +88,21 @@ int main(int argc, char* argv[]) s_program_promise = std::make_shared<std::promise<void>>(); auto future = s_program_promise->get_future(); + if (redir_url.empty()) { + redir_url = default_url; + } try { auto server = sync::RedirectingHttpServer(redir_url, port, s_logger); + server.force_http_redirect(http_redirect); + server.force_websocket_redirect(websocket_redirect); std::cout << "=====================================================\n"; - std::cout << "* Server listen port: " << server.base_url() << "\n"; - std::cout << "* Redirect base url: " << redir_url << "\n"; + std::cout << "* Listen port: " << server.base_url() << "\n"; + std::cout << "* Server URL: " << server.server_url() << "\n"; + std::cout << "* Location details:\n"; + std::cout << "* hostname: " << server.location_hostname() << (http_redirect ? " (redirecting)\n" : "\n"); + std::cout << "* ws_hostname: " << server.location_wshostname() + << (websocket_redirect ? " (redirecting)\n" : "\n"); std::cout << "=====================================================\n" << std::endl; future.get(); } diff --git a/test/object-store/util/sync/baas_admin_api.cpp b/test/object-store/util/sync/baas_admin_api.cpp index 6306f279f1..a8eebd6851 100644 --- a/test/object-store/util/sync/baas_admin_api.cpp +++ b/test/object-store/util/sync/baas_admin_api.cpp @@ -332,6 +332,13 @@ app::Response do_http_request(const app::Request& request) list = curl_slist_append(list, header_str.c_str()); } + // Enable redirection, and don't revert POST to GET for 301/302/303 redirects + // Max redirects is 30 by default + curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1); + curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS_STR, "http,https"); + curl_easy_setopt(curl, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL); + + // Set callbacks to write the response headers and data curl_easy_setopt(curl, CURLOPT_HTTPHEADER, list); curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, curl_write_cb); curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); @@ -602,13 +609,19 @@ static std::optional<sync::RedirectingHttpServer>& get_redirector(const std::str }); }; - if (redirector_enabled() && !redirector && !base_url.empty()) { + if (!base_url.empty() && !redirector && redirector_enabled()) { redirector.emplace(base_url, util::Logger::get_default_logger()); } return redirector; } +std::optional<sync::RedirectingHttpServer>& get_redirector() +{ + // Will not create a redirector if the base_url is empty + return get_redirector({}); +} + class BaasaasLauncher : public Catch::EventListenerBase { public: static std::optional<Baasaas>& get_baasaas_holder() @@ -678,7 +691,7 @@ class BaasaasLauncher : public Catch::EventListenerBase { void testRunEnded(Catch::TestRunStats const&) override { - if (auto& redirector = get_redirector({})) { + if (auto& redirector = get_redirector()) { redirector = std::nullopt; } diff --git a/test/object-store/util/sync/baas_admin_api.hpp b/test/object-store/util/sync/baas_admin_api.hpp index f1d075323e..65eacd432a 100644 --- a/test/object-store/util/sync/baas_admin_api.hpp +++ b/test/object-store/util/sync/baas_admin_api.hpp @@ -22,6 +22,7 @@ #ifdef REALM_ENABLE_AUTH_TESTS #include <util/sync/sync_test_utils.hpp> +#include <util/sync/redirect_server.hpp> #include <realm/object-store/property.hpp> #include <realm/object-store/object_schema.hpp> @@ -282,6 +283,9 @@ std::string get_real_base_url(); // your test is talking to. std::string get_admin_url(); +// Returns a reference to the redirector if it is enabled. +std::optional<sync::RedirectingHttpServer>& get_redirector(); + template <typename Factory> inline app::AppConfig get_config(Factory factory, const AppSession& app_session) { diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index 77ab546466..a1b416d6fc 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -28,12 +28,20 @@ #include <realm/util/uri.hpp> #include <external/json/json.hpp> +#include <iostream> #include <thread> namespace realm::sync { class RedirectingHttpServer { public: + enum Event { + error, + location, + redirect, + ws_redirect, + }; + // Allow the redirecting server to choose the listen port RedirectingHttpServer(std::string redirect_to_base_url, std::shared_ptr<util::Logger> logger) : RedirectingHttpServer(std::move(redirect_to_base_url), 0, std::move(logger)) @@ -43,13 +51,15 @@ class RedirectingHttpServer { // Specify the listen port to use RedirectingHttpServer(std::string redirect_to_base_url, uint16_t listen_port, std::shared_ptr<util::Logger> logger) - : m_redirect_to_base_url(std::move(redirect_to_base_url)) + : m_redirect_to_base_url(trim_url(redirect_to_base_url)) + , m_redirect_to_base_wsurl(make_wsurl(m_redirect_to_base_url)) , m_logger(std::make_shared<util::PrefixLogger>("HTTP Redirector ", std::move(logger))) , m_acceptor(m_service) , m_server_thread([this] { m_service.run_until_stopped(); }) { + reset_state(); network::Endpoint ep; if (listen_port != 0) { // If a port is provided, then configure the endpoint with this port num @@ -58,7 +68,8 @@ class RedirectingHttpServer { m_acceptor.open(ep.protocol()); m_acceptor.bind(ep); ep = m_acceptor.local_endpoint(); - m_listen_port = ep.port(); + m_base_url = util::format("http://localhost:%1", ep.port()); + m_base_wsurl = make_wsurl(m_base_url); m_acceptor.listen(); m_service.post([this](Status status) { REALM_ASSERT(status.is_ok()); @@ -73,9 +84,59 @@ class RedirectingHttpServer { m_server_thread.join(); } + void set_event_hook(std::function<void(Event, std::optional<std::string>)> hook) + { + m_hook = hook; + } + + // If true, http (app services) requests will first hit the redirect server and + // receive a redirect response which will contain the location to the actual + // server. + // NOTE: some http transport redirect implementations may strip the authorization + // header from the request after it is redirected and the user will be logged out + // from the client app as a result. + void force_http_redirect(bool remote = true) + { + m_http_redirect = remote; + } + + // If true, websockets will be first directed to the redirect server which will + // return a redirect close code. The client will then update the location by + // querying the actual server location endpoint (from the 'hostname' location + // value) and open a websocket conneciton to the actual server. + // NOTE: the websocket will never connect if both http and websockets are + // redirecting and will just keep getting the redirect close code. + void force_websocket_redirect(bool force = true) + { + m_websocket_redirect = force; + } + + // Reset the state to default (initial) state after test makes updates + void reset_state() + { + m_http_redirect = false; + m_websocket_redirect = false; + m_hook = nullptr; + } + std::string base_url() const { - return util::format("http://localhost:%1", m_listen_port); + return m_base_url; + } + + std::string server_url() const + { + return m_redirect_to_base_url; + } + + std::string location_hostname() const + { + return m_http_redirect ? m_base_url : m_redirect_to_base_url; + } + + std::string location_wshostname() const + { + return m_websocket_redirect ? m_base_wsurl : m_redirect_to_base_wsurl; } private: @@ -203,6 +264,8 @@ class RedirectingHttpServer { conn->http_server.async_send_response(resp, [this, conn](std::error_code ec) { if (ec && ec != util::error::operation_aborted) { m_logger->warn("Error sending response: [%1]: %2", ec, ec.message()); + if (m_hook) + m_hook(Event::error, ec.message()); } }); } @@ -229,6 +292,8 @@ class RedirectingHttpServer { if (ec) { if (ec != util::error::operation_aborted) { m_logger->warn("Error sending websocket HTTP upgrade response: [%1]: %2", ec, ec.message()); + if (m_hook) + m_hook(Event::error, ec.message()); } return; } @@ -237,9 +302,11 @@ class RedirectingHttpServer { conn->websocket->initiate_server_websocket_after_handshake(); static const std::string_view msg("\x0f\xa3Permanently moved"); - conn->websocket->async_write_close(msg.data(), msg.size(), [conn](std::error_code, size_t) { + conn->websocket->async_write_close(msg.data(), msg.size(), [this, conn](std::error_code, size_t) { conn->logger->debug("Sent close frame with move code"); conn->websocket.reset(); + if (m_hook) + m_hook(Event::ws_redirect, std::nullopt); }); }); } @@ -251,6 +318,7 @@ class RedirectingHttpServer { if (ec == util::error::operation_aborted) { return; } + // Allow additonal connections to be accepted do_accept(); if (ec) { m_logger->error("Error accepting new connection to %1 [%2]: %3", base_url(), ec, ec.message()); @@ -269,17 +337,16 @@ class RedirectingHttpServer { m_logger->debug("Received request: %1", req.path); if (req.path.find("/location") != std::string::npos) { - std::string_view base_url(m_redirect_to_base_url); - auto scheme = base_url.find("://"); - auto ws_url = util::format("ws%1", base_url.substr(scheme)); nlohmann::json body{{"deployment_model", "GLOBAL"}, {"location", "US-VA"}, - {"hostname", m_redirect_to_base_url}, - {"ws_hostname", ws_url}}; + {"hostname", location_hostname()}, + {"ws_hostname", location_wshostname()}}; auto body_str = body.dump(); send_http_response(conn, HTTPStatus::Ok, "Okay", {{"Content-Type", "application/json"}}, std::move(body_str)); + if (m_hook) + m_hook(Event::location, std::nullopt); return; } @@ -298,6 +365,8 @@ class RedirectingHttpServer { auto redirect = util::format("<html><body><p>%1 to <a href=\"%2\">%2</a></p></body></html>", status, location); send_http_response(conn, status, reason, {{"location", location}}, redirect); + if (m_hook) + m_hook(Event::redirect, std::nullopt); return; } @@ -307,9 +376,41 @@ class RedirectingHttpServer { }); } + std::string trim_url(std::string_view str) + { + auto p0 = str.data(); + auto p1 = str.data() + str.size(); + // Trim spaces and forward slashes from the end of the string + while (p1 > p0 && (std::isspace(*(p1 - 1)) || *(p1 - 1) == '/')) + --p1; + // Trim spaces from the beginning of the string + while (p0 < p1 && std::isspace(*p0)) + ++p0; + return std::string(p0, p1 - p0); + } + + std::string make_wsurl(std::string base_url) + { + if (base_url.find("http") == 0) { + // Replace the first 4 ('http') characters with 'ws', so we get 'ws://' or 'wss://' + return base_url.replace(0, 4, "ws"); + } + else { + // If no scheme, return the original base_url + return base_url; + } + } + const std::string m_redirect_to_base_url; - uint16_t m_listen_port; + const std::string m_redirect_to_base_wsurl; const std::shared_ptr<util::Logger> m_logger; + + bool m_http_redirect; + bool m_websocket_redirect; + std::string m_base_url; + std::string m_base_wsurl; + std::function<void(Event, std::optional<std::string>)> m_hook; + network::Service m_service; network::Acceptor m_acceptor; std::thread m_server_thread; From 420d56a419e80cbf6c40637e2fffd5ff00eadf5c Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Thu, 29 Aug 2024 15:21:00 -0400 Subject: [PATCH 08/22] Added test sections and print error on create_user_and_login() failure --- test/object-store/sync/app.cpp | 306 ++++++++++++++------------- test/object-store/util/test_file.cpp | 6 + test/object-store/util/test_file.hpp | 1 + 3 files changed, 162 insertions(+), 151 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 684e7f7f07..ea68c74c94 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3330,14 +3330,15 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { check_value(error_count, errors, "errors"); }; - TestAppSession session(app_session); - auto app = session.app(); - // Make sure the location response points to the actual server get_redirector()->force_http_redirect(false); get_redirector()->force_websocket_redirect(false); + + TestAppSession session{app_session, {}, DeleteApp{false}}; + auto app = session.app(); + // We should have already requested the location when the user was logged in - // when the session was created. + // during the session constructor. auto user1 = app->current_user(); REQUIRE(user1); // Expected location requested 1 time for the original location request, @@ -3346,158 +3347,161 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(app->get_base_url() == get_redirector()->base_url()); REQUIRE(app->get_host_url() == get_redirector()->location_hostname()); - // Switch the location to use the redirector's address for http requests which will - // return redirect responses to redirect the request to the actual server - get_redirector()->force_http_redirect(true); - get_redirector()->force_websocket_redirect(false); - reset_counters(); - // Reset the location flag and the cached location info so the app will request - // the location from the original base URL again upon the next appservices request. - app->reset_location_for_testing(); - // Email registration should complete successfully - AutoVerifiedEmailCredentials creds; - { - auto pf = util::make_promise_future<void>(); - app->provider_client<app::App::UsernamePasswordProviderClient>().register_email( - creds.email, creds.password, - [promise = util::CopyablePromiseHolder<void>(std::move(pf.promise))]( - util::Optional<app::AppError> error) mutable { - if (error) { - promise.get_promise().set_error(error->to_status()); - return; - } - promise.get_promise().emplace_value(); - }); - REQUIRE(pf.future.get_no_throw().is_ok()); - } - // Login should fail since the profile() command does not complete successfully due - // to the authorization headers being stripped from the redirected request - REQUIRE_FALSE(session.log_in_user(creds).is_ok()); - // User was originally logged in, but logged out when profile request failed and - // app's current user was not updated - auto user2 = app->current_user(); - REQUIRE(user2->is_logged_in()); - REQUIRE(user1 == user2); - // Expected location requested 2 times, and at least 1 redirects, all others 0 - check_counters({2}, {true, 1}, {0}, {0}); - REQUIRE(app->get_base_url() == get_redirector()->base_url()); - REQUIRE(app->get_host_url() == get_redirector()->base_url()); - - // Revert the location to point to the actual server's address so the login - // will complete successfully. - get_redirector()->force_http_redirect(false); - get_redirector()->force_websocket_redirect(false); - reset_counters(); - // Log in will refresh the location prior to performing the login - auto result = session.log_in_user(creds); - REQUIRE(result.is_ok()); - // Since the log in completed successfully, app's current user was updated to - // the new user. - auto user3 = result.get_value(); - REQUIRE(user3); - REQUIRE(user3->is_logged_in()); - REQUIRE(user3 == app->current_user()); - REQUIRE(user3 != user2); - // Expected location requested 1 time for location prior to login, all others 0 - check_counters({1}, {0}, {0}, {0}); - REQUIRE(app->get_base_url() == get_redirector()->base_url()); - REQUIRE(app->get_host_url() == get_redirector()->server_url()); - - - auto get_dogs = [](SharedRealm r) -> Results { - wait_for_upload(*r, std::chrono::seconds(10)); - wait_for_download(*r, std::chrono::seconds(10)); - return Results(r, r->read_group().get_table("class_Dog")); - }; - - auto create_one_dog = [](SharedRealm r) { - r->begin_transaction(); - CppContext c; - Object::create(c, r, "Dog", - std::any(AnyDict{{"_id", std::any(ObjectId::gen())}, - {"breed", std::string("bulldog")}, - {"name", std::string("fido")}}), - CreatePolicy::ForceCreate); - r->commit_transaction(); - }; - - const auto schema = get_default_schema(); - const auto partition = random_string(100); - // First websocket connection is not using redirection. Should connect - // directly to the actual server - { - reset_counters(); - SyncTestFile config(user1, partition, schema); - auto r = Realm::get_shared_realm(config); - REQUIRE(get_dogs(r).size() == 0); - create_one_dog(r); - REQUIRE(get_dogs(r).size() == 1); - // The redirect server is not expected to be used... - check_counters({0}, {0}, {0}, {0}); - } - // Switch the location to use the redirector's address for websocket requests which will - // return the 4003 redirect close code, forcing app to update the location and refresh - // the access token. - get_redirector()->force_websocket_redirect(true); - // Since app uses the hostname value returned from the last location response to create - // the server URL for requesting the location, the first location request (due to the - // location_updated flag being reset) needs to return the redirect server for both - // hostname and ws_hostname. When the location is requested a second time due to the - // login request, the location response should include the actual server for the - // hostname (so the login is successful) and the redirect server for the ws_hostname - // so the websocket initially connects to the redirect server. - { - auto& redirector = get_redirector(); - redirector->force_http_redirect(true); - redirector->set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { - std::lock_guard lk(counter_mutex); - switch (event) { - case RedirectEvent::location: - location_count++; - logger->debug("Redirector event: location - count: %1", location_count); - if (location_count == 1) - // No longer sending redirect server as location hostname value - redirector->force_http_redirect(false); - return; - case RedirectEvent::redirect: - redirect_count++; - logger->debug("Redirector event: redirect - count: %1", redirect_count); - return; - case RedirectEvent::ws_redirect: - wsredirect_count++; - logger->debug("Redirector event: ws_redirect - count: %1", wsredirect_count); - return; - case RedirectEvent::error: - error_count++; - logger->error("Redirect server received error: %1", message.value_or("unknown error")); - return; - } - }); - } - - { + SECTION("Appservices requests are redirected") { + // Switch the location to use the redirector's address for http requests which will + // return redirect responses to redirect the request to the actual server + get_redirector()->force_http_redirect(true); + get_redirector()->force_websocket_redirect(false); reset_counters(); // Reset the location flag and the cached location info so the app will request // the location from the original base URL again upon the next appservices request. app->reset_location_for_testing(); - // Create a new user and log in to update the location info - // and start with a new realm - auto result = session.create_user_and_log_in(); - REQUIRE(result.is_ok()); - // The location should have been requested twice; once since the location_updated - // flag was reset and a second time for the login request. One redirect occurred - // for the register_email request, since that was sent to the redirect server. - check_counters({2}, {1}, {0}, {0}); + // Email registration should complete successfully + AutoVerifiedEmailCredentials creds; + { + auto pf = util::make_promise_future<void>(); + app->provider_client<app::App::UsernamePasswordProviderClient>().register_email( + creds.email, creds.password, + [promise = util::CopyablePromiseHolder<void>(std::move(pf.promise))]( + util::Optional<app::AppError> error) mutable { + if (error) { + promise.get_promise().set_error(error->to_status()); + return; + } + promise.get_promise().emplace_value(); + }); + REQUIRE(pf.future.get_no_throw().is_ok()); + } + // Login should fail since the profile() command does not complete successfully due + // to the authorization headers being stripped from the redirected request + REQUIRE_FALSE(session.log_in_user(creds).is_ok()); + // User was originally logged in, but logged out when profile request failed and + // app's current user was not updated + auto user2 = app->current_user(); + REQUIRE(user2->is_logged_in()); + REQUIRE(user1 == user2); + // Expected location requested 2 times, and at least 1 redirects, all others 0 + check_counters({2}, {true, 1}, {0}, {0}); + REQUIRE(app->get_base_url() == get_redirector()->base_url()); + REQUIRE(app->get_host_url() == get_redirector()->base_url()); + + // Revert the location to point to the actual server's address so the login + // will complete successfully. + get_redirector()->force_http_redirect(false); + get_redirector()->force_websocket_redirect(false); reset_counters(); - SyncTestFile config(app->current_user(), partition, schema); - auto r = Realm::get_shared_realm(config); - Results dogs = get_dogs(r); - REQUIRE(dogs.size() == 1); - REQUIRE(dogs.get(0).get<String>("breed") == "bulldog"); - REQUIRE(dogs.get(0).get<String>("name") == "fido"); - // The location should have been requested again and the websocket should have - // been hit, which sent the redirect close code. - check_counters({0}, {0}, {1}, {0}); + // Log in will refresh the location prior to performing the login + auto result = session.log_in_user(creds); + REQUIRE(result.is_ok()); + // Since the log in completed successfully, app's current user was updated to + // the new user. + auto user3 = result.get_value(); + REQUIRE(user3); + REQUIRE(user3->is_logged_in()); + REQUIRE(user3 == app->current_user()); + REQUIRE(user3 != user2); + // Expected location requested 1 time for location prior to login, all others 0 + check_counters({1}, {0}, {0}, {0}); + REQUIRE(app->get_base_url() == get_redirector()->base_url()); + REQUIRE(app->get_host_url() == get_redirector()->server_url()); + } + + SECTION("Websocket connection returns redirection") { + auto get_dogs = [](SharedRealm r) -> Results { + wait_for_upload(*r, std::chrono::seconds(10)); + wait_for_download(*r, std::chrono::seconds(10)); + return Results(r, r->read_group().get_table("class_Dog")); + }; + + auto create_one_dog = [](SharedRealm r) { + r->begin_transaction(); + CppContext c; + Object::create(c, r, "Dog", + std::any(AnyDict{{"_id", std::any(ObjectId::gen())}, + {"breed", std::string("bulldog")}, + {"name", std::string("fido")}}), + CreatePolicy::ForceCreate); + r->commit_transaction(); + }; + + const auto schema = get_default_schema(); + const auto partition = random_string(100); + // First websocket connection is not using redirection. Should connect + // directly to the actual server + { + reset_counters(); + SyncTestFile config(user1, partition, schema); + auto r = Realm::get_shared_realm(config); + REQUIRE(get_dogs(r).size() == 0); + create_one_dog(r); + REQUIRE(get_dogs(r).size() == 1); + // The redirect server is not expected to be used... + check_counters({0}, {0}, {0}, {0}); + } + // Switch the location to use the redirector's address for websocket requests which will + // return the 4003 redirect close code, forcing app to update the location and refresh + // the access token. + get_redirector()->force_websocket_redirect(true); + // Since app uses the hostname value returned from the last location response to create + // the server URL for requesting the location, the first location request (due to the + // location_updated flag being reset) needs to return the redirect server for both + // hostname and ws_hostname. When the location is requested a second time due to the + // login request, the location response should include the actual server for the + // hostname (so the login is successful) and the redirect server for the ws_hostname + // so the websocket initially connects to the redirect server. + { + auto& redirector = get_redirector(); + redirector->force_http_redirect(true); + redirector->set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { + std::lock_guard lk(counter_mutex); + switch (event) { + case RedirectEvent::location: + location_count++; + logger->debug("Redirector event: location - count: %1", location_count); + if (location_count == 1) + // No longer sending redirect server as location hostname value + redirector->force_http_redirect(false); + return; + case RedirectEvent::redirect: + redirect_count++; + logger->debug("Redirector event: redirect - count: %1", redirect_count); + return; + case RedirectEvent::ws_redirect: + wsredirect_count++; + logger->debug("Redirector event: ws_redirect - count: %1", wsredirect_count); + return; + case RedirectEvent::error: + error_count++; + logger->error("Redirect server received error: %1", message.value_or("unknown error")); + return; + } + }); + } + + { + reset_counters(); + // Reset the location flag and the cached location info so the app will request + // the location from the original base URL again upon the next appservices request. + app->reset_location_for_testing(); + // Create a new user and log in to update the location info + // and start with a new realm + auto result = session.create_user_and_log_in(); + REQUIRE(result.is_ok()); + // The location should have been requested twice; once since the location_updated + // flag was reset and a second time for the login request. One redirect occurred + // for the register_email request, since that was sent to the redirect server. + check_counters({2}, {1}, {0}, {0}); + reset_counters(); + SyncTestFile config(app->current_user(), partition, schema); + auto r = Realm::get_shared_realm(config); + Results dogs = get_dogs(r); + REQUIRE(dogs.size() == 1); + REQUIRE(dogs.get(0).get<String>("breed") == "bulldog"); + REQUIRE(dogs.get(0).get<String>("name") == "fido"); + // The location should have been requested again and the websocket should have + // been hit, which sent the redirect close code. + check_counters({0}, {0}, {1}, {0}); + } } } diff --git a/test/object-store/util/test_file.cpp b/test/object-store/util/test_file.cpp index 179192f6c5..3cd63c22cd 100644 --- a/test/object-store/util/test_file.cpp +++ b/test/object-store/util/test_file.cpp @@ -375,6 +375,8 @@ TestAppSession::TestAppSession(AppSession session, Config config, DeleteApp dele } m_app = app::App::get_app(app::App::CacheMode::Disabled, app_config); + if (auto logger = m_app->sync_manager()->get_logger()) + m_logger = logger; // initialize sync client m_app->sync_manager()->get_sync_client(); @@ -419,11 +421,15 @@ StatusWith<realm::app::AppCredentials> TestAppSession::create_user_and_log_in() [this, &creds, promise = util::CopyablePromiseHolder<void>(std::move(pf.promise))]( util::Optional<app::AppError> error) mutable { if (error) { + if (m_logger) + m_logger->error("Failed to create user: %1", error->to_status()); promise.get_promise().set_error(error->to_status()); return; } auto result = log_in_user(creds); if (!result.is_ok()) { + if (m_logger) + m_logger->error("User login failed: %1", result.get_status()); promise.get_promise().set_error(result.get_status()); return; } diff --git a/test/object-store/util/test_file.hpp b/test/object-store/util/test_file.hpp index 2eb7b0bb18..a9b2026368 100644 --- a/test/object-store/util/test_file.hpp +++ b/test/object-store/util/test_file.hpp @@ -382,6 +382,7 @@ class TestAppSession { bool m_delete_app; bool m_delete_storage; std::shared_ptr<realm::app::App> m_app; + std::shared_ptr<realm::util::Logger> m_logger; }; #endif // REALM_ENABLE_AUTH_TESTS From a06e319606483194f43d8f0154684f5afe36677d Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Thu, 29 Aug 2024 18:36:24 -0400 Subject: [PATCH 09/22] Updated changelog; some cleanup; moving redir_server tool to separate PR --- CHANGELOG.md | 2 + src/realm/object-store/sync/app.cpp | 14 --- src/realm/object-store/sync/app.hpp | 8 -- test/object-store/CMakeLists.txt | 4 - test/object-store/tools/CMakeLists.txt | 15 --- test/object-store/tools/redir_server.cpp | 115 ------------------ .../util/sync/redirect_server.hpp | 6 +- test/object-store/util/test_file.cpp | 5 +- 8 files changed, 5 insertions(+), 164 deletions(-) delete mode 100644 test/object-store/tools/CMakeLists.txt delete mode 100644 test/object-store/tools/redir_server.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index 203316df91..78ba03ddda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * Using an empty KeyPath in C API would result in no filtering being done ([#7805](https://github.com/realm/realm-core/issues/7805), since 13.24.0) * Filtering notifications with backlink columns as last element could sometimes give wrong results ([#7530](https://github.com/realm/realm-core/issues/7530), since 11.1.0) * 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) +* If a user authenticated request is redirected, the user will be logged out since the authorization header is removed before request is sent to the new server. The log_in_with_credentials() function will always request the location info from the server prior to logging in to ensure the remote server URL is up to date. ([#8012](https://github.com/realm/realm-core/issues/8012), since v12.9.0) ### 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)) @@ -20,6 +21,7 @@ ### Internals * Update TestAppSession to allow scope-based usage for restarting the local app resources. ([PR #7672](https://github.com/realm/realm-core/pull/7672)) +* Updated test http transport to enable redirect support in the curl library and added test to verify real redirect operation using the redirect server. ([PR #8011](https://github.com/realm/realm-core/pull/8011)) ---------------------------------------------- diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index b39fcf3589..9a95fcbfe8 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -1033,20 +1033,6 @@ void App::reset_location_for_testing() configure_route(m_base_url, ""); } -bool App::set_location_for_testing(const std::string& base_url, const std::string& hostname, - const std::string& ws_hostname, std::optional<std::string> sync_route) -{ - // Simulate a location response from the server to update the location info - nlohmann::json body{ - {"deployment_model", "GLOBAL"}, {"location", "US-VA"}, {"hostname", hostname}, {"ws_hostname", ws_hostname}}; - if (sync_route) { - body["sync_route"] = *sync_route; - } - Response response{static_cast<int>(sync::HTTPStatus::Ok), 0, {{"Content-Type", "application/json"}}, body.dump()}; - auto result = update_location(response, base_url); - return !result.has_value(); -} - void App::refresh_custom_data(const std::shared_ptr<User>& user, UniqueFunction<void(Optional<AppError>)>&& completion) { diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp index 9046e1b70a..6509d20ca2 100644 --- a/src/realm/object-store/sync/app.hpp +++ b/src/realm/object-store/sync/app.hpp @@ -196,14 +196,6 @@ class App : public std::enable_shared_from_this<App>, // Reset the location_updated flag so the next App request will request the location again void reset_location_for_testing() REQUIRES(!m_route_mutex); - // For testing only - // Manually set the base_url and the location and ws_hostname as if it was returned from a - // successful request of the server's location endpoint. This also sets the location_updated - // flag to true so the location will not be requested again. - bool set_location_for_testing(const std::string& base_url, const std::string& hostname, - const std::string& ws_hostname, - std::optional<std::string> sync_route = std::nullopt) REQUIRES(!m_route_mutex); - // MARK: - Provider Clients /// A struct representing a user API key as returned by the App server. diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt index 84fb1d9a97..b39deae4ae 100644 --- a/test/object-store/CMakeLists.txt +++ b/test/object-store/CMakeLists.txt @@ -116,10 +116,6 @@ create_coverage_target(generate-coverage ObjectStoreTests) # add_bundled_test(ObjectStoreTests) add_labeled_test(ObjectStoreTests-local objstore-local ObjectStoreTests "~[baas]") -if(REALM_ENABLE_SYNC) - add_subdirectory(tools) -endif() - # Baas server and custom tests are only allowed when REALM_ENABLE_AUTH_TESTS is set if(REALM_ENABLE_SYNC) if(REALM_ENABLE_AUTH_TESTS) diff --git a/test/object-store/tools/CMakeLists.txt b/test/object-store/tools/CMakeLists.txt deleted file mode 100644 index 1781c0db71..0000000000 --- a/test/object-store/tools/CMakeLists.txt +++ /dev/null @@ -1,15 +0,0 @@ -if(REALM_ENABLE_SYNC) - add_executable(redir_server - redir_server.cpp - ../util/sync/redirect_server.hpp - ) - target_compile_definitions(redir_server PRIVATE - REALM_ENABLE_AUTH_TESTS=1 - ) - target_include_directories(redir_server PRIVATE - ${JSON_INCLUDE_DIR} - ${CMAKE_CURRENT_SOURCE_DIR}/.. - ${CMAKE_CURRENT_SOURCE_DIR}/../.. - ) - target_link_libraries(redir_server PUBLIC ObjectStore TestUtil) -endif() diff --git a/test/object-store/tools/redir_server.cpp b/test/object-store/tools/redir_server.cpp deleted file mode 100644 index 591ef42b8d..0000000000 --- a/test/object-store/tools/redir_server.cpp +++ /dev/null @@ -1,115 +0,0 @@ -#include <util/sync/redirect_server.hpp> -#include <realm/util/future.hpp> -#include <realm/util/logger.hpp> - -#include <future> -#include <iostream> -#include <memory> -#include <signal.h> - -using namespace realm; - -static std::shared_ptr<std::promise<void>> s_program_promise; -static std::shared_ptr<util::Logger> s_logger; - -static void signalHandler(int signum) -{ - std::cerr << "Interrupt signal (" << signum << ") received.\n"; - - // Only complete promise one time - if (s_program_promise) { - s_program_promise->set_value(); - s_program_promise.reset(); - } - - exit(signum); -} - -static void print_usage(std::string_view executable, std::optional<std::string> error = std::nullopt) -{ - if (error) { - std::cerr << executable << " failed: " << *error << std::endl; - } - std::cout << "usage: " << executable - << " [-h|--help] [-r|--http-redirect] [-w|--ws-redirect] [REDIRECT_URL [LISTEN_PORT]]\n" - << std::endl; - exit(error ? 1 : 0); -} - -int main(int argc, char* argv[]) -{ - // redir_server [-h|--help] [REDIRECT_URL [LISTEN_PORT]] - int port = 0; - constexpr std::string_view default_url = "http://localhost:9090"; - bool http_redirect = false; - bool websocket_redirect = false; - std::string redir_url; - int arg_stg_cnt = 0; - for (int i = 1; i < argc; i++) { - std::string arg = argv[i]; - if (arg[0] == '-') { - if (arg == "-h" || arg == "--help") { - print_usage(argv[0]); - } - else if (arg == "-r" || arg == "--http-redirect") { - http_redirect = true; - } - else if (arg == "-w" || arg == "--ws-redirect") { - websocket_redirect = true; - } - else { - print_usage(argv[0], util::format("invalid argument: %1", argv)); - } - } - else { - if (arg_stg_cnt == 0) { - redir_url = arg; - if (redir_url.empty()) { - print_usage(argv[0], "REDIRECT_URL cannot be empty"); - } - } - if (arg_stg_cnt == 1) { - port = atoi(argv[i]); - if (port <= 0 || port > 0xFFFF) { - print_usage(argv[0], util::format("invalid LISTEN_PORT value: %1", port)); - } - } - if (arg_stg_cnt > 1) { - print_usage(argv[0], util::format("invalid argument: %1", arg)); - } - arg_stg_cnt++; - } - } - s_logger = std::make_shared<util::StderrLogger>(util::Logger::Level::debug); - - // register signal SIGINT, SIGTERM, and signal handler - signal(SIGINT, signalHandler); - signal(SIGTERM, signalHandler); - - s_program_promise = std::make_shared<std::promise<void>>(); - auto future = s_program_promise->get_future(); - if (redir_url.empty()) { - redir_url = default_url; - } - - try { - auto server = sync::RedirectingHttpServer(redir_url, port, s_logger); - server.force_http_redirect(http_redirect); - server.force_websocket_redirect(websocket_redirect); - std::cout << "=====================================================\n"; - std::cout << "* Listen port: " << server.base_url() << "\n"; - std::cout << "* Server URL: " << server.server_url() << "\n"; - std::cout << "* Location details:\n"; - std::cout << "* hostname: " << server.location_hostname() << (http_redirect ? " (redirecting)\n" : "\n"); - std::cout << "* ws_hostname: " << server.location_wshostname() - << (websocket_redirect ? " (redirecting)\n" : "\n"); - std::cout << "=====================================================\n" << std::endl; - future.get(); - } - catch (std::exception e) { - std::cerr << "Error running server: " << e.what() << std::endl; - return 1; - } - - return 0; -} diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index a1b416d6fc..a1b2487b35 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -256,11 +256,7 @@ class RedirectingHttpServer { std::string body) { m_logger->debug("sending http response %1: %2 \"%3\"", status, reason, body); - HTTPResponse resp; - resp.status = status; - resp.reason = std::move(reason); - resp.headers = std::move(headers); - resp.body = std::move(body); + HTTPResponse resp{status, std::move(reason), std::move(headers), std::move(body)}; conn->http_server.async_send_response(resp, [this, conn](std::error_code ec) { if (ec && ec != util::error::operation_aborted) { m_logger->warn("Error sending response: [%1]: %2", ec, ec.message()); diff --git a/test/object-store/util/test_file.cpp b/test/object-store/util/test_file.cpp index 3cd63c22cd..079861dae2 100644 --- a/test/object-store/util/test_file.cpp +++ b/test/object-store/util/test_file.cpp @@ -375,11 +375,10 @@ TestAppSession::TestAppSession(AppSession session, Config config, DeleteApp dele } m_app = app::App::get_app(app::App::CacheMode::Disabled, app_config); - if (auto logger = m_app->sync_manager()->get_logger()) - m_logger = logger; - // initialize sync client + // initialize sync client and save a local copy of the logger m_app->sync_manager()->get_sync_client(); + m_logger = m_app->sync_manager()->get_logger(); // If no user creds are supplied, then create the user and log in if (!m_config.user_creds) { auto result = create_user_and_log_in(); From 69c232c7a5278d4231cace19bcb2b890da08917b Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Thu, 29 Aug 2024 20:06:23 -0400 Subject: [PATCH 10/22] Addressed some build and test failures --- test/object-store/CMakeLists.txt | 2 +- test/object-store/sync/app.cpp | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt index b39deae4ae..50801f3dec 100644 --- a/test/object-store/CMakeLists.txt +++ b/test/object-store/CMakeLists.txt @@ -167,7 +167,7 @@ if(REALM_ENABLE_SYNC) ) endif() - find_package(CURL REQUIRED) + find_package(CURL 7.85.0 REQUIRED) target_link_libraries(ObjectStoreTestLib "${CURL_LIBRARY}") target_include_directories(ObjectStoreTestLib PRIVATE "${CURL_INCLUDE_DIRS}") endif() diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index ea68c74c94..e244e33408 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3812,7 +3812,13 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { { if (request.url.find("/location") != std::string::npos) { CHECK(request.method == HttpMethod::get); - CHECK_THAT(request.url, ContainsSubstring(expected_url)); + // Location is now requested again when the user logs in - only check the exepected + // url on the first location request - after that, it will be using the location_url + // value when requesting the location. + if (!location_requested) + CHECK_THAT(request.url, ContainsSubstring(expected_url)); + else + CHECK_THAT(request.url, ContainsSubstring(location_url)); location_requested = true; if (location_returns_error) { completion(app::Response{static_cast<int>(sync::HTTPStatus::NotFound), 0, {}, "404 not found"}); From 3c0cd07f36f2dfa85c26e59aae8c81a56b297f26 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Thu, 29 Aug 2024 20:29:28 -0400 Subject: [PATCH 11/22] More minor updates to fix build and test failures --- test/object-store/CMakeLists.txt | 2 +- test/object-store/sync/app.cpp | 5 ++--- test/object-store/util/sync/baas_admin_api.cpp | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt index 50801f3dec..b39deae4ae 100644 --- a/test/object-store/CMakeLists.txt +++ b/test/object-store/CMakeLists.txt @@ -167,7 +167,7 @@ if(REALM_ENABLE_SYNC) ) endif() - find_package(CURL 7.85.0 REQUIRED) + find_package(CURL REQUIRED) target_link_libraries(ObjectStoreTestLib "${CURL_LIBRARY}") target_include_directories(ObjectStoreTestLib PRIVATE "${CURL_INCLUDE_DIRS}") endif() diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index e244e33408..9fda5b2f11 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3257,7 +3257,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { return; util::ScopeExit cleanup([&]() noexcept { - if (auto& director = get_redirector(); director) { + if (auto& director = get_redirector()) { // Reset the redirector state when the test exits director->reset_state(); } @@ -3345,7 +3345,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // all others 0 since location request prior to login hits actual server check_counters({1}, {0}, {0}, {0}); REQUIRE(app->get_base_url() == get_redirector()->base_url()); - REQUIRE(app->get_host_url() == get_redirector()->location_hostname()); + REQUIRE(app->get_host_url() == get_redirector()->server_url()); SECTION("Appservices requests are redirected") { // Switch the location to use the redirector's address for http requests which will @@ -3477,7 +3477,6 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { } }); } - { reset_counters(); // Reset the location flag and the cached location info so the app will request diff --git a/test/object-store/util/sync/baas_admin_api.cpp b/test/object-store/util/sync/baas_admin_api.cpp index a8eebd6851..6b23817db1 100644 --- a/test/object-store/util/sync/baas_admin_api.cpp +++ b/test/object-store/util/sync/baas_admin_api.cpp @@ -335,7 +335,6 @@ app::Response do_http_request(const app::Request& request) // Enable redirection, and don't revert POST to GET for 301/302/303 redirects // Max redirects is 30 by default curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1); - curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS_STR, "http,https"); curl_easy_setopt(curl, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL); // Set callbacks to write the response headers and data From 5728e1f88942133c3a77edbd186bc5d670a737fc Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 30 Aug 2024 10:53:47 -0400 Subject: [PATCH 12/22] 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 <michael.wilkersonbarker@mongodb.com> Date: Fri, 30 Aug 2024 10:57:38 -0400 Subject: [PATCH 13/22] 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<void(Optional<AppError>)>&& c void App::do_request(std::unique_ptr<Request>&& 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()) { From ebcfe93386319389a72e070b689fd700476d41d6 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Wed, 4 Sep 2024 12:42:32 -0400 Subject: [PATCH 14/22] Updates from review - removed some changes needed by redirect server command line tool --- test/object-store/sync/app.cpp | 52 +++++------- .../object-store/util/sync/baas_admin_api.cpp | 10 +-- .../object-store/util/sync/baas_admin_api.hpp | 4 - .../util/sync/redirect_server.hpp | 85 +++++-------------- 4 files changed, 47 insertions(+), 104 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 9fda5b2f11..c15a58a151 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -18,6 +18,7 @@ #include "collection_fixtures.hpp" #include "util/sync/baas_admin_api.hpp" +#include "util/sync/redirect_server.hpp" #include "util/sync/sync_test_utils.hpp" #include "util/test_path.hpp" #include "util/unit_test_transport.hpp" @@ -3253,23 +3254,14 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { auto app_session = get_runtime_app_session(); // Skip this test if not using the redirect server - if (!get_redirector()) - return; - - util::ScopeExit cleanup([&]() noexcept { - if (auto& director = get_redirector()) { - // Reset the redirector state when the test exits - director->reset_state(); - } - }); - + auto redirector = sync::RedirectingHttpServer(get_real_base_url(), logger); std::mutex counter_mutex; int error_count = 0; int location_count = 0; int redirect_count = 0; int wsredirect_count = 0; using RedirectEvent = sync::RedirectingHttpServer::Event; - get_redirector()->set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { + redirector.set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { std::lock_guard lk(counter_mutex); switch (event) { case RedirectEvent::location: @@ -3331,10 +3323,13 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { }; // Make sure the location response points to the actual server - get_redirector()->force_http_redirect(false); - get_redirector()->force_websocket_redirect(false); + redirector.force_http_redirect(false); + redirector.force_websocket_redirect(false); + + auto tas_config = TestAppSession::Config{}; + tas_config.base_url = redirector.base_url(); - TestAppSession session{app_session, {}, DeleteApp{false}}; + TestAppSession session{app_session, tas_config, DeleteApp{false}}; auto app = session.app(); // We should have already requested the location when the user was logged in @@ -3344,14 +3339,14 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // Expected location requested 1 time for the original location request, // all others 0 since location request prior to login hits actual server check_counters({1}, {0}, {0}, {0}); - REQUIRE(app->get_base_url() == get_redirector()->base_url()); - REQUIRE(app->get_host_url() == get_redirector()->server_url()); + REQUIRE(app->get_base_url() == redirector.base_url()); + REQUIRE(app->get_host_url() == redirector.server_url()); SECTION("Appservices requests are redirected") { // Switch the location to use the redirector's address for http requests which will // return redirect responses to redirect the request to the actual server - get_redirector()->force_http_redirect(true); - get_redirector()->force_websocket_redirect(false); + redirector.force_http_redirect(true); + redirector.force_websocket_redirect(false); reset_counters(); // Reset the location flag and the cached location info so the app will request // the location from the original base URL again upon the next appservices request. @@ -3382,13 +3377,13 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(user1 == user2); // Expected location requested 2 times, and at least 1 redirects, all others 0 check_counters({2}, {true, 1}, {0}, {0}); - REQUIRE(app->get_base_url() == get_redirector()->base_url()); - REQUIRE(app->get_host_url() == get_redirector()->base_url()); + REQUIRE(app->get_base_url() == redirector.base_url()); + REQUIRE(app->get_host_url() == redirector.base_url()); // Revert the location to point to the actual server's address so the login // will complete successfully. - get_redirector()->force_http_redirect(false); - get_redirector()->force_websocket_redirect(false); + redirector.force_http_redirect(false); + redirector.force_websocket_redirect(false); reset_counters(); // Log in will refresh the location prior to performing the login auto result = session.log_in_user(creds); @@ -3402,8 +3397,8 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(user3 != user2); // Expected location requested 1 time for location prior to login, all others 0 check_counters({1}, {0}, {0}, {0}); - REQUIRE(app->get_base_url() == get_redirector()->base_url()); - REQUIRE(app->get_host_url() == get_redirector()->server_url()); + REQUIRE(app->get_base_url() == redirector.base_url()); + REQUIRE(app->get_host_url() == redirector.server_url()); } SECTION("Websocket connection returns redirection") { @@ -3441,7 +3436,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // Switch the location to use the redirector's address for websocket requests which will // return the 4003 redirect close code, forcing app to update the location and refresh // the access token. - get_redirector()->force_websocket_redirect(true); + redirector.force_websocket_redirect(true); // Since app uses the hostname value returned from the last location response to create // the server URL for requesting the location, the first location request (due to the // location_updated flag being reset) needs to return the redirect server for both @@ -3450,9 +3445,8 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // hostname (so the login is successful) and the redirect server for the ws_hostname // so the websocket initially connects to the redirect server. { - auto& redirector = get_redirector(); - redirector->force_http_redirect(true); - redirector->set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { + redirector.force_http_redirect(true); + redirector.set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { std::lock_guard lk(counter_mutex); switch (event) { case RedirectEvent::location: @@ -3460,7 +3454,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { logger->debug("Redirector event: location - count: %1", location_count); if (location_count == 1) // No longer sending redirect server as location hostname value - redirector->force_http_redirect(false); + redirector.force_http_redirect(false); return; case RedirectEvent::redirect: redirect_count++; diff --git a/test/object-store/util/sync/baas_admin_api.cpp b/test/object-store/util/sync/baas_admin_api.cpp index 6b23817db1..c254eabdb6 100644 --- a/test/object-store/util/sync/baas_admin_api.cpp +++ b/test/object-store/util/sync/baas_admin_api.cpp @@ -608,19 +608,13 @@ static std::optional<sync::RedirectingHttpServer>& get_redirector(const std::str }); }; - if (!base_url.empty() && !redirector && redirector_enabled()) { + if (redirector_enabled() && !redirector && !base_url.empty()) { redirector.emplace(base_url, util::Logger::get_default_logger()); } return redirector; } -std::optional<sync::RedirectingHttpServer>& get_redirector() -{ - // Will not create a redirector if the base_url is empty - return get_redirector({}); -} - class BaasaasLauncher : public Catch::EventListenerBase { public: static std::optional<Baasaas>& get_baasaas_holder() @@ -690,7 +684,7 @@ class BaasaasLauncher : public Catch::EventListenerBase { void testRunEnded(Catch::TestRunStats const&) override { - if (auto& redirector = get_redirector()) { + if (auto& redirector = get_redirector({})) { redirector = std::nullopt; } diff --git a/test/object-store/util/sync/baas_admin_api.hpp b/test/object-store/util/sync/baas_admin_api.hpp index 65eacd432a..f1d075323e 100644 --- a/test/object-store/util/sync/baas_admin_api.hpp +++ b/test/object-store/util/sync/baas_admin_api.hpp @@ -22,7 +22,6 @@ #ifdef REALM_ENABLE_AUTH_TESTS #include <util/sync/sync_test_utils.hpp> -#include <util/sync/redirect_server.hpp> #include <realm/object-store/property.hpp> #include <realm/object-store/object_schema.hpp> @@ -283,9 +282,6 @@ std::string get_real_base_url(); // your test is talking to. std::string get_admin_url(); -// Returns a reference to the redirector if it is enabled. -std::optional<sync::RedirectingHttpServer>& get_redirector(); - template <typename Factory> inline app::AppConfig get_config(Factory factory, const AppSession& app_session) { diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index a1b2487b35..42ba3450e9 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -44,14 +44,7 @@ class RedirectingHttpServer { // Allow the redirecting server to choose the listen port RedirectingHttpServer(std::string redirect_to_base_url, std::shared_ptr<util::Logger> logger) - : RedirectingHttpServer(std::move(redirect_to_base_url), 0, std::move(logger)) - { - } - - // Specify the listen port to use - RedirectingHttpServer(std::string redirect_to_base_url, uint16_t listen_port, - std::shared_ptr<util::Logger> logger) - : m_redirect_to_base_url(trim_url(redirect_to_base_url)) + : m_redirect_to_base_url{redirect_to_base_url} , m_redirect_to_base_wsurl(make_wsurl(m_redirect_to_base_url)) , m_logger(std::make_shared<util::PrefixLogger>("HTTP Redirector ", std::move(logger))) , m_acceptor(m_service) @@ -59,12 +52,7 @@ class RedirectingHttpServer { m_service.run_until_stopped(); }) { - reset_state(); network::Endpoint ep; - if (listen_port != 0) { - // If a port is provided, then configure the endpoint with this port num - ep = network::Endpoint(network::make_address("0.0.0.0"), listen_port); - } m_acceptor.open(ep.protocol()); m_acceptor.bind(ep); ep = m_acceptor.local_endpoint(); @@ -95,7 +83,7 @@ class RedirectingHttpServer { // NOTE: some http transport redirect implementations may strip the authorization // header from the request after it is redirected and the user will be logged out // from the client app as a result. - void force_http_redirect(bool remote = true) + void force_http_redirect(bool remote) { m_http_redirect = remote; } @@ -106,19 +94,11 @@ class RedirectingHttpServer { // value) and open a websocket conneciton to the actual server. // NOTE: the websocket will never connect if both http and websockets are // redirecting and will just keep getting the redirect close code. - void force_websocket_redirect(bool force = true) + void force_websocket_redirect(bool force) { m_websocket_redirect = force; } - // Reset the state to default (initial) state after test makes updates - void reset_state() - { - m_http_redirect = false; - m_websocket_redirect = false; - m_hook = nullptr; - } - std::string base_url() const { return m_base_url; @@ -129,16 +109,6 @@ class RedirectingHttpServer { return m_redirect_to_base_url; } - std::string location_hostname() const - { - return m_http_redirect ? m_base_url : m_redirect_to_base_url; - } - - std::string location_wshostname() const - { - return m_websocket_redirect ? m_base_wsurl : m_redirect_to_base_wsurl; - } - private: struct BufferedSocket : network::Socket { BufferedSocket(network::Service& service) @@ -171,7 +141,8 @@ class RedirectingHttpServer { struct Conn : public util::RefCountBase, websocket::Config { Conn(network::Service& service, const std::shared_ptr<util::Logger>& logger) - : logger(logger) + : random(Catch::getSeed()) + , logger(logger) , socket(service) , http_server(socket, logger) { @@ -247,15 +218,16 @@ class RedirectingHttpServer { std::optional<websocket::Socket> websocket; }; - void send_simple_response(util::bind_ptr<Conn> conn, HTTPStatus status, std::string reason, std::string body) + void send_simple_response(util::bind_ptr<Conn> conn, HTTPStatus status, std::string reason, + std::optional<std::string> body) { send_http_response(conn, status, std::move(reason), {}, std::move(body)); } void send_http_response(util::bind_ptr<Conn> conn, HTTPStatus status, std::string reason, HTTPHeaders headers, - std::string body) + std::optional<std::string> body) { - m_logger->debug("sending http response %1: %2 \"%3\"", status, reason, body); + m_logger->debug("sending http response %1: %2 '%3'", status, reason, body.value_or("")); HTTPResponse resp{status, std::move(reason), std::move(headers), std::move(body)}; conn->http_server.async_send_response(resp, [this, conn](std::error_code ec) { if (ec && ec != util::error::operation_aborted) { @@ -322,7 +294,6 @@ class RedirectingHttpServer { } conn->http_server.async_receive_request([this, conn](HTTPRequest req, std::error_code ec) { - static bool use_301 = false; // send 301 on first redirect response if (ec) { if (ec != util::error::operation_aborted) { m_logger->error("Error receiving HTTP request to redirect [%1]: %2", ec, ec.message()); @@ -333,10 +304,11 @@ class RedirectingHttpServer { m_logger->debug("Received request: %1", req.path); if (req.path.find("/location") != std::string::npos) { - nlohmann::json body{{"deployment_model", "GLOBAL"}, - {"location", "US-VA"}, - {"hostname", location_hostname()}, - {"ws_hostname", location_wshostname()}}; + nlohmann::json body{ + {"deployment_model", "GLOBAL"}, + {"location", "US-VA"}, + {"hostname", m_http_redirect ? m_base_url : m_redirect_to_base_url}, + {"ws_hostname", m_websocket_redirect ? m_base_wsurl : m_redirect_to_base_wsurl}}; auto body_str = body.dump(); send_http_response(conn, HTTPStatus::Ok, "Okay", {{"Content-Type", "application/json"}}, @@ -354,13 +326,12 @@ class RedirectingHttpServer { // Send redirect response for appservices calls // Starts with 'http' and contains api path if (req.path.find("/api/client/v2.0/") == 0) { - use_301 = !use_301; - auto status = use_301 ? HTTPStatus::MovedPermanently : HTTPStatus::PermanentRedirect; - auto reason = use_301 ? "Moved Permanently" : "Permanent Redirect"; + // Alternate sending 301 and 308 redirect status codes + auto status = m_use_301 ? HTTPStatus::MovedPermanently : HTTPStatus::PermanentRedirect; + auto reason = m_use_301 ? "Moved Permanently" : "Permanent Redirect"; + m_use_301 = !m_use_301; auto location = m_redirect_to_base_url + req.path; - auto redirect = util::format("<html><body><p>%1 to <a href=\"%2\">%2</a></p></body></html>", - status, location); - send_http_response(conn, status, reason, {{"location", location}}, redirect); + send_http_response(conn, status, reason, {{"location", location}}, std::nullopt); if (m_hook) m_hook(Event::redirect, std::nullopt); return; @@ -372,19 +343,6 @@ class RedirectingHttpServer { }); } - std::string trim_url(std::string_view str) - { - auto p0 = str.data(); - auto p1 = str.data() + str.size(); - // Trim spaces and forward slashes from the end of the string - while (p1 > p0 && (std::isspace(*(p1 - 1)) || *(p1 - 1) == '/')) - --p1; - // Trim spaces from the beginning of the string - while (p0 < p1 && std::isspace(*p0)) - ++p0; - return std::string(p0, p1 - p0); - } - std::string make_wsurl(std::string base_url) { if (base_url.find("http") == 0) { @@ -401,11 +359,12 @@ class RedirectingHttpServer { const std::string m_redirect_to_base_wsurl; const std::shared_ptr<util::Logger> m_logger; - bool m_http_redirect; - bool m_websocket_redirect; + bool m_http_redirect = false; + bool m_websocket_redirect = false; std::string m_base_url; std::string m_base_wsurl; std::function<void(Event, std::optional<std::string>)> m_hook; + bool m_use_301 = true; network::Service m_service; network::Acceptor m_acceptor; From 9a9371d123738c418db1113c87286e17c119877f Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Wed, 4 Sep 2024 14:44:21 -0400 Subject: [PATCH 15/22] rerun validation From 307fdb70653b486e5c91b9ae2e473b5f73bcdf92 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Thu, 5 Sep 2024 11:30:36 -0400 Subject: [PATCH 16/22] Fixed TSAN error --- test/object-store/sync/app.cpp | 54 ++++++------------- .../util/sync/redirect_server.hpp | 8 ++- 2 files changed, 22 insertions(+), 40 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index c15a58a151..b4371b9ed3 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3251,10 +3251,8 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") { TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { auto logger = util::Logger::get_default_logger(); - auto app_session = get_runtime_app_session(); - - // Skip this test if not using the redirect server auto redirector = sync::RedirectingHttpServer(get_real_base_url(), logger); + std::mutex counter_mutex; int error_count = 0; int location_count = 0; @@ -3283,28 +3281,6 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { } }); - struct CounterValue { - bool greater_than; - int value = 0; - CounterValue(bool greater_than, int value) - : greater_than(greater_than) - , value(value) - { - } - CounterValue(int value) - : CounterValue(false, value) - { - } - }; - - auto check_value = [](int value, const CounterValue& check, std::string_view name) { - INFO(util::format("Checking '%1' counter value", name)); - if (check.greater_than) - REQUIRE(value > check.value); - else - REQUIRE(value == check.value); - }; - auto reset_counters = [&] { std::lock_guard lk(counter_mutex); error_count = 0; @@ -3313,13 +3289,12 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { wsredirect_count = 0; }; - auto check_counters = [&](CounterValue locations, CounterValue redirects, CounterValue wsredirects, - CounterValue errors) { + auto check_counters = [&](int locations, int redirects, int wsredirects, int errors) { std::lock_guard lk(counter_mutex); - check_value(location_count, locations, "locations"); - check_value(redirect_count, redirects, "redirects"); - check_value(wsredirect_count, wsredirects, "ws redirects"); - check_value(error_count, errors, "errors"); + REQUIRE(location_count == locations); + REQUIRE(redirect_count == redirects); + REQUIRE(wsredirect_count == wsredirects); + REQUIRE(error_count == errors); }; // Make sure the location response points to the actual server @@ -3329,7 +3304,10 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { auto tas_config = TestAppSession::Config{}; tas_config.base_url = redirector.base_url(); - TestAppSession session{app_session, tas_config, DeleteApp{false}}; + // Since this test defines its own RedirectingHttpServer, the app session doesn't + // need to be retrieved at the beginning of the test to ensure the redirect server + // is initialized. + TestAppSession session{get_runtime_app_session(), tas_config, DeleteApp{false}}; auto app = session.app(); // We should have already requested the location when the user was logged in @@ -3338,7 +3316,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(user1); // Expected location requested 1 time for the original location request, // all others 0 since location request prior to login hits actual server - check_counters({1}, {0}, {0}, {0}); + check_counters(1, 0, 0, 0); REQUIRE(app->get_base_url() == redirector.base_url()); REQUIRE(app->get_host_url() == redirector.server_url()); @@ -3376,7 +3354,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(user2->is_logged_in()); REQUIRE(user1 == user2); // Expected location requested 2 times, and at least 1 redirects, all others 0 - check_counters({2}, {true, 1}, {0}, {0}); + check_counters(2, 4, 0, 0); REQUIRE(app->get_base_url() == redirector.base_url()); REQUIRE(app->get_host_url() == redirector.base_url()); @@ -3396,7 +3374,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(user3 == app->current_user()); REQUIRE(user3 != user2); // Expected location requested 1 time for location prior to login, all others 0 - check_counters({1}, {0}, {0}, {0}); + check_counters(1, 0, 0, 0); REQUIRE(app->get_base_url() == redirector.base_url()); REQUIRE(app->get_host_url() == redirector.server_url()); } @@ -3431,7 +3409,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { create_one_dog(r); REQUIRE(get_dogs(r).size() == 1); // The redirect server is not expected to be used... - check_counters({0}, {0}, {0}, {0}); + check_counters(0, 0, 0, 0); } // Switch the location to use the redirector's address for websocket requests which will // return the 4003 redirect close code, forcing app to update the location and refresh @@ -3483,7 +3461,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // The location should have been requested twice; once since the location_updated // flag was reset and a second time for the login request. One redirect occurred // for the register_email request, since that was sent to the redirect server. - check_counters({2}, {1}, {0}, {0}); + check_counters(2, 1, 0, 0); reset_counters(); SyncTestFile config(app->current_user(), partition, schema); auto r = Realm::get_shared_realm(config); @@ -3493,7 +3471,7 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(dogs.get(0).get<String>("name") == "fido"); // The location should have been requested again and the websocket should have // been hit, which sent the redirect close code. - check_counters({0}, {0}, {1}, {0}); + check_counters(0, 0, 1, 0); } } } diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index 42ba3450e9..6b740d2de4 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -67,8 +67,12 @@ class RedirectingHttpServer { ~RedirectingHttpServer() { - m_acceptor.cancel(); - m_service.stop(); + m_service.post([this](Status status) { + if (status == ErrorCodes::OperationAborted) + return; + m_acceptor.cancel(); + m_service.stop(); + }); m_server_thread.join(); } From 1d1edf9c0b634562989e9857ce5c35fe219f7502 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 6 Sep 2024 23:12:03 -0400 Subject: [PATCH 17/22] Update location after auth failure; updated test comments --- src/realm/object-store/sync/app.cpp | 16 +++++++--- test/object-store/sync/app.cpp | 46 ++++++++++++++++------------- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp index d2c5a3d7e9..a5f5e18391 100644 --- a/src/realm/object-store/sync/app.cpp +++ b/src/realm/object-store/sync/app.cpp @@ -854,8 +854,7 @@ void App::log_in_with_credentials(const AppCredentials& credentials, const std:: completion(user, error); }); }, - // Update location to make sure we're talking to the latest server before logging in - true); + false); } void App::log_in_with_credentials( @@ -1260,8 +1259,11 @@ void App::handle_auth_failure(const AppError& error, std::unique_ptr<Request>&& return; } - // Otherwise we may be able to request a new access token and have the request succeed with that - refresh_access_token(user, false, + // Otherwise we may be able to request a new access token and resend the request request to see + // if it will succeed with that. Also update the location beforehand to ensure the failure + // wasn't because of a redirect handled by the SDK (which strips the Authorization header + // before re-sending the request to the new server) + refresh_access_token(user, true, [self = shared_from_this(), request = std::move(request), completion = std::move(completion), response = std::move(response), user](Optional<AppError>&& error) mutable { if (error) { @@ -1270,6 +1272,12 @@ void App::handle_auth_failure(const AppError& error, std::unique_ptr<Request>&& return; } + // In case the location info was updated, update the original request + // to point to the latest location URL. + auto url = util::Uri::parse(request->url); + request->url = util::format("%1%2%3%4", self->get_host_url(), url.get_path(), + url.get_query(), url.get_frag()); + // Reissue the request with the new access token request->headers = get_request_headers(user, RequestTokenType::AccessToken); self->do_request(std::move(request), [self = self, completion = std::move(completion)]( diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index b4371b9ed3..1d274444bb 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3312,8 +3312,8 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // We should have already requested the location when the user was logged in // during the session constructor. - auto user1 = app->current_user(); - REQUIRE(user1); + auto user1_a = app->current_user(); + REQUIRE(user1_a); // Expected location requested 1 time for the original location request, // all others 0 since location request prior to login hits actual server check_counters(1, 0, 0, 0); @@ -3345,15 +3345,16 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { }); REQUIRE(pf.future.get_no_throw().is_ok()); } - // Login should fail since the profile() command does not complete successfully due + // Login should fail since the profile request does not complete successfully due // to the authorization headers being stripped from the redirected request REQUIRE_FALSE(session.log_in_user(creds).is_ok()); - // User was originally logged in, but logged out when profile request failed and - // app's current user was not updated - auto user2 = app->current_user(); - REQUIRE(user2->is_logged_in()); - REQUIRE(user1 == user2); - // Expected location requested 2 times, and at least 1 redirects, all others 0 + // Since the login failed, the original user1 is still the App's current user + auto user1_b = app->current_user(); + REQUIRE(user1_b->is_logged_in()); + REQUIRE(user1_a == user1_b); + // Expected location requested 2 times: once for register and after first profile + // attempt fails; there are 4 redirects: register, login, get profile, and refresh + // token check_counters(2, 4, 0, 0); REQUIRE(app->get_base_url() == redirector.base_url()); REQUIRE(app->get_host_url() == redirector.base_url()); @@ -3372,9 +3373,10 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(user3); REQUIRE(user3->is_logged_in()); REQUIRE(user3 == app->current_user()); - REQUIRE(user3 != user2); - // Expected location requested 1 time for location prior to login, all others 0 - check_counters(1, 0, 0, 0); + REQUIRE(user3 != user1_b); + // Expected location requested 1 time for location after first profile attempt + // fails; and two redirects: login and the first profile attempt + check_counters(1, 2, 0, 0); REQUIRE(app->get_base_url() == redirector.base_url()); REQUIRE(app->get_host_url() == redirector.server_url()); } @@ -3399,11 +3401,11 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { const auto schema = get_default_schema(); const auto partition = random_string(100); - // First websocket connection is not using redirection. Should connect + // This websocket connection is not using redirection. Should connect // directly to the actual server { reset_counters(); - SyncTestFile config(user1, partition, schema); + SyncTestFile config(user1_a, partition, schema); auto r = Realm::get_shared_realm(config); REQUIRE(get_dogs(r).size() == 0); create_one_dog(r); @@ -3422,8 +3424,8 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // login request, the location response should include the actual server for the // hostname (so the login is successful) and the redirect server for the ws_hostname // so the websocket initially connects to the redirect server. + redirector.force_http_redirect(true); { - redirector.force_http_redirect(true); redirector.set_event_hook([&](RedirectEvent event, std::optional<std::string> message) { std::lock_guard lk(counter_mutex); switch (event) { @@ -3458,10 +3460,11 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { // and start with a new realm auto result = session.create_user_and_log_in(); REQUIRE(result.is_ok()); - // The location should have been requested twice; once since the location_updated - // flag was reset and a second time for the login request. One redirect occurred - // for the register_email request, since that was sent to the redirect server. - check_counters(2, 1, 0, 0); + // The location should have been requested twice; before register email and after + // first profile attempt fails; and three redirects: register email, login, and + // first profile attempt. + // NOTE: The ws_hostname still points to the redirect server + check_counters(2, 3, 0, 0); reset_counters(); SyncTestFile config(app->current_user(), partition, schema); auto r = Realm::get_shared_realm(config); @@ -3469,8 +3472,9 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { REQUIRE(dogs.size() == 1); REQUIRE(dogs.get(0).get<String>("breed") == "bulldog"); REQUIRE(dogs.get(0).get<String>("name") == "fido"); - // The location should have been requested again and the websocket should have - // been hit, which sent the redirect close code. + // The websocket should have redirected one time - the location update hits the + // actual server since the hostname points to its URL after the location update + // during user log in. check_counters(0, 0, 1, 0); } } From ce1aeec8dd5f5c42ad924144b966c9d9298231b9 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 6 Sep 2024 23:15:16 -0400 Subject: [PATCH 18/22] I thought I removed this line... --- test/object-store/util/sync/redirect_server.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index 6b740d2de4..2705cc0530 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -150,7 +150,6 @@ class RedirectingHttpServer { , socket(service) , http_server(socket, logger) { - util::seed_prng_nondeterministically(random); // Throws } // Implement the websocket::Config interface From a4e967a27678191eebaee51819bdeab667982ebe Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 6 Sep 2024 23:25:55 -0400 Subject: [PATCH 19/22] Reverted line now that login is not always requesting location --- test/object-store/sync/app.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 1d274444bb..8b8c7d5827 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3787,13 +3787,7 @@ TEST_CASE("app: base_url", "[sync][app][base_url]") { { if (request.url.find("/location") != std::string::npos) { CHECK(request.method == HttpMethod::get); - // Location is now requested again when the user logs in - only check the exepected - // url on the first location request - after that, it will be using the location_url - // value when requesting the location. - if (!location_requested) - CHECK_THAT(request.url, ContainsSubstring(expected_url)); - else - CHECK_THAT(request.url, ContainsSubstring(location_url)); + CHECK_THAT(request.url, ContainsSubstring(expected_url)); location_requested = true; if (location_returns_error) { completion(app::Response{static_cast<int>(sync::HTTPStatus::NotFound), 0, {}, "404 not found"}); From 761f18b0ab757706d7b8f5c3d1b7065cdf8faa7c Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 6 Sep 2024 23:30:21 -0400 Subject: [PATCH 20/22] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fee698757..ad3444d5c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ ### Fixed * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?) -* If a user authenticated request is redirected, the user will be logged out since the authorization header is removed before request is sent to the new server. The log_in_with_credentials() function will always request the location info from the server prior to logging in to ensure the remote server URL is up to date. ([#8012](https://github.com/realm/realm-core/issues/8012), since v12.9.0) +* If a user authenticated app services (http) request is redirected, the user will be logged out since the authorization header is removed before request is sent to the new server. If an authenticated request fails, the location will now be updated prior to refreshing the access token to ensure the remote server URL is up to date. ([#8012](https://github.com/realm/realm-core/issues/8012), since v12.9.0) ### 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)) From 49961ab1c4112c3c7e18ed3a3def0b9889d1547a Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Fri, 6 Sep 2024 23:41:33 -0400 Subject: [PATCH 21/22] a little more cleanup --- test/object-store/sync/app.cpp | 16 ++++++++-------- test/object-store/util/sync/redirect_server.hpp | 3 ++- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 8b8c7d5827..e0162ed4e5 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -3264,19 +3264,19 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { switch (event) { case RedirectEvent::location: location_count++; - logger->debug("Redirector event: location - count: %1", location_count); + logger->trace("Redirector event: location - count: %1", location_count); return; case RedirectEvent::redirect: redirect_count++; - logger->debug("Redirector event: redirect - count: %1", redirect_count); + logger->trace("Redirector event: redirect - count: %1", redirect_count); return; case RedirectEvent::ws_redirect: wsredirect_count++; - logger->debug("Redirector event: ws_redirect - count: %1", wsredirect_count); + logger->trace("Redirector event: ws_redirect - count: %1", wsredirect_count); return; case RedirectEvent::error: error_count++; - logger->error("Redirect server received error: %1", message.value_or("unknown error")); + logger->trace("Redirect server received error: %1", message.value_or("unknown error")); return; } }); @@ -3431,22 +3431,22 @@ TEST_CASE("app: network transport handles redirection", "[sync][app][baas]") { switch (event) { case RedirectEvent::location: location_count++; - logger->debug("Redirector event: location - count: %1", location_count); + logger->trace("Redirector event: location - count: %1", location_count); if (location_count == 1) // No longer sending redirect server as location hostname value redirector.force_http_redirect(false); return; case RedirectEvent::redirect: redirect_count++; - logger->debug("Redirector event: redirect - count: %1", redirect_count); + logger->trace("Redirector event: redirect - count: %1", redirect_count); return; case RedirectEvent::ws_redirect: wsredirect_count++; - logger->debug("Redirector event: ws_redirect - count: %1", wsredirect_count); + logger->trace("Redirector event: ws_redirect - count: %1", wsredirect_count); return; case RedirectEvent::error: error_count++; - logger->error("Redirect server received error: %1", message.value_or("unknown error")); + logger->trace("Redirect server received error: %1", message.value_or("unknown error")); return; } }); diff --git a/test/object-store/util/sync/redirect_server.hpp b/test/object-store/util/sync/redirect_server.hpp index 2705cc0530..d40ec2f07f 100644 --- a/test/object-store/util/sync/redirect_server.hpp +++ b/test/object-store/util/sync/redirect_server.hpp @@ -28,7 +28,8 @@ #include <realm/util/uri.hpp> #include <external/json/json.hpp> -#include <iostream> +#include <catch2/catch_all.hpp> + #include <thread> namespace realm::sync { From ee81ab44d186adfe190b43d9bd8473a6ff4f67ee Mon Sep 17 00:00:00 2001 From: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Date: Sat, 7 Sep 2024 01:17:03 -0400 Subject: [PATCH 22/22] Fixed refresh access token test --- test/object-store/sync/app.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index e0162ed4e5..1a00715ca7 100644 --- a/test/object-store/sync/app.cpp +++ b/test/object-store/sync/app.cpp @@ -5367,8 +5367,10 @@ TEST_CASE("app: refresh access token unit tests", "[sync][app][user][token]") { SECTION("refresh token ensure flow is correct") { /* Expected flow: + Location - first http request since app was just created Login - this gets access and refresh tokens Get profile - throw back a 401 error + Location - return location response Refresh token - get a new token for the user Get profile - get the profile with the new token */ @@ -5400,13 +5402,13 @@ TEST_CASE("app: refresh access token unit tests", "[sync][app][user][token]") { } } else if (request.url.find("/session") != std::string::npos && request.method == HttpMethod::post) { - CHECK(state.get() == TestState::profile_1); + CHECK(state.get() == TestState::location); state.transition_to(TestState::refresh); nlohmann::json json{{"access_token", good_access_token2}}; completion({200, 0, {}, json.dump()}); } else if (request.url.find("/location") != std::string::npos) { - CHECK(state.get() == TestState::unknown); + CHECK((state.get() == TestState::unknown || state.get() == TestState::profile_1)); state.transition_to(TestState::location); CHECK(request.method == HttpMethod::get); completion({200,