Skip to content

Fix for b/74357976 and some cleanup. #887

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

Closed
wants to merge 1 commit into from
Closed

Fix for b/74357976 and some cleanup. #887

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions Firestore/Source/Auth/FSTCredentialsProvider.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = @"";
Copy link
Contributor

Choose a reason for hiding this comment

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

string_view can be initialized with nullptr, in which case, the .data() returns nullptr, which is what the prepareHeadersForRPC relies on whether to prepare the header or fallback to default. (

firebase-ios-sdk/Firestore/Source/Remote/FSTDatastore.mm

Line 347 in b7750b5

rpc.oauth2AccessToken = token.data() == nullptr ? nil : util::WrapNSString(token);
)

i.e. token string_view can differentiate between nil and "". For nil, rpc.oauth2AccessToken is nil, which makes it fallback to default (without actually write that header line) v.s. for "", which makes it write a header line with token being empty string. This also makes me think we should never allow token being empty string, since it will cause the header being invalid to the server.

}
completion(Token(util::MakeStringView(token), self->_currentUser), error);
}
};
Expand Down
3 changes: 1 addition & 2 deletions Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why isn't this Token::Invalid() still?

}

- (void)setUserChangeListener:(nullable FSTVoidUserBlock)block {
Expand Down
8 changes: 5 additions & 3 deletions Firestore/Source/Remote/FSTDatastore.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
}];
Expand All @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just pass the Token down into here? Then this could be

token.is_valid() ? util::WrapNSString(token.token()) : nil;

rpc.requestHeaders[kXGoogAPIClientHeader] = [FSTDatastore googAPIClientHeaderValue];
// This header is used to improve routing and project isolation by the backend.
rpc.requestHeaders[kGoogleCloudResourcePrefix] =
Expand Down
3 changes: 2 additions & 1 deletion Firestore/Source/Remote/FSTStream.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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];
Expand Down
2 changes: 1 addition & 1 deletion Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
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 the bug in the line below:

token can be nil but we're still invoking the constructor that creates a token that appears valid, which then triggered all the rest of the code to behave incorrectly. Your other changes are also reasonable, but I think we have to be considerably more defensive about the values we accept from auth.

I think we should probably move to something like what I did for User. We should construct the right kind of token (vaild or not) given our inputs rather than making the constructor blindly set is_valid to true or false.

Copy link
Contributor

Choose a reason for hiding this comment

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

The C++ credential-provider is not used yet in the release. The porting of credential-provider is after the release cut.

Expand Down
3 changes: 3 additions & 0 deletions Firestore/core/src/firebase/firestore/auth/token.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 == ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

If this assertion failed I'm not sure what I would do about it; as it stands I can barely understand it.

Secondarily this seems like a weak assertion given that we're filling in is_valid_(true) above.

Tertiarily this seems backwards: we should be constructing the right kind token given our inputs rather than requiring every caller to get this right. We clearly haven't done that here so I propose we move all the callers to a factory method that returns a valid token given valid inputs or an Invalid token otherwise. At that point we don't have to fail if auth gives us a crazy value:

If the user is unauthenticated or the token is empty return invalid. Otherwise fill in a valid Token (and this constructor becomes private).

In the interest of good citizenry we could also make the assertion that these align, but that would be for reporting bad values to FIRAuth and less for catching our code out.

}

Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) {
Expand Down
13 changes: 7 additions & 6 deletions Firestore/core/src/firebase/firestore/auth/token.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
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 seem right. A valid token is not "". An invalid token might be "" but this should have asserted before.

const std::string& token() const {
FIREBASE_ASSERT(is_valid_);
return token_;
Expand All @@ -55,21 +55,22 @@ class Token {
* state on disk, etc.).
*/
const User& user() const {
FIREBASE_ASSERT(is_valid_);
Copy link
Contributor

Choose a reason for hiding this comment

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

Code somewhere actually obtains user from invalid token; seems that's why you also changed empty-credential-provider to passing-in a valid token with invalid content. I guess we first need to come with what exactly Token will be, in real world and in test cases. That can help define the logic better. Say, can we make the following assumption:

In real world:

  • Tried to get token for anonymous user: token obj is invalid and both token-string and user are undefined (i.e. the caller should check the validity and not depends on the token-string and user);
  • Get token failed for a sign-in user: token obj is invalid and token-string is undefined and user are sign-in (possible? do we ever need to recover user from invalid token.);
  • Get token succeeded for a sign-in user: token obj is valid and token string is non-empty and user is sign-in;
  • Could Auth ever return a valid token for anonymous user?

In test cases: all of above besides

  • Tried to get token for anonymous user: token-string is undefined or empty string whatever to make the header fallback to default in test and user is anonymous. Should this obj be valid or invalid?

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:
Expand Down
4 changes: 4 additions & 0 deletions Firestore/core/src/firebase/firestore/util/string_apple.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#import <Foundation/Foundation.h>

#include "absl/strings/string_view.h"
#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"

namespace firebase {
namespace firestore {
Expand Down Expand Up @@ -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]);
}
Expand Down
2 changes: 1 addition & 1 deletion Firestore/core/test/firebase/firestore/auth/token_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down