RCORE-2253 Redirected user authenticated app requests cause user to be logged out and location is not updated

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?.?.?)
-* None.
+* 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))
@@ -17,7 +17,7 @@
 -----------
 
 ### Internals
-* None.
+* Enabled curl redirect support for the test http network transport and added a test that uses the redirect server, if enabled, to validate redirections being handled by the network transport implementation. ([PR #8011](https://github.com/realm/realm-core/pull/8011))
 
 ----------------------------------------------
 

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,12 @@ 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, "");
+}
 
 void App::refresh_custom_data(const std::shared_ptr<User>& user,
                               UniqueFunction<void(Optional<AppError>)>&& completion)

src/realm/object-store/sync/app.hpp

@@ -192,6 +192,10 @@ 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);
+
     // MARK: - Provider Clients
 
     /// A struct representing a user API key as returned by the App server.

test/object-store/sync/app.cpp

@@ -3248,6 +3248,262 @@ 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()) {
+            // 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");
+    };
+
+    // 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
+    // during the session constructor.
+    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()->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);
+        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());
+    }
+
+    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});
+        }
+    }
+}
+
 TEST_CASE("app: sync logs contain baas coid", "[sync][app][baas]") {
     class InMemoryLogger : public util::Logger {
     public:
@@ -3555,7 +3811,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"});

test/object-store/util/sync/baas_admin_api.cpp

@@ -332,6 +332,12 @@ 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_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 +608,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 +690,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;
         }
 

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)
 {