From dd7e4faef215f1d03aadc2f43b8ecae6c2190fa6 Mon Sep 17 00:00:00 2001 From: Thomas Goyne Date: Wed, 2 Aug 2023 09:41:16 -0700 Subject: [PATCH] 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. --- CHANGELOG.md | 11 +- src/realm/object-store/sync/sync_manager.cpp | 8 +- src/realm/object-store/sync/sync_user.cpp | 118 ++++++++----------- src/realm/object-store/sync/sync_user.hpp | 21 ++-- test/object-store/c_api/c_api.cpp | 2 +- test/object-store/realm.cpp | 2 +- test/object-store/sync/app.cpp | 36 ++++-- test/object-store/sync/user.cpp | 8 +- 8 files changed, 92 insertions(+), 114 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1205d179189..2d80254703d 100644 --- 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 index 3fd8e2cbc36..0b7b06cfb9e 100644 --- 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 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 index 1b235a4cb31..1f113287856 100644 --- 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 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 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> 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> SyncUser::revive_sessions() { std::vector> sessions_to_revive; @@ -239,37 +257,19 @@ std::vector> SyncUser::revive_sessions() void SyncUser::update_access_token(std::string&& token) { - std::vector> 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(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(threshold); + return !m_access_token.token.empty() && m_access_token.expires_at < static_cast(threshold); } } // namespace realm diff --git a/src/realm/object-store/sync/sync_user.hpp b/src/realm/object-store/sync/sync_user.hpp index 994607bec44..3f41cc77d99 100644 --- 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, 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, 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, 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> revive_sessions() REQUIRES(m_mutex); @@ -305,9 +301,6 @@ class SyncUser : public std::enable_shared_from_this, public Subscriba // used to locate existing files. std::vector 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 index 2c6637d851f..481dec645e6 100644 --- 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 index 666955ecb8a..4cd47753832 100644 --- 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, SyncError) { diff --git a/test/object-store/sync/app.cpp b/test/object-store/sync/app.cpp index 6d8c7c2d811..5251e180bda 100644 --- 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 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> 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 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 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; diff --git a/test/object-store/sync/user.cpp b/test/object-store/sync/user.cpp index 51a046cdb27..f4643789bdd 100644 --- a/test/object-store/sync/user.cpp +++ b/test/object-store/sync/user.cpp @@ -97,19 +97,19 @@ TEST_CASE("sync_user: update state and tokens", "[sync][user]") { REQUIRE(user->is_logged_in()); REQUIRE(user->refresh_token() == refresh_token); - user->update_state_and_tokens(SyncUser::State::LoggedIn, second_access_token, second_refresh_token); + user->log_in(second_access_token, second_refresh_token); REQUIRE(user->is_logged_in()); REQUIRE(user->refresh_token() == second_refresh_token); - user->update_state_and_tokens(SyncUser::State::LoggedOut, "", ""); + user->log_out(); REQUIRE(!user->is_logged_in()); REQUIRE(user->refresh_token().empty()); - user->update_state_and_tokens(SyncUser::State::LoggedIn, access_token, refresh_token); + user->log_in(access_token, refresh_token); REQUIRE(user->is_logged_in()); REQUIRE(user->refresh_token() == refresh_token); - sync_manager->remove_user(identity); + user->invalidate(); } TEST_CASE("sync_user: SyncManager `get_existing_logged_in_user()` API", "[sync][user]") {