Skip to content

Commit fef7bad

Browse files
rosalyntanrosalyntan
authored
Fix client-side throttling for Play Integrity flows (#5395)
Changes: - Successful `GeneratePlayIntegrityChallenge` call should not reset `RetryManager` backoff - Cache in-flight App Check token fetching tasks - Update `403` errors to throttle with exponential backoff rather than a flat one-day period (to match [iOS](https://github.com/firebase/firebase-ios-sdk/blob/f3a14c249e001b3c894bb943ecc3973f50e60a49/FirebaseAppCheck/Sources/Core/Backoff/FIRAppCheckBackoffWrapper.m#L264-L268))
1 parent ffdee32 commit fef7bad

File tree

7 files changed

+51
-45
lines changed

7 files changed

+51
-45
lines changed

‎appcheck/firebase-appcheck-playintegrity/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
# Unreleased
2+
* [fixed] Fixed client-side throttling in Play Integrity flows.
3+
24
* [unchanged] Updated to keep [app_check] SDK versions aligned.
35

46
# 17.0.0

‎appcheck/firebase-appcheck/CHANGELOG.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
# Unreleased
22
* [fixed] Fixed a bug causing internal tests to depend directly on `firebase-common`.
33

4+
* [fixed] Fixed client-side throttling in Play Integrity flows.
5+
46
* [changed] Added Kotlin extensions (KTX) APIs from `com.google.firebase:firebase-appcheck-ktx`
57
to `com.google.firebase:firebase-appcheck` under the `com.google.firebase.appcheck` package.
68
For details, see the
@@ -12,8 +14,6 @@
1214
now deprecated. As early as April 2024, we'll no longer release KTX modules. For details, see the
1315
[FAQ about this initiative](https://firebase.google.com/docs/android/kotlin-migration)
1416

15-
16-
1717
# 17.0.1
1818
* [changed] Internal updates to allow Firebase SDKs to obtain limited-use tokens.
1919

‎appcheck/firebase-appcheck/src/main/java/com/google/firebase/appcheck/internal/DefaultFirebaseAppCheck.java

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ public class DefaultFirebaseAppCheck extends FirebaseAppCheck {
6161
private AppCheckProviderFactory appCheckProviderFactory;
6262
private AppCheckProvider appCheckProvider;
6363
private AppCheckToken cachedToken;
64+
private Task<AppCheckToken> cachedTokenTask;
6465

6566
public DefaultFirebaseAppCheck(
6667
@NonNull FirebaseApp firebaseApp,
@@ -192,24 +193,27 @@ public Task<AppCheckTokenResult> getToken(boolean forceRefresh) {
192193
DefaultAppCheckTokenResult.constructFromError(
193194
new FirebaseException("No AppCheckProvider installed.")));
194195
}
195-
// TODO: Cache the in-flight task.
196-
return fetchTokenFromProvider()
197-
.continueWithTask(
198-
liteExecutor,
199-
appCheckTokenTask -> {
200-
if (appCheckTokenTask.isSuccessful()) {
201-
return Tasks.forResult(
202-
DefaultAppCheckTokenResult.constructFromAppCheckToken(
203-
appCheckTokenTask.getResult()));
204-
}
205-
// If the token exchange failed, return a dummy token for integrators to attach
206-
// in their headers.
207-
return Tasks.forResult(
208-
DefaultAppCheckTokenResult.constructFromError(
209-
new FirebaseException(
210-
appCheckTokenTask.getException().getMessage(),
211-
appCheckTokenTask.getException())));
212-
});
196+
if (cachedTokenTask == null
197+
|| cachedTokenTask.isComplete()
198+
|| cachedTokenTask.isCanceled()) {
199+
cachedTokenTask = fetchTokenFromProvider();
200+
}
201+
return cachedTokenTask.continueWithTask(
202+
liteExecutor,
203+
appCheckTokenTask -> {
204+
if (appCheckTokenTask.isSuccessful()) {
205+
return Tasks.forResult(
206+
DefaultAppCheckTokenResult.constructFromAppCheckToken(
207+
appCheckTokenTask.getResult()));
208+
}
209+
// If the token exchange failed, return a dummy token for integrators to attach
210+
// in their headers.
211+
return Tasks.forResult(
212+
DefaultAppCheckTokenResult.constructFromError(
213+
new FirebaseException(
214+
appCheckTokenTask.getException().getMessage(),
215+
appCheckTokenTask.getException())));
216+
});
213217
});
214218
}
215219

@@ -247,7 +251,12 @@ public Task<AppCheckToken> getAppCheckToken(boolean forceRefresh) {
247251
if (appCheckProvider == null) {
248252
return Tasks.forException(new FirebaseException("No AppCheckProvider installed."));
249253
}
250-
return fetchTokenFromProvider();
254+
if (cachedTokenTask == null
255+
|| cachedTokenTask.isComplete()
256+
|| cachedTokenTask.isCanceled()) {
257+
cachedTokenTask = fetchTokenFromProvider();
258+
}
259+
return cachedTokenTask;
251260
});
252261
}
253262

