diff options
author | rsgowman <rgowman@google.com> | 2018-03-21 11:04:40 -0400 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-03-21 11:04:40 -0400 |
commit | 308acc09bfaf6dabf4b6d5f5e39f33854df8ce34 (patch) | |
tree | 3706bbbe40d08569795634fd2f30a07fd348b399 /Firestore | |
parent | d924771453d000e708bd5d239da3bae4feb489ac (diff) |
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.
Diffstat (limited to 'Firestore')
12 files changed, 98 insertions, 91 deletions
diff --git a/Firestore/Source/Remote/FSTDatastore.mm b/Firestore/Source/Remote/FSTDatastore.mm index e63017a..63499df 100644 --- a/Firestore/Source/Remote/FSTDatastore.mm +++ b/Firestore/Source/Remote/FSTDatastore.mm @@ -305,20 +305,18 @@ typedef GRPCProtoCall * (^RPCFactory)(void); // TODO(mikelehen): We should force a refresh if the previous RPC failed due to an expired token, // but I'm not sure how to detect that right now. http://b/32762461 _credentials->GetToken( - /*force_refresh=*/false, - [self, rpcFactory, errorHandler](Token result, const int64_t error_code, - const absl::string_view error_msg) { - NSError *error = util::WrapNSError(error_code, error_msg); + /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr<Token> result) { [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ - if (error) { - errorHandler(error); + if (!result.ok()) { + errorHandler(util::MakeNSError(result.status())); } else { GRPCProtoCall *rpc = rpcFactory(); + const Token &token = result.ValueOrDie(); [FSTDatastore prepareHeadersForRPC:rpc databaseID:&self.databaseInfo->database_id() - token:(result.user().is_authenticated() ? result.token() - : absl::string_view())]; + token:(token.user().is_authenticated() ? token.token() + : absl::string_view())]; [rpc start]; } }]; diff --git a/Firestore/Source/Remote/FSTStream.mm b/Firestore/Source/Remote/FSTStream.mm index 019b0bc..a5c36c8 100644 --- a/Firestore/Source/Remote/FSTStream.mm +++ b/Firestore/Source/Remote/FSTStream.mm @@ -265,17 +265,15 @@ static const NSTimeInterval kIdleTimeout = 60.0; _delegate = delegate; _credentials->GetToken( - /*force_refresh=*/false, - [self](Token result, const int64_t error_code, const absl::string_view error_msg) { - NSError *error = util::WrapNSError(error_code, error_msg); + /*force_refresh=*/false, [self](util::StatusOr<Token> result) { [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ - [self resumeStartWithToken:result error:error]; + [self resumeStartWithToken:result]; }]; }); } /** Add an access token to our RPC, after obtaining one from the credentials provider. */ -- (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { +- (void)resumeStartWithToken:(const util::StatusOr<Token> &)result { [self.workerDispatchQueue verifyIsCurrentQueue]; if (self.state == FSTStreamStateStopped) { @@ -287,9 +285,9 @@ static const NSTimeInterval kIdleTimeout = 60.0; // TODO(mikelehen): We should force a refresh if the previous RPC failed due to an expired token, // but I'm not sure how to detect that right now. http://b/32762461 - if (error) { + if (!result.ok()) { // RPC has not been started yet, so just invoke higher-level close handler. - [self handleStreamClose:error]; + [self handleStreamClose:util::MakeNSError(result.status())]; return; } @@ -297,6 +295,7 @@ static const NSTimeInterval kIdleTimeout = 60.0; _rpc = [self createRPCWithRequestsWriter:self.requestsWriter]; [_rpc setResponseDispatchQueue:self.workerDispatchQueue.queue]; + const Token &token = result.ValueOrDie(); [FSTDatastore prepareHeadersForRPC:_rpc databaseID:&self.databaseInfo->database_id() 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 |