Change CredentialsProvider::TokenListener to use StatusOr<Token> (#945)

* Change CredentialsProvider::TokenListener to use StatusOr

Rather than a token plus error code/msg.

* Eliminate the concept of an invalid Token

Instead, we'll just use StatusOr<Token>. Note that unauthenticated
tokens are handled as a special case; they're created via:
  Token::Unauthenticated()
and are otherwise "valid", though attempting to retrieve the raw token
on one of these tokens will cause an assertion failure.

10 files changed, 86 insertions, 76 deletions

diff --git a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h
index b9a8a24..1aa76df 100644
--- a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h
+++ b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h
@@ -23,6 +23,7 @@
 #include "Firestore/core/include/firebase/firestore/firestore_errors.h"
 #include "Firestore/core/src/firebase/firestore/auth/token.h"
 #include "Firestore/core/src/firebase/firestore/auth/user.h"
+#include "Firestore/core/src/firebase/firestore/util/statusor.h"
 #include "absl/strings/string_view.h"
 
 namespace firebase {
@@ -30,12 +31,7 @@ namespace firestore {
 namespace auth {
 
 // `TokenErrorListener` is a listener that gets a token or an error.
-// token: An auth token as a string, or nullptr if error occurred.
-// error_code: The error code if one occurred, or else FirestoreErrorCode::Ok.
-// error_msg: The error if one occurred, or else nullptr.
-typedef std::function<void(
-    Token token, const int64_t error_code, const absl::string_view error_msg)>
-    TokenListener;
+typedef std::function<void(util::StatusOr<Token>)> TokenListener;
 
 // Listener notified with a User change.
 typedef std::function<void(User user)> UserChangeListener;
diff --git a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc
index 0fa70c0..da4198d 100644
--- a/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc
+++ b/Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc
@@ -26,8 +26,9 @@ void EmptyCredentialsProvider::GetToken(bool force_refresh,
                                         TokenListener completion) {
   UNUSED(force_refresh);
   if (completion) {
-    // Invalid token will force the GRPC fallback to use default settings.
-    completion(Token::Invalid(), FirestoreErrorCode::Ok, "");
+    // Unauthenticated token will force the GRPC fallback to use default
+    // settings.
+    completion(Token::Unauthenticated());
   }
 }
 
diff --git a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm
index 1babe82..2bd3acc 100644
--- a/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm
+++ b/Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm
@@ -23,6 +23,9 @@
 #include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 
+// NB: This is also defined in Firestore/Source/Public/FIRFirestoreErrors.h
+NSString* const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain";
+
 namespace firebase {
 namespace firestore {
 namespace auth {
@@ -84,27 +87,38 @@ void FirebaseCredentialsProvider::GetToken(bool force_refresh,
   int initial_user_counter = contents_->user_counter;
 
   std::weak_ptr<Contents> weak_contents = contents_;
-  void (^get_token_callback)(NSString*, NSError*) = ^(
-      NSString* _Nullable token, NSError* _Nullable error) {
-    std::shared_ptr<Contents> contents = weak_contents.lock();
-    if (!contents) {
-      return;
-    }
-
-    std::unique_lock<std::mutex> lock(contents->mutex);
-    if (initial_user_counter != contents->user_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(Token::Invalid(), FirestoreErrorCode::Aborted,
-                 "getToken aborted due to user change.");
-    } else {
-      completion(
-          Token{util::MakeStringView(token), contents->current_user},
-          error == nil ? FirestoreErrorCode::Ok : error.code,
-          error == nil ? "" : util::MakeStringView(error.localizedDescription));
-    }
-  };
+  void (^get_token_callback)(NSString*, NSError*) =
+      ^(NSString* _Nullable token, NSError* _Nullable error) {
+        std::shared_ptr<Contents> contents = weak_contents.lock();
+        if (!contents) {
+          return;
+        }
+
+        std::unique_lock<std::mutex> lock(contents->mutex);
+        if (initial_user_counter != contents->user_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."));
+        } else {
+          if (error == nil) {
+            if (token != nil) {
+              completion(
+                  Token{util::MakeStringView(token), contents->current_user});
+            } else {
+              completion(Token::Unauthenticated());
+            }
+          } else {
+            FirestoreErrorCode error_code = FirestoreErrorCode::Unknown;
+            if (error.domain == FIRFirestoreErrorDomain) {
+              error_code = static_cast<FirestoreErrorCode>(error.code);
+            }
+            completion(util::Status(
+                error_code, util::MakeStringView(error.localizedDescription)));
+          }
+        }
+      };
 
   [contents_->app getTokenForcingRefresh:force_refresh
                             withCallback:get_token_callback];
diff --git a/Firestore/core/src/firebase/firestore/auth/token.cc b/Firestore/core/src/firebase/firestore/auth/token.cc
index 4ee1b69..6fe5fc4 100644
--- a/Firestore/core/src/firebase/firestore/auth/token.cc
+++ b/Firestore/core/src/firebase/firestore/auth/token.cc
@@ -21,15 +21,13 @@ namespace firestore {
 namespace auth {
 
 Token::Token(const absl::string_view token, const User& user)
-    : token_(token), user_(user), is_valid_(true) {
+    : token_(token), user_(user) {
 }
 
-Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) {
-}
-
-const Token& Token::Invalid() {
-  static const Token kInvalidToken;
-  return kInvalidToken;
+const Token& Token::Unauthenticated() {
+  static const Token kUnauthenticatedToken(absl::string_view(),
+                                           User::Unauthenticated());
+  return kUnauthenticatedToken;
 }
 
 }  // namespace auth
diff --git a/Firestore/core/src/firebase/firestore/auth/token.h b/Firestore/core/src/firebase/firestore/auth/token.h
index ff8d2f0..4b2f3aa 100644
--- a/Firestore/core/src/firebase/firestore/auth/token.h
+++ b/Firestore/core/src/firebase/firestore/auth/token.h
@@ -46,7 +46,7 @@ class Token {
 
   /** The actual raw token. */
   const std::string& token() const {
-    FIREBASE_ASSERT(is_valid_);
+    FIREBASE_ASSERT(user_.is_authenticated());
     return token_;
   }
 
@@ -59,25 +59,17 @@ class Token {
   }
 
   /**
-   * Whether the token is a valid one.
+   * Returns a token for an unauthenticated user.
    *
-   * ## Portability notes: Invalid token is the equivalent of nil in the iOS
-   * token implementation. We use value instead of pointer for Token instance in
-   * the C++ migration.
+   * ## Portability notes: An unauthenticated token is the equivalent of
+   * nil/null in the iOS/TypeScript token implementation. We use a reference
+   * instead of a pointer for Token instances in the C++ migration.
    */
-  bool is_valid() const {
-    return is_valid_;
-  }
-
-  /** Returns an invalid token. */
-  static const Token& Invalid();
+  static const Token& Unauthenticated();
 
  private:
-  Token();
-
   const std::string token_;
   const User user_;
-  const bool is_valid_;
 };
 
 }  // namespace auth
diff --git a/Firestore/core/src/firebase/firestore/util/error_apple.h b/Firestore/core/src/firebase/firestore/util/error_apple.h
index e31cfd6..e7c77c9 100644
--- a/Firestore/core/src/firebase/firestore/util/error_apple.h
+++ b/Firestore/core/src/firebase/firestore/util/error_apple.h
@@ -24,6 +24,7 @@
 
 #include "Firestore/Source/Public/FIRFirestoreErrors.h"  // for FIRFirestoreErrorDomain
 #include "Firestore/core/include/firebase/firestore/firestore_errors.h"
+#include "Firestore/core/src/firebase/firestore/util/status.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 #include "absl/strings/string_view.h"
 
@@ -32,7 +33,7 @@ namespace firestore {
 namespace util {
 
 // Translates a set of error_code and error_msg to an NSError.
-inline NSError* WrapNSError(const int64_t error_code,
+inline NSError* MakeNSError(const int64_t error_code,
                             const absl::string_view error_msg) {
   if (error_code == FirestoreErrorCode::Ok) {
     return nil;
@@ -43,6 +44,10 @@ inline NSError* WrapNSError(const int64_t error_code,
              userInfo:@{NSLocalizedDescriptionKey : WrapNSString(error_msg)}];
 }
 
+inline NSError* MakeNSError(const util::Status& status) {
+  return MakeNSError(status.code(), status.error_message());
+}
+
 }  // namespace util
 }  // namespace firestore
 }  // namespace firebase
diff --git a/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc b/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc
index 6895061..69c3def 100644
--- a/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc
+++ b/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc
@@ -16,6 +16,7 @@
 
 #include "Firestore/core/src/firebase/firestore/auth/credentials_provider.h"
 
+#include "Firestore/core/src/firebase/firestore/util/statusor.h"
 #include "gtest/gtest.h"
 
 namespace firebase {
@@ -25,11 +26,8 @@ namespace auth {
 #define UNUSED(x) (void)(x)
 
 TEST(CredentialsProvider, Typedef) {
-  TokenListener token_listener = [](Token token, const int64_t error_code,
-                                    const absl::string_view error_msg) {
+  TokenListener token_listener = [](util::StatusOr<Token> token) {
     UNUSED(token);
-    UNUSED(error_code);
-    UNUSED(error_msg);
   };
   EXPECT_NE(nullptr, token_listener);
   EXPECT_TRUE(token_listener);
diff --git a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc
index 3b487f3..60845e5 100644
--- a/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc
+++ b/Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc
@@ -16,6 +16,7 @@
 
 #include "Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.h"
 
+#include "Firestore/core/src/firebase/firestore/util/statusor.h"
 #include "gtest/gtest.h"
 
 namespace firebase {
@@ -25,14 +26,13 @@ namespace auth {
 TEST(EmptyCredentialsProvider, GetToken) {
   EmptyCredentialsProvider credentials_provider;
   credentials_provider.GetToken(
-      /*force_refresh=*/true, [](Token token, const int64_t error_code,
-                                 const absl::string_view error_msg) {
-        EXPECT_FALSE(token.is_valid());
+      /*force_refresh=*/true, [](util::StatusOr<Token> result) {
+        EXPECT_TRUE(result.ok());
+        const Token& token = result.ValueOrDie();
+        EXPECT_ANY_THROW(token.token());
         const User& user = token.user();
         EXPECT_EQ("", user.uid());
         EXPECT_FALSE(user.is_authenticated());
-        EXPECT_EQ(FirestoreErrorCode::Ok, error_code);
-        EXPECT_EQ("", error_msg);
       });
 }
 
diff --git a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm
index 3660d53..9d358b5 100644
--- a/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm
+++ b/Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm
@@ -17,9 +17,11 @@
 #include "Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h"
 
 #import <FirebaseCore/FIRApp.h>
+
 #import <FirebaseCore/FIRAppInternal.h>
 #import <FirebaseCore/FIROptionsInternal.h>
 
+#include "Firestore/core/src/firebase/firestore/util/statusor.h"
 #include "Firestore/core/src/firebase/firestore/util/string_apple.h"
 #include "Firestore/core/test/firebase/firestore/testutil/app_testing.h"
 
@@ -29,43 +31,49 @@ namespace firebase {
 namespace firestore {
 namespace auth {
 
-FIRApp* AppWithFakeUid(NSString* _Nullable uid) {
+FIRApp* AppWithFakeUidAndToken(NSString* _Nullable uid,
+                               NSString* _Nullable token) {
   FIRApp* app = testutil::AppForUnitTesting();
   app.getUIDImplementation = ^NSString* {
     return uid;
   };
+  app.getTokenImplementation = ^(BOOL, FIRTokenCallback callback) {
+    callback(token, nil);
+  };
   return app;
 }
 
+FIRApp* AppWithFakeUid(NSString* _Nullable uid) {
+  return AppWithFakeUidAndToken(uid, uid == nil ? nil : @"default token");
+}
+
 TEST(FirebaseCredentialsProviderTest, GetTokenUnauthenticated) {
   FIRApp* app = AppWithFakeUid(nil);
 
   FirebaseCredentialsProvider credentials_provider(app);
   credentials_provider.GetToken(
-      /*force_refresh=*/true, [](Token token, const int64_t error_code,
-                                 const absl::string_view error_msg) {
-        EXPECT_EQ("", token.token());
+      /*force_refresh=*/true, [](util::StatusOr<Token> result) {
+        EXPECT_TRUE(result.ok());
+        const Token& token = result.ValueOrDie();
+        EXPECT_ANY_THROW(token.token());
         const User& user = token.user();
         EXPECT_EQ("", user.uid());
         EXPECT_FALSE(user.is_authenticated());
-        EXPECT_EQ(FirestoreErrorCode::Ok, error_code) << error_code;
-        EXPECT_EQ("", error_msg) << error_msg;
       });
 }
 
 TEST(FirebaseCredentialsProviderTest, GetToken) {
-  FIRApp* app = AppWithFakeUid(@"fake uid");
+  FIRApp* app = AppWithFakeUidAndToken(@"fake uid", @"token for fake uid");
 
   FirebaseCredentialsProvider credentials_provider(app);
   credentials_provider.GetToken(
-      /*force_refresh=*/true, [](Token token, const int64_t error_code,
-                                 const absl::string_view error_msg) {
-        EXPECT_EQ("", token.token());
+      /*force_refresh=*/true, [](util::StatusOr<Token> result) {
+        EXPECT_TRUE(result.ok());
+        const Token& token = result.ValueOrDie();
+        EXPECT_EQ("token for fake uid", token.token());
         const User& user = token.user();
         EXPECT_EQ("fake uid", user.uid());
         EXPECT_TRUE(user.is_authenticated());
-        EXPECT_EQ(FirestoreErrorCode::Ok, error_code) << error_code;
-        EXPECT_EQ("", error_msg) << error_msg;
       });
 }
 
diff --git a/Firestore/core/test/firebase/firestore/auth/token_test.cc b/Firestore/core/test/firebase/firestore/auth/token_test.cc
index 8f784d6..e34053e 100644
--- a/Firestore/core/test/firebase/firestore/auth/token_test.cc
+++ b/Firestore/core/test/firebase/firestore/auth/token_test.cc
@@ -26,14 +26,12 @@ TEST(Token, Getter) {
   Token token("token", User("abc"));
   EXPECT_EQ("token", token.token());
   EXPECT_EQ(User("abc"), token.user());
-  EXPECT_TRUE(token.is_valid());
 }
 
-TEST(Token, InvalidToken) {
-  const Token& token = Token::Invalid();
+TEST(Token, UnauthenticatedToken) {
+  const Token& token = Token::Unauthenticated();
   EXPECT_ANY_THROW(token.token());
   EXPECT_EQ(User::Unauthenticated(), token.user());
-  EXPECT_FALSE(token.is_valid());
 }
 
 }  // namespace auth