Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replacing FSTGetTokenResult by C++ Token implementation #805

Merged
merged 10 commits into from
Feb 20, 2018
Merged
34 changes: 4 additions & 30 deletions Firestore/Source/Auth/FSTCredentialsProvider.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,53 +16,27 @@

#import <Foundation/Foundation.h>

#include "Firestore/core/src/firebase/firestore/auth/token.h"
#include "Firestore/core/src/firebase/firestore/auth/user.h"

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

Expand Down
32 changes: 4 additions & 28 deletions Firestore/Source/Auth/FSTCredentialsProvider.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Expand Down Expand Up @@ -141,11 +119,9 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
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);
}
};
};
Expand Down
5 changes: 4 additions & 1 deletion Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -29,7 +31,8 @@ @implementation FSTEmptyCredentialsProvider

- (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 {
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Core/FSTFirestoreClient.mm
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ - (instancetype)initWithDatabaseInfo:(const DatabaseInfo &)databaseInfo
__block bool initialized = false;
__block User initialUser;
FSTWeakify(self);
_credentialsProvider.userChangeListener = ^(const User &user) {
_credentialsProvider.userChangeListener = ^(User user) {
FSTStrongify(self);
if (self) {
if (!initialized) {
Expand Down
3 changes: 2 additions & 1 deletion Firestore/Source/Remote/FSTDatastore.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using const* if you want to allow nulls. Otherwise, adjust the docstring to state how to not provide a token. (An empty string_view might be a legit way to do this.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it accepts nullptr.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: absl::string_view{nullptr} is not valid (if I read what is meant here correctly), similar to how std::string{nullptr} is undefined behavior. It should be either absl::string_view{nullptr, 0} or absl::NullSafeStringView(possibly_null_argument).


/** Looks up a list of documents in datastore. */
- (void)lookupDocuments:(NSArray<FSTDocumentKey *> *)keys
Expand Down
16 changes: 10 additions & 6 deletions Firestore/Source/Remote/FSTDatastore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -301,16 +303,18 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory
// 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];
}
}];
Expand All @@ -334,8 +338,8 @@ - (FSTWriteStream *)createWriteStream {
/** 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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see comment in FSTDatastore.h:87)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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] =
Expand Down
21 changes: 11 additions & 10 deletions Firestore/Source/Remote/FSTStream.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -251,18 +253,17 @@ - (void)startWithDelegate:(id)delegate {
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;
Expand All @@ -284,7 +285,7 @@ - (void)resumeStartWithToken:(FSTGetTokenResult *)token error:(NSError *)error {
_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];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<void(const Token& token, const absl::string_view error)>
typedef std::function<void(Token token, const absl::string_view error)>
TokenListener;

// Listener notified with a User change.
typedef std::function<void(const User& user)> UserChangeListener;
typedef std::function<void(User user)> UserChangeListener;

/**
* Provides methods for getting the uid and token for the current user and
Expand Down
10 changes: 9 additions & 1 deletion Firestore/core/src/firebase/firestore/auth/token.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 19 additions & 0 deletions Firestore/core/src/firebase/firestore/auth/token.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <string>

#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 {
Expand All @@ -45,6 +46,7 @@ class Token {

/** The actual raw token. */
const std::string& token() const {
FIREBASE_ASSERT(is_valid_);
return token_;
}

Expand All @@ -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. */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a porting note since this concept of an invalid token doesn't exist elsewhere. Something to indicate why we need it, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

static const Token& Invalid();

private:
Token();

const std::string token_;
const User user_;
const bool is_valid_;
};

} // namespace auth
Expand Down
11 changes: 10 additions & 1 deletion Firestore/core/src/firebase/firestore/util/string_apple.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<void*>(static_cast<const void*>(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]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand All @@ -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());
});
Expand Down
Loading