From 4ddfc1e398441afde84337170e3e765bff36f390 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 27 Jan 2025 15:42:26 +0000 Subject: [PATCH 01/12] Refresh identity token on SyncOrgKeys notification message --- libs/common/src/services/notifications.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/common/src/services/notifications.service.ts b/libs/common/src/services/notifications.service.ts index f88c904bee1..416a99f3a9a 100644 --- a/libs/common/src/services/notifications.service.ts +++ b/libs/common/src/services/notifications.service.ts @@ -192,6 +192,7 @@ export class NotificationsService implements NotificationsServiceAbstraction { break; case NotificationType.SyncOrgKeys: if (isAuthenticated) { + await this.apiService.refreshIdentityToken(); await this.syncService.fullSync(true); // Stop so a reconnect can be made await this.signalrConnection.stop(); From 1398c49f9a29fd902100c6fa5c2301a24fd48c1d Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 27 Jan 2025 15:46:41 +0000 Subject: [PATCH 02/12] Fix floating promise in signalrConnection onclose handler by adding await for reconnect --- libs/common/src/services/notifications.service.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/libs/common/src/services/notifications.service.ts b/libs/common/src/services/notifications.service.ts index 416a99f3a9a..fd160ddebc8 100644 --- a/libs/common/src/services/notifications.service.ts +++ b/libs/common/src/services/notifications.service.ts @@ -97,11 +97,9 @@ export class NotificationsService implements NotificationsServiceAbstraction { this.signalrConnection.on("Heartbeat", (data: any) => { /*console.log('Heartbeat!');*/ }); - this.signalrConnection.onclose(() => { + this.signalrConnection.onclose(async () => { this.connected = false; - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.reconnect(true); + await this.reconnect(true); }); this.inited = true; if (await this.isAuthedAndUnlocked()) { From cfe0ea46bb249129d5f04bb02696835162947ce4 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 27 Jan 2025 16:07:00 +0000 Subject: [PATCH 03/12] Refactor NotificationsService to encapsulate SignalR connection setup in a separate method --- .../src/services/notifications.service.ts | 46 ++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/libs/common/src/services/notifications.service.ts b/libs/common/src/services/notifications.service.ts index fd160ddebc8..0180c4d7481 100644 --- a/libs/common/src/services/notifications.service.ts +++ b/libs/common/src/services/notifications.service.ts @@ -80,27 +80,7 @@ export class NotificationsService implements NotificationsServiceAbstraction { this.signalrConnection = null; } - this.signalrConnection = new signalR.HubConnectionBuilder() - .withUrl(this.url + "/hub", { - accessTokenFactory: () => this.apiService.getActiveBearerToken(), - skipNegotiation: true, - transport: signalR.HttpTransportType.WebSockets, - }) - .withHubProtocol(new signalRMsgPack.MessagePackHubProtocol() as signalR.IHubProtocol) - // .configureLogging(signalR.LogLevel.Trace) - .build(); - - this.signalrConnection.on("ReceiveMessage", (data: any) => - this.processNotification(new NotificationResponse(data)), - ); - // eslint-disable-next-line - this.signalrConnection.on("Heartbeat", (data: any) => { - /*console.log('Heartbeat!');*/ - }); - this.signalrConnection.onclose(async () => { - this.connected = false; - await this.reconnect(true); - }); + this.signalrConnection = this.setupConnection(); this.inited = true; if (await this.isAuthedAndUnlocked()) { await this.reconnect(false); @@ -276,4 +256,28 @@ export class NotificationsService implements NotificationsServiceAbstraction { max = Math.floor(max); return Math.floor(Math.random() * (max - min + 1)) + min; } + + private setupConnection(): signalR.HubConnection { + const connection = new signalR.HubConnectionBuilder() + .withUrl(this.url + "/hub", { + accessTokenFactory: () => this.apiService.getActiveBearerToken(), + skipNegotiation: true, + transport: signalR.HttpTransportType.WebSockets, + }) + .withHubProtocol(new signalRMsgPack.MessagePackHubProtocol() as signalR.IHubProtocol) + .build(); + + connection.on("ReceiveMessage", (data: any) => + this.processNotification(new NotificationResponse(data)), + ); + connection.on("Heartbeat", (data: any) => { + /*console.log('Heartbeat!');*/ + }); + connection.onclose(async () => { + this.connected = false; + await this.reconnect(true); + }); + + return connection; + } } From 8acbc16021ba82a80f90c1fa64292f20427c189f Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 27 Jan 2025 16:07:47 +0000 Subject: [PATCH 04/12] Set up SignalR connection on reconnect --- libs/common/src/services/notifications.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/common/src/services/notifications.service.ts b/libs/common/src/services/notifications.service.ts index 0180c4d7481..d1fce510d9d 100644 --- a/libs/common/src/services/notifications.service.ts +++ b/libs/common/src/services/notifications.service.ts @@ -228,6 +228,7 @@ export class NotificationsService implements NotificationsServiceAbstraction { } try { + this.signalrConnection = this.setupConnection(); await this.signalrConnection.start(); this.connected = true; if (sync) { From fffbb5bd4751dc6f9ced876ca96f83c40f1bd676 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 27 Jan 2025 16:19:32 +0000 Subject: [PATCH 05/12] Add PushSyncOrgKeysOnRevokeRestore feature flag --- libs/common/src/enums/feature-flag.enum.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index a988bdbf6a7..74f629e2aa1 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -9,6 +9,7 @@ export enum FeatureFlag { AccountDeprovisioning = "pm-10308-account-deprovisioning", VerifiedSsoDomainEndpoint = "pm-12337-refactor-sso-details-endpoint", PM14505AdminConsoleIntegrationPage = "pm-14505-admin-console-integration-page", + PushSyncOrgKeysOnRevokeRestore = "pm-17168-push-sync-org-keys-on-revoke-restore", /* Autofill */ BlockBrowserInjectionsByDomain = "block-browser-injections-by-domain", @@ -66,6 +67,7 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.AccountDeprovisioning]: FALSE, [FeatureFlag.VerifiedSsoDomainEndpoint]: FALSE, [FeatureFlag.PM14505AdminConsoleIntegrationPage]: FALSE, + [FeatureFlag.PushSyncOrgKeysOnRevokeRestore]: FALSE, /* Autofill */ [FeatureFlag.BlockBrowserInjectionsByDomain]: FALSE, From f4e46259ac0de725bea6521f41eddf9c7050a779 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 27 Jan 2025 16:20:06 +0000 Subject: [PATCH 06/12] Add feature flag check for PushSyncOrgKeysOnRevokeRestore in NotificationsService --- libs/common/src/services/notifications.service.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/libs/common/src/services/notifications.service.ts b/libs/common/src/services/notifications.service.ts index d1fce510d9d..3bb7ccace5a 100644 --- a/libs/common/src/services/notifications.service.ts +++ b/libs/common/src/services/notifications.service.ts @@ -25,6 +25,8 @@ import { StateService } from "../platform/abstractions/state.service"; import { ScheduledTaskNames } from "../platform/scheduling/scheduled-task-name.enum"; import { TaskSchedulerService } from "../platform/scheduling/task-scheduler.service"; import { SyncService } from "../vault/abstractions/sync/sync.service.abstraction"; +import { ConfigService } from "../platform/abstractions/config/config.service"; +import { FeatureFlag } from "../enums/feature-flag.enum"; export class NotificationsService implements NotificationsServiceAbstraction { private signalrConnection: signalR.HubConnection; @@ -46,6 +48,7 @@ export class NotificationsService implements NotificationsServiceAbstraction { private authService: AuthService, private messagingService: MessagingService, private taskSchedulerService: TaskSchedulerService, + private configService: ConfigService, ) { this.taskSchedulerService.registerTaskHandler( ScheduledTaskNames.notificationsReconnectTimeout, @@ -170,7 +173,9 @@ export class NotificationsService implements NotificationsServiceAbstraction { break; case NotificationType.SyncOrgKeys: if (isAuthenticated) { - await this.apiService.refreshIdentityToken(); + if (this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { + await this.apiService.refreshIdentityToken(); + } await this.syncService.fullSync(true); // Stop so a reconnect can be made await this.signalrConnection.stop(); @@ -228,7 +233,10 @@ export class NotificationsService implements NotificationsServiceAbstraction { } try { - this.signalrConnection = this.setupConnection(); + if (this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { + this.signalrConnection = this.setupConnection(); + } + await this.signalrConnection.start(); this.connected = true; if (sync) { From 934791cce315115c7cd4578d481a47393c484392 Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 28 Jan 2025 12:17:06 +0000 Subject: [PATCH 07/12] Add ConfigService for NotificationsService in jslib-services.module.ts --- libs/angular/src/services/jslib-services.module.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 50095e55400..2b195f47507 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -892,6 +892,7 @@ const safeProviders: SafeProvider[] = [ AuthServiceAbstraction, MessagingServiceAbstraction, TaskSchedulerService, + ConfigService, ], }), safeProvider({ From 839eacc2ef518b5930f78274851ab6de4619a05c Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 28 Jan 2025 12:28:44 +0000 Subject: [PATCH 08/12] Update NotificationsService to await feature flag checks for PushSyncOrgKeysOnRevokeRestore --- libs/common/src/services/notifications.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/common/src/services/notifications.service.ts b/libs/common/src/services/notifications.service.ts index 3bb7ccace5a..72ee8d4fe89 100644 --- a/libs/common/src/services/notifications.service.ts +++ b/libs/common/src/services/notifications.service.ts @@ -11,6 +11,7 @@ import { NotificationsService as NotificationsServiceAbstraction } from "../abst import { AuthService } from "../auth/abstractions/auth.service"; import { AuthenticationStatus } from "../auth/enums/authentication-status"; import { NotificationType } from "../enums"; +import { FeatureFlag } from "../enums/feature-flag.enum"; import { NotificationResponse, SyncCipherNotification, @@ -18,6 +19,7 @@ import { SyncSendNotification, } from "../models/response/notification.response"; import { AppIdService } from "../platform/abstractions/app-id.service"; +import { ConfigService } from "../platform/abstractions/config/config.service"; import { EnvironmentService } from "../platform/abstractions/environment.service"; import { LogService } from "../platform/abstractions/log.service"; import { MessagingService } from "../platform/abstractions/messaging.service"; @@ -25,8 +27,6 @@ import { StateService } from "../platform/abstractions/state.service"; import { ScheduledTaskNames } from "../platform/scheduling/scheduled-task-name.enum"; import { TaskSchedulerService } from "../platform/scheduling/task-scheduler.service"; import { SyncService } from "../vault/abstractions/sync/sync.service.abstraction"; -import { ConfigService } from "../platform/abstractions/config/config.service"; -import { FeatureFlag } from "../enums/feature-flag.enum"; export class NotificationsService implements NotificationsServiceAbstraction { private signalrConnection: signalR.HubConnection; @@ -173,7 +173,7 @@ export class NotificationsService implements NotificationsServiceAbstraction { break; case NotificationType.SyncOrgKeys: if (isAuthenticated) { - if (this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { + if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { await this.apiService.refreshIdentityToken(); } await this.syncService.fullSync(true); @@ -233,7 +233,7 @@ export class NotificationsService implements NotificationsServiceAbstraction { } try { - if (this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { + if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { this.signalrConnection = this.setupConnection(); } From ff7eef9591b6a299804c29490662fcbf61cca51b Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 28 Jan 2025 12:52:05 +0000 Subject: [PATCH 09/12] Add ConfigService to MainBackground --- apps/browser/src/background/main.background.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 192455f9691..4d53fdff393 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1045,6 +1045,7 @@ export default class MainBackground { this.authService, this.messagingService, this.taskSchedulerService, + this.configService, ); this.fido2UserInterfaceService = new BrowserFido2UserInterfaceService(this.authService); From 9f6ecfbbf29df9b92ebe8d7dac60d3104b973a1b Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Mon, 3 Feb 2025 12:42:57 +0000 Subject: [PATCH 10/12] Refresh identity token on SyncOrgKeys notification if feature flag is enabled --- apps/browser/src/background/main.background.ts | 2 ++ libs/angular/src/services/jslib-services.module.ts | 2 ++ .../internal/default-notifications.service.spec.ts | 6 ++++++ .../internal/default-notifications.service.ts | 8 ++++++++ 4 files changed, 18 insertions(+) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index b75847dbbfe..cb897b22dda 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1067,6 +1067,8 @@ export default class MainBackground { new SignalRConnectionService(this.apiService, this.logService), this.authService, this.webPushConnectionService, + this.apiService, + this.configService, ); this.fido2UserInterfaceService = new BrowserFido2UserInterfaceService(this.authService); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 36082f879b9..ce98a10532f 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -890,6 +890,8 @@ const safeProviders: SafeProvider[] = [ SignalRConnectionService, AuthServiceAbstraction, WebPushConnectionService, + ApiServiceAbstraction, + ConfigService, ], }), safeProvider({ diff --git a/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts b/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts index e24069a9fbe..f694cd0eb9e 100644 --- a/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts +++ b/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts @@ -2,6 +2,8 @@ import { mock, MockProxy } from "jest-mock-extended"; import { BehaviorSubject, bufferCount, firstValueFrom, ObservedValueOf, Subject } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { awaitAsync } from "../../../../spec"; import { Matrix } from "../../../../spec/matrix"; @@ -36,6 +38,8 @@ describe("NotificationsService", () => { let signalRNotificationConnectionService: MockProxy; let authService: MockProxy; let webPushNotificationConnectionService: MockProxy; + let apiService: MockProxy; + let configService: MockProxy; let activeAccount: BehaviorSubject>; @@ -102,6 +106,8 @@ describe("NotificationsService", () => { signalRNotificationConnectionService, authService, webPushNotificationConnectionService, + apiService, + configService, ); }); diff --git a/libs/common/src/platform/notifications/internal/default-notifications.service.ts b/libs/common/src/platform/notifications/internal/default-notifications.service.ts index c6b330857a4..caa3fbb4773 100644 --- a/libs/common/src/platform/notifications/internal/default-notifications.service.ts +++ b/libs/common/src/platform/notifications/internal/default-notifications.service.ts @@ -11,6 +11,9 @@ import { } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; +import { ApiService } from "@bitwarden/common/abstractions/api.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { AccountService } from "../../../auth/abstractions/account.service"; import { AuthService } from "../../../auth/abstractions/auth.service"; @@ -52,6 +55,8 @@ export class DefaultNotificationsService implements NotificationsServiceAbstract private readonly signalRConnectionService: SignalRConnectionService, private readonly authService: AuthService, private readonly webPushConnectionService: WebPushConnectionService, + private readonly apiService: ApiService, + private readonly configService: ConfigService, ) { this.notifications$ = this.accountService.activeAccount$.pipe( map((account) => account?.id), @@ -182,6 +187,9 @@ export class DefaultNotificationsService implements NotificationsServiceAbstract await this.syncService.fullSync(true); break; case NotificationType.SyncOrgKeys: + if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { + await this.apiService.refreshIdentityToken(); + } await this.syncService.fullSync(true); this.activitySubject.next("inactive"); // Force a disconnect this.activitySubject.next("active"); // Allow a reconnect From 1764a8bc646e241fabad2f7c25b0a2992905b6ca Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Tue, 4 Feb 2025 09:33:10 +0000 Subject: [PATCH 11/12] Add comment to clarify why the identity token needs to be refreshed --- .../notifications/internal/default-notifications.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/libs/common/src/platform/notifications/internal/default-notifications.service.ts b/libs/common/src/platform/notifications/internal/default-notifications.service.ts index caa3fbb4773..4bdd112658d 100644 --- a/libs/common/src/platform/notifications/internal/default-notifications.service.ts +++ b/libs/common/src/platform/notifications/internal/default-notifications.service.ts @@ -188,6 +188,7 @@ export class DefaultNotificationsService implements NotificationsServiceAbstract break; case NotificationType.SyncOrgKeys: if (await this.configService.getFeatureFlag(FeatureFlag.PushSyncOrgKeysOnRevokeRestore)) { + // Refresh the identity token to ensure organization roles in JWT claims are up-to-date await this.apiService.refreshIdentityToken(); } await this.syncService.fullSync(true); From 57d9548328cbdf487f570d9ac4041f3ddb9fa0ee Mon Sep 17 00:00:00 2001 From: Rui Tome Date: Wed, 5 Feb 2025 16:26:00 +0000 Subject: [PATCH 12/12] Add mocks for ApiService and ConfigService in NotificationsService tests --- .../internal/default-notifications.service.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts b/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts index f694cd0eb9e..84189754bef 100644 --- a/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts +++ b/libs/common/src/platform/notifications/internal/default-notifications.service.spec.ts @@ -66,6 +66,8 @@ describe("NotificationsService", () => { signalRNotificationConnectionService = mock(); authService = mock(); webPushNotificationConnectionService = mock(); + apiService = mock(); + configService = mock(); activeAccount = new BehaviorSubject>(null); accountService.activeAccount$ = activeAccount.asObservable();