‎appcheck/firebase-appcheck/src/main/java/com/google/firebase/appcheck/internal/NetworkClient.java

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,8 @@ public AppCheckTokenResponse exchangeAttestationForAppCheckToken(
121121
throw new FirebaseException("Too many attempts.");
122122
}
123123
URL url = new URL(String.format(getUrlTemplate(tokenType), projectId, appId, apiKey));
124-
String response = makeNetworkRequest(url, requestBytes, retryManager);
124+
String response =
125+
makeNetworkRequest(url, requestBytes, retryManager, /* resetRetryManagerOnSuccess= */ true);
125126
return AppCheckTokenResponse.fromJsonString(response);
126127
}
127128

@@ -138,11 +139,15 @@ public String generatePlayIntegrityChallenge(
138139
}
139140
URL url =
140141
new URL(String.format(PLAY_INTEGRITY_CHALLENGE_URL_TEMPLATE, projectId, appId, apiKey));
141-
return makeNetworkRequest(url, requestBytes, retryManager);
142+
return makeNetworkRequest(
143+
url, requestBytes, retryManager, /* resetRetryManagerOnSuccess= */ false);
142144
}
143145

144146
private String makeNetworkRequest(
145-
@NonNull URL url, @NonNull byte[] requestBytes, @NonNull RetryManager retryManager)
147+
@NonNull URL url,
148+
@NonNull byte[] requestBytes,
149+
@NonNull RetryManager retryManager,
150+
boolean resetRetryManagerOnSuccess)
146151
throws FirebaseException, IOException, JSONException {
147152
HttpURLConnection urlConnection = createHttpUrlConnection(url);
148153

@@ -187,7 +192,9 @@ private String makeNetworkRequest(
187192
+ " body: "
188193
+ httpErrorResponse.getErrorMessage());
189194
}
190-
retryManager.resetBackoffOnSuccess();
195+
if (resetRetryManagerOnSuccess) {
196+
retryManager.resetBackoffOnSuccess();
197+
}
191198
return responseBody;
192199
} finally {
193200
urlConnection.disconnect();

‎appcheck/firebase-appcheck/src/main/java/com/google/firebase/appcheck/internal/RetryManager.java

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
public class RetryManager {
2727

2828
@VisibleForTesting static final int BAD_REQUEST_ERROR_CODE = 400;
29-
@VisibleForTesting static final int FORBIDDEN_ERROR_CODE = 403;
3029
@VisibleForTesting static final int NOT_FOUND_ERROR_CODE = 404;
3130
@VisibleForTesting static final long MAX_EXP_BACKOFF_MILLIS = 4 * 60 * 60 * 1000; // 4 hours
3231
@VisibleForTesting static final long ONE_DAY_MILLIS = 24 * 60 * 60 * 1000; // 24 hours
@@ -91,9 +90,7 @@ public void updateBackoffOnFailure(int errorCode) {
9190

9291
@BackoffStrategyType
9392
private static int getBackoffStrategyByErrorCode(int errorCode) {
94-
if (errorCode == BAD_REQUEST_ERROR_CODE
95-
|| errorCode == FORBIDDEN_ERROR_CODE
96-
|| errorCode == NOT_FOUND_ERROR_CODE) {
93+
if (errorCode == BAD_REQUEST_ERROR_CODE || errorCode == NOT_FOUND_ERROR_CODE) {
9794
return ONE_DAY;
9895
}
9996
return EXPONENTIAL;

‎appcheck/firebase-appcheck/src/test/java/com/google/firebase/appcheck/internal/NetworkClientTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ public void generatePlayIntegrityChallenge_successResponse_returnsJsonString() t
325325
verify(mockOutputStream)
326326
.write(JSON_REQUEST.getBytes(), /* off= */ 0, JSON_REQUEST.getBytes().length);
327327
verify(mockRetryManager, never()).updateBackoffOnFailure(anyInt());
328-
verify(mockRetryManager).resetBackoffOnSuccess();
328+
verify(mockRetryManager, never()).resetBackoffOnSuccess();
329329
verifyRequestHeaders();
330330
}
331331

‎appcheck/firebase-appcheck/src/test/java/com/google/firebase/appcheck/internal/RetryManagerTest.java

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

1717
import static com.google.common.truth.Truth.assertThat;
1818
import static com.google.firebase.appcheck.internal.RetryManager.BAD_REQUEST_ERROR_CODE;
19-
import static com.google.firebase.appcheck.internal.RetryManager.FORBIDDEN_ERROR_CODE;
2019
import static com.google.firebase.appcheck.internal.RetryManager.MAX_EXP_BACKOFF_MILLIS;
2120
import static com.google.firebase.appcheck.internal.RetryManager.NOT_FOUND_ERROR_CODE;
2221
import static com.google.firebase.appcheck.internal.RetryManager.ONE_DAY_MILLIS;
@@ -60,14 +59,6 @@ public void updateBackoffOnFailure_badRequestError_oneDayRetryStrategy() {
6059
.isEqualTo(CURRENT_TIME_MILLIS + ONE_DAY_MILLIS);
6160
}
6261

63-
@Test
64-
public void updateBackoffOnFailure_forbiddenError_oneDayRetryStrategy() {
65-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
66-
67-
assertThat(retryManager.getNextRetryTimeMillis())
68-
.isEqualTo(CURRENT_TIME_MILLIS + ONE_DAY_MILLIS);
69-
}
70-
7162
@Test
7263
public void updateBackoffOnFailure_notFoundError_oneDayRetryStrategy() {
7364
retryManager.updateBackoffOnFailure(NOT_FOUND_ERROR_CODE);
@@ -78,9 +69,9 @@ public void updateBackoffOnFailure_notFoundError_oneDayRetryStrategy() {
7869

7970
@Test
8071
public void updateBackoffOnFailure_oneDayRetryStrategy_multipleRetries() {
81-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
82-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
83-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
72+
retryManager.updateBackoffOnFailure(BAD_REQUEST_ERROR_CODE);
73+
retryManager.updateBackoffOnFailure(BAD_REQUEST_ERROR_CODE);
74+
retryManager.updateBackoffOnFailure(BAD_REQUEST_ERROR_CODE);
8475

8576
// The backoff period should not increase for consecutive failed retries with the ONE_DAY
8677
// strategy.
@@ -133,7 +124,7 @@ public void updateBackoffOnFailure_exponentialRetryStrategy() {
133124

134125
@Test
135126
public void canRetry_beforeNextRetryTime() {
136-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
127+
retryManager.updateBackoffOnFailure(BAD_REQUEST_ERROR_CODE);
137128

138129
// Sanity check.
139130
assertThat(mockClock.currentTimeMillis()).isEqualTo(CURRENT_TIME_MILLIS);
@@ -145,7 +136,7 @@ public void canRetry_beforeNextRetryTime() {
145136

146137
@Test
147138
public void canRetry_afterNextRetryTime() {
148-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
139+
retryManager.updateBackoffOnFailure(BAD_REQUEST_ERROR_CODE);
149140
long nextRetryMillis = retryManager.getNextRetryTimeMillis();
150141

151142
when(mockClock.currentTimeMillis()).thenReturn(nextRetryMillis + 1);
@@ -154,7 +145,7 @@ public void canRetry_afterNextRetryTime() {
154145

155146
@Test
156147
public void resetBackoffOnSuccess() {
157-
retryManager.updateBackoffOnFailure(FORBIDDEN_ERROR_CODE);
148+
retryManager.updateBackoffOnFailure(BAD_REQUEST_ERROR_CODE);
158149
// Sanity check.
159150
assertThat(retryManager.getNextRetryTimeMillis())
160151
.isEqualTo(CURRENT_TIME_MILLIS + ONE_DAY_MILLIS);

0 commit comments

Comments
 (0)