Merge pull request #6837 from realm/tg/user-provider

Fix handling of users with multiple identities

Author: tgoyne

CHANGELOG.md

@@ -5,13 +5,24 @@
 
 ### Fixed
 * <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
-* None.
+* Logging into a single user using multiple auth providers created a separate SyncUser per auth provider. This mostly worked, but had some quirks:
+  - Sync sessions would not necessarily be associated with the specific SyncUser used to create them. As a result, querying a user for its sessions could give incorrect results, and logging one user out could close the wrong sessions.
+  - Existing local synchronized Realm files created using version of Realm from August - November 2020 would sometimes not be opened correctly and would instead be redownloaded.
+  - 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.
+  ([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
-* None.
+* 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.
+* The metadata Realm used to store sync users has had its schema version bumped. It is automatically migrated to the new version on first open. Downgrading to older version of Realm after upgrading will discard stored user tokens and require logging back in.
 
 -----------
 

src/realm.h

@@ -3232,13 +3232,9 @@ RLM_API realm_user_state_e realm_user_get_state(const realm_user_t* user) RLM_AP
 RLM_API bool realm_user_get_all_identities(const realm_user_t* user, realm_user_identity_t* out_identities,
                                            size_t capacity, size_t* out_n);
 
-RLM_API const char* realm_user_get_local_identity(const realm_user_t*) RLM_API_NOEXCEPT;
-
 // returned pointer must be manually released with realm_free()
 RLM_API char* realm_user_get_device_id(const realm_user_t*) RLM_API_NOEXCEPT;
 
-RLM_API realm_auth_provider_e realm_user_get_auth_provider(const realm_user_t*) RLM_API_NOEXCEPT;
-
 /**
  * Log out the user and mark it as logged out.
  *

src/realm/collection.hpp

@@ -19,7 +19,7 @@ struct CollectionIterator;
 /// Collections are bound to particular properties of an object. In a
 /// collection's public interface, the implementation must take care to keep the
 /// object consistent with the persisted state, mindful of the fact that the
-/// state may have changed as a consquence of modifications from other instances
+/// state may have changed as a consequence of modifications from other instances
 /// referencing the same persisted state.
 class CollectionBase {
 public:

src/realm/object-store/c_api/app.cpp

@@ -642,11 +642,6 @@ RLM_API bool realm_user_get_all_identities(const realm_user_t* user, realm_user_
     });
 }
 
-RLM_API const char* realm_user_get_local_identity(const realm_user_t* user) noexcept
-{
-    return (*user)->local_identity().c_str();
-}
-
 RLM_API char* realm_user_get_device_id(const realm_user_t* user) noexcept
 {
     if ((*user)->has_device_id()) {
@@ -656,11 +651,6 @@ RLM_API char* realm_user_get_device_id(const realm_user_t* user) noexcept
     return nullptr;
 }
 
-RLM_API realm_auth_provider_e realm_user_get_auth_provider(const realm_user_t* user) noexcept
-{
-    return realm_auth_provider_e(enum_from_provider_type((*user)->provider_type()));
-}
-
 RLM_API bool realm_user_log_out(realm_user_t* user)
 {
     return wrap_err([&] {

src/realm/object-store/c_api/types.hpp

@@ -661,7 +661,7 @@ struct realm_user : realm::c_api::WrapC, std::shared_ptr<realm::SyncUser> {
     bool equals(const WrapC& other) const noexcept final
     {
         if (auto ptr = dynamic_cast<const realm_user*>(&other)) {
-            return *get() == *(ptr->get());
+            return get() == ptr->get();
         }
         return false;
     }

src/realm/object-store/impl/realm_coordinator.cpp

@@ -183,7 +183,7 @@ void RealmCoordinator::set_config(const Realm::Config& config)
         if (config.sync_config) {
             auto old_user = m_config.sync_config->user;
             auto new_user = config.sync_config->user;
-            if (old_user && new_user && *old_user != *new_user) {
+            if (old_user != new_user) {
                 throw LogicError(
                     ErrorCodes::MismatchedConfig,
                     util::format("Realm at path '%1' already opened with different sync user.", config.path));

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

@@ -595,9 +595,8 @@ void App::get_profile(const std::shared_ptr<SyncUser>& sync_user,
                         SyncUserIdentity(get<std::string>(doc, "id"), get<std::string>(doc, "provider_type")));
                 }
 
-                sync_user->update_identities(identities);
-                sync_user->update_user_profile(SyncUserProfile(get<BsonDocument>(profile_json, "data")));
-                sync_user->set_state(SyncUser::State::LoggedIn);
+                sync_user->update_user_profile(std::move(identities),
+                                               SyncUserProfile(get<BsonDocument>(profile_json, "data")));
                 self->m_sync_manager->set_current_user(sync_user->identity());
                 self->emit_change_to_subscribers(*self);
             }
@@ -644,7 +643,7 @@ void App::log_in_with_credentials(
     // is already an anonymous session active, reuse it
     if (credentials.provider() == AuthProvider::ANONYMOUS) {
         for (auto&& user : m_sync_manager->all_users()) {
-            if (user->provider_type() == credentials.provider_as_string() && user->is_logged_in()) {
+            if (user->is_anonymous()) {
                 completion(switch_user(user), util::none);
                 return;
             }
@@ -686,8 +685,7 @@ void App::log_in_with_credentials(
                 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"));
+                        get<std::string>(json, "access_token"), get<std::string>(json, "device_id"));
                 }
             }
             catch (const AppError& e) {
@@ -758,11 +756,7 @@ std::shared_ptr<SyncUser> App::switch_user(const std::shared_ptr<SyncUser>& user
     if (!user || user->state() != SyncUser::State::LoggedIn) {
         throw AppError(ErrorCodes::ClientUserNotLoggedIn, "User is no longer valid or is logged out");
     }
-
-    auto users = m_sync_manager->all_users();
-    auto it = std::find(users.begin(), users.end(), user);
-
-    if (it == users.end()) {
+    if (!verify_user_present(user)) {
         throw AppError(ErrorCodes::ClientUserNotFound, "User does not exist");
     }
 

src/realm/object-store/sync/impl/sync_file.cpp

@@ -295,11 +295,11 @@ bool SyncFileManager::copy_realm_file(const std::string& old_path, const std::st
     return true;
 }
 
-bool SyncFileManager::remove_realm(const std::string& user_identity, const std::string& local_identity,
+bool SyncFileManager::remove_realm(const std::string& user_identity,
+                                   const std::vector<std::string>& legacy_user_identities,
                                    const std::string& raw_realm_path, const std::string& partition) const
 {
-    util::Optional<std::string> existing =
-        get_existing_realm_file_path(user_identity, local_identity, raw_realm_path, partition);
+    auto existing = get_existing_realm_file_path(user_identity, legacy_user_identities, raw_realm_path, partition);
     if (existing) {
         return remove_realm(*existing);
     }
@@ -327,10 +327,10 @@ static bool try_file_remove(const std::string& path) noexcept
     }
 }
 
-util::Optional<std::string> SyncFileManager::get_existing_realm_file_path(const std::string& user_identity,
-                                                                          const std::string& local_user_identity,
-                                                                          const std::string& realm_file_name,
-                                                                          const std::string& partition) const
+util::Optional<std::string>
+SyncFileManager::get_existing_realm_file_path(const std::string& user_identity,
+                                              const std::vector<std::string>& legacy_user_identities,
+                                              const std::string& realm_file_name, const std::string& partition) const
 {
     std::string preferred_name = preferred_realm_path_without_suffix(user_identity, realm_file_name);
     if (try_file_exists(preferred_name)) {
@@ -365,14 +365,14 @@ util::Optional<std::string> SyncFileManager::get_existing_realm_file_path(const
         }
     }
 
-    if (!local_user_identity.empty()) {
+    for (auto& legacy_identity : legacy_user_identities) {
         // retain support for legacy paths
-        std::string old_path = legacy_realm_file_path(local_user_identity, realm_file_name);
+        std::string old_path = legacy_realm_file_path(legacy_identity, realm_file_name);
         if (try_file_exists(old_path)) {
             return old_path;
         }
         // retain support for legacy local identity paths
-        std::string old_local_identity_path = legacy_local_identity_path(local_user_identity, partition);
+        std::string old_local_identity_path = legacy_local_identity_path(legacy_identity, partition);
         if (try_file_exists(old_local_identity_path)) {
             return old_local_identity_path;
         }
@@ -381,11 +381,12 @@ util::Optional<std::string> SyncFileManager::get_existing_realm_file_path(const
     return util::none;
 }
 
-std::string SyncFileManager::realm_file_path(const std::string& user_identity, const std::string& local_user_identity,
+std::string SyncFileManager::realm_file_path(const std::string& user_identity,
+                                             const std::vector<std::string>& legacy_user_identities,
                                              const std::string& realm_file_name, const std::string& partition) const
 {
-    util::Optional<std::string> existing_path =
-        get_existing_realm_file_path(user_identity, local_user_identity, realm_file_name, partition);
+    auto existing_path =
+        get_existing_realm_file_path(user_identity, legacy_user_identities, realm_file_name, partition);
     if (existing_path) {
         return *existing_path;
     }

src/realm/object-store/sync/impl/sync_file.hpp

@@ -69,15 +69,16 @@ class SyncFileManager {
     static bool try_file_exists(const std::string& path) noexcept;
 
     util::Optional<std::string> get_existing_realm_file_path(const std::string& user_identity,
-                                                             const std::string& local_user_identity,
+                                                             const std::vector<std::string>& legacy_user_identities,
                                                              const std::string& realm_file_name,
                                                              const std::string& partition) const;
     /// Return the path for a given Realm, creating the user directory if it does not already exist.
-    std::string realm_file_path(const std::string& user_identity, const std::string& local_user_identity,
+    std::string realm_file_path(const std::string& user_identity,
+                                const std::vector<std::string>& legacy_user_identities,
                                 const std::string& realm_file_name, const std::string& partition) const;
 
     /// Remove the Realm at a given path for a given user. Returns `true` if the remove operation fully succeeds.
-    bool remove_realm(const std::string& user_identity, const std::string& local_identity,
+    bool remove_realm(const std::string& user_identity, const std::vector<std::string>& legacy_user_identities,
                       const std::string& realm_file_name, const std::string& partition) const;
 
     /// Remove the Realm whose primary Realm file is located at `absolute_path`. Returns `true` if the remove