Re-establish streams when App Check token expires. #5902

ehsannas · 2022-01-18T21:54:03Z

changeset-bot · 2022-01-18T21:54:06Z

🦋 Changeset detected

Latest commit: 27702c1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@firebase/firestore	Patch
firebase	Patch
@firebase/firestore-compat	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

google-oss-bot · 2022-01-18T22:12:52Z

Size Report ¹

Affected Products

@firebase/auth-compat
Type Base (4bd3a88) Merge (17bd41f) Diff
browser 20.0 kB 20.1 kB +83 B (+0.4%)
esm5 26.9 kB 26.9 kB +87 B (+0.3%)
main 29.4 kB 29.5 kB +87 B (+0.3%)
module 20.0 kB 20.1 kB +83 B (+0.4%)
@firebase/auth/cordova
Type Base (4bd3a88) Merge (17bd41f) Diff
browser 180 kB 180 kB +28 B (+0.0%)
module 180 kB 180 kB +28 B (+0.0%)
@firebase/auth/internal
Type Base (4bd3a88) Merge (17bd41f) Diff
browser 163 kB 163 kB +12 B (+0.0%)
esm5 212 kB 212 kB +28 B (+0.0%)
main 179 kB 179 kB +28 B (+0.0%)
module 163 kB 163 kB +12 B (+0.0%)
@firebase/auth/react-native
Type Base (4bd3a88) Merge (17bd41f) Diff
browser 163 kB 164 kB +893 B (+0.5%)
module 163 kB 164 kB +893 B (+0.5%)

`@firebase/firestore`

Type	Base (`4bd3a88`)	Merge (`17bd41f`)	Diff
browser	228 kB	229 kB	+1.01 kB (+0.4%)
esm5	285 kB	286 kB	+1.09 kB (+0.4%)
main	453 kB	455 kB	+1.55 kB (+0.3%)
module	228 kB	229 kB	+1.01 kB (+0.4%)
react-native	228 kB	229 kB	+1.01 kB (+0.4%)

`bundle`

Type	Base (`4bd3a88`)	Merge (`17bd41f`)	Diff
firestore (Persistence)	229 kB	230 kB	+274 B (+0.1%)
firestore (Query Cursors)	189 kB	189 kB	+274 B (+0.1%)
firestore (Query)	190 kB	190 kB	+274 B (+0.1%)
firestore (Read data once)	178 kB	179 kB	+274 B (+0.2%)
firestore (Realtime updates)	181 kB	181 kB	+274 B (+0.2%)
firestore (Transaction)	163 kB	163 kB	+274 B (+0.2%)
firestore (Write data)	162 kB	163 kB	+274 B (+0.2%)

`firebase`

