Skip to content

Commit 308acc0

Browse files
rsgowmanrsgowman
authored
Change CredentialsProvider::TokenListener to use StatusOr<Token> (#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.
1 parent d924771 commit 308acc0

12 files changed

+98
-91
lines changed

‎Firestore/Source/Remote/FSTDatastore.mm

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -305,20 +305,18 @@ - (void)invokeRPCWithFactory:(GRPCProtoCall * (^)(void))rpcFactory
305305
// TODO(mikelehen): We should force a refresh if the previous RPC failed due to an expired token,
306306
// but I'm not sure how to detect that right now. http://b/32762461
307307
_credentials->GetToken(
308-
/*force_refresh=*/false,
309-
[self, rpcFactory, errorHandler](Token result, const int64_t error_code,
310-
const absl::string_view error_msg) {
311-
NSError *error = util::WrapNSError(error_code, error_msg);
308+
/*force_refresh=*/false, [self, rpcFactory, errorHandler](util::StatusOr<Token> result) {
312309
[self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{
313-
if (error) {
314-
errorHandler(error);
310+
if (!result.ok()) {
311+
errorHandler(util::MakeNSError(result.status()));
315312
} else {
316313
GRPCProtoCall *rpc = rpcFactory();
314+
const Token &token = result.ValueOrDie();
317315
[FSTDatastore
318316
prepareHeadersForRPC:rpc
319317
databaseID:&self.databaseInfo->database_id()
320-
token:(result.user().is_authenticated() ? result.token()
321-
: absl::string_view())];
318+
token:(token.user().is_authenticated() ? token.token()
319+
: absl::string_view())];
322320
[rpc start];
323321
}
324322
}];

‎Firestore/Source/Remote/FSTStream.mm

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -265,17 +265,15 @@ - (void)startWithDelegate:(id)delegate {
265265
_delegate = delegate;
266266

267267
_credentials->GetToken(
268-
/*force_refresh=*/false,
269-
[self](Token result, const int64_t error_code, const absl::string_view error_msg) {
270-
NSError *error = util::WrapNSError(error_code, error_msg);
268+
/*force_refresh=*/false, [self](util::StatusOr<Token> result) {
271269
[self.workerDispatchQueue dispatchAsyncAllowingSameQueue:^{
272-
[self resumeStartWithToken:result error:error];
270+
[self resumeStartWithToken:result];
273271
}];
274272
});
275273
}
276274

277275
/** Add an access token to our RPC, after obtaining one from the credentials provider. */
278-
- (void)resumeStartWithToken:(const Token &)token error:(NSError *)error {
276+
- (void)resumeStartWithToken:(const util::StatusOr<Token> &)result {
279277
[self.workerDispatchQueue verifyIsCurrentQueue];
280278

281279
if (self.state == FSTStreamStateStopped) {
@@ -287,16 +285,17 @@ - (void)resumeStartWithToken:(const Token &)token error:(NSError *)error {
287285

288286
// TODO(mikelehen): We should force a refresh if the previous RPC failed due to an expired token,
289287
// but I'm not sure how to detect that right now. http://b/32762461
290-
if (error) {
288+
if (!result.ok()) {
291289
// RPC has not been started yet, so just invoke higher-level close handler.
292-
[self handleStreamClose:error];
290+
[self handleStreamClose:util::MakeNSError(result.status())];
293291
return;
294292
}
295293

296294
self.requestsWriter = [[FSTBufferedWriter alloc] init];
297295
_rpc = [self createRPCWithRequestsWriter:self.requestsWriter];
298296
[_rpc setResponseDispatchQueue:self.workerDispatchQueue.queue];
299297

298+
const Token &token = result.ValueOrDie();
300299
[FSTDatastore
301300
prepareHeadersForRPC:_rpc
302301
databaseID:&self.databaseInfo->database_id()

‎Firestore/core/src/firebase/firestore/auth/credentials_provider.h

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,15 @@
2323
#include "Firestore/core/include/firebase/firestore/firestore_errors.h"
2424
#include "Firestore/core/src/firebase/firestore/auth/token.h"
2525
#include "Firestore/core/src/firebase/firestore/auth/user.h"
26+
#include "Firestore/core/src/firebase/firestore/util/statusor.h"
2627
#include "absl/strings/string_view.h"
2728

2829
namespace firebase {
2930
namespace firestore {
3031
namespace auth {
3132

3233
// `TokenErrorListener` is a listener that gets a token or an error.
33-
// token: An auth token as a string, or nullptr if error occurred.
34-
// error_code: The error code if one occurred, or else FirestoreErrorCode::Ok.
35-
// error_msg: The error if one occurred, or else nullptr.
36-
typedef std::function<void(
37-
Token token, const int64_t error_code, const absl::string_view error_msg)>
38-
TokenListener;
34+
typedef std::function<void(util::StatusOr<Token>)> TokenListener;
3935

4036
// Listener notified with a User change.
4137
typedef std::function<void(User user)> UserChangeListener;

‎Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.cc

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,9 @@ void EmptyCredentialsProvider::GetToken(bool force_refresh,
2626
TokenListener completion) {
2727
UNUSED(force_refresh);
2828
if (completion) {
29-
// Invalid token will force the GRPC fallback to use default settings.
30-
completion(Token::Invalid(), FirestoreErrorCode::Ok, "");
29+
// Unauthenticated token will force the GRPC fallback to use default
30+
// settings.
31+
completion(Token::Unauthenticated());
3132
}
3233
}
3334

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

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
#include "Firestore/core/src/firebase/firestore/util/firebase_assert.h"
2424
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"
2525

26+
// NB: This is also defined in Firestore/Source/Public/FIRFirestoreErrors.h
27+
NSString* const FIRFirestoreErrorDomain = @"FIRFirestoreErrorDomain";
28+
2629
namespace firebase {
2730
namespace firestore {
2831
namespace auth {
@@ -84,27 +87,38 @@
8487
int initial_user_counter = contents_->user_counter;
8588

8689
std::weak_ptr<Contents> weak_contents = contents_;
87-
void (^get_token_callback)(NSString*, NSError*) = ^(
88-
NSString* _Nullable token, NSError* _Nullable error) {
89-
std::shared_ptr<Contents> contents = weak_contents.lock();
90-
if (!contents) {
91-
return;
92-
}
93-
94-
std::unique_lock<std::mutex> lock(contents->mutex);
95-
if (initial_user_counter != contents->user_counter) {
96-
// Cancel the request since the user changed while the request was
97-
// outstanding so the response is likely for a previous user (which
98-
// user, we can't be sure).
99-
completion(Token::Invalid(), FirestoreErrorCode::Aborted,
100-
"getToken aborted due to user change.");
101-
} else {
102-
completion(
103-
Token{util::MakeStringView(token), contents->current_user},
104-
error == nil ? FirestoreErrorCode::Ok : error.code,
105-
error == nil ? "" : util::MakeStringView(error.localizedDescription));
106-
}
107-
};
90+
void (^get_token_callback)(NSString*, NSError*) =
91+
^(NSString* _Nullable token, NSError* _Nullable error) {
92+
std::shared_ptr<Contents> contents = weak_contents.lock();
93+
if (!contents) {
94+
return;
95+
}
96+
97+
std::unique_lock<std::mutex> lock(contents->mutex);
98+
if (initial_user_counter != contents->user_counter) {
99+
// Cancel the request since the user changed while the request was
100+
// outstanding so the response is likely for a previous user (which
101+
// user, we can't be sure).
102+
completion(util::Status(FirestoreErrorCode::Aborted,
103+
"getToken aborted due to user change."));
104+
} else {
105+
if (error == nil) {
106+
if (token != nil) {
107+
completion(
108+
Token{util::MakeStringView(token), contents->current_user});
109+
} else {
110+
completion(Token::Unauthenticated());
111+
}
112+
} else {
113+
FirestoreErrorCode error_code = FirestoreErrorCode::Unknown;
114+
if (error.domain == FIRFirestoreErrorDomain) {
115+
error_code = static_cast<FirestoreErrorCode>(error.code);
116+
}
117+
completion(util::Status(
118+
error_code, util::MakeStringView(error.localizedDescription)));
119+
}
120+
}
121+
};
108122

109123
[contents_->app getTokenForcingRefresh:force_refresh
110124
withCallback:get_token_callback];

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,13 @@ namespace firestore {
2121
namespace auth {
2222

2323
Token::Token(const absl::string_view token, const User& user)
24-
: token_(token), user_(user), is_valid_(true) {
24+
: token_(token), user_(user) {
2525
}
2626

27-
Token::Token() : token_(), user_(User::Unauthenticated()), is_valid_(false) {
28-
}
29-
30-
const Token& Token::Invalid() {
31-
static const Token kInvalidToken;
32-
return kInvalidToken;
27+
const Token& Token::Unauthenticated() {
28+
static const Token kUnauthenticatedToken(absl::string_view(),
29+
User::Unauthenticated());
30+
return kUnauthenticatedToken;
3331
}
3432

3533
} // namespace auth

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

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class Token {
4646

4747
/** The actual raw token. */
4848
const std::string& token() const {
49-
FIREBASE_ASSERT(is_valid_);
49+
FIREBASE_ASSERT(user_.is_authenticated());
5050
return token_;
5151
}
5252

@@ -59,25 +59,17 @@ class Token {
5959
}
6060

6161
/**
62-
* Whether the token is a valid one.
62+
* Returns a token for an unauthenticated user.
6363
*
64-
* ## Portability notes: Invalid token is the equivalent of nil in the iOS
65-
* token implementation. We use value instead of pointer for Token instance in
66-
* the C++ migration.
64+
* ## Portability notes: An unauthenticated token is the equivalent of
65+
* nil/null in the iOS/TypeScript token implementation. We use a reference
66+
* instead of a pointer for Token instances in the C++ migration.
6767
*/
68-
bool is_valid() const {
69-
return is_valid_;
70-
}
71-
72-
/** Returns an invalid token. */
73-
static const Token& Invalid();
68+
static const Token& Unauthenticated();
7469

7570
private:
76-
Token();
77-
7871
const std::string token_;
7972
const User user_;
80-
const bool is_valid_;
8173
};
8274

8375
} // namespace auth

‎Firestore/core/src/firebase/firestore/util/error_apple.h

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
#include "Firestore/Source/Public/FIRFirestoreErrors.h" // for FIRFirestoreErrorDomain
2626
#include "Firestore/core/include/firebase/firestore/firestore_errors.h"
27+
#include "Firestore/core/src/firebase/firestore/util/status.h"
2728
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"
2829
#include "absl/strings/string_view.h"
2930

@@ -32,7 +33,7 @@ namespace firestore {
3233
namespace util {
3334

3435
// Translates a set of error_code and error_msg to an NSError.
35-
inline NSError* WrapNSError(const int64_t error_code,
36+
inline NSError* MakeNSError(const int64_t error_code,
3637
const absl::string_view error_msg) {
3738
if (error_code == FirestoreErrorCode::Ok) {
3839
return nil;
@@ -43,6 +44,10 @@ inline NSError* WrapNSError(const int64_t error_code,
4344
userInfo:@{NSLocalizedDescriptionKey : WrapNSString(error_msg)}];
4445
}
4546

47+
inline NSError* MakeNSError(const util::Status& status) {
48+
return MakeNSError(status.code(), status.error_message());
49+
}
50+
4651
} // namespace util
4752
} // namespace firestore
4853
} // namespace firebase

‎Firestore/core/test/firebase/firestore/auth/credentials_provider_test.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include "Firestore/core/src/firebase/firestore/auth/credentials_provider.h"
1818

19+
#include "Firestore/core/src/firebase/firestore/util/statusor.h"
1920
#include "gtest/gtest.h"
2021

2122
namespace firebase {
@@ -25,11 +26,8 @@ namespace auth {
2526
#define UNUSED(x) (void)(x)
2627

2728
TEST(CredentialsProvider, Typedef) {
28-
TokenListener token_listener = [](Token token, const int64_t error_code,
29-
const absl::string_view error_msg) {
29+
TokenListener token_listener = [](util::StatusOr<Token> token) {
3030
UNUSED(token);
31-
UNUSED(error_code);
32-
UNUSED(error_msg);
3331
};
3432
EXPECT_NE(nullptr, token_listener);
3533
EXPECT_TRUE(token_listener);

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
#include "Firestore/core/src/firebase/firestore/auth/empty_credentials_provider.h"
1818

19+
#include "Firestore/core/src/firebase/firestore/util/statusor.h"
1920
#include "gtest/gtest.h"
2021

2122
namespace firebase {
@@ -25,14 +26,13 @@ namespace auth {
2526
TEST(EmptyCredentialsProvider, GetToken) {
2627
EmptyCredentialsProvider credentials_provider;
2728
credentials_provider.GetToken(
28-
/*force_refresh=*/true, [](Token token, const int64_t error_code,
29-
const absl::string_view error_msg) {
30-
EXPECT_FALSE(token.is_valid());
29+
/*force_refresh=*/true, [](util::StatusOr<Token> result) {
30+
EXPECT_TRUE(result.ok());
31+
const Token& token = result.ValueOrDie();
32+
EXPECT_ANY_THROW(token.token());
3133
const User& user = token.user();
3234
EXPECT_EQ("", user.uid());
3335
EXPECT_FALSE(user.is_authenticated());
34-
EXPECT_EQ(FirestoreErrorCode::Ok, error_code);
35-
EXPECT_EQ("", error_msg);
3636
});
3737
}
3838

‎Firestore/core/test/firebase/firestore/auth/firebase_credentials_provider_test.mm

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,11 @@
1717
#include "Firestore/core/src/firebase/firestore/auth/firebase_credentials_provider_apple.h"
1818

1919
#import <FirebaseCore/FIRApp.h>
20+
2021
#import <FirebaseCore/FIRAppInternal.h>
2122
#import <FirebaseCore/FIROptionsInternal.h>
2223

24+
#include "Firestore/core/src/firebase/firestore/util/statusor.h"
2325
#include "Firestore/core/src/firebase/firestore/util/string_apple.h"
2426
#include "Firestore/core/test/firebase/firestore/testutil/app_testing.h"
2527

@@ -29,43 +31,49 @@
2931
namespace firestore {
3032
namespace auth {
3133

32-
FIRApp* AppWithFakeUid(NSString* _Nullable uid) {
34+
FIRApp* AppWithFakeUidAndToken(NSString* _Nullable uid,
35+
NSString* _Nullable token) {
3336
FIRApp* app = testutil::AppForUnitTesting();
3437
app.getUIDImplementation = ^NSString* {
3538
return uid;
3639
};
40+
app.getTokenImplementation = ^(BOOL, FIRTokenCallback callback) {
41+
callback(token, nil);
42+
};
3743
return app;
3844
}
3945

46+
FIRApp* AppWithFakeUid(NSString* _Nullable uid) {
47+
return AppWithFakeUidAndToken(uid, uid == nil ? nil : @"default token");
48+
}
49+
4050
TEST(FirebaseCredentialsProviderTest, GetTokenUnauthenticated) {
4151
FIRApp* app = AppWithFakeUid(nil);
4252

4353
FirebaseCredentialsProvider credentials_provider(app);
4454
credentials_provider.GetToken(
45-
/*force_refresh=*/true, [](Token token, const int64_t error_code,
46-
const absl::string_view error_msg) {
47-
EXPECT_EQ("", token.token());
55+
/*force_refresh=*/true, [](util::StatusOr<Token> result) {
56+
EXPECT_TRUE(result.ok());
57+
const Token& token = result.ValueOrDie();
58+
EXPECT_ANY_THROW(token.token());
4859
const User& user = token.user();
4960
EXPECT_EQ("", user.uid());
5061
EXPECT_FALSE(user.is_authenticated());
51-
EXPECT_EQ(FirestoreErrorCode::Ok, error_code) << error_code;
52-
EXPECT_EQ("", error_msg) << error_msg;
5362
});
5463
}
5564

5665
TEST(FirebaseCredentialsProviderTest, GetToken) {
57-
FIRApp* app = AppWithFakeUid(@"fake uid");
66+
FIRApp* app = AppWithFakeUidAndToken(@"fake uid", @"token for fake uid");
5867

5968
FirebaseCredentialsProvider credentials_provider(app);
6069
credentials_provider.GetToken(
61-
/*force_refresh=*/true, [](Token token, const int64_t error_code,
62-
const absl::string_view error_msg) {
63-
EXPECT_EQ("", token.token());
70+
/*force_refresh=*/true, [](util::StatusOr<Token> result) {
71+
EXPECT_TRUE(result.ok());
72+
const Token& token = result.ValueOrDie();
73+
EXPECT_EQ("token for fake uid", token.token());
6474
const User& user = token.user();
6575
EXPECT_EQ("fake uid", user.uid());
6676
EXPECT_TRUE(user.is_authenticated());
67-
EXPECT_EQ(FirestoreErrorCode::Ok, error_code) << error_code;
68-
EXPECT_EQ("", error_msg) << error_msg;
6977
});
7078
}
7179

‎Firestore/core/test/firebase/firestore/auth/token_test.cc

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,12 @@ TEST(Token, Getter) {
2626
Token token("token", User("abc"));
2727
EXPECT_EQ("token", token.token());
2828
EXPECT_EQ(User("abc"), token.user());
29-
EXPECT_TRUE(token.is_valid());
3029
}
3130

32-
TEST(Token, InvalidToken) {
33-
const Token& token = Token::Invalid();
31+
TEST(Token, UnauthenticatedToken) {
32+
const Token& token = Token::Unauthenticated();
3433
EXPECT_ANY_THROW(token.token());
3534
EXPECT_EQ(User::Unauthenticated(), token.user());
36-
EXPECT_FALSE(token.is_valid());
3735
}
3836

3937
} // namespace auth

0 commit comments

Comments
 (0)