Fix for b/74357976 and some cleanup. #887

mikelehen · 2018-03-08T03:51:34Z

I'm not completely sure what the intention was but it seems like in the C++ port we ended up conflating nil GetTokenResult objects (when getToken() results in an error) with nil token strings inside a valid GetTokenResult object (for unauthenticated users). Additionally we were creating absl::string_views wrapping a nullptr in some cases, which seems to be questionable (https://stackoverflow.com/a/10771938)?

I've taken a stab at fixing this by:

Only using Token::Invalid() when getToken() results in an error.
Using a (valid) Token containing token="" for unauthenticated users.

I'm not sure if this is the best fix (not sure how we're deciding to deal with null things in C++), and it's not the most minimal fix, but it at least hopefully demonstrates the issue so we can decide how we actually want to fix it.

FYI- This commit is based off the 4.10.0 tag currently and won't merge cleanly to master.

wilhuff · 2018-03-08T04:07:53Z

Firestore/Source/Auth/FSTEmptyCredentialsProvider.mm

@@ -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);


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

wilhuff · 2018-03-08T04:10:23Z

Firestore/Source/Remote/FSTDatastore.mm

@@ -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);


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

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

wilhuff · 2018-03-08T04:16:10Z

Firestore/core/src/firebase/firestore/auth/token.h

@@ -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. */


This doesn't seem right. A valid token is not "". An invalid token might be "" but this should have asserted before.

wilhuff · 2018-03-08T04:33:56Z

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm

@@ -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(


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.

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

wilhuff · 2018-03-08T04:37:23Z

Firestore/core/src/firebase/firestore/auth/token.cc

@@ -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 == ""));


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.

zxu123 · 2018-03-08T15:05:03Z

Firestore/core/src/firebase/firestore/auth/token.h

@@ -55,21 +55,22 @@ class Token {
   * state on disk, etc.).
   */
  const User& user() const {
+    FIREBASE_ASSERT(is_valid_);


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?

zxu123 · 2018-03-08T15:25:34Z

Firestore/Source/Auth/FSTCredentialsProvider.mm

@@ -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 = @"";


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.

zxu123

LGTM. resolved offline.

zxu123 · 2018-03-08T15:59:13Z

Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.mm

@@ -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(


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

wilhuff · 2018-03-21T21:38:07Z

This has been superseded by #945.

Fix for b/74357976 and some cleanup.

4a947ea

mikelehen assigned wilhuff and zxu123 Mar 8, 2018

googlebot added the cla: yes label Mar 8, 2018

wilhuff reviewed Mar 8, 2018

View reviewed changes

zxu123 reviewed Mar 8, 2018

View reviewed changes

mikelehen mentioned this pull request Mar 8, 2018

Minimal fix for b/74357976. #890

Merged

wilhuff closed this Mar 21, 2018

wilhuff deleted the mikelehen/fix-unauthenticated-access branch March 21, 2018 21:38

firebase locked and limited conversation to collaborators Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for b/74357976 and some cleanup. #887

Fix for b/74357976 and some cleanup. #887

mikelehen commented Mar 8, 2018 •

edited

Loading

wilhuff Mar 8, 2018

wilhuff Mar 8, 2018

wilhuff Mar 8, 2018

wilhuff Mar 8, 2018

zxu123 Mar 8, 2018

wilhuff Mar 8, 2018

zxu123 Mar 8, 2018

zxu123 Mar 8, 2018

zxu123 left a comment

zxu123 Mar 8, 2018

wilhuff commented Mar 21, 2018 •

edited

Loading

Fix for b/74357976 and some cleanup. #887

Fix for b/74357976 and some cleanup. #887

Conversation

mikelehen commented Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zxu123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilhuff commented Mar 21, 2018 • edited Loading

mikelehen commented Mar 8, 2018 •

edited

Loading

wilhuff commented Mar 21, 2018 •

edited

Loading