From 7a4a2ea10844afd6a58dace46854fae74399f55c Mon Sep 17 00:00:00 2001 From: zxu Date: Tue, 20 Feb 2018 12:25:39 -0500 Subject: replacing FSTGetTokenResult by C++ Token implementation (#805) * replacing Auth/FSTUser by C++ auth implementation * address changes * replacing FSTGetTokenResult by C++ Token implementation * address changes * address changes * fix another const& v.s. dispatch bug * fix more const& v.s. dispatch bug zxu123 committed * fix * passing by value in callback --- Firestore/Source/Auth/FSTCredentialsProvider.h | 34 +++------------------- Firestore/Source/Auth/FSTCredentialsProvider.mm | 32 +++----------------- .../Source/Auth/FSTEmptyCredentialsProvider.mm | 5 +++- Firestore/Source/Core/FSTFirestoreClient.mm | 2 +- Firestore/Source/Remote/FSTDatastore.h | 3 +- Firestore/Source/Remote/FSTDatastore.mm | 16 ++++++---- Firestore/Source/Remote/FSTStream.mm | 21 ++++++------- .../firebase/firestore/auth/credentials_provider.h | 4 +-- .../core/src/firebase/firestore/auth/token.cc | 10 ++++++- Firestore/core/src/firebase/firestore/auth/token.h | 19 ++++++++++++ .../src/firebase/firestore/util/string_apple.h | 11 ++++++- .../firestore/auth/credentials_provider_test.cc | 6 ++-- .../auth/empty_credentials_provider_test.cc | 5 ++-- .../auth/firebase_credentials_provider_test.mm | 5 ++-- .../test/firebase/firestore/auth/token_test.cc | 8 +++++ 15 files changed, 90 insertions(+), 91 deletions(-) diff --git a/Firestore/Source/Auth/FSTCredentialsProvider.h b/Firestore/Source/Auth/FSTCredentialsProvider.h index 230a22d..d2f04e0 100644 --- a/Firestore/Source/Auth/FSTCredentialsProvider.h +++ b/Firestore/Source/Auth/FSTCredentialsProvider.h @@ -16,6 +16,7 @@ #import +#include "Firestore/core/src/firebase/firestore/auth/token.h" #include "Firestore/core/src/firebase/firestore/auth/user.h" NS_ASSUME_NONNULL_BEGIN @@ -23,46 +24,19 @@ NS_ASSUME_NONNULL_BEGIN @class FIRApp; @class FSTDispatchQueue; -#pragma mark - FSTGetTokenResult - -/** - * The current User and the authentication token provided by the underlying authentication - * mechanism. This is the result of calling -[FSTCredentialsProvider getTokenForcingRefresh]. - * - * ## Portability notes: no TokenType on iOS - * - * The TypeScript client supports 1st party Oauth tokens (for the Firebase Console to auth as the - * developer) and OAuth2 tokens for the node.js sdk to auth with a service account. We don't have - * plans to support either case on mobile so there's no TokenType here. - */ -// TODO(mcg): Rename FSTToken, change parameter order to line up with the other platforms. -@interface FSTGetTokenResult : NSObject - -- (instancetype)init NS_UNAVAILABLE; -- (instancetype)initWithUser:(const firebase::firestore::auth::User &)user - token:(NSString *_Nullable)token NS_DESIGNATED_INITIALIZER; - -/** The user with which the token is associated (used for persisting user state on disk, etc.). */ -@property(nonatomic, assign, readonly) const firebase::firestore::auth::User &user; - -/** The actual raw token. */ -@property(nonatomic, copy, nullable, readonly) NSString *token; - -@end - #pragma mark - Typedefs /** * `FSTVoidTokenErrorBlock` is a block that gets a token or an error. * - * @param token An auth token as a string. + * @param token An auth token, either valid or invalid when error occurred. * @param error The error if one occurred, or else `nil`. */ -typedef void (^FSTVoidGetTokenResultBlock)(FSTGetTokenResult *_Nullable token, +typedef void (^FSTVoidGetTokenResultBlock)(firebase::firestore::auth::Token token, NSError *_Nullable error); /** Listener block notified with a User. */ -typedef void (^FSTVoidUserBlock)(const firebase::firestore::auth::User &user); +typedef void (^FSTVoidUserBlock)(firebase::firestore::auth::User user); #pragma mark - FSTCredentialsProvider diff --git a/Firestore/Source/Auth/FSTCredentialsProvider.mm b/Firestore/Source/Auth/FSTCredentialsProvider.mm index cf045e3..084f313 100644 --- a/Firestore/Source/Auth/FSTCredentialsProvider.mm +++ b/Firestore/Source/Auth/FSTCredentialsProvider.mm @@ -25,38 +25,16 @@ #import "Firestore/Source/Util/FSTClasses.h" #import "Firestore/Source/Util/FSTDispatchQueue.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/string_apple.h" namespace util = firebase::firestore::util; +using firebase::firestore::auth::Token; using firebase::firestore::auth::User; NS_ASSUME_NONNULL_BEGIN -#pragma mark - FSTGetTokenResult - -@interface FSTGetTokenResult () { - User _user; -} - -@end - -@implementation FSTGetTokenResult - -- (instancetype)initWithUser:(const User &)user token:(NSString *_Nullable)token { - if (self = [super init]) { - _user = user; - _token = token; - } - return self; -} - -- (const User &)user { - return _user; -} - -@end - #pragma mark - FSTFirebaseCredentialsProvider @interface FSTFirebaseCredentialsProvider () { /** The current user as reported to us via our AuthStateDidChangeListener. */ @@ -141,11 +119,9 @@ NS_ASSUME_NONNULL_BEGIN NSError *cancelError = [NSError errorWithDomain:FIRFirestoreErrorDomain code:FIRFirestoreErrorCodeAborted userInfo:errorInfo]; - completion(nil, cancelError); + completion(Token::Invalid(), cancelError); } else { - FSTGetTokenResult *result = - [[FSTGetTokenResult alloc] initWithUser:_currentUser token:token]; - completion(result, error); + completion(Token(util::MakeStringView(token), _currentUser), error); } }; }; diff --git a/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm b/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm index 8139d79..77c08d1 100644 --- a/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm +++ b/Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm @@ -19,8 +19,10 @@ #import "Firestore/Source/Util/FSTAssert.h" #import "Firestore/Source/Util/FSTDispatchQueue.h" +#include "Firestore/core/src/firebase/firestore/auth/token.h" #include "Firestore/core/src/firebase/firestore/auth/user.h" +using firebase::firestore::auth::Token; using firebase::firestore::auth::User; NS_ASSUME_NONNULL_BEGIN @@ -29,7 +31,8 @@ NS_ASSUME_NONNULL_BEGIN - (void)getTokenForcingRefresh:(BOOL)forceRefresh completion:(FSTVoidGetTokenResultBlock)completion { - completion(nil, nil); + // Invalid token will force the GRPC fallback to use default settings. + completion(Token::Invalid(), nil); } - (void)setUserChangeListener:(nullable FSTVoidUserBlock)block { diff --git a/Firestore/Source/Core/FSTFirestoreClient.mm b/Firestore/Source/Core/FSTFirestoreClient.mm index 823f488..2ef7279 100644 --- a/Firestore/Source/Core/FSTFirestoreClient.mm +++ b/Firestore/Source/Core/FSTFirestoreClient.mm @@ -103,7 +103,7 @@ NS_ASSUME_NONNULL_BEGIN __block bool initialized = false; __block User initialUser; FSTWeakify(self); - _credentialsProvider.userChangeListener = ^(const User &user) { + _credentialsProvider.userChangeListener = ^(User user) { FSTStrongify(self); if (self) { if (!initialized) { diff --git a/Firestore/Source/Remote/FSTDatastore.h b/Firestore/Source/Remote/FSTDatastore.h index 9edaf96..481b6e8 100644 --- a/Firestore/Source/Remote/FSTDatastore.h +++ b/Firestore/Source/Remote/FSTDatastore.h @@ -20,6 +20,7 @@ #include "Firestore/core/src/firebase/firestore/core/database_info.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" +#include "absl/strings/string_view.h" @class FSTDocumentKey; @class FSTDispatchQueue; @@ -83,7 +84,7 @@ NS_ASSUME_NONNULL_BEGIN /** Adds headers to the RPC including any OAuth access token if provided .*/ + (void)prepareHeadersForRPC:(GRPCCall *)rpc databaseID:(const firebase::firestore::model::DatabaseId *)databaseID - token:(nullable NSString *)token; + token:(const absl::string_view)token; /** Looks up a list of documents in datastore. */ - (void)lookupDocuments:(NSArray *)keys diff --git a/Firestore/Source/Remote/FSTDatastore.mm b/Firestore/Source/Remote/FSTDatastore.mm index 8017c58..a6029ee 100644 --- a/Firestore/Source/Remote/FSTDatastore.mm +++ b/Firestore/Source/Remote/FSTDatastore.mm @@ -35,11 +35,13 @@ #import "Firestore/Protos/objc/google/firestore/v1beta1/Firestore.pbrpc.h" +#include "Firestore/core/src/firebase/firestore/auth/token.h" #include "Firestore/core/src/firebase/firestore/core/database_info.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" namespace util = firebase::firestore::util; +using firebase::firestore::auth::Token; using firebase::firestore::core::DatabaseInfo; using firebase::firestore::model::DatabaseId; @@ -301,16 +303,18 @@ typedef GRPCProtoCall * (^RPCFactory)(void); // but I'm not sure how to detect that right now. http://b/32762461 [self.credentials getTokenForcingRefresh:NO - completion:^(FSTGetTokenResult *_Nullable result, NSError *_Nullable error) { + completion:^(Token result, NSError *_Nullable error) { error = [FSTDatastore firestoreErrorForError:error]; [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ if (error) { errorHandler(error); } else { GRPCProtoCall *rpc = rpcFactory(); - [FSTDatastore prepareHeadersForRPC:rpc - databaseID:&self.databaseInfo->database_id() - token:result.token]; + [FSTDatastore + prepareHeadersForRPC:rpc + databaseID:&self.databaseInfo->database_id() + token:(result.is_valid() ? result.token() + : absl::string_view())]; [rpc start]; } }]; @@ -334,8 +338,8 @@ typedef GRPCProtoCall * (^RPCFactory)(void); /** Adds headers to the RPC including any OAuth access token if provided .*/ + (void)prepareHeadersForRPC:(GRPCCall *)rpc databaseID:(const DatabaseId *)databaseID - token:(nullable NSString *)token { - rpc.oauth2AccessToken = token; + token:(const absl::string_view)token { + rpc.oauth2AccessToken = token.data() == nullptr ? 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 f859fbb..079ae72 100644 --- a/Firestore/Source/Remote/FSTStream.mm +++ b/Firestore/Source/Remote/FSTStream.mm @@ -35,11 +35,13 @@ #import "Firestore/Protos/objc/google/firestore/v1beta1/Firestore.pbrpc.h" +#include "Firestore/core/src/firebase/firestore/auth/token.h" #include "Firestore/core/src/firebase/firestore/core/database_info.h" #include "Firestore/core/src/firebase/firestore/model/database_id.h" #include "Firestore/core/src/firebase/firestore/util/string_apple.h" namespace util = firebase::firestore::util; +using firebase::firestore::auth::Token; using firebase::firestore::core::DatabaseInfo; using firebase::firestore::model::DatabaseId; @@ -256,18 +258,17 @@ static const NSTimeInterval kIdleTimeout = 60.0; FSTAssert(_delegate == nil, @"Delegate must be nil"); _delegate = delegate; - [self.credentials - getTokenForcingRefresh:NO - completion:^(FSTGetTokenResult *_Nullable result, NSError *_Nullable error) { - error = [FSTDatastore firestoreErrorForError:error]; - [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ - [self resumeStartWithToken:result error:error]; - }]; - }]; + [self.credentials getTokenForcingRefresh:NO + completion:^(Token result, NSError *_Nullable error) { + error = [FSTDatastore firestoreErrorForError:error]; + [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{ + [self resumeStartWithToken:result error:error]; + }]; + }]; } /** Add an access token to our RPC, after obtaining one from the credentials provider. */ -- (void)resumeStartWithToken:(FSTGetTokenResult *)token error:(NSError *)error { +- (void)resumeStartWithToken:(const Token &)token error:(NSError *)error { if (self.state == FSTStreamStateStopped) { // Streams can be stopped while waiting for authorization. return; @@ -289,7 +290,7 @@ static const NSTimeInterval kIdleTimeout = 60.0; _rpc = [self createRPCWithRequestsWriter:self.requestsWriter]; [FSTDatastore prepareHeadersForRPC:_rpc databaseID:&self.databaseInfo->database_id() - token:token.token]; + token:(token.is_valid() ? token.token() : absl::string_view())]; 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/credentials_provider.h b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h index 2a52c99..917f8e1 100644 --- a/Firestore/core/src/firebase/firestore/auth/credentials_provider.h +++ b/Firestore/core/src/firebase/firestore/auth/credentials_provider.h @@ -31,11 +31,11 @@ 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: The error if one occurred, or else nullptr. -typedef std::function +typedef std::function TokenListener; // Listener notified with a User change. -typedef std::function UserChangeListener; +typedef std::function UserChangeListener; /** * Provides methods for getting the uid and token for the current user and diff --git a/Firestore/core/src/firebase/firestore/auth/token.cc b/Firestore/core/src/firebase/firestore/auth/token.cc index 0618ddb..4ee1b69 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.cc +++ b/Firestore/core/src/firebase/firestore/auth/token.cc @@ -21,7 +21,15 @@ namespace firestore { namespace auth { Token::Token(const absl::string_view token, const User& user) - : token_(token), user_(user) { + : token_(token), user_(user), is_valid_(true) { +} + +Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) { +} + +const Token& Token::Invalid() { + static const Token kInvalidToken; + return kInvalidToken; } } // namespace auth diff --git a/Firestore/core/src/firebase/firestore/auth/token.h b/Firestore/core/src/firebase/firestore/auth/token.h index f3b7363..ff8d2f0 100644 --- a/Firestore/core/src/firebase/firestore/auth/token.h +++ b/Firestore/core/src/firebase/firestore/auth/token.h @@ -20,6 +20,7 @@ #include #include "Firestore/core/src/firebase/firestore/auth/user.h" +#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h" #include "absl/strings/string_view.h" namespace firebase { @@ -45,6 +46,7 @@ class Token { /** The actual raw token. */ const std::string& token() const { + FIREBASE_ASSERT(is_valid_); return token_; } @@ -56,9 +58,26 @@ class Token { return user_; } + /** + * Whether the token is a valid one. + * + * ## 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. + */ + bool is_valid() const { + return is_valid_; + } + + /** Returns an invalid token. */ + static const Token& Invalid(); + private: + Token(); + const std::string token_; const User user_; + const bool is_valid_; }; } // namespace auth diff --git a/Firestore/core/src/firebase/firestore/util/string_apple.h b/Firestore/core/src/firebase/firestore/util/string_apple.h index fe2a487..3f6b814 100644 --- a/Firestore/core/src/firebase/firestore/util/string_apple.h +++ b/Firestore/core/src/firebase/firestore/util/string_apple.h @@ -42,7 +42,16 @@ inline NSString* WrapNSStringNoCopy(const absl::string_view str) { return WrapNSStringNoCopy(str.data()); } -// Creates an absl::string_view wrapper for the contents of the given NSString. +// Translates a string_view string to the equivalent NSString by making a copy. +inline NSString* WrapNSString(const absl::string_view str) { + return [[NSString alloc] + initWithBytes:const_cast(static_cast(str.data())) + length:str.length() + encoding:NSUTF8StringEncoding]; +} + +// Creates an absl::string_view wrapper for the contents of the given +// NSString. inline absl::string_view MakeStringView(NSString* str) { return absl::string_view( [str UTF8String], [str lengthOfBytesUsingEncoding:NSUTF8StringEncoding]); 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 1748422..9ae71ba 100644 --- a/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc @@ -25,7 +25,7 @@ namespace auth { #define UNUSED(x) (void)(x) TEST(CredentialsProvider, Typedef) { - TokenListener token_listener = [](const Token& token, + TokenListener token_listener = [](Token token, const absl::string_view error) { UNUSED(token); UNUSED(error); @@ -37,9 +37,7 @@ TEST(CredentialsProvider, Typedef) { EXPECT_EQ(nullptr, token_listener); EXPECT_FALSE(token_listener); - UserChangeListener user_change_listener = [](const User& user) { - UNUSED(user); - }; + UserChangeListener user_change_listener = [](User user) { UNUSED(user); }; EXPECT_NE(nullptr, user_change_listener); EXPECT_TRUE(user_change_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 123f952..39012f0 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 @@ -25,8 +25,7 @@ namespace auth { TEST(EmptyCredentialsProvider, GetToken) { EmptyCredentialsProvider credentials_provider; credentials_provider.GetToken( - /*force_refresh=*/true, - [](const Token& token, const absl::string_view error) { + /*force_refresh=*/true, [](Token token, const absl::string_view error) { EXPECT_EQ("", token.token()); const User& user = token.user(); EXPECT_EQ("", user.uid()); @@ -37,7 +36,7 @@ TEST(EmptyCredentialsProvider, GetToken) { TEST(EmptyCredentialsProvider, SetListener) { EmptyCredentialsProvider credentials_provider; - credentials_provider.SetUserChangeListener([](const User& user) { + credentials_provider.SetUserChangeListener([](User user) { EXPECT_EQ("", user.uid()); EXPECT_FALSE(user.is_authenticated()); }); 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 8d2b361..e98d3d8 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 @@ -65,8 +65,7 @@ TEST_F(FirebaseCredentialsProviderTest, GetToken) { FirebaseCredentialsProvider credentials_provider([FIRApp defaultApp]); credentials_provider.GetToken( - /*force_refresh=*/true, - [](const Token& token, const absl::string_view error) { + /*force_refresh=*/true, [](Token token, const absl::string_view error) { EXPECT_EQ("", token.token()); const User& user = token.user(); EXPECT_EQ("I'm a fake uid.", user.uid()); @@ -82,7 +81,7 @@ TEST_F(FirebaseCredentialsProviderTest, SetListener) { } FirebaseCredentialsProvider credentials_provider([FIRApp defaultApp]); - credentials_provider.SetUserChangeListener([](const User& user) { + credentials_provider.SetUserChangeListener([](User user) { EXPECT_EQ("I'm a fake uid.", user.uid()); EXPECT_TRUE(user.is_authenticated()); }); diff --git a/Firestore/core/test/firebase/firestore/auth/token_test.cc b/Firestore/core/test/firebase/firestore/auth/token_test.cc index a0f2c48..8f784d6 100644 --- a/Firestore/core/test/firebase/firestore/auth/token_test.cc +++ b/Firestore/core/test/firebase/firestore/auth/token_test.cc @@ -26,6 +26,14 @@ 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(); + EXPECT_ANY_THROW(token.token()); + EXPECT_EQ(User::Unauthenticated(), token.user()); + EXPECT_FALSE(token.is_valid()); } } // namespace auth -- cgit v1.2.3