Change CredentialsProvider::TokenListener to use StatusOr<Token> #945

rsgowman · 2018-03-20T14:58:28Z

Rather than a token plus error code/msg.

This eliminates the concept of an "invalid" token, though an unauthenticated token is still possible.

Rather than a token plus error code/msg. Note that this is a direct conversion. In particular, it is still possible to have a token that isn't valid, i.e. this is still possible: ``` void foo(StatusOr<Token> token) { assert(token.ok()); Token& t = token.ValueOrDie(); assert(!t.is_valid()); } ```

Instead, we'll just use StatusOr<Token>. Note that unauthenticated tokens are handled as a special case; they're created via: Token::Unauthenticated() and are otherwise "valid", though attempting to retrieve the raw token on one of these tokens will cause an assertion failure.

rsgowman · 2018-03-20T14:59:45Z

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

@@ -46,7 +46,7 @@ class Token {

  /** The actual raw token. */
  const std::string& token() const {
-    FIREBASE_ASSERT(is_valid_);
+    FIREBASE_ASSERT(user_.is_authenticated());


This should prevent us from retrieving the token for an unauthenticated user. However, there's no reason we couldn't return it anyways (it's just the empty string).

We could change this to a dev assert, or eliminate it altogether.

rsgowman · 2018-03-20T15:00:56Z

No manual testing yet (will start now), but I wanted to get this out early to ensure this is the right approach.

wilhuff

Basic idea LGTM. A few minor notes about details.

wilhuff · 2018-03-20T15:20:19Z

Firestore/Source/Remote/FSTDatastore.mm

-                                       const absl::string_view error_msg) {
-        NSError *error = util::WrapNSError(error_code, error_msg);
+      /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr<Token> result) {
+        NSError *error = util::WrapNSError(result.status());


We've been calling these utilities two different things: "wrap parameter" or "make return type". We have a few cases where we did "wrap return type" but these seem to have been in error (mostly stemming from WrapNSStringNoCopy really wanting to convey that it was well and truly wrapping the underlying string.

I don't feel terribly strongly about this, but it seems like we could move toward a standard naming for this. Without expressing too strong a preference either way, how about MakeNSError?

I like that. (I like WrapParameter less, as WrapNSError can now take either a Status or an int/msg). Done.

I agree that WrapNSError sounds as if an existing NSError were wrapped in some object. Since this is a conversion, I can also suggest ToNSError or AsNSError in addition to MakeNSError.

Any chance, error_msg could be populated through? error code might be enough but error message comes handy to provide debug info.

If we've been doing 'make return type', then I prefer that for consistencies sake (and have done that here.)

(Though I like 'to return type' slightly better.)

error_msg should already be populated by the WrapNSError/MakeNSError call. (StatusOr contains both a code and an error message.)

wilhuff · 2018-03-20T15:24:27Z

Firestore/Source/Remote/FSTDatastore.mm

-                                       const absl::string_view error_msg) {
-        NSError *error = util::WrapNSError(error_code, error_msg);
+      /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr<Token> result) {
+        NSError *error = util::WrapNSError(result.status());
        [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{
          if (error) {


We should defer wrapping the error until we need it for interoperating so that we can get closer to the end state of just using status. This should be:

if (!result.ok()) { errorHandler(MakeNSError(result.status())); } // ...

wilhuff · 2018-03-20T15:28:36Z

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

 }

-Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) {
+Token::Token() : token_(), user_(User::Unauthenticated()) {


Is it reasonable to just wholesale avoid having this constructor?

We want callers to use Token::Unauthenticated when they mean this.

It's private, so callers can't use it anyways. But it also no longer adds any value. Removed.

wilhuff · 2018-03-20T15:31:09Z

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

-          error == nil ? FirestoreErrorCode::Ok : error.code,
-          error == nil ? "" : util::MakeStringView(error.localizedDescription));
+      if (error == nil) {
+        completion(Token{util::MakeStringView(token), contents->current_user});


This creates a semi-broken string view, wherein the contents are actually the nullptr. If token is nil, MakeStringView doesn't do anything special to ensure the string view is the empty string.

What does [str lengthOfBytesUsingEncoding:NSUTF8StringEncoding] return if str is nil?

Objective-C has a notion of nil dispatch. Essentially if you call a method on nil the result will be some zero value appropriate to the type: a nil pointer for NSObject-derived pointer types (including NSString), nullptr for unmanaged pointer types, zero for integral types, etc.

I believe MakeStringView would construct an absl::string_view{nullptr, 0} if passed a nil token.

Thanks for the explanation. I think it's a valid way to create an empty string_view (doc), unless some code relies on the assumption that empty string view is == "".

wilhuff · 2018-03-20T15:36:03Z

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

-
-  /** Returns an invalid token. */
-  static const Token& Invalid();
+  /** Returns a token for an unauthenticated user. */


Please get in the habit of double-checking whether or not there are equivalents on the other platforms and either:

avoid the new concept,

port use concepts over,

make PORTABILITY NOTES indicating the deviation and why, or

file bugs to sort this out as a follow-up

There is no equivalent concept on Android and the production code path in the credential provider doesn't actually use this to create the unauthenticated token.

wilhuff · 2018-03-20T15:37:20Z

Firestore/core/test/firebase/firestore/auth/empty_credentials_provider_test.cc

+        EXPECT_TRUE(result.ok());
+        const Token& token = result.ValueOrDie();
+        EXPECT_ANY_THROW(token.token());
+        const User& user = result.ValueOrDie().user();


You can make this token.user().

var-const · 2018-03-20T16:27:06Z

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

-          error == nil ? FirestoreErrorCode::Ok : error.code,
-          error == nil ? "" : util::MakeStringView(error.localizedDescription));
+      if (error == nil) {
+        completion(Token{util::MakeStringView(token), contents->current_user});


What does [str lengthOfBytesUsingEncoding:NSUTF8StringEncoding] return if str is nil?

var-const · 2018-03-20T16:28:32Z

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

+          error_code = static_cast<FirestoreErrorCode>(error.code);
+        }
+        completion(
+            util::Status(FirestoreErrorCode::Unknown,


Should you use error_code instead of Unknown here?

Yup (good catch).

var-const · 2018-03-20T16:34:21Z

Firestore/Source/Remote/FSTDatastore.mm

-                                       const absl::string_view error_msg) {
-        NSError *error = util::WrapNSError(error_code, error_msg);
+      /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr<Token> result) {
+        NSError *error = util::WrapNSError(result.status());


I agree that WrapNSError sounds as if an existing NSError were wrapped in some object. Since this is a conversion, I can also suggest ToNSError or AsNSError in addition to MakeNSError.

zxu123 · 2018-03-20T18:47:40Z

Firestore/Source/Remote/FSTDatastore.mm

-                                       const absl::string_view error_msg) {
-        NSError *error = util::WrapNSError(error_code, error_msg);
+      /*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr<Token> result) {
+        NSError *error = util::WrapNSError(result.status());


Any chance, error_msg could be populated through? error code might be enough but error message comes handy to provide debug info.

zxu123 · 2018-03-20T18:53:12Z

Firestore/Source/Remote/FSTDatastore.mm

        [self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{
          if (error) {
            errorHandler(error);
          } else {
            GRPCProtoCall *rpc = rpcFactory();
+            const Token &token = result.ValueOrDie();


Just my 1 cent, check result rather than error in the condition is safer to call result.ValueOrDie(). Then we only assume statusor's logic rather than also depending on MakeNSError's.

(See Gil's comment / done.)

zxu123 · 2018-03-20T19:00:23Z

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

-const Token& Token::Invalid() {
-  static const Token kInvalidToken;
-  return kInvalidToken;
+const Token& Token::Unauthenticated() {


Invalid() is added for nil. Now we don't need it. Do we want to add Unauthenticated()?

Previously, "invalid" meant either: (a) error condition, or (b) nil.
Now, "unauthenticated" means only (b) nil.

So really, you could view this PR as:

a rename from invalid -> unauthenticated

remove error conditions from this state (since we can now track that via Status/StatusOr)

wilhuff

LGTM

…ebase#945) * Change CredentialsProvider::TokenListener to use StatusOr Rather than a token plus error code/msg. * Eliminate the concept of an invalid Token Instead, we'll just use StatusOr<Token>. Note that unauthenticated tokens are handled as a special case; they're created via: Token::Unauthenticated() and are otherwise "valid", though attempting to retrieve the raw token on one of these tokens will cause an assertion failure.

rsgowman added 2 commits March 19, 2018 17:38

googlebot added the cla: yes label Mar 20, 2018

rsgowman commented Mar 20, 2018

View reviewed changes

rsgowman requested review from zxu123 and var-const March 20, 2018 15:01

wilhuff suggested changes Mar 20, 2018

View reviewed changes

var-const suggested changes Mar 20, 2018

View reviewed changes

zxu123 reviewed Mar 20, 2018

View reviewed changes

rsgowman added 2 commits March 20, 2018 15:42

review feedback

c8faf26

style.sh

dc61654

var-const approved these changes Mar 20, 2018

View reviewed changes

wilhuff approved these changes Mar 20, 2018

View reviewed changes

rsgowman merged commit 308acc0 into master Mar 21, 2018

rsgowman deleted the rsgowman/token_err_handling2 branch March 21, 2018 15:04

wilhuff mentioned this pull request Mar 21, 2018

Fix for b/74357976 and some cleanup. #887

Closed

firebase locked and limited conversation to collaborators Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change CredentialsProvider::TokenListener to use StatusOr<Token> #945

Change CredentialsProvider::TokenListener to use StatusOr<Token> #945

rsgowman commented Mar 20, 2018

rsgowman Mar 20, 2018

rsgowman commented Mar 20, 2018

wilhuff left a comment

wilhuff Mar 20, 2018

rsgowman Mar 20, 2018

var-const Mar 20, 2018

zxu123 Mar 20, 2018

rsgowman Mar 20, 2018

rsgowman Mar 20, 2018

wilhuff Mar 20, 2018

rsgowman Mar 20, 2018

wilhuff Mar 20, 2018

rsgowman Mar 20, 2018

wilhuff Mar 20, 2018

rsgowman Mar 20, 2018

var-const Mar 20, 2018

wilhuff Mar 20, 2018

var-const Mar 20, 2018

wilhuff Mar 20, 2018

rsgowman Mar 20, 2018

wilhuff Mar 20, 2018

rsgowman Mar 20, 2018

var-const Mar 20, 2018

var-const Mar 20, 2018

rsgowman Mar 20, 2018

var-const Mar 20, 2018

zxu123 Mar 20, 2018

zxu123 Mar 20, 2018

rsgowman Mar 20, 2018

zxu123 Mar 20, 2018

rsgowman Mar 20, 2018

wilhuff left a comment

Change CredentialsProvider::TokenListener to use StatusOr<Token> #945

Change CredentialsProvider::TokenListener to use StatusOr<Token> #945

Conversation

rsgowman commented Mar 20, 2018

Choose a reason for hiding this comment

rsgowman commented Mar 20, 2018

wilhuff left a 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

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

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

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

wilhuff left a comment

Choose a reason for hiding this comment