Type	Base (`4bd3a88`)	Merge (`17bd41f`)	Diff
firebase-auth-compat.js	123 kB	123 kB	-29 B (-0.0%)
firebase-auth-cordova.js	91 B	462 kB	+462 kB (+508079.1%)
firebase-auth-react-native.js	101 B	486 kB	+486 kB (+480885.1%)
firebase-auth.js	411 kB	411 kB	+12 B (+0.0%)
firebase-compat.js	753 kB	754 kB	+243 B (+0.0%)
firebase-firestore-compat.js	281 kB	281 kB	+272 B (+0.1%)
firebase-firestore.js	770 kB	772 kB	+2.36 kB (+0.3%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/aAZtPSO83l.html

google-oss-bot · 2022-01-18T22:27:41Z

Size Analysis Report ¹

This report is too large (335,472 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/lTQs3ik2R4.html

schmidt-sebastian · 2022-01-18T22:01:05Z

.changeset/happy-badgers-leave.md

+"@firebase/firestore": patch
+---
+
+Re-establish streams when App Check token expires.


This is technically correct, but doesn't quite tell the user what problem this fixes. Can you focus on the user impact?

packages/firestore/src/remote/remote_store.ts

schmidt-sebastian · 2022-01-21T22:30:18Z

As discussed offline, the fix is fine given the backend limitations. Can you update the changelog?

schmidt-sebastian · 2022-01-24T17:30:57Z

packages/firestore/src/core/firestore_client.ts

-    this.appCheckCredentials.start(asyncQueue, () => Promise.resolve());
+    this.appCheckCredentials.start(asyncQueue, async newAppCheckToken => {
+      logDebug(LOG_TAG, 'Received new app check token=', newAppCheckToken);
+      await this.appCheckCredentialListener(newAppCheckToken);


await adds a lot of code for older JS configurations. You can just use return here and remove async/await.

schmidt-sebastian · 2022-01-24T18:10:02Z

packages/firestore/src/remote/remote_store.ts

+    remoteStoreImpl.onlineStateTracker.set(OnlineState.Unknown);
+  }
+  remoteStoreImpl.offlineCauses.delete(OfflineCause.CredentialChange);
+  await enableNetworkInternal(remoteStoreImpl);


We don't need a separate method here I think - we should be able to just re-use remoteStoreHandleCredentialChange

schmidt-sebastian · 2022-01-24T18:10:08Z

.changeset/happy-badgers-leave.md

+Fixed bug: Firestore listeners stopped working and received a "Permission Denied"
+error when App Check token expired (listener was active longer than the App
+Check token TTL configured in the Firebase console). The issue does not occur if
+listeners were renewed for other reasons such as Authentication token renewal,
+listener being idle for a long time, page refresh, etc.


Maybe:

Suggested change

Fixed bug: Firestore listeners stopped working and received a "Permission Denied"

error when App Check token expired (listener was active longer than the App

Check token TTL configured in the Firebase console). The issue does not occur if

listeners were renewed for other reasons such as Authentication token renewal,

listener being idle for a long time, page refresh, etc.

Fixed an AppCheck issue that caused Firestore listeners to stop working and receive a

"Permission Denied" error. This issue only occurred for AppCheck users that set their

expiration time to under an hour

schmidt-sebastian · 2022-01-25T21:07:51Z

packages/firestore/src/core/firestore_client.ts

@@ -234,6 +246,9 @@ export async function setOnlineComponentProvider(
  client.setCredentialChangeListener(user =>
    remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user)
  );
+  client.setAppCheckTokenChangeListener((appCheckToken, user) =>


Suggested change

client.setAppCheckTokenChangeListener((appCheckToken, user) =>

client.setAppCheckTokenChangeListener((_, user) =>

Does the AppCheckTokenListener really return the user?

AppCheckTokenListener only provides the new app check token, not the user. I take the new app check token, as well as the latest user from the firestore_client to call remoteStoreHandleCredentialChange.

remoteStoreHandleCredentialChange needs to know the latest user because if the user has changed, it'll do a bunch of things related to user changes. By passing it the current user, that logic will be bypassed and only the restarting of the streams occurs.

Hm, this is a bit of a strange API as the user has nothing to do with the AppCheck token. But I see that this simplifies things a bit, as you otherwise have to manage the current user in yet another place. Let's leave as is for now and remove once the backend fixes the underlying issue.

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

Re-establish streams when App Check token expires.

fb27ee5

ehsannas requested a review from schmidt-sebastian January 18, 2022 21:54

ehsannas assigned ehsannas and schmidt-sebastian Jan 18, 2022

ehsannas removed their assignment Jan 18, 2022

Create happy-badgers-leave.md

3da0c5b

ehsannas requested a review from egilmorez as a code owner January 18, 2022 21:55

schmidt-sebastian assigned ehsannas and unassigned schmidt-sebastian Jan 18, 2022

ehsannas assigned schmidt-sebastian and unassigned ehsannas Jan 20, 2022

schmidt-sebastian reviewed Jan 20, 2022

View reviewed changes

schmidt-sebastian assigned ehsannas and unassigned schmidt-sebastian Jan 21, 2022

Better patch message.

016631e

ehsannas assigned schmidt-sebastian and unassigned ehsannas Jan 24, 2022

schmidt-sebastian reviewed Jan 24, 2022

View reviewed changes

ehsannas added 3 commits January 24, 2022 13:10

better patch message.

c9c0386

address comments.

d01b6e7

Remove outdated comment.

e9123f3

ehsannas requested a review from schmidt-sebastian January 25, 2022 00:32

schmidt-sebastian reviewed Jan 25, 2022

View reviewed changes

Update packages/firestore/src/core/firestore_client.ts

27702c1

Co-authored-by: Sebastian Schmidt <mrschmidt@google.com>

schmidt-sebastian approved these changes Jan 25, 2022

View reviewed changes

ehsannas merged commit e28b0e4 into master Jan 26, 2022

ehsannas deleted the ehsann/firestore-appcheck-token-expiry branch January 26, 2022 18:57

google-oss-bot mentioned this pull request Jan 26, 2022

Version Packages #5939

Merged

firebase locked and limited conversation to collaborators Feb 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-establish streams when App Check token expires. #5902

Re-establish streams when App Check token expires. #5902

ehsannas commented Jan 18, 2022

changeset-bot bot commented Jan 18, 2022 •

edited

Loading

google-oss-bot commented Jan 18, 2022 •

edited

Loading

`@firebase/auth-compat`

`@firebase/auth/cordova`

`@firebase/auth/internal`

`@firebase/auth/react-native`

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Jan 18, 2022 •

edited

Loading

schmidt-sebastian Jan 18, 2022

schmidt-sebastian commented Jan 21, 2022

schmidt-sebastian Jan 24, 2022

ehsannas Jan 24, 2022

schmidt-sebastian Jan 24, 2022

ehsannas Jan 24, 2022

schmidt-sebastian Jan 24, 2022

ehsannas Jan 24, 2022

schmidt-sebastian Jan 25, 2022

schmidt-sebastian Jan 25, 2022

ehsannas Jan 25, 2022

schmidt-sebastian Jan 25, 2022

	client.setAppCheckTokenChangeListener((appCheckToken, user) =>
	client.setAppCheckTokenChangeListener((_, user) =>

Re-establish streams when App Check token expires. #5902

Re-establish streams when App Check token expires. #5902

Conversation

ehsannas commented Jan 18, 2022

changeset-bot bot commented Jan 18, 2022 • edited Loading

🦋 Changeset detected

google-oss-bot commented Jan 18, 2022 • edited Loading

Size Report 1

Affected Products

@firebase/auth-compat

@firebase/auth/cordova

@firebase/auth/internal

@firebase/auth/react-native

@firebase/firestore

bundle

firebase

Test Logs

google-oss-bot commented Jan 18, 2022 • edited Loading

Size Analysis Report 1

Test Logs

Choose a reason for hiding this comment

schmidt-sebastian commented Jan 21, 2022

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

changeset-bot bot commented Jan 18, 2022 •

edited

Loading

google-oss-bot commented Jan 18, 2022 •

edited

Loading

Size Report ¹

`@firebase/auth-compat`

`@firebase/auth/cordova`

`@firebase/auth/internal`

`@firebase/auth/react-native`

`@firebase/firestore`

`bundle`

`firebase`

google-oss-bot commented Jan 18, 2022 •

edited

Loading

Size Analysis Report ¹