realm · michael-wb · May 23, 2023 · May 15, 2023 · May 15, 2023 · May 16, 2023
diff --git 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?.?.?)
-* None.
+* Access token refresh for websockets was not updating the location metadata ([#6630](https://github.com/realm/realm-core/issues/6630), since v13.9.3)
 
 ### Breaking changes
 * None.

diff --git a/src/realm/object-store/sync/app.cpp b/src/realm/object-store/sync/app.cpp
@@ -301,9 +301,15 @@ void App::configure(const SyncClientConfig& sync_client_config)
     auto sync_route = make_sync_route(m_app_route);
     m_sync_manager->configure(shared_from_this(), sync_route, sync_client_config);
     if (auto metadata = m_sync_manager->app_metadata()) {
-        // If there is app metadata stored, then update the hostname/syncroute using that info
+        // If there is app metadata stored, then set up the initial hostname/syncroute
+        // using that info - it will be updated upon first request to the server
         update_hostname(metadata);
     }
+    {
+        std::lock_guard<std::mutex> lock(*m_route_mutex);
+        // Always update the location after the app is configured/re-configured
+        m_location_updated = false;
+    }
 }
 
 bool App::init_logger()
@@ -337,7 +343,7 @@ void App::log_error(const char* message, Params&&... params)
 
 std::string App::make_sync_route(const std::string& http_app_route)
 {
-    // change the scheme in the base url to ws from http to satisfy the sync client
+    // change the scheme in the base url from http to ws to satisfy the sync client URL
     auto sync_route = http_app_route + sync_path;
     size_t uri_scheme_start = sync_route.find("http");
     if (uri_scheme_start == 0)
@@ -659,35 +665,45 @@ void App::log_in_with_credentials(
     BsonDocument body = credentials.serialize_as_bson();
     attach_auth_options(body);
 
-    do_request({HttpMethod::post, route, m_request_timeout_ms,
-                get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()},
-               [completion = std::move(completion), credentials, linking_user,
-                self = shared_from_this()](const Response& response) mutable {
-                   if (auto error = AppUtils::check_for_errors(response)) {
-                       self->log_error("App: log_in_with_credentials failed: %1 message: %2",
-                                       response.http_status_code, error->what());
-                       return completion(nullptr, std::move(error));
-                   }
+    // To ensure the location metadata is always kept up to date, requery the location info before
+    // logging in the user. Since some SDK network transports (e.g. for JS) automatically handle
+    // redirects, the location is not updated when a redirect response from the server is received.
+    // This is especially necessary when the deployment model is changed, where the redirect response
+    // provides information about the new location of the server and a location info update is
+    // triggered. If the App never receives a redirect response from the server (because it was
+    // automatically handled) after the deployment model was changed and the user was logged out,
+    // the HTTP and websocket URL values will never be updated with the new server location.
+    do_request(
+        {HttpMethod::post, route, m_request_timeout_ms,
+         get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()},
+        [completion = std::move(completion), credentials, linking_user,
+         self = shared_from_this()](const Response& response) mutable {
+            if (auto error = AppUtils::check_for_errors(response)) {
+                self->log_error("App: log_in_with_credentials failed: %1 message: %2", response.http_status_code,
+                                error->what());
+                return completion(nullptr, std::move(error));
+            }
 
-                   std::shared_ptr<realm::SyncUser> sync_user = linking_user;
-                   try {
-                       auto json = parse<BsonDocument>(response.body);
-                       if (linking_user) {
-                           linking_user->update_access_token(get<std::string>(json, "access_token"));
-                       }
-                       else {
-                           sync_user = self->m_sync_manager->get_user(
-                               get<std::string>(json, "user_id"), get<std::string>(json, "refresh_token"),
-                               get<std::string>(json, "access_token"), credentials.provider_as_string(),
-                               get<std::string>(json, "device_id"));
-                       }
-                   }
-                   catch (const AppError& e) {
-                       return completion(nullptr, e);
-                   }
+            std::shared_ptr<realm::SyncUser> sync_user = linking_user;
+            try {
+                auto json = parse<BsonDocument>(response.body);
+                if (linking_user) {
+                    linking_user->update_access_token(get<std::string>(json, "access_token"));
+                }
+                else {
+                    sync_user = self->m_sync_manager->get_user(
+                        get<std::string>(json, "user_id"), get<std::string>(json, "refresh_token"),
+                        get<std::string>(json, "access_token"), credentials.provider_as_string(),
+                        get<std::string>(json, "device_id"));
+                }
+            }
+            catch (const AppError& e) {
+                return completion(nullptr, e);
+            }
 
-                   self->get_profile(sync_user, std::move(completion));
-               });
+            self->get_profile(sync_user, std::move(completion));
+        },
+        false);
 }
 
 void App::log_in_with_credentials(
@@ -865,20 +881,25 @@ void App::init_app_metadata(UniqueFunction<void(const Optional<Response>&)>&& co
 {
     std::string route;
 
-    if (!new_hostname && m_location_updated) {
+    {
+        std::unique_lock<std::mutex> lock(*m_route_mutex);
         // Skip if the app_metadata/location data has already been initialized and a new hostname is not provided
-        return completion(util::none); // early return
-    }
-    else {
-        std::lock_guard<std::mutex> lock(*m_route_mutex);
-        route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route());
+        if (!new_hostname && m_location_updated) {
+            // Release the lock before calling the completion function
+            lock.unlock();
+            return completion(util::none); // early return
+        }
+        else {
+            route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route());
+        }
     }
-
     Request req;
     req.method = HttpMethod::get;
     req.url = route;
     req.timeout_ms = m_request_timeout_ms;
 
+    log_debug("App: request location: %1", route);
+
     m_config.transport->send_request_to_server(req, [self = shared_from_this(),
                                                      completion = std::move(completion)](const Response& response) {
         // If the response contains an error, then pass it up
@@ -902,7 +923,10 @@ void App::init_app_metadata(UniqueFunction<void(const Optional<Response>&)>&& co
                 // No metadata in use, update the hostname and sync route directly
                 self->update_hostname(hostname, ws_hostname);
             }
-            self->m_location_updated = true;
+            {
+                std::lock_guard<std::mutex> lock(*self->m_route_mutex);
+                self->m_location_updated = true;
+            }
         }
         catch (const AppError&) {
             // Pass the response back to completion
@@ -950,23 +974,34 @@ void App::update_metadata_and_resend(Request&& request, UniqueFunction<void(cons
         new_hostname);
 }
 
-void App::do_request(Request&& request, UniqueFunction<void(const Response&)>&& completion)
+void App::do_request(Request&& request, UniqueFunction<void(const Response&)>&& completion, bool update_location)
 {
-    request.timeout_ms = default_timeout_ms;
+    // Make sure the timeout value is set to the configured request timeout value
+    request.timeout_ms = m_request_timeout_ms;
 
-    // Refresh the location metadata every time an app is created to ensure the http and
-    // websocket URL information is up to date.
-    if (m_location_updated) {
-        m_config.transport->send_request_to_server(
-            std::move(request), [self = shared_from_this(), completion = std::move(completion)](
-                                    Request&& request, const Response& response) mutable {
-                self->handle_possible_redirect_response(std::move(request), response, std::move(completion));
-            });
-        return; // early return
+    // Refresh the location metadata every time an app is created (or when requested) to ensure the http
+    // and websocket URL information is up to date.
+    {
+        std::unique_lock<std::mutex> lock(*m_route_mutex);
+        if (update_location) {
+            // Force the location to be updated before sending the request.
+            m_location_updated = false;
+        }
+        if (!m_location_updated) {
+            lock.unlock();
+            // Location metadata has not yet been received after the App was created, update the location
+            // info and then send the request
+            update_metadata_and_resend(std::move(request), std::move(completion));
+            return; // early return
+        }
     }
 
-    // if we do not have metadata yet, update the metadata and resend the request
-    update_metadata_and_resend(std::move(request), std::move(completion));
+    // location info has already been received after App was created
+    m_config.transport->send_request_to_server(
+        std::move(request), [self = shared_from_this(), completion = std::move(completion)](
+                                Request&& request, const Response& response) mutable {
+            self->handle_possible_redirect_response(std::move(request), response, std::move(completion));
+        });
 }
 
 void App::handle_possible_redirect_response(Request&& request, const Response& response,
@@ -1096,33 +1131,29 @@ void App::refresh_access_token(const std::shared_ptr<SyncUser>& sync_user, bool
         route = util::format("%1/auth/session", m_base_route);
     }
 
-    log_debug("App: refresh_access_token: email: %1", sync_user->user_profile().email());
+    log_debug("App: refresh_access_token: email: %1 %2", sync_user->user_profile().email(),
+              update_location ? "(updating location)" : "");
 
-    Request request{HttpMethod::post, std::move(route), m_request_timeout_ms,
-                    get_request_headers(sync_user, RequestTokenType::RefreshToken)};
-    auto handler = [completion = std::move(completion), sync_user](const Response& response) {
-        if (auto error = AppUtils::check_for_errors(response)) {
-            return completion(std::move(error));
-        }
-
-        try {
-            auto json = parse<BsonDocument>(response.body);
-            sync_user->update_access_token(get<std::string>(json, "access_token"));
-        }
-        catch (AppError& err) {
-            return completion(std::move(err));
-        }
+    // If update_location is set, force the location info to be updated before sending the request
+    do_request(
+        {HttpMethod::post, std::move(route), m_request_timeout_ms,
+         get_request_headers(sync_user, RequestTokenType::RefreshToken)},
+        [completion = std::move(completion), sync_user](const Response& response) {
+            if (auto error = AppUtils::check_for_errors(response)) {
+                return completion(std::move(error));
+            }
 
-        return completion(util::none);
-    };
+            try {
+                auto json = parse<BsonDocument>(response.body);
+                sync_user->update_access_token(get<std::string>(json, "access_token"));
+            }
+            catch (AppError& err) {
+                return completion(std::move(err));
+            }
 
-    if (update_location) {
-        // If update_location, update the location metadata before sending the request
-        update_metadata_and_resend(std::move(request), std::move(handler));
-    }
-    else {
-        do_request(std::move(request), std::move(handler));
-    }
+            return completion(util::none);
+        },
+        update_location);
 }
 
 std::string App::function_call_url_path() const

diff --git a/src/realm/object-store/sync/app.hpp b/src/realm/object-store/sync/app.hpp
@@ -403,13 +403,17 @@ class App : public std::enable_shared_from_this<App>,
     friend class OnlyForTesting;
 
     Config m_config;
-    mutable std::unique_ptr<std::mutex> m_route_mutex = std::make_unique<std::mutex>();
+
+    // mutable to allow locking for reads in const functions
+    // this is a shared pointer to support the App move constructor
+    mutable std::shared_ptr<std::mutex> m_route_mutex = std::make_shared<std::mutex>();
     std::string m_base_url;
     std::string m_base_route;
     std::string m_app_route;
     std::string m_auth_route;
-    uint64_t m_request_timeout_ms;
     bool m_location_updated = false;
+
+    uint64_t m_request_timeout_ms;
     std::shared_ptr<SyncManager> m_sync_manager;
     std::shared_ptr<util::Logger> m_logger_ptr;
 
@@ -463,15 +467,15 @@ class App : public std::enable_shared_from_this<App>,
     void update_metadata_and_resend(Request&& request, util::UniqueFunction<void(const Response&)>&& completion,
                                     const util::Optional<std::string>& new_hostname = util::none);
 
-    void basic_request(std::string&& route, std::string&& body,
-                       util::UniqueFunction<void(util::Optional<AppError>)>&& completion);
     void post(std::string&& route, util::UniqueFunction<void(util::Optional<AppError>)>&& completion,
               const bson::BsonDocument& body);
 
     /// Performs a request to the Stitch server. This request does not contain authentication state.
     /// @param request The request to be performed
     /// @param completion Returns the response from the server
-    void do_request(Request&& request, util::UniqueFunction<void(const Response&)>&& completion);
+    /// @param update_location Force the location metadata to be updated prior to sending the request
+    void do_request(Request&& request, util::UniqueFunction<void(const Response&)>&& completion,
+                    bool update_location = false);
 
     /// Check to see if hte response is a redirect and handle, otherwise pass the response to compleetion
     /// @param request The request to be performed (in case it needs to be sent again)

diff --git a/src/realm/object-store/sync/sync_session.cpp b/src/realm/object-store/sync/sync_session.cpp
@@ -763,13 +763,16 @@ void SyncSession::handle_error(sync::SessionErrorInfo error)
         }
     }
     else if (error_code.category() == sync::websocket::websocket_error_category()) {
+        using WebSocketError = sync::websocket::WebSocketError;
+        auto websocket_error = static_cast<WebSocketError>(error_code.value());
+
         // The server replies with '401: unauthorized' if the access token is invalid, expired, revoked, or the user
         // is disabled. In this scenario we attempt an automatic token refresh and if that succeeds continue as
         // normal. If the refresh request also fails with 401 then we need to stop retrying and pass along the error;
         // see handle_refresh().
-        bool redirect_occurred = error_code == sync::websocket::WebSocketError::websocket_moved_permanently;
-        if (redirect_occurred || error_code == sync::websocket::WebSocketError::websocket_unauthorized ||
-            error_code == sync::websocket::WebSocketError::websocket_abnormal_closure) {
+        bool redirect_occurred = websocket_error == WebSocketError::websocket_moved_permanently;
+        if (redirect_occurred || websocket_error == WebSocketError::websocket_unauthorized ||
+            websocket_error == WebSocketError::websocket_abnormal_closure) {
             if (auto u = user()) {
                 // If a redirection occurred, the location metadata will be updated before refreshing the access
                 // token.
@@ -780,14 +783,13 @@ void SyncSession::handle_error(sync::SessionErrorInfo error)
 
         // If the websocket was closed cleanly or if the socket disappeared, don't notify the user as an error
         // since the sync client will retry.
-        if (error_code == sync::websocket::WebSocketError::websocket_read_error ||
-            error_code == sync::websocket::WebSocketError::websocket_write_error) {
+        if (websocket_error == WebSocketError::websocket_read_error ||
+            websocket_error == WebSocketError::websocket_write_error) {
             return;
         }
 
         // Surface a simplified websocket error to the user.
-        auto simplified_error = sync::websocket::get_simplified_websocket_error(
-            static_cast<sync::websocket::WebSocketError>(error_code.value()));
+        auto simplified_error = sync::websocket::get_simplified_websocket_error(websocket_error);
         std::error_code new_error_code(simplified_error, sync::websocket::websocket_error_category());
         error = sync::SessionErrorInfo(new_error_code, error.message, error.try_again);
     }

diff --git a/test/object-store/CMakeLists.txt b/test/object-store/CMakeLists.txt
@@ -119,9 +119,12 @@ if(REALM_TEST_LOGGING)
     )
 
     if(REALM_TEST_LOGGING_LEVEL)
+        message(STATUS "Test logging level: ${REALM_TEST_LOGGING_LEVEL}")
         target_compile_definitions(ObjectStoreTests PRIVATE
             TEST_LOGGING_LEVEL=${REALM_TEST_LOGGING_LEVEL}
         )
+    else()
+        message(STATUS "Test logging enabled")
     endif()
 endif()