Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
address changes
  • Loading branch information
zxu123 committed Feb 16, 2018
commit 9fb7911e58fafeb8fc19b179277cfa634cf09582
2 changes: 1 addition & 1 deletion Firestore/Source/Auth/FSTCredentialsProvider.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ NS_ASSUME_NONNULL_BEGIN
/**
* `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)(const firebase::firestore::auth::Token &token,

Copy link
Copy Markdown
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
Copy Markdown
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.

Expand Down
2 changes: 1 addition & 1 deletion Firestore/Source/Auth/FSTCredentialsProvider.mm
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
NSError *cancelError = [NSError errorWithDomain:FIRFirestoreErrorDomain
code:FIRFirestoreErrorCodeAborted
userInfo:errorInfo];
completion({"", User::Unauthenticated()}, cancelError);
completion(Token::Invalid(), cancelError);
} else {
Token result(util::MakeStringView(token), _currentUser);
completion(result, 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({"", User::Unauthenticated()}, nil);
// Invalid token will force the GRPC fallback to use default settings.
completion(Token::Invalid(), nil);
}

- (void)setUserChangeListener:(nullable FSTVoidUserBlock)block {
Expand Down
43 changes: 24 additions & 19 deletions Firestore/Source/Remote/FSTDatastore.mm
Original file line number Diff line number Diff line change
Expand Up @@ -301,22 +301,24 @@ - (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:^(const 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()];
[rpc start];
}
}];
}];
[self.credentials
getTokenForcingRefresh:NO
completion:^(const 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.is_valid() ? result.token()
: absl::string_view())];
[rpc start];
}
}];
}];
}

- (FSTWatchStream *)createWatchStream {
Expand All @@ -338,9 +340,12 @@ + (void)prepareHeadersForRPC:(GRPCCall *)rpc
databaseID:(const DatabaseId *)databaseID
token:(const absl::string_view)token {

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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];
token.data() == nullptr
? nil
: [[NSString alloc]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this could be a WrapNSString in string_apple.h (to parallel WrapNSStringNoCopy). I'm sure we'll see this again.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

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
2 changes: 1 addition & 1 deletion Firestore/Source/Remote/FSTStream.mm
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ - (void)resumeStartWithToken:(const Token &)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
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
12 changes: 12 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,19 @@ class Token {
return user_;
}

bool is_valid() const {
return is_valid_;
}

/** Returns an invalid token. */

Copy link
Copy Markdown
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
Copy Markdown
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
8 changes: 8 additions & 0 deletions Firestore/core/test/firebase/firestore/auth/token_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down