From fb27ee5751fd0d596a46c6b0a952903b0bdf7f8b Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Tue, 18 Jan 2022 15:50:36 -0600 Subject: [PATCH 1/7] Re-establish streams when App Check token expires. --- .../firestore/src/core/firestore_client.ts | 17 ++++++++++++++- packages/firestore/src/remote/remote_store.ts | 21 +++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index dabdc35edf3..6cbb21030be 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -41,6 +41,7 @@ import { RemoteStore, remoteStoreDisableNetwork, remoteStoreEnableNetwork, + remoteStoreHandleAppCheckTokenChange, remoteStoreHandleCredentialChange } from '../remote/remote_store'; import { JsonProtoSerializer } from '../remote/serializer'; @@ -99,6 +100,8 @@ export class FirestoreClient { private readonly clientId = AutoId.newId(); private authCredentialListener: CredentialChangeListener = () => Promise.resolve(); + private appCheckCredentialListener: CredentialChangeListener = () => + Promise.resolve(); offlineComponents?: OfflineComponentProvider; onlineComponents?: OnlineComponentProvider; @@ -123,7 +126,10 @@ export class FirestoreClient { this.user = user; }); // Register an empty credentials change listener to activate token refresh. - 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); + }); } async getConfiguration(): Promise { @@ -142,6 +148,12 @@ export class FirestoreClient { this.authCredentialListener = listener; } + setAppCheckTokenChangeListener( + listener: (token: string) => Promise + ): void { + this.appCheckCredentialListener = listener; + } + /** * Checks that the client has not been terminated. Ensures that other methods on * this class cannot be called after the client is terminated. @@ -234,6 +246,9 @@ export async function setOnlineComponentProvider( client.setCredentialChangeListener(user => remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user) ); + client.setAppCheckTokenChangeListener(() => + remoteStoreHandleAppCheckTokenChange(onlineComponentProvider.remoteStore) + ); client.onlineComponents = onlineComponentProvider; } diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index f7a55ac0c5d..d63b8e8d4e2 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -876,6 +876,27 @@ export async function remoteStoreHandleCredentialChange( await enableNetworkInternal(remoteStoreImpl); } +export async function remoteStoreHandleAppCheckTokenChange( + remoteStore: RemoteStore +): Promise { + const remoteStoreImpl = debugCast(remoteStore, RemoteStoreImpl); + remoteStoreImpl.asyncQueue.verifyOperationInProgress(); + + logDebug(LOG_TAG, 'RemoteStore received new App Check token.'); + const usesNetwork = canUseNetwork(remoteStoreImpl); + + // Tear down and re-create our network streams. This will ensure a new + // App Check token is retrieved and used for setting up the streams. + remoteStoreImpl.offlineCauses.add(OfflineCause.CredentialChange); + await disableNetworkInternal(remoteStoreImpl); + if (usesNetwork) { + // Don't set the network status to Unknown if we are offline. + remoteStoreImpl.onlineStateTracker.set(OnlineState.Unknown); + } + remoteStoreImpl.offlineCauses.delete(OfflineCause.CredentialChange); + await enableNetworkInternal(remoteStoreImpl); +} + /** * Toggles the network state when the client gains or loses its primary lease. */ From 3da0c5bb0c3a2f2efae5777b1d8d170cc23e5687 Mon Sep 17 00:00:00 2001 From: Ehsan Date: Tue, 18 Jan 2022 15:55:01 -0600 Subject: [PATCH 2/7] Create happy-badgers-leave.md --- .changeset/happy-badgers-leave.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/happy-badgers-leave.md diff --git a/.changeset/happy-badgers-leave.md b/.changeset/happy-badgers-leave.md new file mode 100644 index 00000000000..067f2fdd98c --- /dev/null +++ b/.changeset/happy-badgers-leave.md @@ -0,0 +1,5 @@ +--- +"@firebase/firestore": patch +--- + +Re-establish streams when App Check token expires. From 016631ef9245b39a1fa65a3b868bd92ea694124e Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 24 Jan 2022 10:23:40 -0600 Subject: [PATCH 3/7] Better patch message. --- .changeset/happy-badgers-leave.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.changeset/happy-badgers-leave.md b/.changeset/happy-badgers-leave.md index 067f2fdd98c..1e1899fb7ba 100644 --- a/.changeset/happy-badgers-leave.md +++ b/.changeset/happy-badgers-leave.md @@ -2,4 +2,8 @@ "@firebase/firestore": patch --- -Re-establish streams when App Check token expires. +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. From c9c0386e479c99e1ed1715fa45b89a70eb9dff41 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 24 Jan 2022 13:10:40 -0600 Subject: [PATCH 4/7] better patch message. --- .changeset/happy-badgers-leave.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.changeset/happy-badgers-leave.md b/.changeset/happy-badgers-leave.md index 1e1899fb7ba..8e182c5a138 100644 --- a/.changeset/happy-badgers-leave.md +++ b/.changeset/happy-badgers-leave.md @@ -2,8 +2,6 @@ "@firebase/firestore": patch --- -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. From d01b6e72144e65c941e37456e303f5d5d28a4aec Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 24 Jan 2022 14:08:22 -0600 Subject: [PATCH 5/7] address comments. --- .../firestore/src/core/firestore_client.ts | 17 ++++++++------- packages/firestore/src/remote/remote_store.ts | 21 ------------------- 2 files changed, 9 insertions(+), 29 deletions(-) diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index 6cbb21030be..b2866b69fda 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -41,7 +41,6 @@ import { RemoteStore, remoteStoreDisableNetwork, remoteStoreEnableNetwork, - remoteStoreHandleAppCheckTokenChange, remoteStoreHandleCredentialChange } from '../remote/remote_store'; import { JsonProtoSerializer } from '../remote/serializer'; @@ -100,8 +99,10 @@ export class FirestoreClient { private readonly clientId = AutoId.newId(); private authCredentialListener: CredentialChangeListener = () => Promise.resolve(); - private appCheckCredentialListener: CredentialChangeListener = () => - Promise.resolve(); + private appCheckCredentialListener: ( + appCheckToken: string, + user: User + ) => Promise = () => Promise.resolve(); offlineComponents?: OfflineComponentProvider; onlineComponents?: OnlineComponentProvider; @@ -126,9 +127,9 @@ export class FirestoreClient { this.user = user; }); // Register an empty credentials change listener to activate token refresh. - this.appCheckCredentials.start(asyncQueue, async newAppCheckToken => { + this.appCheckCredentials.start(asyncQueue, newAppCheckToken => { logDebug(LOG_TAG, 'Received new app check token=', newAppCheckToken); - await this.appCheckCredentialListener(newAppCheckToken); + return this.appCheckCredentialListener(newAppCheckToken, this.user); }); } @@ -149,7 +150,7 @@ export class FirestoreClient { } setAppCheckTokenChangeListener( - listener: (token: string) => Promise + listener: (appCheckToken: string, user: User) => Promise ): void { this.appCheckCredentialListener = listener; } @@ -246,8 +247,8 @@ export async function setOnlineComponentProvider( client.setCredentialChangeListener(user => remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user) ); - client.setAppCheckTokenChangeListener(() => - remoteStoreHandleAppCheckTokenChange(onlineComponentProvider.remoteStore) + client.setAppCheckTokenChangeListener((appCheckToken, user) => + remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user) ); client.onlineComponents = onlineComponentProvider; } diff --git a/packages/firestore/src/remote/remote_store.ts b/packages/firestore/src/remote/remote_store.ts index d63b8e8d4e2..f7a55ac0c5d 100644 --- a/packages/firestore/src/remote/remote_store.ts +++ b/packages/firestore/src/remote/remote_store.ts @@ -876,27 +876,6 @@ export async function remoteStoreHandleCredentialChange( await enableNetworkInternal(remoteStoreImpl); } -export async function remoteStoreHandleAppCheckTokenChange( - remoteStore: RemoteStore -): Promise { - const remoteStoreImpl = debugCast(remoteStore, RemoteStoreImpl); - remoteStoreImpl.asyncQueue.verifyOperationInProgress(); - - logDebug(LOG_TAG, 'RemoteStore received new App Check token.'); - const usesNetwork = canUseNetwork(remoteStoreImpl); - - // Tear down and re-create our network streams. This will ensure a new - // App Check token is retrieved and used for setting up the streams. - remoteStoreImpl.offlineCauses.add(OfflineCause.CredentialChange); - await disableNetworkInternal(remoteStoreImpl); - if (usesNetwork) { - // Don't set the network status to Unknown if we are offline. - remoteStoreImpl.onlineStateTracker.set(OnlineState.Unknown); - } - remoteStoreImpl.offlineCauses.delete(OfflineCause.CredentialChange); - await enableNetworkInternal(remoteStoreImpl); -} - /** * Toggles the network state when the client gains or loses its primary lease. */ From e9123f3b326e78305165c74f1c20e9daaa8fd2c1 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 24 Jan 2022 14:12:01 -0600 Subject: [PATCH 6/7] Remove outdated comment. --- packages/firestore/src/core/firestore_client.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index b2866b69fda..f8f7250b454 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -126,7 +126,6 @@ export class FirestoreClient { await this.authCredentialListener(user); this.user = user; }); - // Register an empty credentials change listener to activate token refresh. this.appCheckCredentials.start(asyncQueue, newAppCheckToken => { logDebug(LOG_TAG, 'Received new app check token=', newAppCheckToken); return this.appCheckCredentialListener(newAppCheckToken, this.user); From 27702c14afdc2a06a710648680e7ebda5b09460d Mon Sep 17 00:00:00 2001 From: Ehsan Date: Tue, 25 Jan 2022 15:09:11 -0600 Subject: [PATCH 7/7] Update packages/firestore/src/core/firestore_client.ts Co-authored-by: Sebastian Schmidt --- packages/firestore/src/core/firestore_client.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/core/firestore_client.ts b/packages/firestore/src/core/firestore_client.ts index f8f7250b454..858b40929ad 100644 --- a/packages/firestore/src/core/firestore_client.ts +++ b/packages/firestore/src/core/firestore_client.ts @@ -246,7 +246,7 @@ export async function setOnlineComponentProvider( client.setCredentialChangeListener(user => remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user) ); - client.setAppCheckTokenChangeListener((appCheckToken, user) => + client.setAppCheckTokenChangeListener((_, user) => remoteStoreHandleCredentialChange(onlineComponentProvider.remoteStore, user) ); client.onlineComponents = onlineComponentProvider;