Adjust SyncUser's internal API to no longer permit invalid states

SyncUser previously allowed setting the state to LoggedIn without setting its tokens, and conversely allowed setting tokens while not logged in. This was error-prone and happened to result in a lock-order inversion due to that both the state and the tokens had to be checked in places where we only wanted to care about one of them.
realm · Sep 19, 2023 · 64bf842 · 64bf842
1 parent a365369
commit 64bf842
Show file tree

Hide file tree

Showing 8 changed files with 92 additions and 114 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,13 +11,14 @@
   - Removing one of the SyncUsers would delete all local Realm files for all SyncUsers for that user.
   - Deleting the server-side user via one of the SyncUsers left the other SyncUsers in an invalid state.
   - A SyncUser which was originally created via anonymous login and then linked to an identity would still be treated as an anonymous users and removed entirely on logout.
-  (since v10.0.0)
-* Reading existing logged-in users on app startup from the sync metadata Realm performed three no-op writes per user on the metadata Realm.
+  ([PR #6837](https://github.com/realm/realm-core/pull/6837), since v10.0.0)
+* Reading existing logged-in users on app startup from the sync metadata Realm performed three no-op writes per user on the metadata Realm ([PR #6837](https://github.com/realm/realm-core/pull/6837), since v10.0.0).
+* If a user was logged out while an access token refresh was in progress, the refresh completing would mark the user as logged in again and the user would be in an inconsistent state ([PR #6837](https://github.com/realm/realm-core/pull/6837), since v10.0.0).
 
 ### Breaking changes
-* SyncUser::provider_type() and realm_user_get_auth_provider() have been removed. Users don't have provider types; identities do.
-* SyncUser no longer has a `local_identity()`. `identity()` has been guaranteed to be unique per App ever since v10.
-* SyncUser no longer overrides operator==. Pointer equality should be used to compare sync users.
+* SyncUser::provider_type() and realm_user_get_auth_provider() have been removed. Users don't have provider types; identities do. `SyncUser::is_anonymous()` is a more correct version of checking if the provider type is anonymous ([PR #6837](https://github.com/realm/realm-core/pull/6837)).
+* SyncUser no longer has a `local_identity()`. `identity()` has been guaranteed to be unique per App ever since v10 ([PR #6837](https://github.com/realm/realm-core/pull/6837)).
+* SyncUser no longer overrides operator==. Pointer equality should be used to compare sync users ([PR #6837](https://github.com/realm/realm-core/pull/6837)).
 
 ### Compatibility
 * Fileformat: Generates files with format v23. Reads and automatically upgrade from fileformat v5.

diff --git a/src/realm/object-store/sync/sync_manager.cpp b/src/realm/object-store/sync/sync_manager.cpp
@@ -355,7 +355,7 @@ std::shared_ptr<SyncUser> SyncManager::get_user(const std::string& user_id, cons
     else { // LoggedOut => LoggedIn
         auto user = *it;
         REALM_ASSERT(user->state() != SyncUser::State::Removed);
-        user->update_state_and_tokens(SyncUser::State::LoggedIn, std::move(access_token), std::move(refresh_token));
+        user->log_in(access_token, refresh_token);
         return user;
     }
 }
@@ -443,10 +443,8 @@ void SyncManager::set_current_user(const std::string& user_id)
 void SyncManager::remove_user(const std::string& user_id)
 {
     util::CheckedLockGuard lock(m_user_mutex);
-    auto user = get_user_for_identity(user_id);
-    if (!user)
-        return;
-    user->set_state(SyncUser::State::Removed);
+    if (auto user = get_user_for_identity(user_id))
+        user->invalidate();
 }
 
 void SyncManager::delete_user(const std::string& user_id)

diff --git a/src/realm/object-store/sync/sync_user.cpp b/src/realm/object-store/sync/sync_user.cpp
@@ -93,6 +93,7 @@ SyncUser::SyncUser(const std::string& refresh_token, const std::string& id, cons
     , m_device_id(device_id)
     , m_sync_manager(sync_manager)
 {
+    REALM_ASSERT(!access_token.empty() && !refresh_token.empty());
     {
         std::lock_guard lock(s_binding_context_factory_mutex);
         if (s_binding_context_factory) {
@@ -120,6 +121,15 @@ SyncUser::SyncUser(const SyncUserMetadata& data, SyncManager* sync_manager)
     , m_device_id(data.device_id())
     , m_sync_manager(sync_manager)
 {
+    // Check for inconsistent state in the metadata Realm. This shouldn't happen,
+    // but previous versions could sometimes mark a user as logged in with an
+    // empty refresh token.
+    if (m_state == State::LoggedIn && (m_refresh_token.token.empty() || m_access_token.token.empty())) {
+        m_state = State::LoggedOut;
+        m_refresh_token = {};
+        m_access_token = {};
+    }
+
     {
         std::lock_guard lock(s_binding_context_factory_mutex);
         if (s_binding_context_factory) {
@@ -132,8 +142,10 @@ std::shared_ptr<SyncManager> SyncUser::sync_manager() const
 {
     util::CheckedLockGuard lk(m_mutex);
     if (m_state == State::Removed) {
-        throw std::logic_error(util::format(
-            "Cannot start a sync session for user '%1' because this user has been removed.", identity()));
+        throw app::AppError(
+            ErrorCodes::ClientUserNotFound,
+            util::format("Cannot start a sync session for user '%1' because this user has been removed.",
+                         m_identity));
     }
     REALM_ASSERT(m_sync_manager);
     return m_sync_manager->shared_from_this();
@@ -184,33 +196,22 @@ std::shared_ptr<SyncSession> SyncUser::session_for_on_disk_path(const std::strin
     return locked;
 }
 
-void SyncUser::update_state_and_tokens(SyncUser::State state, const std::string& access_token,
-                                       const std::string& refresh_token)
+void SyncUser::log_in(const std::string& access_token, const std::string& refresh_token)
 {
+    REALM_ASSERT(!access_token.empty());
+    REALM_ASSERT(!refresh_token.empty());
     std::vector<std::shared_ptr<SyncSession>> sessions_to_revive;
     {
         util::CheckedLockGuard lock1(m_mutex);
         util::CheckedLockGuard lock2(m_tokens_mutex);
-        m_state = state;
-        m_access_token = access_token.empty() ? RealmJWT{} : RealmJWT(access_token);
-        m_refresh_token = refresh_token.empty() ? RealmJWT{} : RealmJWT(refresh_token);
-        switch (m_state) {
-            case State::Removed:
-                // Call set_state() rather than update_state_and_tokens to remove a user.
-                REALM_UNREACHABLE();
-            case State::LoggedIn:
-                sessions_to_revive = revive_sessions();
-                break;
-            case State::LoggedOut: {
-                REALM_ASSERT(m_access_token == RealmJWT{});
-                REALM_ASSERT(m_refresh_token == RealmJWT{});
-                break;
-            }
-        }
+        m_state = State::LoggedIn;
+        m_access_token = RealmJWT(access_token);
+        m_refresh_token = RealmJWT(refresh_token);
+        sessions_to_revive = revive_sessions();
 
         m_sync_manager->perform_metadata_update([&](const auto& manager) {
             auto metadata = manager.get_or_make_user_metadata(m_identity);
-            metadata->set_state_and_tokens(state, access_token, refresh_token);
+            metadata->set_state_and_tokens(State::LoggedIn, access_token, refresh_token);
         });
     }
     // (Re)activate all pending sessions.
@@ -223,6 +224,23 @@ void SyncUser::update_state_and_tokens(SyncUser::State state, const std::string&
     emit_change_to_subscribers(*this);
 }
 
+void SyncUser::invalidate()
+{
+    {
+        util::CheckedLockGuard lock1(m_mutex);
+        util::CheckedLockGuard lock2(m_tokens_mutex);
+        m_state = State::Removed;
+        m_access_token = {};
+        m_refresh_token = {};
+
+        m_sync_manager->perform_metadata_update([&](const auto& manager) {
+            auto metadata = manager.get_or_make_user_metadata(m_identity);
+            metadata->set_state_and_tokens(State::Removed, "", "");
+        });
+    }
+    emit_change_to_subscribers(*this);
+}
+
 std::vector<std::shared_ptr<SyncSession>> SyncUser::revive_sessions()
 {
     std::vector<std::shared_ptr<SyncSession>> sessions_to_revive;
@@ -239,37 +257,19 @@ std::vector<std::shared_ptr<SyncSession>> SyncUser::revive_sessions()
 
 void SyncUser::update_access_token(std::string&& token)
 {
-    std::vector<std::shared_ptr<SyncSession>> sessions_to_revive;
     {
         util::CheckedLockGuard lock(m_mutex);
-        util::CheckedLockGuard lock2(m_tokens_mutex);
-        switch (m_state) {
-            case State::Removed:
-                return;
-            case State::LoggedIn:
-                m_access_token = RealmJWT(std::move(token));
-                break;
-            case State::LoggedOut: {
-                m_access_token = RealmJWT(std::move(token));
-                m_state = State::LoggedIn;
-                sessions_to_revive = revive_sessions();
-                break;
-            }
-        }
+        if (m_state != State::LoggedIn)
+            return;
 
+        util::CheckedLockGuard lock2(m_tokens_mutex);
+        m_access_token = RealmJWT(std::move(token));
         m_sync_manager->perform_metadata_update([&, raw_access_token = m_access_token.token](const auto& manager) {
             auto metadata = manager.get_or_make_user_metadata(m_identity);
             metadata->set_access_token(raw_access_token);
         });
     }
 
-    // (Re)activate all pending sessions.
-    // Note that we do this after releasing the lock, since the session may
-    // need to access protected User state in the process of binding itself.
-    for (auto& session : sessions_to_revive) {
-        session->revive_if_needed();
-    }
-
     emit_change_to_subscribers(*this);
 }
 
@@ -333,13 +333,7 @@ void SyncUser::log_out()
 bool SyncUser::is_logged_in() const
 {
     util::CheckedLockGuard lock(m_mutex);
-    util::CheckedLockGuard lock2(m_tokens_mutex);
-    return do_is_logged_in();
-}
-
-bool SyncUser::do_is_logged_in() const
-{
-    return !m_access_token.token.empty() && !m_refresh_token.token.empty() && m_state == State::LoggedIn;
+    return m_state == State::LoggedIn;
 }
 
 bool SyncUser::is_anonymous() const
@@ -351,15 +345,10 @@ bool SyncUser::is_anonymous() const
 
 bool SyncUser::do_is_anonymous() const
 {
-    return do_is_logged_in() && m_user_identities.size() == 1 &&
+    return m_state == State::LoggedIn && m_user_identities.size() == 1 &&
            m_user_identities[0].provider_type == app::IdentityProviderAnonymous;
 }
 
-void SyncUser::invalidate()
-{
-    set_state(SyncUser::State::Removed);
-}
-
 std::string SyncUser::refresh_token() const
 {
     util::CheckedLockGuard lock(m_tokens_mutex);
@@ -390,18 +379,6 @@ SyncUser::State SyncUser::state() const
     return m_state;
 }
 
-void SyncUser::set_state(SyncUser::State state)
-{
-    util::CheckedLockGuard lock(m_mutex);
-    m_state = state;
-
-    REALM_ASSERT(m_sync_manager);
-    m_sync_manager->perform_metadata_update([&](const auto& manager) {
-        auto metadata = manager.get_or_make_user_metadata(m_identity);
-        metadata->set_state(state);
-    });
-}
-
 SyncUserProfile SyncUser::user_profile() const
 {
     util::CheckedLockGuard lock(m_mutex);
@@ -506,12 +483,11 @@ bool SyncUser::access_token_refresh_required() const
 {
     using namespace std::chrono;
     constexpr size_t buffer_seconds = 5; // arbitrary
-    util::CheckedLockGuard lock(m_mutex);
-    util::CheckedLockGuard lock2(m_tokens_mutex);
+    util::CheckedLockGuard lock(m_tokens_mutex);
     const auto now = duration_cast<seconds>(system_clock::now().time_since_epoch()).count() +
                      m_seconds_to_adjust_time_for_testing.load(std::memory_order_relaxed);
     const auto threshold = now - buffer_seconds;
-    return do_is_logged_in() && m_access_token.expires_at < static_cast<int64_t>(threshold);
+    return !m_access_token.token.empty() && m_access_token.expires_at < static_cast<int64_t>(threshold);
 }
 
 } // namespace realm

diff --git a/src/realm/object-store/sync/sync_user.hpp b/src/realm/object-store/sync/sync_user.hpp
@@ -243,15 +243,12 @@ class SyncUser : public std::enable_shared_from_this<SyncUser>, public Subscriba
              const std::string& device_id, SyncManager* sync_manager);
     SyncUser(const SyncUserMetadata& data, SyncManager* sync_manager);
 
-    void set_state(SyncUser::State state) REQUIRES(!m_mutex);
+    // Atomically set the user to be logged in and update both tokens.
+    void log_in(const std::string& access_token, const std::string& refresh_token)
+        REQUIRES(!m_mutex, !m_tokens_mutex);
 
-    // Update the user's state and refresh/access tokens atomically in a Realm transaction.
-    // If the user is transitioning between LoggedIn and LoggedOut, then the access_token and
-    // refresh token must be empty, and likewise must not be empty if transitioning between
-    // logged out and logged in.
-    // Note that this is called by the SyncManager, and should not be directly called.
-    void update_state_and_tokens(SyncUser::State state, const std::string& access_token,
-                                 const std::string& refresh_token) REQUIRES(!m_mutex, !m_tokens_mutex);
+    // Atomically set the user to be removed and remove tokens.
+    void invalidate() REQUIRES(!m_mutex, !m_tokens_mutex);
 
     // Update the user's access token. If the user is logged out, it will log itself back in.
     // Note that this is called by the SyncManager, and should not be directly called.
@@ -276,7 +273,7 @@ class SyncUser : public std::enable_shared_from_this<SyncUser>, public Subscriba
 
     /// Checks the expiry on the access token against the local time and if it is invalid or expires soon, returns
     /// true.
-    bool access_token_refresh_required() const REQUIRES(!m_mutex, !m_tokens_mutex);
+    bool access_token_refresh_required() const REQUIRES(!m_tokens_mutex);
 
     // Hook for testing access token timeouts
     void set_seconds_to_adjust_time_for_testing(int seconds)
@@ -292,8 +289,7 @@ class SyncUser : public std::enable_shared_from_this<SyncUser>, public Subscriba
     static SyncUserContextFactory s_binding_context_factory;
     static std::mutex s_binding_context_factory_mutex;
 
-    bool do_is_logged_in() const REQUIRES(m_tokens_mutex, m_mutex);
-    bool do_is_anonymous() const REQUIRES(m_tokens_mutex, m_mutex);
+    bool do_is_anonymous() const REQUIRES(m_mutex);
 
     std::vector<std::shared_ptr<SyncSession>> revive_sessions() REQUIRES(m_mutex);
 
@@ -305,9 +301,6 @@ class SyncUser : public std::enable_shared_from_this<SyncUser>, public Subscriba
     // used to locate existing files.
     std::vector<std::string> m_legacy_identities;
 
-    // Mark the user as invalid, since a fatal user-related error was encountered.
-    void invalidate() REQUIRES(!m_mutex);
-
     mutable util::CheckedMutex m_mutex;
 
     // Set by the server. The unique ID of the user account on the Realm Application.

diff --git a/test/object-store/c_api/c_api.cpp b/test/object-store/c_api/c_api.cpp
@@ -5006,7 +5006,7 @@ TEST_CASE("C API - async_open", "[sync][pbs][c_api]") {
         realm_sync_config_t* sync_config = realm_sync_config_new(&user, "realm");
         realm_sync_config_set_initial_subscription_handler(sync_config, task_init_subscription, false, nullptr,
                                                            nullptr);
-        sync_config->user->update_state_and_tokens(SyncUser::State::LoggedIn, invalid_token, invalid_token);
+        sync_config->user->log_in(invalid_token, invalid_token);
 
         realm_config_set_path(config, test_config.path.c_str());
         realm_config_set_schema_version(config, 1);

diff --git a/test/object-store/realm.cpp b/test/object-store/realm.cpp
@@ -1184,7 +1184,7 @@ TEST_CASE("Get Realm using Async Open", "[sync][pbs][async open]") {
         TestSyncManager tsm(tsm_config);
 
         SyncTestFile config(tsm.app(), "realm");
-        config.sync_config->user->update_state_and_tokens(SyncUser::State::LoggedIn, invalid_token, invalid_token);
+        config.sync_config->user->log_in(invalid_token, invalid_token);
 
         bool got_error = false;
         config.sync_config->error_handler = [&](std::shared_ptr<SyncSession>, SyncError) {

diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp
@@ -2868,8 +2868,8 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") {
         std::shared_ptr<SyncUser> user = app->current_user();
         REQUIRE(user);
         REQUIRE(!user->access_token_refresh_required());
-        // Make the SyncUser behave as if the client clock is 31 minutes fast, so the token looks expired locallaly
-        // (access tokens have an lifetime of 30 mintutes today).
+        // Make the SyncUser behave as if the client clock is 31 minutes fast, so the token looks expired locally
+        // (access tokens have an lifetime of 30 minutes today).
         user->set_seconds_to_adjust_time_for_testing(31 * 60);
         REQUIRE(user->access_token_refresh_required());
 
@@ -2982,6 +2982,17 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") {
             REQUIRE(!user->is_logged_in());
         }
 
+        SECTION("User is left logged out if logged out while the refresh is in progress") {
+            REQUIRE(user->is_logged_in());
+            transport->request_hook = [&](const Request&) {
+                user->log_out();
+            };
+            SyncTestFile config(app, partition, schema);
+            auto r = Realm::get_shared_realm(config);
+            REQUIRE_FALSE(user->is_logged_in());
+            REQUIRE(user->state() == SyncUser::State::LoggedOut);
+        }
+
         SECTION("Requests that receive an error are retried on a backoff") {
             using namespace std::chrono;
             std::vector<time_point<steady_clock>> response_times;
@@ -3193,11 +3204,10 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") {
                                      anon_user->identity()));
             });
 
-            REQUIRE_THROWS_MATCHES(
-                Realm::get_shared_realm(config), std::logic_error,
-                Catch::Matchers::Message(
-                    util::format("Cannot start a sync session for user '%1' because this user has been removed.",
-                                 anon_user->identity())));
+            REQUIRE_EXCEPTION(
+                Realm::get_shared_realm(config), ClientUserNotFound,
+                util::format("Cannot start a sync session for user '%1' because this user has been removed.",
+                             anon_user->identity()));
         }
 
         SECTION("Opening a Realm with a removed email user results produces an exception") {
@@ -3216,11 +3226,11 @@ TEST_CASE("app: sync integration", "[sync][pbs][app][baas]") {
             REQUIRE_FALSE(email_user->is_logged_in());
             REQUIRE(email_user->state() == SyncUser::State::Removed);
 
-            // should not be able to open a sync'd Realm with an invalid user
-            REQUIRE_THROWS_MATCHES(
-                Realm::get_shared_realm(config), std::logic_error,
-                Catch::Matchers::Message(util::format(
-                    "Cannot start a sync session for user '%1' because this user has been removed.", user_ident)));
+            // should not be able to open a synced Realm with an invalid user
+            REQUIRE_EXCEPTION(
+                Realm::get_shared_realm(config), ClientUserNotFound,
+                util::format("Cannot start a sync session for user '%1' because this user has been removed.",
+                             user_ident));
 
             std::shared_ptr<SyncUser> new_user_instance = log_in(app, creds);
             // the previous instance is still invalid
@@ -4891,7 +4901,7 @@ TEST_CASE("app: app destroyed during token refresh", "[sync][app][user][token]")
             // Ignore these errors, since there's not really an app out there...
             // Primarily make sure we don't crash unexpectedly
             std::vector<const char*> expected_errors = {"Bad WebSocket", "Connection Failed", "user has been removed",
-                                                        "Connection refused"};
+                                                        "Connection refused", "The user is not logged in"};
             auto expected =
                 std::find_if(expected_errors.begin(), expected_errors.end(), [error](const char* err_msg) {
                     return error.status.reason().find(err_msg) != std::string::npos;