Skip to content

Commit edf5973

Browse files
paulb777paulb777
authored
[Firestore] Fix crash fetching Auth and App Check tokens (#15558)
1 parent 1feabe5 commit edf5973

7 files changed

+67
-51
lines changed

‎Firestore/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
# Unreleased
2+
- [fixed] Fix crash while fetching Auth and App Check tokens. (#15281)
23
- [feature] `Pipeline` support is now available for the `Enterprise edition` as a public review feature. (#15625)
34
- [fixed] Fixed an issue where the returned object in transaction blocks could not
45
pass across actor boundaries in Swift 6 (#15467).

‎Firestore/core/src/credentials/firebase_app_check_credentials_provider_apple.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ namespace firestore {
3838
namespace credentials {
3939

4040
class FirebaseAppCheckCredentialsProvider
41-
: public CredentialsProvider<std::string, std::string> {
41+
: public CredentialsProvider<std::string, std::string>,
42+
public std::enable_shared_from_this<FirebaseAppCheckCredentialsProvider> {
4243
public:
4344
FirebaseAppCheckCredentialsProvider(FIRApp* app,
4445
id<FIRAppCheckInterop> app_check);

‎Firestore/core/src/credentials/firebase_app_check_credentials_provider_apple.mm‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -77,8 +77,11 @@
7777

7878
void FirebaseAppCheckCredentialsProvider::GetToken(
7979
TokenListener<std::string> completion) {
80-
std::weak_ptr<Contents> weak_contents = contents_;
81-
if (contents_->app_check) {
80+
// Capture self via a shared_ptr to ensure that the credentials provider
81+
// is not deallocated while the getToken request is outstanding.
82+
std::shared_ptr<FirebaseAppCheckCredentialsProvider> self =
83+
shared_from_this();
84+
if (self->contents_->app_check) {
8285
void (^get_token_callback)(id<FIRAppCheckTokenResultInterop>) =
8386
^(id<FIRAppCheckTokenResultInterop> result) {
8487
if (result.error != nil) {
@@ -90,13 +93,13 @@
9093

9194
// Retrieve a cached or generate a new FAC Token. If forcingRefresh == YES
9295
// always generates a new token and updates the cache.
93-
[contents_->app_check getTokenForcingRefresh:force_refresh_
94-
completion:get_token_callback];
96+
[self->contents_->app_check getTokenForcingRefresh:self->force_refresh_
97+
completion:get_token_callback];
9598
} else {
9699
// If there's no AppCheck provider, call back immediately with a nil token.
97100
completion(std::string{""});
98101
}
99-
force_refresh_ = false;
102+
self->force_refresh_ = false;
100103
}
101104

102105
void FirebaseAppCheckCredentialsProvider::SetCredentialChangeListener(

‎Firestore/core/src/credentials/firebase_auth_credentials_provider_apple.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ namespace credentials {
5353
* For non-Apple desktop build, this is right now just a stub.
5454
*/
5555
class FirebaseAuthCredentialsProvider
56-
: public CredentialsProvider<AuthToken, User> {
56+
: public CredentialsProvider<AuthToken, User>,
57+
public std::enable_shared_from_this<FirebaseAuthCredentialsProvider> {
5758
public:
5859
// TODO(zxu123): Provide a ctor to accept the C++ Firebase Games App, which
5960
// deals all platforms. Right now, only works for FIRApp*.

‎Firestore/core/src/credentials/firebase_auth_credentials_provider_apple.mm‎

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -81,26 +81,23 @@
8181
// fail if there is a token change while the request is outstanding.
8282
int initial_token_counter = contents_->token_counter;
8383

84-
std::weak_ptr<Contents> weak_contents = contents_;
84+
// Capture self via a shared_ptr to ensure that the credentials provider
85+
// is not deallocated while the getToken request is outstanding.
86+
std::shared_ptr<FirebaseAuthCredentialsProvider> self = shared_from_this();
8587
void (^get_token_callback)(NSString*, NSError*) =
8688
^(NSString* _Nullable token, NSError* _Nullable error) {
87-
std::shared_ptr<Contents> contents = weak_contents.lock();
88-
if (!contents) {
89-
return;
90-
}
91-
92-
std::unique_lock<std::mutex> lock(contents->mutex);
93-
if (initial_token_counter != contents->token_counter) {
89+
std::unique_lock<std::mutex> lock(self->contents_->mutex);
90+
if (initial_token_counter != self->contents_->token_counter) {
9491
// Cancel the request since the user changed while the request was
9592
// outstanding so the response is likely for a previous user (which
9693
// user, we can't be sure).
9794
LOG_DEBUG("GetToken aborted due to token change.");
98-
return GetToken(completion);
95+
return self->GetToken(completion);
9996
} else {
10097
if (error == nil) {
10198
if (token != nil) {
102-
completion(
103-
AuthToken{util::MakeString(token), contents->current_user});
99+
completion(AuthToken{util::MakeString(token),
100+
self->contents_->current_user});
104101
} else {
105102
completion(AuthToken::Unauthenticated());
106103
}

‎Firestore/core/test/unit/credentials/firebase_app_check_credentials_provider_test.mm‎

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -97,8 +97,9 @@ - (nonnull NSString*)tokenDidChangeNotificationName {
9797
auto token_promise = std::make_shared<std::promise<std::string>>();
9898

9999
FIRApp* app = testutil::AppForUnitTesting();
100-
FirebaseAppCheckCredentialsProvider credentials_provider(app, nil);
101-
credentials_provider.GetToken(
100+
auto credentials_provider =
101+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, nil);
102+
credentials_provider->GetToken(
102103
[token_promise](util::StatusOr<std::string> result) {
103104
EXPECT_TRUE(result.ok());
104105
const std::string& token = result.ValueOrDie();
@@ -115,8 +116,9 @@ - (nonnull NSString*)tokenDidChangeNotificationName {
115116
FIRApp* app = testutil::AppForUnitTesting();
116117
FSTAppCheckFake* app_check =
117118
[[FSTAppCheckFake alloc] initWithToken:@"fake token"];
118-
FirebaseAppCheckCredentialsProvider credentials_provider(app, app_check);
119-
credentials_provider.GetToken([](util::StatusOr<std::string> result) {
119+
auto credentials_provider =
120+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, app_check);
121+
credentials_provider->GetToken([](util::StatusOr<std::string> result) {
120122
EXPECT_TRUE(result.ok());
121123
const std::string& token = result.ValueOrDie();
122124
EXPECT_EQ("fake token", token);
@@ -127,20 +129,22 @@ - (nonnull NSString*)tokenDidChangeNotificationName {
127129
FIRApp* app = testutil::AppForUnitTesting();
128130
FSTAppCheckFake* app_check =
129131
[[FSTAppCheckFake alloc] initWithToken:@"fake token"];
130-
FirebaseAppCheckCredentialsProvider credentials_provider(app, app_check);
131-
credentials_provider.SetCredentialChangeListener(
132+
auto credentials_provider =
133+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, app_check);
134+
credentials_provider->SetCredentialChangeListener(
132135
[](std::string token) { EXPECT_EQ("", token); });
133136

134-
credentials_provider.SetCredentialChangeListener(nullptr);
137+
credentials_provider->SetCredentialChangeListener(nullptr);
135138
}
136139

137140
TEST(FirebaseAppCheckCredentialsProviderTest, InvalidateToken) {
138141
FIRApp* app = testutil::AppForUnitTesting();
139142
FSTAppCheckFake* app_check =
140143
[[FSTAppCheckFake alloc] initWithToken:@"fake token"];
141-
FirebaseAppCheckCredentialsProvider credentials_provider(app, app_check);
142-
credentials_provider.InvalidateToken();
143-
credentials_provider.GetToken(
144+
auto credentials_provider =
145+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, app_check);
146+
credentials_provider->InvalidateToken();
147+
credentials_provider->GetToken(
144148
[&app_check](util::StatusOr<std::string> result) {
145149
EXPECT_TRUE(result.ok());
146150
EXPECT_TRUE(app_check.forceRefreshTriggered);
@@ -154,9 +158,10 @@ - (nonnull NSString*)tokenDidChangeNotificationName {
154158

155159
FIRApp* app = testutil::AppForUnitTesting();
156160
FSTAppCheckFake* app_check = [[FSTAppCheckFake alloc] initWithToken:@""];
157-
FirebaseAppCheckCredentialsProvider credentials_provider(app, app_check);
161+
auto credentials_provider =
162+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, app_check);
158163

159-
credentials_provider.SetCredentialChangeListener(
164+
credentials_provider->SetCredentialChangeListener(
160165
[token_promise](const std::string& result) {
161166
if (result != "") {
162167
token_promise->set_value(result);
@@ -186,9 +191,10 @@ - (nonnull NSString*)tokenDidChangeNotificationName {
186191

187192
FIRApp* app = testutil::AppForUnitTesting();
188193
FSTAppCheckFake* app_check = [[FSTAppCheckFake alloc] initWithToken:@""];
189-
FirebaseAppCheckCredentialsProvider credentials_provider(app, app_check);
194+
auto credentials_provider =
195+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, app_check);
190196

191-
credentials_provider.SetCredentialChangeListener(
197+
credentials_provider->SetCredentialChangeListener(
192198
[token_promise](const std::string& result) {
193199
if (result != "") {
194200
token_promise->set_value(result);
@@ -227,9 +233,10 @@ - (nonnull NSString*)tokenDidChangeNotificationName {
227233

228234
FIRApp* app = testutil::AppForUnitTesting();
229235
FSTAppCheckFake* app_check = [[FSTAppCheckFake alloc] initWithToken:@""];
230-
FirebaseAppCheckCredentialsProvider credentials_provider(app, app_check);
236+
auto credentials_provider =
237+
std::make_shared<FirebaseAppCheckCredentialsProvider>(app, app_check);
231238

232-
credentials_provider.SetCredentialChangeListener(
239+
credentials_provider->SetCredentialChangeListener(
233240
[token_promise](const std::string& result) {
234241
if (result != "") {
235242
token_promise->set_value(result);

‎Firestore/core/test/unit/credentials/firebase_auth_credentials_provider_test.mm‎

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -75,8 +75,9 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
7575
auto token_promise = std::make_shared<std::promise<AuthToken>>();
7676

7777
FIRApp* app = testutil::AppForUnitTesting();
78-
FirebaseAuthCredentialsProvider credentials_provider(app, nil);
79-
credentials_provider.GetToken(
78+
auto credentials_provider =
79+
std::make_shared<FirebaseAuthCredentialsProvider>(app, nil);
80+
credentials_provider->GetToken(
8081
[token_promise](util::StatusOr<AuthToken> result) {
8182
EXPECT_TRUE(result.ok());
8283
const AuthToken& token = result.ValueOrDie();
@@ -99,8 +100,9 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
99100
TEST(FirebaseAuthCredentialsProviderTest, GetTokenUnauthenticated) {
100101
FIRApp* app = testutil::AppForUnitTesting();
101102
FSTAuthFake* auth = [[FSTAuthFake alloc] initWithToken:nil uid:nil];
102-
FirebaseAuthCredentialsProvider credentials_provider(app, auth);
103-
credentials_provider.GetToken([](util::StatusOr<AuthToken> result) {
103+
auto credentials_provider =
104+
std::make_shared<FirebaseAuthCredentialsProvider>(app, auth);
105+
credentials_provider->GetToken([](util::StatusOr<AuthToken> result) {
104106
EXPECT_TRUE(result.ok());
105107
const AuthToken& token = result.ValueOrDie();
106108
EXPECT_ANY_THROW(token.token());
@@ -114,8 +116,9 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
114116
FIRApp* app = testutil::AppForUnitTesting();
115117
FSTAuthFake* auth = [[FSTAuthFake alloc] initWithToken:@"token for fake uid"
116118
uid:@"fake uid"];
117-
FirebaseAuthCredentialsProvider credentials_provider(app, auth);
118-
credentials_provider.GetToken([](util::StatusOr<AuthToken> result) {
119+
auto credentials_provider =
120+
std::make_shared<FirebaseAuthCredentialsProvider>(app, auth);
121+
credentials_provider->GetToken([](util::StatusOr<AuthToken> result) {
119122
EXPECT_TRUE(result.ok());
120123
const AuthToken& token = result.ValueOrDie();
121124
EXPECT_EQ("token for fake uid", token.token());
@@ -129,22 +132,24 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
129132
FIRApp* app = testutil::AppForUnitTesting();
130133
FSTAuthFake* auth = [[FSTAuthFake alloc] initWithToken:@"default token"
131134
uid:@"fake uid"];
132-
FirebaseAuthCredentialsProvider credentials_provider(app, auth);
133-
credentials_provider.SetCredentialChangeListener([](User user) {
135+
auto credentials_provider =
136+
std::make_shared<FirebaseAuthCredentialsProvider>(app, auth);
137+
credentials_provider->SetCredentialChangeListener([](User user) {
134138
EXPECT_EQ("fake uid", user.uid());
135139
EXPECT_TRUE(user.is_authenticated());
136140
});
137141

138-
credentials_provider.SetCredentialChangeListener(nullptr);
142+
credentials_provider->SetCredentialChangeListener(nullptr);
139143
}
140144

141145
TEST(FirebaseAuthCredentialsProviderTest, InvalidateToken) {
142146
FIRApp* app = testutil::AppForUnitTesting();
143147
FSTAuthFake* auth = [[FSTAuthFake alloc] initWithToken:@"token for fake uid"
144148
uid:@"fake uid"];
145-
FirebaseAuthCredentialsProvider credentials_provider(app, auth);
146-
credentials_provider.InvalidateToken();
147-
credentials_provider.GetToken([&auth](util::StatusOr<AuthToken> result) {
149+
auto credentials_provider =
150+
std::make_shared<FirebaseAuthCredentialsProvider>(app, auth);
151+
credentials_provider->InvalidateToken();
152+
credentials_provider->GetToken([&auth](util::StatusOr<AuthToken> result) {
148153
EXPECT_TRUE(result.ok());
149154
EXPECT_TRUE(auth.forceRefreshTriggered);
150155
const AuthToken& token = result.ValueOrDie();
@@ -160,14 +165,15 @@ - (void)getTokenForcingRefresh:(BOOL)forceRefresh
160165
FIRApp* app = testutil::AppForUnitTesting();
161166
FSTAuthFake* auth = [[FSTAuthFake alloc] initWithToken:@"token for fake uid"
162167
uid:@"fake uid"];
163-
FirebaseAuthCredentialsProvider credentials_provider(app, auth);
168+
auto credentials_provider =
169+
std::make_shared<FirebaseAuthCredentialsProvider>(app, auth);
164170

165-
std::thread thread1([&credentials_provider] {
166-
credentials_provider.GetToken(
171+
std::thread thread1([credentials_provider] {
172+
credentials_provider->GetToken(
167173
[](util::StatusOr<AuthToken> result) { EXPECT_TRUE(result.ok()); });
168174
});
169-
std::thread thread2([&credentials_provider] {
170-
credentials_provider.GetToken(
175+
std::thread thread2([credentials_provider] {
176+
credentials_provider->GetToken(
171177
[](util::StatusOr<AuthToken> result) { EXPECT_TRUE(result.ok()); });
172178
});
173179

0 commit comments

Comments
 (0)