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.

Sign up for GitHub

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

Jump to bottom

replacing FSTGetTokenResult by C++ Token implementation #805

Merged
merged 10 commits into from
Feb 20, 2018
Merged

replacing FSTGetTokenResult by C++ Token implementation #805

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dc4f02c
replacing Auth/FSTUser by C++ auth implementation
zxu123 Feb 15, 2018
b502b8e
address changes
zxu123 Feb 15, 2018
bfbb0e2
replacing FSTGetTokenResult by C++ Token implementation
zxu123 Feb 15, 2018
a64ca8c
Merge branch 'master' into cpp/port_token
zxu123 Feb 15, 2018
9fb7911
address changes
zxu123 Feb 16, 2018
369f812
address changes
zxu123 Feb 16, 2018
6e4fdef
fix another const& v.s. dispatch bug
zxu123 Feb 16, 2018
5cb11ca
fix more const& v.s. dispatch bug zxu123 committed
zxu123 Feb 16, 2018
6775d9b
fix
zxu123 Feb 16, 2018
7d10621
passing by value in callback
zxu123 Feb 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 2 additions & 28 deletions Firestore/Source/Auth/FSTCredentialsProvider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,40 +16,14 @@

#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

/**
Expand All @@ -58,7 +32,7 @@ NS_ASSUME_NONNULL_BEGIN
* @param token An auth token as a string.
* @param error The error if one occurred, or else `nil`.
*/
typedef void (^FSTVoidGetTokenResultBlock)(FSTGetTokenResult *_Nullable token,
typedef void (^FSTVoidGetTokenResultBlock)(const firebase::firestore::auth::Token &token,
Copy link
Member

Choose a reason for hiding this comment

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

The objective-c code suggests this may be nullable? If so a const* may be required rather than const&. OTOH, if an invalid token is represented by a concrete Token object (rather than by null) then this may be fine. (Either way, you may want to adjust the docstring to clarify.)

Update: Oh, I see below that it is indeed a concrete object rather than null, so the signature is fine. Suggested change to the docstring:

@param token An auth token or X

but X depends on what you pass in. (see below). Possibilities:
X=" nil if an error occurred"
X=" Token::invalid() if an error occurred"

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. FYI: the code here will be removed by my next PR to replace the rest of Auth.

NSError *_Nullable error);

/** Listener block notified with a User. */
Expand Down
31 changes: 4 additions & 27 deletions Firestore/Source/Auth/FSTCredentialsProvider.mm
View file
Open in desktop
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,10 +119,9 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
NSError *cancelError = [NSError errorWithDomain:FIRFirestoreErrorDomain
code:FIRFirestoreErrorCodeAborted
userInfo:errorInfo];
completion(nil, cancelError);
completion({"", User::Unauthenticated()}, cancelError);
Copy link
Member

Choose a reason for hiding this comment

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

Two minor nits:

  1. I don't know what {"", User::Unauth()} is. I suppose I could go look at the signature for completion... but I'm pretty lazy. Maybe do this instead: completion(Token{"", User::Unauth()}, nil);
  2. Rather than relying on invalid values, you might consider adding a static Token::Invalid() factory method as well as a Token::IsInvalid() or similar instance method. (Or you could just use null like the obj-c code did.)

Both optional.

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

} else {
FSTGetTokenResult *result =
[[FSTGetTokenResult alloc] initWithUser:_currentUser token:token];
Token result(util::MakeStringView(token), _currentUser);
completion(result, error);
}
};
Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ @implementation FSTEmptyCredentialsProvider

- (void)getTokenForcingRefresh:(BOOL)forceRefresh
completion:(FSTVoidGetTokenResultBlock)completion {
completion(nil, nil);
completion({"", User::Unauthenticated()}, nil);
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 FSTCredentialsProvider.mm:122)

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

}

- (void)setUserChangeListener:(nullable FSTVoidUserBlock)block {
Expand Down
3 changes: 2 additions & 1 deletion Firestore/Source/Remote/FSTDatastore.h
View file
Open in desktop
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
41 changes: 23 additions & 18 deletions Firestore/Source/Remote/FSTDatastore.mm
View file
Open in desktop
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 @@ -299,22 +301,22 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory
errorHandler:(FSTVoidErrorBlock)errorHandler {
// 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
[self.credentials
getTokenForcingRefresh:NO
completion:^(FSTGetTokenResult *_Nullable 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];
[rpc start];
}
}];
}];
[self.credentials getTokenForcingRefresh:NO
completion:^(const Token &result, NSError *_Nullable error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a semantic mess. If there was an error there should be no token. This is why in the old signature both are nullable. Only one can be non-null.

We really need something like util::StatusOr to represent this.

For now let's make it clearer at the call site that we're generating an invalid token. If you added something like this to Token it would be clearer.

static Token Token::Invalid() {
  return {"", User::Unauthenticated()};
}

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

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()];
[rpc start];
}
}];
}];
}

- (FSTWatchStream *)createWatchStream {
Expand All @@ -334,8 +336,11 @@ - (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 =
Copy link
Contributor

Choose a reason for hiding this comment

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

This change isn't right. If there's no token this is setting this property to @"" where previously it would set it to nil.

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. empty check might be enough since an empty string token will cause the corruption of http header; so valid token will never be empty string. But let me check the nullptr instead just in case some test (not actually taking via a http call) may use empty string.

[[NSString alloc] initWithBytes:const_cast<void *>(static_cast<const void *>(token.data()))
length:token.length()
encoding:NSUTF8StringEncoding];
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
View file
Open in desktop
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:^(const Token &result, NSError *_Nullable error) {
error = [FSTDatastore firestoreErrorForError:error];
[self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{
[self resumeStartWithToken:result error:error];
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work. I'm surprised this isn't crashing you.

The token passed into the completion handler is a stack allocated temporary whose reference you're capturing to pass into the block passed into the dispatch queue. By the time that block actually runs the original token will have been destroyed.

You need to capture this by value rather than by reference to avoid these lifetime issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cannot change the signature, which has to match. I've made a copy; see whether this works. FYI: either ways the test all pass.

}];
}];
}

/** 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.token()];
FSTAssert(_callbackFilter == nil, @"GRX Filter must be nil");
_callbackFilter = [[FSTCallbackFilter alloc] initWithStream:self];
[_rpc startWithWriteable:_callbackFilter];
Expand Down