From 4a947eab3f1859688a54c8d462275b1383ac2f71 Mon Sep 17 00:00:00 2001 From: Michael Lehenbauer Date: Wed, 7 Mar 2018 19:36:03 -0800 Subject: [PATCH] Fix for b/74357976 and some cleanup. --- Firestore/Source/Auth/FSTCredentialsProvider.mm | 4 ++++ .../Source/Auth/FSTEmptyCredentialsProvider.mm | 3 +-- Firestore/Source/Remote/FSTDatastore.mm | 8 +++++--- Firestore/Source/Remote/FSTStream.mm | 3 ++- .../auth/firebase_credentials_provider_apple.mm | 2 +- Firestore/core/src/firebase/firestore/auth/token.cc | 3 +++ Firestore/core/src/firebase/firestore/auth/token.h | 13 +++++++------ .../core/src/firebase/firestore/util/string_apple.h | 4 ++++ .../core/test/firebase/firestore/auth/token_test.cc | 2 +- 9 files changed, 28 insertions(+), 14 deletions(-) diff --git a/Firestore/Source/Auth/FSTCredentialsProvider.mm b/Firestore/Source/Auth/FSTCredentialsProvider.mm index e0dc8aad7bb..7e0c484d109 100644 --- a/Firestore/Source/Auth/FSTCredentialsProvider.mm +++ b/Firestore/Source/Auth/FSTCredentialsProvider.mm @@ -121,6 +121,10 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh userInfo:errorInfo]; completion(Token::Invalid(), cancelError); } else { + if (!token) { + // We use "" to represent the lack of a token in the C++ Token object. + token = @""; + } completion(Token(util::MakeStringView(token), self->_currentUser), error); } }; diff --git a/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm b/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm index 77c08d149d1..2596362ee90 100644 --- a/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm +++ b/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm @@ -31,8 +31,7 @@ @implementation FSTEmptyCredentialsProvider - (void)getTokenForcingRefresh:(BOOL)forceRefresh completion:(FSTVoidGetTokenResultBlock)completion { - // Invalid token will force the GRPC fallback to use default settings. - completion(Token::Invalid(), nil); + completion(Token::Token("", User::Unauthenticated()), nil); } - (void)setUserChangeListener:(nullable FSTVoidUserBlock)block { diff --git a/Firestore/Source/Remote/FSTDatastore.mm b/Firestore/Source/Remote/FSTDatastore.mm index a6029ee732e..058f6603e3d 100644 --- a/Firestore/Source/Remote/FSTDatastore.mm +++ b/Firestore/Source/Remote/FSTDatastore.mm @@ -309,12 +309,13 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory if (error) { errorHandler(error); } else { + FSTAssert(result.is_valid(), + @"invalid token result even though there was no error."); GRPCProtoCall *rpc = rpcFactory(); [FSTDatastore prepareHeadersForRPC:rpc databaseID:&self.databaseInfo->database_id() - token:(result.is_valid() ? result.token() - : absl::string_view())]; + token:result.token()]; [rpc start]; } }]; @@ -339,7 +340,8 @@ - (FSTWriteStream *)createWriteStream { + (void)prepareHeadersForRPC:(GRPCCall *)rpc databaseID:(const DatabaseId *)databaseID token:(const absl::string_view)token { - rpc.oauth2AccessToken = token.data() == nullptr ? nil : util::WrapNSString(token); + // "" represents the lack of a token. + rpc.oauth2AccessToken = token == "" ? nil : util::WrapNSString(token); rpc.requestHeaders[kXGoogAPIClientHeader] = [FSTDatastore googAPIClientHeaderValue]; // This header is used to improve routing and project isolation by the backend. rpc.requestHeaders[kGoogleCloudResourcePrefix] = diff --git a/Firestore/Source/Remote/FSTStream.mm b/Firestore/Source/Remote/FSTStream.mm index a9aa245ad4f..bee828b1b06 100644 --- a/Firestore/Source/Remote/FSTStream.mm +++ b/Firestore/Source/Remote/FSTStream.mm @@ -274,6 +274,7 @@ - (void)startWithDelegate:(id)delegate { /** Add an access token to our RPC, after obtaining one from the credentials provider. */ - (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { + FSTAssert(token.is_valid(), @"resumeStartWithToken: called with invalid token."); [self.workerDispatchQueue verifyIsCurrentQueue]; if (self.state == FSTStreamStateStopped) { @@ -297,7 +298,7 @@ - (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { [FSTDatastore prepareHeadersForRPC:_rpc databaseID:&self.databaseInfo->database_id() - token:(token.is_valid() ? token.token() : absl::string_view())]; + token:(token.token())]; FSTAssert(_callbackFilter == nil, @"GRX Filter must be nil"); _callbackFilter = [[FSTCallbackFilter alloc] initWithStream:self]; [_rpc startWithWriteable:_callbackFilter]; 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 fe3cb243d57..7af400c6758 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 @@ -96,7 +96,7 @@ // 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({"", User::Unauthenticated()}, + completion(Token::Invalid(), "getToken aborted due to user change."); } else { completion( diff --git a/Firestore/core/src/firebase/firestore/auth/token.cc b/Firestore/core/src/firebase/firestore/auth/token.cc index 4ee1b698e85..54762194a38 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.cc +++ b/Firestore/core/src/firebase/firestore/auth/token.cc @@ -22,6 +22,9 @@ namespace auth { Token::Token(const absl::string_view token, const User& user) : token_(token), user_(user), is_valid_(true) { + // We use "" to represent the absence of a token, which should + // be the case if the user is unauthenticated. + FIREBASE_ASSERT((!user_.is_authenticated()) == (token == "")); } Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) { diff --git a/Firestore/core/src/firebase/firestore/auth/token.h b/Firestore/core/src/firebase/firestore/auth/token.h index ff8d2f02f6e..73d20f3f486 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.h +++ b/Firestore/core/src/firebase/firestore/auth/token.h @@ -44,7 +44,7 @@ class Token { public: Token(const absl::string_view token, const User& user); - /** The actual raw token. */ + /** The actual raw token; will be "" if the user is unauthenticated. */ const std::string& token() const { FIREBASE_ASSERT(is_valid_); return token_; @@ -55,21 +55,22 @@ class Token { * state on disk, etc.). */ const User& user() const { + FIREBASE_ASSERT(is_valid_); return user_; } /** - * Whether the token is a valid one. + * Whether this Token object is valid. All methods will throw if it's not. * - * ## 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 invalid Token is the equivalent of nil in the + * iOS token implementation. We use value instead of pointer for Token + * instances in the C++ migration. */ bool is_valid() const { return is_valid_; } - /** Returns an invalid token. */ + /** Returns an invalid Token. */ static const Token& Invalid(); private: diff --git a/Firestore/core/src/firebase/firestore/util/string_apple.h b/Firestore/core/src/firebase/firestore/util/string_apple.h index 3f6b814da69..f9fb461b58a 100644 --- a/Firestore/core/src/firebase/firestore/util/string_apple.h +++ b/Firestore/core/src/firebase/firestore/util/string_apple.h @@ -23,6 +23,7 @@ #import #include "absl/strings/string_view.h" +#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h" namespace firebase { namespace firestore { @@ -53,6 +54,9 @@ inline NSString* WrapNSString(const absl::string_view str) { // Creates an absl::string_view wrapper for the contents of the given // NSString. inline absl::string_view MakeStringView(NSString* str) { + // Avoid creating nullptr string_views since it's undefined behavior once + // we assign it to an std::string (https://stackoverflow.com/a/10771938). + FIREBASE_ASSERT(str != nil); return absl::string_view( [str UTF8String], [str lengthOfBytesUsingEncoding:NSUTF8StringEncoding]); } diff --git a/Firestore/core/test/firebase/firestore/auth/token_test.cc b/Firestore/core/test/firebase/firestore/auth/token_test.cc index 8f784d65197..fd61aa93168 100644 --- a/Firestore/core/test/firebase/firestore/auth/token_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/token_test.cc @@ -32,7 +32,7 @@ TEST(Token, Getter) { TEST(Token, InvalidToken) { const Token& token = Token::Invalid(); EXPECT_ANY_THROW(token.token()); - EXPECT_EQ(User::Unauthenticated(), token.user()); + EXPECT_ANY_THROW(token.user()); EXPECT_FALSE(token.is_valid()); }