firebase-ios-sdk/1499: Restart streams on any token change. (#1692)

[Port of firebase/firebase-js-sdk#1120]

Fixes #1499.

This reworks our "user listener" into a "credential change listener" that
fires on any token change, even if the User did not change. This allows
us to restart our streams (but not switch mutation queues, etc.) on token
changes.

Author: mikelehen

Firestore/Example/Tests/SpecTests/FSTSyncEngineTestDriver.mm

@@ -250,7 +250,7 @@ - (void)runTimer:(FSTTimerID)timerID {
 - (void)changeUser:(const User &)user {
   _currentUser = user;
   [self.dispatchQueue dispatchSync:^{
-    [self.syncEngine userDidChange:user];
+    [self.syncEngine credentialDidChangeWithUser:user];
   }];
 }
 

Firestore/Source/Core/FSTFirestoreClient.mm

@@ -128,8 +128,8 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
     auto userPromise = std::make_shared<std::promise<User>>();
 
     __weak typeof(self) weakSelf = self;
-    auto userChangeListener = [initialized = false, userPromise, weakSelf,
-                               workerDispatchQueue](User user) mutable {
+    auto credentialChangeListener = [initialized = false, userPromise, weakSelf,
+                                     workerDispatchQueue](User user) mutable {
       typeof(self) strongSelf = weakSelf;
       if (!strongSelf) return;
 
@@ -138,14 +138,14 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
         userPromise->set_value(user);
       } else {
         [workerDispatchQueue dispatchAsync:^{
-          [strongSelf userDidChange:user];
+          [strongSelf credentialDidChangeWithUser:user];
         }];
       }
     };
 
-    _credentialsProvider->SetUserChangeListener(userChangeListener);
+    _credentialsProvider->SetCredentialChangeListener(credentialChangeListener);
 
-    // Defer initialization until we get the current user from the userChangeListener. This is
+    // Defer initialization until we get the current user from the credentialChangeListener. This is
     // guaranteed to be synchronously dispatched onto our worker queue, so we will be initialized
     // before any subsequently queued work runs.
     [_workerDispatchQueue dispatchAsync:^{
@@ -159,6 +159,7 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
 - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistence {
   // Do all of our initialization on our own dispatch queue.
   [self.workerDispatchQueue verifyIsCurrentQueue];
+  LOG_DEBUG("Initializing. Current user: %s", user.uid());
 
   // Note: The initialization work must all be synchronous (we can't dispatch more work) since
   // external write/listen operations could get queued to run before that subsequent work
@@ -213,11 +214,11 @@ - (void)initializeWithUser:(const User &)user usePersistence:(BOOL)usePersistenc
   [_remoteStore start];
 }
 
-- (void)userDidChange:(const User &)user {
+- (void)credentialDidChangeWithUser:(const User &)user {
   [self.workerDispatchQueue verifyIsCurrentQueue];
 
-  LOG_DEBUG("User Changed: %s", user.uid());
-  [self.syncEngine userDidChange:user];
+  LOG_DEBUG("Credential Changed. Current user: %s", user.uid());
+  [self.syncEngine credentialDidChangeWithUser:user];
 }
 
 - (void)applyChangedOnlineState:(FSTOnlineState)onlineState {
@@ -245,7 +246,7 @@ - (void)enableNetworkWithCompletion:(nullable FSTVoidErrorBlock)completion {
 
 - (void)shutdownWithCompletion:(nullable FSTVoidErrorBlock)completion {
   [self.workerDispatchQueue dispatchAsync:^{
-    self->_credentialsProvider->SetUserChangeListener(nullptr);
+    self->_credentialsProvider->SetCredentialChangeListener(nullptr);
 
     [self.remoteStore shutdown];
     [self.persistence shutdown];

Firestore/Source/Core/FSTSyncEngine.h

@@ -100,7 +100,7 @@ NS_ASSUME_NONNULL_BEGIN
                    updateBlock:(FSTTransactionBlock)updateBlock
                     completion:(FSTVoidIDErrorBlock)completion;
 
-- (void)userDidChange:(const firebase::firestore::auth::User &)user;
+- (void)credentialDidChangeWithUser:(const firebase::firestore::auth::User &)user;
 
 /** Applies an FSTOnlineState change to the sync engine and notifies any views of the change. */
 - (void)applyChangedOnlineState:(FSTOnlineState)onlineState;

Firestore/Source/Core/FSTSyncEngine.mm

@@ -560,15 +560,18 @@ - (void)removeLimboTargetForKey:(const DocumentKey &)key {
   return _limboTargetsByKey;
 }
 
-- (void)userDidChange:(const User &)user {
+- (void)credentialDidChangeWithUser:(const firebase::firestore::auth::User &)user {
+  BOOL userChanged = (_currentUser != user);
   _currentUser = user;
 
-  // Notify local store and emit any resulting events from swapping out the mutation queue.
-  FSTMaybeDocumentDictionary *changes = [self.localStore userDidChange:user];
-  [self emitNewSnapshotsWithChanges:changes remoteEvent:nil];
+  if (userChanged) {
+    // Notify local store and emit any resulting events from swapping out the mutation queue.
+    FSTMaybeDocumentDictionary *changes = [self.localStore userDidChange:user];
+    [self emitNewSnapshotsWithChanges:changes remoteEvent:nil];
+  }
 
   // Notify remote store so it can restart its streams.
-  [self.remoteStore userDidChange:user];
+  [self.remoteStore credentialDidChange];
 }
 
 - (firebase::firestore::model::DocumentKeySet)remoteKeysForTarget:(FSTBoxedTargetID *)targetId {

Firestore/Source/Remote/FSTRemoteStore.h

@@ -129,7 +129,7 @@ NS_ASSUME_NONNULL_BEGIN
  * In response the remote store tears down streams and clears up any tracked operations that should
  * not persist across users. Restarts the streams if appropriate.
  */
-- (void)userDidChange:(const firebase::firestore::auth::User &)user;
+- (void)credentialDidChange;
 
 /** Listens to the target identified by the given FSTQueryData. */
 - (void)listenToTargetWithQueryData:(FSTQueryData *)queryData;

Firestore/Source/Remote/FSTRemoteStore.mm

@@ -209,12 +209,12 @@ - (void)shutdown {
   [self.onlineStateTracker updateState:FSTOnlineStateUnknown];
 }
 
-- (void)userDidChange:(const User &)user {
-  LOG_DEBUG("FSTRemoteStore %s changing users: %s", (__bridge void *)self, user.uid());
+- (void)credentialDidChange {
   if ([self isNetworkEnabled]) {
     // Tear down and re-create our network streams. This will ensure we get a fresh auth token
     // for the new user and re-fill the write pipeline with new mutations from the LocalStore
     // (since mutations are per-user).
+    LOG_DEBUG("FSTRemoteStore %s restarting streams for new credential", (__bridge void *)self);
     [self disableNetworkInternal];
     [self.onlineStateTracker updateState:FSTOnlineStateUnknown];
     [self enableNetwork];

Firestore/core/src/firebase/firestore/auth/credentials_provider.cc

@@ -20,7 +20,7 @@ namespace firebase {
 namespace firestore {
 namespace auth {
 
-CredentialsProvider::CredentialsProvider() : user_change_listener_(nullptr) {
+CredentialsProvider::CredentialsProvider() : change_listener_(nullptr) {
 }
 
 CredentialsProvider::~CredentialsProvider() {

Firestore/core/src/firebase/firestore/auth/credentials_provider.h

@@ -33,8 +33,8 @@ namespace auth {
 // `TokenErrorListener` is a listener that gets a token or an error.
 using TokenListener = std::function<void(util::StatusOr<Token>)>;
 
-// Listener notified with a User change.
-using UserChangeListener = std::function<void(User user)>;
+// Listener notified with a credential change.
+using CredentialChangeListener = std::function<void(User user)>;
 
 /**
  * Provides methods for getting the uid and token for the current user and
@@ -56,22 +56,24 @@ class CredentialsProvider {
   virtual void InvalidateToken() = 0;
 
   /**
-   * Sets the listener to be notified of user changes (sign-in / sign-out). It
-   * is immediately called once with the initial user.
+   * Sets the listener to be notified of credential changes (sign-in /
+   * sign-out, token changes). It is immediately called once with the initial
+   * user.
    *
    * Call with nullptr to remove previous listener.
    */
-  virtual void SetUserChangeListener(UserChangeListener listener) = 0;
+  virtual void SetCredentialChangeListener(
+      CredentialChangeListener changeListener) = 0;
 
  protected:
   /**
-   * A listener to be notified of user changes (sign-in / sign-out). It is
-   * immediately called once with the initial user.
+   * A listener to be notified of credential changes (sign-in / sign-out, token
+   * changes). It is immediately called once with the initial user.
    *
    * Note that this block will be called back on an arbitrary thread that is not
    * the normal Firestore worker thread.
    */
-  UserChangeListener user_change_listener_;
+  CredentialChangeListener change_listener_;
 };
 
 }  // namespace auth

Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc

@@ -28,10 +28,10 @@ void EmptyCredentialsProvider::GetToken(TokenListener completion) {
   }
 }
 
-void EmptyCredentialsProvider::SetUserChangeListener(
-    UserChangeListener listener) {
-  if (listener) {
-    listener(User::Unauthenticated());
+void EmptyCredentialsProvider::SetCredentialChangeListener(
+    CredentialChangeListener changeListener) {
+  if (changeListener) {
+    changeListener(User::Unauthenticated());
   }
 }
 

Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.h

@@ -28,7 +28,8 @@ class EmptyCredentialsProvider : public CredentialsProvider {
  public:
   void GetToken(TokenListener completion) override;
   void InvalidateToken() override;
-  void SetUserChangeListener(UserChangeListener listener) override;
+  void SetCredentialChangeListener(
+      CredentialChangeListener changeListener) override;
 };
 
 }  // namespace auth

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h

@@ -43,9 +43,9 @@ namespace auth {
  * `FirebaseCredentialsProvider` uses Firebase Auth via `FIRApp` to get an auth
  * token.
  *
- * NOTE: To simplify the implementation, it requires that you set
- * `userChangeListener` with a non-`nil` value no more than once and don't call
- * `getTokenForcingRefresh:` after setting it to `nil`.
+ * NOTE: To simplify the implementation, it requires that you call
+ * `SetCredentialChangeListener()` with a non-nullptr value no more than once
+ * and don't call `GetToken()` after setting it to `nullptr`.
  *
  * This class must be implemented in a thread-safe manner since it is accessed
  * from the thread backing our internal worker queue and the callbacks from
@@ -70,7 +70,8 @@ class FirebaseCredentialsProvider : public CredentialsProvider {
 
   void GetToken(TokenListener completion) override;
 
-  void SetUserChangeListener(UserChangeListener listener) override;
+  void SetCredentialChangeListener(
+      CredentialChangeListener changeListener) override;
 
   void InvalidateToken() override;
 
@@ -96,10 +97,10 @@ class FirebaseCredentialsProvider : public CredentialsProvider {
     User current_user;
 
     /**
-     * Counter used to detect if the user changed while a
-     * -getTokenForcingRefresh: request was outstanding.
+     * Counter used to detect if the token changed while a GetToken() request
+     * was outstanding.
      */
-    int user_counter = 0;
+    int token_counter = 0;
 
     std::mutex mutex;
 

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm

@@ -61,14 +61,11 @@
 
                 NSString* user_id =
                     user_info[FIRAuthStateDidChangeInternalNotificationUIDKey];
-                User new_user = User::FromUid(user_id);
-                if (new_user != contents->current_user) {
-                  contents->current_user = new_user;
-                  contents->user_counter++;
-                  UserChangeListener listener = user_change_listener_;
-                  if (listener) {
-                    listener(contents->current_user);
-                  }
+                contents->current_user = User::FromUid(user_id);
+                contents->token_counter++;
+                CredentialChangeListener listener = change_listener_;
+                if (listener) {
+                  listener(contents->current_user);
                 }
               }];
 }
@@ -86,9 +83,9 @@
   HARD_ASSERT(auth_listener_handle_,
               "GetToken cannot be called after listener removed.");
 
-  // Take note of the current value of the userCounter so that this method can
-  // fail if there is a user change while the request is outstanding.
-  int initial_user_counter = contents_->user_counter;
+  // Take note of the current value of the tokenCounter so that this method can
+  // fail if there is a token change while the request is outstanding.
+  int initial_token_counter = contents_->token_counter;
 
   std::weak_ptr<Contents> weak_contents = contents_;
   void (^get_token_callback)(NSString*, NSError*) = ^(
@@ -99,12 +96,12 @@
     }
 
     std::unique_lock<std::mutex> lock(contents->mutex);
-    if (initial_user_counter != contents->user_counter) {
+    if (initial_token_counter != contents->token_counter) {
       // Cancel the request since the user changed while the request was
       // outstanding so the response is likely for a previous user (which
       // user, we can't be sure).
       completion(util::Status(FirestoreErrorCode::Aborted,
-                              "getToken aborted due to user change."));
+                              "getToken aborted due to token change."));
     } else {
       if (error == nil) {
         if (token != nil) {
@@ -139,21 +136,20 @@
   contents_->force_refresh = true;
 }
 
-void FirebaseCredentialsProvider::SetUserChangeListener(
-    UserChangeListener listener) {
+void FirebaseCredentialsProvider::SetCredentialChangeListener(
+    CredentialChangeListener changeListener) {
   std::unique_lock<std::mutex> lock(contents_->mutex);
-  if (listener) {
-    HARD_ASSERT(!user_change_listener_, "set user_change_listener twice!");
+  if (changeListener) {
+    HARD_ASSERT(!change_listener_, "set change_listener twice!");
     // Fire initial event.
-    listener(contents_->current_user);
+    changeListener(contents_->current_user);
   } else {
-    HARD_ASSERT(auth_listener_handle_, "removed user_change_listener twice!");
-    HARD_ASSERT(user_change_listener_,
-                "user_change_listener removed without being set!");
+    HARD_ASSERT(auth_listener_handle_, "removed change_listener twice!");
+    HARD_ASSERT(change_listener_, "change_listener removed without being set!");
     [[NSNotificationCenter defaultCenter] removeObserver:auth_listener_handle_];
     auth_listener_handle_ = nil;
   }
-  user_change_listener_ = listener;
+  change_listener_ = changeListener;
 }
 
 }  // namespace auth

Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc

@@ -36,7 +36,9 @@ TEST(CredentialsProvider, Typedef) {
   EXPECT_EQ(nullptr, token_listener);
   EXPECT_FALSE(token_listener);
 
-  UserChangeListener user_change_listener = [](User user) { UNUSED(user); };
+  CredentialChangeListener user_change_listener = [](User user) {
+    UNUSED(user);
+  };
   EXPECT_NE(nullptr, user_change_listener);
   EXPECT_TRUE(user_change_listener);
 

Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc

@@ -37,12 +37,12 @@ TEST(EmptyCredentialsProvider, GetToken) {
 
 TEST(EmptyCredentialsProvider, SetListener) {
   EmptyCredentialsProvider credentials_provider;
-  credentials_provider.SetUserChangeListener([](User user) {
+  credentials_provider.SetCredentialChangeListener([](User user) {
     EXPECT_EQ("", user.uid());
     EXPECT_FALSE(user.is_authenticated());
   });
 
-  credentials_provider.SetUserChangeListener(nullptr);
+  credentials_provider.SetCredentialChangeListener(nullptr);
 }
 
 TEST(EmptyCredentialsProvider, InvalidateToken) {

Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm

@@ -134,12 +134,12 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
   FSTAuthFake* auth =
       [[FSTAuthFake alloc] initWithToken:@"default token" uid:@"fake uid"];
   FirebaseCredentialsProvider credentials_provider(app, auth);
-  credentials_provider.SetUserChangeListener([](User user) {
+  credentials_provider.SetCredentialChangeListener([](User user) {
     EXPECT_EQ("fake uid", user.uid());
     EXPECT_TRUE(user.is_authenticated());
   });
 
-  credentials_provider.SetUserChangeListener(nullptr);
+  credentials_provider.SetCredentialChangeListener(nullptr);
 }
 
 TEST(FirebaseCredentialsProviderTest, InvalidateToken) {