From 880ae5f193e4ee716ef59a24932cfef9b7392491 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Mon, 24 Feb 2025 21:58:56 -0500 Subject: [PATCH 01/25] Passed in userId on RemovePasswordComponent. --- .../components/remove-password.component.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index 74cb00a14b8..a6acb1d9312 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -2,11 +2,11 @@ // @ts-strict-ignore import { Directive, OnInit } from "@angular/core"; import { Router } from "@angular/router"; -import { firstValueFrom, map } from "rxjs"; +import { firstValueFrom } from "rxjs"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -21,7 +21,7 @@ export class RemovePasswordComponent implements OnInit { loading = true; organization: Organization; - email: string; + migratingUser: Account; constructor( private router: Router, @@ -36,9 +36,9 @@ export class RemovePasswordComponent implements OnInit { ) {} async ngOnInit() { - this.organization = await this.keyConnectorService.getManagingOrganization(); - this.email = await firstValueFrom( - this.accountService.activeAccount$.pipe(map((a) => a?.email)), + this.migratingUser = await firstValueFrom(this.accountService.activeAccount$); + this.organization = await this.keyConnectorService.getManagingOrganization( + this.migratingUser.id, ); await this.syncService.fullSync(false); this.loading = false; @@ -46,7 +46,7 @@ export class RemovePasswordComponent implements OnInit { convert = async () => { this.continuing = true; - this.actionPromise = this.keyConnectorService.migrateUser(); + this.actionPromise = this.keyConnectorService.migrateUser(this.migratingUser.id); try { await this.actionPromise; @@ -55,7 +55,7 @@ export class RemovePasswordComponent implements OnInit { title: null, message: this.i18nService.t("removedMasterPassword"), }); - await this.keyConnectorService.removeConvertAccountRequired(); + await this.keyConnectorService.removeConvertAccountRequired(this.migratingUser.id); // 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.router.navigate([""]); @@ -88,7 +88,7 @@ export class RemovePasswordComponent implements OnInit { title: null, message: this.i18nService.t("leftOrganization"), }); - await this.keyConnectorService.removeConvertAccountRequired(); + await this.keyConnectorService.removeConvertAccountRequired(this.migratingUser.id); // 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.router.navigate([""]); From 4e3bc4c2c15ec2dd9b7750bdf32a136a4e999a6b Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Mon, 24 Feb 2025 22:03:21 -0500 Subject: [PATCH 02/25] Added userId on other references to KeyConnectorService methods --- apps/cli/src/commands/convert-to-key-connector.command.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/apps/cli/src/commands/convert-to-key-connector.command.ts b/apps/cli/src/commands/convert-to-key-connector.command.ts index 8c8a3fc9a10..8e1a7a6c9f3 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.ts @@ -39,7 +39,7 @@ export class ConvertToKeyConnectorCommand { ); } - const organization = await this.keyConnectorService.getManagingOrganization(); + const organization = await this.keyConnectorService.getManagingOrganization(this.userId); const answer: inquirer.Answers = await inquirer.createPromptModule({ output: process.stderr })({ type: "list", @@ -71,7 +71,7 @@ export class ConvertToKeyConnectorCommand { throw e; } - await this.keyConnectorService.removeConvertAccountRequired(); + await this.keyConnectorService.removeConvertAccountRequired(this.userId); await this.keyConnectorService.setUsesKeyConnector(true, this.userId); // Update environment URL - required for api key login @@ -83,7 +83,7 @@ export class ConvertToKeyConnectorCommand { return Response.success(); } else if (answer.convert === "leave") { await this.organizationApiService.leave(organization.id); - await this.keyConnectorService.removeConvertAccountRequired(); + await this.keyConnectorService.removeConvertAccountRequired(this.userId); return Response.success(); } else { await this.logout(); From 8a5835b0e920eb850b67062d89f6eed642b31544 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Mon, 10 Mar 2025 22:06:45 +0000 Subject: [PATCH 03/25] remove password component refactor, test coverage, enabled strict --- .../auth/popup/remove-password.component.html | 9 +- .../src/auth/remove-password.component.html | 14 +- .../remove-password.component.spec.ts | 197 ++++++++++++++++++ .../components/remove-password.component.ts | 84 ++++---- 4 files changed, 248 insertions(+), 56 deletions(-) create mode 100644 libs/angular/src/auth/components/remove-password.component.spec.ts diff --git a/apps/browser/src/auth/popup/remove-password.component.html b/apps/browser/src/auth/popup/remove-password.component.html index 793bcff3e09..22e31c8241c 100644 --- a/apps/browser/src/auth/popup/remove-password.component.html +++ b/apps/browser/src/auth/popup/remove-password.component.html @@ -13,12 +13,7 @@

{{ "convertOrganizationEncryptionDesc" | i18n: organization.name }}

- - diff --git a/libs/angular/src/auth/components/remove-password.component.spec.ts b/libs/angular/src/auth/components/remove-password.component.spec.ts new file mode 100644 index 00000000000..b1eff233f7d --- /dev/null +++ b/libs/angular/src/auth/components/remove-password.component.spec.ts @@ -0,0 +1,197 @@ +import { Router } from "@angular/router"; +import { mock } from "jest-mock-extended"; + +import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; +import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { SyncService } from "@bitwarden/common/platform/sync"; +import { mockAccountServiceWith } from "@bitwarden/common/spec"; +import { UserId } from "@bitwarden/common/types/guid"; +import { DialogService, ToastService } from "@bitwarden/components"; + +import { RemovePasswordComponent } from "./remove-password.component"; + +describe("RemovePasswordComponent", () => { + let component: RemovePasswordComponent; + + const userId = "test-user-id" as UserId; + const organization = { + id: "test-organization-id", + name: "test-organization-name", + } as Organization; + + const accountService = mockAccountServiceWith(userId); + + const mockRouter = mock(); + const mockSyncService = mock(); + const mockI18nService = mock(); + const mockKeyConnectorService = mock(); + const mockOrganizationApiService = mock(); + const mockDialogService = mock(); + const mockToastService = mock(); + + beforeEach(async () => { + jest.clearAllMocks(); + + await accountService.switchAccount(userId); + + component = new RemovePasswordComponent( + mockRouter, + accountService, + mockSyncService, + mockI18nService, + mockKeyConnectorService, + mockOrganizationApiService, + mockDialogService, + mockToastService, + ); + }); + + describe("ngOnInit", () => { + it("should set activeUserId and organization", async () => { + mockKeyConnectorService.getManagingOrganization.mockResolvedValue(organization); + + await component.ngOnInit(); + + expect(component["activeUserId"]).toBe("test-user-id"); + expect(component.organization).toEqual(organization); + expect(component.loading).toEqual(false); + + expect(mockKeyConnectorService.getManagingOrganization).toHaveBeenCalledWith(userId); + expect(mockSyncService.fullSync).toHaveBeenCalledWith(false); + }); + + it("should throw an error if no active account is found", async () => { + await accountService.switchAccount(null as unknown as UserId); + + await expect(component.ngOnInit()).rejects.toThrow(new Error("No active account found")); + }); + + it("should throw an error if no organization is found", async () => { + mockKeyConnectorService.getManagingOrganization.mockResolvedValue( + null as unknown as Organization, + ); + + await expect(component.ngOnInit()).rejects.toThrow(new Error("No organization found")); + }); + }); + + describe("get action", () => { + it.each([ + [true, false], + [false, true], + [true, true], + ])( + "should return true when continuing is $continuing and leaving is $leaving", + (continuing, leaving) => { + component.continuing = continuing; + component.leaving = leaving; + + expect(component.action).toBe(true); + }, + ); + + it("should return false when continuing and leaving are both false", () => { + component.continuing = false; + component.leaving = false; + + expect(component.action).toBe(false); + }); + }); + + describe("convert", () => { + beforeEach(async () => { + mockKeyConnectorService.getManagingOrganization.mockResolvedValue(organization); + + await component.ngOnInit(); + }); + + it("should call migrateUser and show success toast", async () => { + mockI18nService.t.mockReturnValue("removed master password"); + + await component.convert(); + + expect(component.continuing).toBe(true); + expect(mockKeyConnectorService.migrateUser).toHaveBeenCalledWith(userId); + expect(mockKeyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "success", + message: "removed master password", + }); + expect(mockRouter.navigate).toHaveBeenCalledWith([""]); + }); + + it("should handle errors and show error toast", async () => { + const errorMessage = "Can't migrate user error"; + mockKeyConnectorService.migrateUser.mockRejectedValue(new Error(errorMessage)); + mockI18nService.t.mockReturnValue("error occurred"); + + await component.convert(); + + expect(component.continuing).toBe(false); + expect(mockKeyConnectorService.migrateUser).toHaveBeenCalledWith(userId); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "error", + title: "error occurred", + message: errorMessage, + }); + expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + }); + + describe("leave", () => { + beforeEach(async () => { + mockKeyConnectorService.getManagingOrganization.mockResolvedValue(organization); + + await component.ngOnInit(); + }); + + it("should call leave and show success toast", async () => { + mockDialogService.openSimpleDialog.mockResolvedValue(true); + mockI18nService.t.mockReturnValue("left organization"); + + await component.leave(); + + expect(component.leaving).toBe(true); + expect(mockOrganizationApiService.leave).toHaveBeenCalledWith(organization.id); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "success", + message: "left organization", + }); + expect(mockKeyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); + expect(mockRouter.navigate).toHaveBeenCalledWith([""]); + }); + + it("should handle errors and show error toast", async () => { + const errorMessage = "Can't leave organization error"; + mockDialogService.openSimpleDialog.mockResolvedValue(true); + mockOrganizationApiService.leave.mockRejectedValue(new Error(errorMessage)); + mockI18nService.t.mockReturnValue("error occurred"); + + await component.leave(); + + expect(component.leaving).toBe(false); + expect(mockOrganizationApiService.leave).toHaveBeenCalledWith(organization.id); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "error", + title: "error occurred", + message: errorMessage, + }); + expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + + it("should not call leave if dialog is canceled", async () => { + mockDialogService.openSimpleDialog.mockResolvedValue(false); + + await component.leave(); + + expect(component.leaving).toBe(false); + expect(mockOrganizationApiService.leave).not.toHaveBeenCalled(); + expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index a6acb1d9312..53e3da2cc08 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -1,33 +1,29 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { Directive, OnInit } from "@angular/core"; import { Router } from "@angular/router"; import { firstValueFrom } from "rxjs"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; -import { Account, AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; +import { UserId } from "@bitwarden/common/types/guid"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { DialogService, ToastService } from "@bitwarden/components"; @Directive() export class RemovePasswordComponent implements OnInit { - actionPromise: Promise; continuing = false; leaving = false; loading = true; - organization: Organization; - migratingUser: Account; + organization!: Organization; + private activeUserId!: UserId; constructor( private router: Router, private accountService: AccountService, private syncService: SyncService, - private platformUtilsService: PlatformUtilsService, private i18nService: I18nService, private keyConnectorService: KeyConnectorService, private organizationApiService: OrganizationApiServiceAbstraction, @@ -36,35 +32,47 @@ export class RemovePasswordComponent implements OnInit { ) {} async ngOnInit() { - this.migratingUser = await firstValueFrom(this.accountService.activeAccount$); - this.organization = await this.keyConnectorService.getManagingOrganization( - this.migratingUser.id, - ); + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + if (activeAccount == null) { + throw new Error("No active account found"); + } + this.activeUserId = activeAccount.id; + + this.organization = await this.keyConnectorService.getManagingOrganization(this.activeUserId); + if (this.organization == null) { + throw new Error("No organization found"); + } await this.syncService.fullSync(false); this.loading = false; } + get action() { + return this.continuing || this.leaving; + } + convert = async () => { this.continuing = true; - this.actionPromise = this.keyConnectorService.migrateUser(this.migratingUser.id); try { - await this.actionPromise; + await this.keyConnectorService.migrateUser(this.activeUserId); + this.toastService.showToast({ variant: "success", - title: null, message: this.i18nService.t("removedMasterPassword"), }); - await this.keyConnectorService.removeConvertAccountRequired(this.migratingUser.id); - // 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.router.navigate([""]); + await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId); + + await this.router.navigate([""]); } catch (e) { - this.toastService.showToast({ - variant: "error", - title: this.i18nService.t("errorOccurred"), - message: e.message, - }); + this.continuing = false; + + if (e instanceof Error) { + this.toastService.showToast({ + variant: "error", + title: this.i18nService.t("errorOccurred"), + message: e.message, + }); + } } }; @@ -79,25 +87,27 @@ export class RemovePasswordComponent implements OnInit { return false; } + this.leaving = true; try { - this.leaving = true; - this.actionPromise = this.organizationApiService.leave(this.organization.id); - await this.actionPromise; + await this.organizationApiService.leave(this.organization.id); + this.toastService.showToast({ variant: "success", - title: null, message: this.i18nService.t("leftOrganization"), }); - await this.keyConnectorService.removeConvertAccountRequired(this.migratingUser.id); - // 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.router.navigate([""]); + await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId); + + await this.router.navigate([""]); } catch (e) { - this.toastService.showToast({ - variant: "error", - title: this.i18nService.t("errorOccurred"), - message: e, - }); + this.leaving = false; + + if (e instanceof Error) { + this.toastService.showToast({ + variant: "error", + title: this.i18nService.t("errorOccurred"), + message: e.message, + }); + } } }; } From 0e9d1beff822a405136cc2d5f62fc803d8da24f6 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Mon, 10 Mar 2025 22:23:40 +0000 Subject: [PATCH 04/25] explicit user id provided to key connector service --- apps/cli/src/commands/convert-to-key-connector.command.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/cli/src/commands/convert-to-key-connector.command.ts b/apps/cli/src/commands/convert-to-key-connector.command.ts index 8e1a7a6c9f3..65c793bf337 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.ts @@ -65,7 +65,7 @@ export class ConvertToKeyConnectorCommand { if (answer.convert === "remove") { try { - await this.keyConnectorService.migrateUser(); + await this.keyConnectorService.migrateUser(this.userId); } catch (e) { await this.logout(); throw e; From 578668e7d02f952948f06372513e0b57ed602c32 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Mon, 10 Mar 2025 22:39:15 +0000 Subject: [PATCH 05/25] redirect to / instead when user not logged in or not managing organization --- .../components/remove-password.component.spec.ts | 16 +++++++++++----- .../auth/components/remove-password.component.ts | 14 ++++++++++++-- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/libs/angular/src/auth/components/remove-password.component.spec.ts b/libs/angular/src/auth/components/remove-password.component.spec.ts index b1eff233f7d..ddabd690306 100644 --- a/libs/angular/src/auth/components/remove-password.component.spec.ts +++ b/libs/angular/src/auth/components/remove-password.component.spec.ts @@ -5,6 +5,7 @@ import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-conso import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { SyncService } from "@bitwarden/common/platform/sync"; import { mockAccountServiceWith } from "@bitwarden/common/spec"; import { UserId } from "@bitwarden/common/types/guid"; @@ -37,6 +38,7 @@ describe("RemovePasswordComponent", () => { await accountService.switchAccount(userId); component = new RemovePasswordComponent( + mock(), mockRouter, accountService, mockSyncService, @@ -62,18 +64,22 @@ describe("RemovePasswordComponent", () => { expect(mockSyncService.fullSync).toHaveBeenCalledWith(false); }); - it("should throw an error if no active account is found", async () => { + it("should redirect to login when no active account is found", async () => { await accountService.switchAccount(null as unknown as UserId); - await expect(component.ngOnInit()).rejects.toThrow(new Error("No active account found")); + await component.ngOnInit(); + + expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); - it("should throw an error if no organization is found", async () => { + it("should redirect to login when no organization is found", async () => { mockKeyConnectorService.getManagingOrganization.mockResolvedValue( null as unknown as Organization, ); - await expect(component.ngOnInit()).rejects.toThrow(new Error("No organization found")); + await component.ngOnInit(); + + expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); }); @@ -183,7 +189,7 @@ describe("RemovePasswordComponent", () => { expect(mockRouter.navigate).not.toHaveBeenCalled(); }); - it("should not call leave if dialog is canceled", async () => { + it("should not call leave when dialog is canceled", async () => { mockDialogService.openSimpleDialog.mockResolvedValue(false); await component.leave(); diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index 53e3da2cc08..8fd4e7bba95 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -7,6 +7,7 @@ import { Organization } from "@bitwarden/common/admin-console/models/domain/orga import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { UserId } from "@bitwarden/common/types/guid"; import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { DialogService, ToastService } from "@bitwarden/components"; @@ -21,6 +22,7 @@ export class RemovePasswordComponent implements OnInit { private activeUserId!: UserId; constructor( + private logService: LogService, private router: Router, private accountService: AccountService, private syncService: SyncService, @@ -34,13 +36,21 @@ export class RemovePasswordComponent implements OnInit { async ngOnInit() { const activeAccount = await firstValueFrom(this.accountService.activeAccount$); if (activeAccount == null) { - throw new Error("No active account found"); + this.logService.info( + "[Key Connector remove password] No active account found, redirecting to login.", + ); + await this.router.navigate([""]); + return; } this.activeUserId = activeAccount.id; this.organization = await this.keyConnectorService.getManagingOrganization(this.activeUserId); if (this.organization == null) { - throw new Error("No organization found"); + this.logService.info( + "[Key Connector remove password] No organization found, redirecting to login.", + ); + await this.router.navigate([""]); + return; } await this.syncService.fullSync(false); this.loading = false; From b0d09c8b24d92f19c7efad47fbe63527da8abdd9 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Mon, 10 Mar 2025 23:46:13 +0000 Subject: [PATCH 06/25] key connector service explicit user id --- .../browser/src/background/main.background.ts | 1 - .../service-container/service-container.ts | 1 - .../abstractions/key-connector.service.ts | 33 +++++++++++-------- .../services/key-connector.service.spec.ts | 19 +++++------ .../auth/services/key-connector.service.ts | 12 +++---- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index cd65220936e..f8b5f63710a 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -731,7 +731,6 @@ export default class MainBackground { this.badgeSettingsService = new BadgeSettingsService(this.stateProvider); this.policyApiService = new PolicyApiService(this.policyService, this.apiService); this.keyConnectorService = new KeyConnectorService( - this.accountService, this.masterPasswordService, this.keyService, this.apiService, diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index 0e776375e6a..3825e6777c8 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -560,7 +560,6 @@ export class ServiceContainer { this.policyApiService = new PolicyApiService(this.policyService, this.apiService); this.keyConnectorService = new KeyConnectorService( - this.accountService, this.masterPasswordService, this.keyService, this.apiService, diff --git a/libs/common/src/auth/abstractions/key-connector.service.ts b/libs/common/src/auth/abstractions/key-connector.service.ts index 9bcca1f5619..699fa7ad036 100644 --- a/libs/common/src/auth/abstractions/key-connector.service.ts +++ b/libs/common/src/auth/abstractions/key-connector.service.ts @@ -1,22 +1,29 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import { Organization } from "../../admin-console/models/domain/organization"; import { UserId } from "../../types/guid"; import { IdentityTokenResponse } from "../models/response/identity-token.response"; export abstract class KeyConnectorService { - setMasterKeyFromUrl: (url: string, userId: UserId) => Promise; - getManagingOrganization: (userId?: UserId) => Promise; - getUsesKeyConnector: (userId: UserId) => Promise; - migrateUser: (userId?: UserId) => Promise; - userNeedsMigration: (userId: UserId) => Promise; - convertNewSsoUserToKeyConnector: ( + abstract setMasterKeyFromUrl(url: string, userId: UserId): Promise; + + abstract getManagingOrganization(userId: UserId): Promise; + + abstract getUsesKeyConnector(userId: UserId): Promise; + + abstract migrateUser(userId: UserId): Promise; + + abstract userNeedsMigration(userId: UserId): Promise; + + abstract convertNewSsoUserToKeyConnector( tokenResponse: IdentityTokenResponse, orgId: string, userId: UserId, - ) => Promise; - setUsesKeyConnector: (enabled: boolean, userId: UserId) => Promise; - setConvertAccountRequired: (status: boolean, userId?: UserId) => Promise; - getConvertAccountRequired: () => Promise; - removeConvertAccountRequired: (userId?: UserId) => Promise; + ): Promise; + + abstract setUsesKeyConnector(enabled: boolean, userId: UserId): Promise; + + abstract setConvertAccountRequired(status: boolean, userId: UserId): Promise; + + abstract getConvertAccountRequired(): Promise; + + abstract removeConvertAccountRequired(userId: UserId): Promise; } diff --git a/libs/common/src/auth/services/key-connector.service.spec.ts b/libs/common/src/auth/services/key-connector.service.spec.ts index ec03c7ece55..3ed4df1dd85 100644 --- a/libs/common/src/auth/services/key-connector.service.spec.ts +++ b/libs/common/src/auth/services/key-connector.service.spec.ts @@ -56,7 +56,6 @@ describe("KeyConnectorService", () => { stateProvider = new FakeStateProvider(accountService); keyConnectorService = new KeyConnectorService( - accountService, masterPasswordService, keyService, apiService, @@ -98,7 +97,7 @@ describe("KeyConnectorService", () => { organizationService.organizations$.mockReturnValue(of(orgs)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toEqual(orgs[0]); @@ -113,7 +112,7 @@ describe("KeyConnectorService", () => { organizationService.organizations$.mockReturnValue(of(orgs)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toBeUndefined(); @@ -128,7 +127,7 @@ describe("KeyConnectorService", () => { organizationService.organizations$.mockReturnValue(of(orgs)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toBeUndefined(); @@ -143,7 +142,7 @@ describe("KeyConnectorService", () => { organizationService.organizations$.mockReturnValue(of(orgs)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toBeUndefined(); @@ -157,7 +156,7 @@ describe("KeyConnectorService", () => { const newValue = true; - await keyConnectorService.setConvertAccountRequired(newValue); + await keyConnectorService.setConvertAccountRequired(newValue, mockUserId); expect(await keyConnectorService.getConvertAccountRequired()).toBe(newValue); }); @@ -166,9 +165,9 @@ describe("KeyConnectorService", () => { const state = stateProvider.activeUser.getFake(CONVERT_ACCOUNT_TO_KEY_CONNECTOR); state.nextState(false); - const newValue: boolean = null; + const newValue: boolean | null = null; - await keyConnectorService.setConvertAccountRequired(newValue); + await keyConnectorService.setConvertAccountRequired(newValue, mockUserId); expect(await keyConnectorService.getConvertAccountRequired()).toBe(newValue); }); @@ -258,7 +257,7 @@ describe("KeyConnectorService", () => { jest.spyOn(apiService, "postUserKeyToKeyConnector").mockResolvedValue(); // Act - await keyConnectorService.migrateUser(); + await keyConnectorService.migrateUser(mockUserId); // Assert expect(keyConnectorService.getManagingOrganization).toHaveBeenCalled(); @@ -284,7 +283,7 @@ describe("KeyConnectorService", () => { try { // Act - await keyConnectorService.migrateUser(); + await keyConnectorService.migrateUser(mockUserId); } catch { // Assert expect(logService.error).toHaveBeenCalledWith(error); diff --git a/libs/common/src/auth/services/key-connector.service.ts b/libs/common/src/auth/services/key-connector.service.ts index f6f76579ee5..3a3419dbae1 100644 --- a/libs/common/src/auth/services/key-connector.service.ts +++ b/libs/common/src/auth/services/key-connector.service.ts @@ -4,7 +4,6 @@ import { firstValueFrom } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { Argon2KdfConfig, KdfConfig, @@ -57,8 +56,8 @@ export const CONVERT_ACCOUNT_TO_KEY_CONNECTOR = new UserKeyDefinition; private convertAccountToKeyConnectorState: ActiveUserState; + constructor( - private accountService: AccountService, private masterPasswordService: InternalMasterPasswordServiceAbstraction, private keyService: KeyService, private apiService: ApiService, @@ -91,8 +90,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { return loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; } - async migrateUser(userId?: UserId) { - userId ??= (await firstValueFrom(this.accountService.activeAccount$))?.id; + async migrateUser(userId: UserId) { const organization = await this.getManagingOrganization(userId); const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId)); const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64); @@ -121,7 +119,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { } } - async getManagingOrganization(userId?: UserId): Promise { + async getManagingOrganization(userId: UserId): Promise { const orgs = await firstValueFrom(this.organizationService.organizations$(userId)); return orgs.find( (o) => @@ -184,7 +182,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postSetKeyConnectorKey(setPasswordRequest); } - async setConvertAccountRequired(status: boolean, userId?: UserId) { + async setConvertAccountRequired(status: boolean | null, userId: UserId) { await this.stateProvider.setUserState(CONVERT_ACCOUNT_TO_KEY_CONNECTOR, status, userId); } @@ -192,7 +190,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { return firstValueFrom(this.convertAccountToKeyConnectorState.state$); } - async removeConvertAccountRequired(userId?: UserId) { + async removeConvertAccountRequired(userId: UserId) { await this.setConvertAccountRequired(null, userId); } From eb2069d56d4e22ccf145e8f1b57a4f8e457059b4 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Mon, 10 Mar 2025 23:57:03 +0000 Subject: [PATCH 07/25] key connector service no longer requires account service --- libs/angular/src/services/jslib-services.module.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 9ee49a30689..c39b6fce62d 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -971,7 +971,6 @@ const safeProviders: SafeProvider[] = [ provide: KeyConnectorServiceAbstraction, useClass: KeyConnectorService, deps: [ - AccountServiceAbstraction, InternalMasterPasswordServiceAbstraction, KeyService, ApiServiceAbstraction, From d496863e8d2bf3790750bef1bc4f6dc1e09c2e53 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 11 Mar 2025 07:30:57 +0000 Subject: [PATCH 08/25] key connector service missing null type --- libs/common/src/auth/abstractions/key-connector.service.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/common/src/auth/abstractions/key-connector.service.ts b/libs/common/src/auth/abstractions/key-connector.service.ts index 699fa7ad036..3ce79dd10d4 100644 --- a/libs/common/src/auth/abstractions/key-connector.service.ts +++ b/libs/common/src/auth/abstractions/key-connector.service.ts @@ -21,7 +21,7 @@ export abstract class KeyConnectorService { abstract setUsesKeyConnector(enabled: boolean, userId: UserId): Promise; - abstract setConvertAccountRequired(status: boolean, userId: UserId): Promise; + abstract setConvertAccountRequired(status: boolean | null, userId: UserId): Promise; abstract getConvertAccountRequired(): Promise; From d7c85331ae740611259d278ec202a3f4f5bf42ca Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 11 Mar 2025 08:36:43 +0000 Subject: [PATCH 09/25] cli convert to key connector unit tests --- apps/cli/src/auth/commands/unlock.command.ts | 1 - .../convert-to-key-connector.command.spec.ts | 143 ++++++++++++++++++ .../convert-to-key-connector.command.ts | 4 - .../src/models/response/message.response.ts | 4 +- 4 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 apps/cli/src/commands/convert-to-key-connector.command.spec.ts diff --git a/apps/cli/src/auth/commands/unlock.command.ts b/apps/cli/src/auth/commands/unlock.command.ts index f33a9cf347a..7d72dd30506 100644 --- a/apps/cli/src/auth/commands/unlock.command.ts +++ b/apps/cli/src/auth/commands/unlock.command.ts @@ -78,7 +78,6 @@ export class UnlockCommand { userId, this.keyConnectorService, this.environmentService, - this.syncService, this.organizationApiService, this.logout, ); diff --git a/apps/cli/src/commands/convert-to-key-connector.command.spec.ts b/apps/cli/src/commands/convert-to-key-connector.command.spec.ts new file mode 100644 index 00000000000..ed3d0e0810d --- /dev/null +++ b/apps/cli/src/commands/convert-to-key-connector.command.spec.ts @@ -0,0 +1,143 @@ +import { createPromptModule } from "inquirer"; +import { mock } from "jest-mock-extended"; +import { of } from "rxjs"; + +import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; +import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { + Environment, + EnvironmentService, + Region, + Urls, +} from "@bitwarden/common/platform/abstractions/environment.service"; +import { UserId } from "@bitwarden/common/types/guid"; + +import { Response } from "../models/response"; +import { MessageResponse } from "../models/response/message.response"; + +import { ConvertToKeyConnectorCommand } from "./convert-to-key-connector.command"; + +jest.mock("inquirer", () => { + return { + createPromptModule: jest.fn(() => jest.fn(() => Promise.resolve({ convert: "" }))), + }; +}); + +describe("ConvertToKeyConnectorCommand", () => { + let command: ConvertToKeyConnectorCommand; + + const userId = "test-user-id" as UserId; + const organization = { + id: "test-organization-id", + name: "Test Organization", + keyConnectorUrl: "https://keyconnector.example.com", + } as Organization; + + const keyConnectorService = mock(); + const environmentService = mock(); + const organizationApiService = mock(); + const logout = jest.fn(); + + beforeEach(async () => { + command = new ConvertToKeyConnectorCommand( + userId, + keyConnectorService, + environmentService, + organizationApiService, + logout, + ); + }); + + describe("run", () => { + it("should logout and return error response if no interaction available", async () => { + process.env.BW_NOINTERACTION = "true"; + + const response = await command.run(); + + expect(response).not.toBeNull(); + expect(response.success).toEqual(false); + expect(response).toEqual( + Response.error( + new MessageResponse( + "An organization you are a member of is using Key Connector. In order to access the vault, you must opt-in to Key Connector now via the web vault. You have been logged out.", + null, + ), + ), + ); + expect(logout).toHaveBeenCalled(); + }); + + it("should logout and return error response if interaction answer is exit", async () => { + process.env.BW_NOINTERACTION = "false"; + keyConnectorService.getManagingOrganization.mockResolvedValue(organization); + + (createPromptModule as jest.Mock).mockImplementation(() => + jest.fn(() => Promise.resolve({ convert: "exit" })), + ); + + const response = await command.run(); + + expect(response).not.toBeNull(); + expect(response.success).toEqual(false); + expect(response).toEqual(Response.error("You have been logged out.")); + expect(logout).toHaveBeenCalled(); + }); + + it("should key connector migrate user and return success response if answer is remove", async () => { + process.env.BW_NOINTERACTION = "false"; + keyConnectorService.getManagingOrganization.mockResolvedValue(organization); + environmentService.environment$ = of({ + getUrls: () => + ({ + keyConnector: "old-key-connector-url", + }) as Urls, + } as Environment); + + (createPromptModule as jest.Mock).mockImplementation(() => + jest.fn(() => Promise.resolve({ convert: "remove" })), + ); + + const response = await command.run(); + + expect(response).not.toBeNull(); + expect(response.success).toEqual(true); + expect(keyConnectorService.migrateUser).toHaveBeenCalledWith(userId); + expect(keyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); + expect(keyConnectorService.setUsesKeyConnector).toHaveBeenCalledWith(true, userId); + expect(environmentService.setEnvironment).toHaveBeenCalledWith(Region.SelfHosted, { + keyConnector: organization.keyConnectorUrl, + } as Urls); + }); + + it("should logout and throw error if key connector migrate user fails", async () => { + process.env.BW_NOINTERACTION = "false"; + keyConnectorService.getManagingOrganization.mockResolvedValue(organization); + + (createPromptModule as jest.Mock).mockImplementation(() => + jest.fn(() => Promise.resolve({ convert: "remove" })), + ); + + keyConnectorService.migrateUser.mockRejectedValue(new Error("Migration failed")); + + await expect(command.run()).rejects.toThrow("Migration failed"); + expect(logout).toHaveBeenCalled(); + }); + + it("should leave organization and return success response if answer is leave", async () => { + process.env.BW_NOINTERACTION = "false"; + keyConnectorService.getManagingOrganization.mockResolvedValue(organization); + + (createPromptModule as jest.Mock).mockImplementation(() => + jest.fn(() => Promise.resolve({ convert: "leave" })), + ); + + const response = await command.run(); + + expect(response).not.toBeNull(); + expect(response.success).toEqual(true); + expect(organizationApiService.leave).toHaveBeenCalledWith(organization.id); + expect(keyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); + }); + }); +}); diff --git a/apps/cli/src/commands/convert-to-key-connector.command.ts b/apps/cli/src/commands/convert-to-key-connector.command.ts index 65c793bf337..a87483af625 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.ts @@ -1,5 +1,3 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore import * as inquirer from "inquirer"; import { firstValueFrom } from "rxjs"; @@ -10,7 +8,6 @@ import { Region, } from "@bitwarden/common/platform/abstractions/environment.service"; import { UserId } from "@bitwarden/common/types/guid"; -import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { Response } from "../models/response"; import { MessageResponse } from "../models/response/message.response"; @@ -20,7 +17,6 @@ export class ConvertToKeyConnectorCommand { private readonly userId: UserId, private keyConnectorService: KeyConnectorService, private environmentService: EnvironmentService, - private syncService: SyncService, private organizationApiService: OrganizationApiServiceAbstraction, private logout: () => Promise, ) {} diff --git a/apps/cli/src/models/response/message.response.ts b/apps/cli/src/models/response/message.response.ts index dcbe3b92216..889d0982f83 100644 --- a/apps/cli/src/models/response/message.response.ts +++ b/apps/cli/src/models/response/message.response.ts @@ -5,11 +5,11 @@ import { BaseResponse } from "./base.response"; export class MessageResponse implements BaseResponse { object: string; title: string; - message: string; + message: string | null; raw: string; noColor = false; - constructor(title: string, message: string) { + constructor(title: string, message: string | null) { this.object = "message"; this.title = title; this.message = message; From 3e6c3b07c788466ef58ce00938198959a4cfcead Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Thu, 13 Mar 2025 12:28:01 +0000 Subject: [PATCH 10/25] remove unnecessary SyncService --- apps/cli/src/auth/commands/unlock.command.ts | 2 -- apps/cli/src/base-program.ts | 1 - apps/cli/src/oss-serve-configurator.ts | 1 - apps/cli/src/program.ts | 1 - 4 files changed, 5 deletions(-) diff --git a/apps/cli/src/auth/commands/unlock.command.ts b/apps/cli/src/auth/commands/unlock.command.ts index 7d72dd30506..2b55efba2cb 100644 --- a/apps/cli/src/auth/commands/unlock.command.ts +++ b/apps/cli/src/auth/commands/unlock.command.ts @@ -14,7 +14,6 @@ import { EnvironmentService } from "@bitwarden/common/platform/abstractions/envi import { Utils } from "@bitwarden/common/platform/misc/utils"; import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service"; import { MasterKey } from "@bitwarden/common/types/key"; -import { SyncService } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction"; import { KeyService } from "@bitwarden/key-management"; import { ConvertToKeyConnectorCommand } from "../../commands/convert-to-key-connector.command"; @@ -32,7 +31,6 @@ export class UnlockCommand { private logService: ConsoleLogService, private keyConnectorService: KeyConnectorService, private environmentService: EnvironmentService, - private syncService: SyncService, private organizationApiService: OrganizationApiServiceAbstraction, private logout: () => Promise, ) {} diff --git a/apps/cli/src/base-program.ts b/apps/cli/src/base-program.ts index 14c930b804a..40b95df64e4 100644 --- a/apps/cli/src/base-program.ts +++ b/apps/cli/src/base-program.ts @@ -179,7 +179,6 @@ export abstract class BaseProgram { this.serviceContainer.logService, this.serviceContainer.keyConnectorService, this.serviceContainer.environmentService, - this.serviceContainer.syncService, this.serviceContainer.organizationApiService, this.serviceContainer.logout, ); diff --git a/apps/cli/src/oss-serve-configurator.ts b/apps/cli/src/oss-serve-configurator.ts index dec09447839..81165d8c74e 100644 --- a/apps/cli/src/oss-serve-configurator.ts +++ b/apps/cli/src/oss-serve-configurator.ts @@ -142,7 +142,6 @@ export class OssServeConfigurator { this.serviceContainer.logService, this.serviceContainer.keyConnectorService, this.serviceContainer.environmentService, - this.serviceContainer.syncService, this.serviceContainer.organizationApiService, async () => await this.serviceContainer.logout(), ); diff --git a/apps/cli/src/program.ts b/apps/cli/src/program.ts index c6b79c7dff2..1f5b928ce0e 100644 --- a/apps/cli/src/program.ts +++ b/apps/cli/src/program.ts @@ -277,7 +277,6 @@ export class Program extends BaseProgram { this.serviceContainer.logService, this.serviceContainer.keyConnectorService, this.serviceContainer.environmentService, - this.serviceContainer.syncService, this.serviceContainer.organizationApiService, async () => await this.serviceContainer.logout(), ); From ea231e355b363be846373d5431c8a951b8105c5b Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Thu, 13 Mar 2025 13:21:12 +0000 Subject: [PATCH 11/25] error toast not showing on ErrorResponse --- .../remove-password.component.spec.ts | 23 +++++++++++++++---- .../components/remove-password.component.ts | 5 ++-- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/libs/angular/src/auth/components/remove-password.component.spec.ts b/libs/angular/src/auth/components/remove-password.component.spec.ts index ddabd690306..5e9d823525a 100644 --- a/libs/angular/src/auth/components/remove-password.component.spec.ts +++ b/libs/angular/src/auth/components/remove-password.component.spec.ts @@ -4,6 +4,7 @@ import { mock } from "jest-mock-extended"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { SyncService } from "@bitwarden/common/platform/sync"; @@ -128,9 +129,16 @@ describe("RemovePasswordComponent", () => { expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); - it("should handle errors and show error toast", async () => { + it("should handle response errors and show error toast", async () => { const errorMessage = "Can't migrate user error"; - mockKeyConnectorService.migrateUser.mockRejectedValue(new Error(errorMessage)); + mockKeyConnectorService.migrateUser.mockRejectedValue( + new ErrorResponse( + { + message: errorMessage, + }, + 404, + ), + ); mockI18nService.t.mockReturnValue("error occurred"); await component.convert(); @@ -170,10 +178,17 @@ describe("RemovePasswordComponent", () => { expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); - it("should handle errors and show error toast", async () => { + it("should handle response errors and show error toast", async () => { const errorMessage = "Can't leave organization error"; mockDialogService.openSimpleDialog.mockResolvedValue(true); - mockOrganizationApiService.leave.mockRejectedValue(new Error(errorMessage)); + mockOrganizationApiService.leave.mockRejectedValue( + new ErrorResponse( + { + message: errorMessage, + }, + 404, + ), + ); mockI18nService.t.mockReturnValue("error occurred"); await component.leave(); diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index 8fd4e7bba95..baf986d3211 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -6,6 +6,7 @@ import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-conso import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { UserId } from "@bitwarden/common/types/guid"; @@ -76,7 +77,7 @@ export class RemovePasswordComponent implements OnInit { } catch (e) { this.continuing = false; - if (e instanceof Error) { + if (e instanceof ErrorResponse) { this.toastService.showToast({ variant: "error", title: this.i18nService.t("errorOccurred"), @@ -111,7 +112,7 @@ export class RemovePasswordComponent implements OnInit { } catch (e) { this.leaving = false; - if (e instanceof Error) { + if (e instanceof ErrorResponse) { this.toastService.showToast({ variant: "error", title: this.i18nService.t("errorOccurred"), From 8a303d46c66e9223ac87351c46a6a0320df11cb2 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Thu, 13 Mar 2025 16:48:46 +0000 Subject: [PATCH 12/25] bad import due to merge conflict --- apps/cli/src/commands/convert-to-key-connector.command.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/cli/src/commands/convert-to-key-connector.command.spec.ts b/apps/cli/src/commands/convert-to-key-connector.command.spec.ts index ed3d0e0810d..f86fcafa551 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.spec.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.spec.ts @@ -4,7 +4,7 @@ import { of } from "rxjs"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; -import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { KeyConnectorService } from "@bitwarden/common/key-management/key-connector/abstractions/key-connector.service"; import { Environment, EnvironmentService, From 14c079dc225acc8967ace6c05bd226f2ae104165 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Thu, 13 Mar 2025 16:49:55 +0000 Subject: [PATCH 13/25] bad import due to merge conflict --- .../src/auth/components/remove-password.component.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/angular/src/auth/components/remove-password.component.spec.ts b/libs/angular/src/auth/components/remove-password.component.spec.ts index 5e9d823525a..00a90db9169 100644 --- a/libs/angular/src/auth/components/remove-password.component.spec.ts +++ b/libs/angular/src/auth/components/remove-password.component.spec.ts @@ -3,7 +3,7 @@ import { mock } from "jest-mock-extended"; import { OrganizationApiServiceAbstraction } from "@bitwarden/common/admin-console/abstractions/organization/organization-api.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; -import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { KeyConnectorService } from "@bitwarden/common/key-management/key-connector/abstractions/key-connector.service"; import { ErrorResponse } from "@bitwarden/common/models/response/error.response"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; From 0569b2243bd2c106035a5e8dccaea1626a7b67d3 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Thu, 13 Mar 2025 16:54:27 +0000 Subject: [PATCH 14/25] missing loading in remove password component for browser extension --- apps/browser/src/auth/popup/remove-password.component.html | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/apps/browser/src/auth/popup/remove-password.component.html b/apps/browser/src/auth/popup/remove-password.component.html index 22e31c8241c..272f727a7bb 100644 --- a/apps/browser/src/auth/popup/remove-password.component.html +++ b/apps/browser/src/auth/popup/remove-password.component.html @@ -8,7 +8,12 @@
-
+
+
+ +
+
+

{{ "convertOrganizationEncryptionDesc" | i18n: organization.name }}

From 285a61a997548361fb40b12c7f5f0d14601002d6 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Thu, 13 Mar 2025 17:06:23 +0000 Subject: [PATCH 15/25] error handling in remove password component --- .../remove-password.component.spec.ts | 41 ++++++++++++++++++- .../components/remove-password.component.ts | 37 +++++++++-------- 2 files changed, 58 insertions(+), 20 deletions(-) diff --git a/libs/angular/src/auth/components/remove-password.component.spec.ts b/libs/angular/src/auth/components/remove-password.component.spec.ts index 00a90db9169..973f5e7c0f1 100644 --- a/libs/angular/src/auth/components/remove-password.component.spec.ts +++ b/libs/angular/src/auth/components/remove-password.component.spec.ts @@ -129,7 +129,25 @@ describe("RemovePasswordComponent", () => { expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); - it("should handle response errors and show error toast", async () => { + it("should handle errors and show error toast", async () => { + const errorMessage = "Can't migrate user error"; + mockKeyConnectorService.migrateUser.mockRejectedValue(new Error(errorMessage)); + mockI18nService.t.mockReturnValue("error occurred"); + + await component.convert(); + + expect(component.continuing).toBe(false); + expect(mockKeyConnectorService.migrateUser).toHaveBeenCalledWith(userId); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "error", + title: "error occurred", + message: errorMessage, + }); + expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + + it("should handle error response and show error toast", async () => { const errorMessage = "Can't migrate user error"; mockKeyConnectorService.migrateUser.mockRejectedValue( new ErrorResponse( @@ -178,7 +196,26 @@ describe("RemovePasswordComponent", () => { expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); - it("should handle response errors and show error toast", async () => { + it("should handle error response and show error toast", async () => { + const errorMessage = "Can't leave organization error"; + mockDialogService.openSimpleDialog.mockResolvedValue(true); + mockOrganizationApiService.leave.mockRejectedValue(new Error(errorMessage)); + mockI18nService.t.mockReturnValue("error occurred"); + + await component.leave(); + + expect(component.leaving).toBe(false); + expect(mockOrganizationApiService.leave).toHaveBeenCalledWith(organization.id); + expect(mockToastService.showToast).toHaveBeenCalledWith({ + variant: "error", + title: "error occurred", + message: errorMessage, + }); + expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + + it("should handle error response and show error toast", async () => { const errorMessage = "Can't leave organization error"; mockDialogService.openSimpleDialog.mockResolvedValue(true); mockOrganizationApiService.leave.mockRejectedValue( diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index 2ecf49701b5..ca04dd24cdc 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -61,7 +61,7 @@ export class RemovePasswordComponent implements OnInit { return this.continuing || this.leaving; } - convert = async () => { + async convert() { this.continuing = true; try { @@ -77,17 +77,11 @@ export class RemovePasswordComponent implements OnInit { } catch (e) { this.continuing = false; - if (e instanceof ErrorResponse) { - this.toastService.showToast({ - variant: "error", - title: this.i18nService.t("errorOccurred"), - message: e.message, - }); - } + this.handleActionError(e); } - }; + } - leave = async () => { + async leave() { const confirmed = await this.dialogService.openSimpleDialog({ title: this.organization.name, content: { key: "leaveOrganizationConfirmation" }, @@ -112,13 +106,20 @@ export class RemovePasswordComponent implements OnInit { } catch (e) { this.leaving = false; - if (e instanceof ErrorResponse) { - this.toastService.showToast({ - variant: "error", - title: this.i18nService.t("errorOccurred"), - message: e.message, - }); - } + this.handleActionError(e); } - }; + } + + handleActionError(e: unknown) { + let message = ""; + if (e instanceof ErrorResponse || e instanceof Error) { + message = e.message; + } + + this.toastService.showToast({ + variant: "error", + title: this.i18nService.t("errorOccurred"), + message: message, + }); + } } From c2b103dc18c3498615690429d58a7e5309985e50 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Wed, 19 Mar 2025 22:48:00 +0000 Subject: [PATCH 16/25] organization observable race condition in key-connector --- .../abstractions/key-connector.service.ts | 2 +- .../services/key-connector.service.ts | 24 +++++++++++-------- .../src/platform/sync/default-sync.service.ts | 13 ++++++---- 3 files changed, 23 insertions(+), 16 deletions(-) diff --git a/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts b/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts index 17b2b4d91c9..e7f6d54b070 100644 --- a/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts @@ -11,7 +11,7 @@ export abstract class KeyConnectorService { abstract migrateUser(userId: UserId): Promise; - abstract userNeedsMigration(userId: UserId): Promise; + abstract userNeedsMigration(userId: UserId, organizations: Organization[]): Promise; abstract convertNewSsoUserToKeyConnector( tokenResponse: IdentityTokenResponse, diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.ts index 5c21a4b618c..8871369dc62 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts @@ -82,9 +82,9 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { return firstValueFrom(this.stateProvider.getUserState$(USES_KEY_CONNECTOR, userId)); } - async userNeedsMigration(userId: UserId) { + async userNeedsMigration(userId: UserId, organizations: Organization[]) { const loggedInUsingSso = await this.tokenService.getIsExternal(userId); - const requiredByOrganization = (await this.getManagingOrganization(userId)) != null; + const requiredByOrganization = this.findManagingOrganization(organizations) != null; const userIsNotUsingKeyConnector = !(await this.getUsesKeyConnector(userId)); return loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; @@ -120,14 +120,8 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { } async getManagingOrganization(userId: UserId): Promise { - const orgs = await firstValueFrom(this.organizationService.organizations$(userId)); - return orgs.find( - (o) => - o.keyConnectorEnabled && - o.type !== OrganizationUserType.Admin && - o.type !== OrganizationUserType.Owner && - !o.isProviderUser, - ); + const organizations = await firstValueFrom(this.organizationService.organizations$(userId)); + return this.findManagingOrganization(organizations); } async convertNewSsoUserToKeyConnector( @@ -203,4 +197,14 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { } throw new Error("Key Connector error"); } + + private findManagingOrganization(organizations: Organization[]) { + return organizations.find( + (o) => + o.keyConnectorEnabled && + o.type !== OrganizationUserType.Admin && + o.type !== OrganizationUserType.Owner && + !o.isProviderUser, + ); + } } diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index 656267d864c..dd4d7b82b49 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -7,6 +7,7 @@ import { CollectionData, CollectionDetailsResponse, } from "@bitwarden/admin-console/common"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { KeyService } from "@bitwarden/key-management"; // FIXME: remove `src` and fix import @@ -213,15 +214,13 @@ export class DefaultSyncService extends CoreSyncService { await this.providerService.save(providers, response.id); - await this.syncProfileOrganizations(response, response.id); + const organizations = await this.syncProfileOrganizations(response, response.id); - if (await this.keyConnectorService.userNeedsMigration(response.id)) { + if (await this.keyConnectorService.userNeedsMigration(response.id, organizations)) { await this.keyConnectorService.setConvertAccountRequired(true, response.id); this.messageSender.send("convertAccountToKeyConnector"); } else { - // 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.keyConnectorService.removeConvertAccountRequired(response.id); + await this.keyConnectorService.removeConvertAccountRequired(response.id); } } @@ -293,6 +292,10 @@ export class DefaultSyncService extends CoreSyncService { }); await this.organizationService.replace(organizations, userId); + + return Object.values(organizations).map( + (organizationData) => new Organization(organizationData), + ); } private async syncFolders(response: FolderResponse[], userId: UserId) { From 3d088fcdc6296fd73e63c818c118705030e01005 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 09:07:36 +0000 Subject: [PATCH 17/25] usesKeyConnector always returns boolean --- .../key-connector/services/key-connector.service.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.ts index 8871369dc62..a593566a0c5 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts @@ -78,8 +78,10 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.stateProvider.getUser(userId, USES_KEY_CONNECTOR).update(() => usesKeyConnector); } - getUsesKeyConnector(userId: UserId): Promise { - return firstValueFrom(this.stateProvider.getUserState$(USES_KEY_CONNECTOR, userId)); + async getUsesKeyConnector(userId: UserId): Promise { + return ( + (await firstValueFrom(this.stateProvider.getUserState$(USES_KEY_CONNECTOR, userId))) ?? false + ); } async userNeedsMigration(userId: UserId, organizations: Organization[]) { From 1bda7d44fb07832cb5d408564f53bbd730570d78 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 09:07:52 +0000 Subject: [PATCH 18/25] unit test coverage --- .../services/key-connector.service.spec.ts | 208 +++++++++++++++--- 1 file changed, 172 insertions(+), 36 deletions(-) diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts index 44ced294ee2..bf2ea5f2e0f 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts @@ -1,7 +1,8 @@ import { mock } from "jest-mock-extended"; -import { of } from "rxjs"; +import { firstValueFrom, of } from "rxjs"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { KeyService } from "@bitwarden/key-management"; import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../../spec"; @@ -72,27 +73,82 @@ describe("KeyConnectorService", () => { expect(keyConnectorService).not.toBeFalsy(); }); - describe("setUsesKeyConnector()", () => { - it("should update the usesKeyConnectorState with the provided value", async () => { - const state = stateProvider.activeUser.getFake(USES_KEY_CONNECTOR); + describe("setUsesKeyConnector", () => { + it("should update the usesKeyConnectorState with the provided false value", async () => { + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); state.nextState(false); - const newValue = true; + await keyConnectorService.setUsesKeyConnector(true, mockUserId); - await keyConnectorService.setUsesKeyConnector(newValue, mockUserId); + expect(await firstValueFrom(state.state$)).toBe(true); + }); - expect(await keyConnectorService.getUsesKeyConnector(mockUserId)).toBe(newValue); + it("should update the usesKeyConnectorState with the provided true value", async () => { + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); + state.nextState(true); + + await keyConnectorService.setUsesKeyConnector(false, mockUserId); + + expect(await firstValueFrom(state.state$)).toBe(false); }); }); - describe("getManagingOrganization()", () => { + describe("getUsesKeyConnector", () => { + it("should return false when uses key connector state is not set", async () => { + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); + state.nextState(null); + + const usesKeyConnector = await keyConnectorService.getUsesKeyConnector(mockUserId); + + expect(usesKeyConnector).toEqual(false); + }); + + it("should return false when uses key connector state is set to false", async () => { + stateProvider.getUserState$(USES_KEY_CONNECTOR, mockUserId); + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); + state.nextState(false); + + const usesKeyConnector = await keyConnectorService.getUsesKeyConnector(mockUserId); + + expect(usesKeyConnector).toEqual(false); + }); + + it("should return true when uses key connector state is set to true", async () => { + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); + state.nextState(true); + + const usesKeyConnector = await keyConnectorService.getUsesKeyConnector(mockUserId); + + expect(usesKeyConnector).toEqual(true); + }); + }); + + describe("getManagingOrganization", () => { it("should return the managing organization with key connector enabled", async () => { // Arrange const orgs = [ - organizationData(true, true, "https://key-connector-url.com", 2, false), - organizationData(false, true, "https://key-connector-url.com", 2, false), - organizationData(true, false, "https://key-connector-url.com", 2, false), - organizationData(true, true, "https://other-url.com", 2, false), + organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ), + organizationData( + false, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ), + organizationData( + true, + false, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ), + organizationData(true, true, "https://other-url.com", OrganizationUserType.User, false), ]; organizationService.organizations$.mockReturnValue(of(orgs)); @@ -106,8 +162,20 @@ describe("KeyConnectorService", () => { it("should return undefined if no managing organization with key connector enabled is found", async () => { // Arrange const orgs = [ - organizationData(true, false, "https://key-connector-url.com", 2, false), - organizationData(false, false, "https://key-connector-url.com", 2, false), + organizationData( + true, + false, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ), + organizationData( + false, + false, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ), ]; organizationService.organizations$.mockReturnValue(of(orgs)); @@ -136,8 +204,20 @@ describe("KeyConnectorService", () => { it("should return undefined if user is a Provider", async () => { // Arrange const orgs = [ - organizationData(true, true, "https://key-connector-url.com", 2, true), - organizationData(false, true, "https://key-connector-url.com", 2, true), + organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + true, + ), + organizationData( + false, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + true, + ), ]; organizationService.organizations$.mockReturnValue(of(orgs)); @@ -149,7 +229,7 @@ describe("KeyConnectorService", () => { }); }); - describe("setConvertAccountRequired()", () => { + describe("setConvertAccountRequired", () => { it("should update the convertAccountToKeyConnectorState with the provided value", async () => { const state = stateProvider.activeUser.getFake(CONVERT_ACCOUNT_TO_KEY_CONNECTOR); state.nextState(false); @@ -158,7 +238,7 @@ describe("KeyConnectorService", () => { await keyConnectorService.setConvertAccountRequired(newValue, mockUserId); - expect(await keyConnectorService.getConvertAccountRequired()).toBe(newValue); + expect(await firstValueFrom(state.state$)).toBe(newValue); }); it("should remove the convertAccountToKeyConnectorState", async () => { @@ -169,36 +249,83 @@ describe("KeyConnectorService", () => { await keyConnectorService.setConvertAccountRequired(newValue, mockUserId); - expect(await keyConnectorService.getConvertAccountRequired()).toBe(newValue); + expect(await firstValueFrom(state.state$)).toBe(newValue); }); }); - describe("userNeedsMigration()", () => { + describe("userNeedsMigration", () => { it("should return true if the user needs migration", async () => { // token tokenService.getIsExternal.mockResolvedValue(true); // create organization object - const data = organizationData(true, true, "https://key-connector-url.com", 2, false); - organizationService.organizations$.mockReturnValue(of([data])); + const data = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); // uses KeyConnector - const state = stateProvider.activeUser.getFake(USES_KEY_CONNECTOR); + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); state.nextState(false); - const result = await keyConnectorService.userNeedsMigration(mockUserId); + const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); expect(result).toBe(true); }); - it("should return false if the user does not need migration", async () => { + it("should return false when not logged in with sso", async () => { tokenService.getIsExternal.mockResolvedValue(false); - const data = organizationData(false, false, "https://key-connector-url.com", 2, false); - organizationService.organizations$.mockReturnValue(of([data])); + const data = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); - const state = stateProvider.activeUser.getFake(USES_KEY_CONNECTOR); + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); + state.nextState(false); + + const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); + + expect(result).toBe(false); + }); + + it("should return false when key connector not enabled in organization", async () => { + tokenService.getIsExternal.mockResolvedValue(true); + const data = organizationData( + true, + false, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); + + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); + state.nextState(false); + + const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); + + expect(result).toBe(false); + }); + + it("should return false when user does not need key connector", async () => { + tokenService.getIsExternal.mockResolvedValue(true); + const data = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); + + const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); state.nextState(true); - const result = await keyConnectorService.userNeedsMigration(mockUserId); + + const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); expect(result).toBe(false); }); @@ -220,10 +347,7 @@ describe("KeyConnectorService", () => { // Assert expect(apiService.getMasterKeyFromKeyConnector).toHaveBeenCalledWith(url); - expect(masterPasswordService.mock.setMasterKey).toHaveBeenCalledWith( - masterKey, - expect.any(String), - ); + expect(masterPasswordService.mock.setMasterKey).toHaveBeenCalledWith(masterKey, mockUserId); }); it("should handle errors thrown during the process", async () => { @@ -245,10 +369,16 @@ describe("KeyConnectorService", () => { }); }); - describe("migrateUser()", () => { + describe("migrateUser", () => { it("should migrate the user to the key connector", async () => { // Arrange - const organization = organizationData(true, true, "https://key-connector-url.com", 2, false); + const organization = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); const masterKey = getMockMasterKey(); masterPasswordService.masterKeySubject.next(masterKey); const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64); @@ -270,7 +400,13 @@ describe("KeyConnectorService", () => { it("should handle errors thrown during migration", async () => { // Arrange - const organization = organizationData(true, true, "https://key-connector-url.com", 2, false); + const organization = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); const masterKey = getMockMasterKey(); const keyConnectorRequest = new KeyConnectorUserKeyRequest(masterKey.encKeyB64); const error = new Error("Failed to post user key to key connector"); From 60dc468814744217966b2c39537fdedc457b0dc2 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 11:16:51 +0000 Subject: [PATCH 19/25] key connector reactive --- .../browser/src/background/main.background.ts | 2 ++ .../service-container/service-container.ts | 2 ++ .../src/services/jslib-services.module.ts | 2 ++ .../services/key-connector.service.spec.ts | 4 +++ .../services/key-connector.service.ts | 36 +++++++++++++++++-- .../src/platform/sync/default-sync.service.ts | 14 +------- 6 files changed, 45 insertions(+), 15 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index d427c2f4bd6..ac27caa1e23 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -733,6 +733,7 @@ export default class MainBackground { this.badgeSettingsService = new BadgeSettingsService(this.stateProvider); this.policyApiService = new PolicyApiService(this.policyService, this.apiService); this.keyConnectorService = new KeyConnectorService( + this.accountService, this.masterPasswordService, this.keyService, this.apiService, @@ -742,6 +743,7 @@ export default class MainBackground { this.keyGenerationService, logoutCallback, this.stateProvider, + this.messagingService, ); const sdkClientFactory = flagEnabled("sdk") diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index 7fb6782e7e9..131866d722f 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -563,6 +563,7 @@ export class ServiceContainer { this.policyApiService = new PolicyApiService(this.policyService, this.apiService); this.keyConnectorService = new KeyConnectorService( + this.accountService, this.masterPasswordService, this.keyService, this.apiService, @@ -572,6 +573,7 @@ export class ServiceContainer { this.keyGenerationService, logoutCallback, this.stateProvider, + this.messagingService, ); this.twoFactorService = new TwoFactorService( diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index b8a943edbc0..887efd74059 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -977,6 +977,7 @@ const safeProviders: SafeProvider[] = [ provide: KeyConnectorServiceAbstraction, useClass: KeyConnectorService, deps: [ + AccountServiceAbstraction, InternalMasterPasswordServiceAbstraction, KeyService, ApiServiceAbstraction, @@ -986,6 +987,7 @@ const safeProviders: SafeProvider[] = [ KeyGenerationServiceAbstraction, LOGOUT_CALLBACK, StateProvider, + MessagingServiceAbstraction, ], }), safeProvider({ diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts index bf2ea5f2e0f..21df1cbbd69 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts @@ -3,6 +3,7 @@ import { firstValueFrom, of } from "rxjs"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { KeyService } from "@bitwarden/key-management"; import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../../spec"; @@ -36,6 +37,7 @@ describe("KeyConnectorService", () => { const logService = mock(); const organizationService = mock(); const keyGenerationService = mock(); + const messagingService = mock(); let stateProvider: FakeStateProvider; @@ -57,6 +59,7 @@ describe("KeyConnectorService", () => { stateProvider = new FakeStateProvider(accountService); keyConnectorService = new KeyConnectorService( + accountService, masterPasswordService, keyService, apiService, @@ -66,6 +69,7 @@ describe("KeyConnectorService", () => { keyGenerationService, async () => {}, stateProvider, + messagingService, ); }); diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.ts index a593566a0c5..e5da277f556 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts @@ -1,8 +1,10 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { firstValueFrom } from "rxjs"; +import { combineLatest, filter, firstValueFrom, of, switchMap } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { Argon2KdfConfig, KdfConfig, @@ -58,6 +60,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private convertAccountToKeyConnectorState: ActiveUserState; constructor( + accountService: AccountService, private masterPasswordService: InternalMasterPasswordServiceAbstraction, private keyService: KeyService, private apiService: ApiService, @@ -67,11 +70,40 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private keyGenerationService: KeyGenerationService, private logoutCallback: (logoutReason: LogoutReason, userId?: string) => Promise, private stateProvider: StateProvider, + private messagingService: MessagingService, ) { this.usesKeyConnectorState = this.stateProvider.getActive(USES_KEY_CONNECTOR); this.convertAccountToKeyConnectorState = this.stateProvider.getActive( CONVERT_ACCOUNT_TO_KEY_CONNECTOR, ); + + accountService.activeAccount$ + .pipe( + filter((account) => account != null), + switchMap((account) => + combineLatest([ + of(account.id), + this.organizationService + .organizations$(account.id) + .pipe(filter((organizations) => organizations != null)), + tokenService.hasAccessToken$(account.id).pipe(filter((hasToken) => hasToken)), + ]), + ), + switchMap(async ([userId, organizations]) => { + const needsMigration = await this.userNeedsMigration(userId, organizations); + if (needsMigration) { + await this.setConvertAccountRequired(true, userId); + } else { + await this.removeConvertAccountRequired(userId); + } + return needsMigration; + }), + ) + .subscribe((needsMigration) => { + if (needsMigration) { + this.messagingService.send("convertAccountToKeyConnector"); + } + }); } async setUsesKeyConnector(usesKeyConnector: boolean, userId: UserId) { @@ -84,7 +116,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { ); } - async userNeedsMigration(userId: UserId, organizations: Organization[]) { + async userNeedsMigration(userId: UserId, organizations: Organization[]): Promise { const loggedInUsingSso = await this.tokenService.getIsExternal(userId); const requiredByOrganization = this.findManagingOrganization(organizations) != null; const userIsNotUsingKeyConnector = !(await this.getUsesKeyConnector(userId)); diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index dd4d7b82b49..8b551678e2e 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -7,7 +7,6 @@ import { CollectionData, CollectionDetailsResponse, } from "@bitwarden/admin-console/common"; -import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { KeyService } from "@bitwarden/key-management"; // FIXME: remove `src` and fix import @@ -214,14 +213,7 @@ export class DefaultSyncService extends CoreSyncService { await this.providerService.save(providers, response.id); - const organizations = await this.syncProfileOrganizations(response, response.id); - - if (await this.keyConnectorService.userNeedsMigration(response.id, organizations)) { - await this.keyConnectorService.setConvertAccountRequired(true, response.id); - this.messageSender.send("convertAccountToKeyConnector"); - } else { - await this.keyConnectorService.removeConvertAccountRequired(response.id); - } + await this.syncProfileOrganizations(response, response.id); } private async setForceSetPasswordReasonIfNeeded(profileResponse: ProfileResponse) { @@ -292,10 +284,6 @@ export class DefaultSyncService extends CoreSyncService { }); await this.organizationService.replace(organizations, userId); - - return Object.values(organizations).map( - (organizationData) => new Organization(organizationData), - ); } private async syncFolders(response: FolderResponse[], userId: UserId) { From e1f8450ed621adf32eabfc2242bda46e76b23d47 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 17:06:11 +0000 Subject: [PATCH 20/25] reactive key connector service --- .../convert-to-key-connector.command.ts | 3 -- .../components/remove-password.component.ts | 11 +++-- .../abstractions/key-connector.service.ts | 4 -- .../services/key-connector.service.ts | 44 ++++++++++--------- 4 files changed, 28 insertions(+), 34 deletions(-) diff --git a/apps/cli/src/commands/convert-to-key-connector.command.ts b/apps/cli/src/commands/convert-to-key-connector.command.ts index 2f1f47a087c..3ba5ec662e4 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.ts @@ -67,9 +67,6 @@ export class ConvertToKeyConnectorCommand { throw e; } - await this.keyConnectorService.removeConvertAccountRequired(this.userId); - await this.keyConnectorService.setUsesKeyConnector(true, this.userId); - // Update environment URL - required for api key login const env = await firstValueFrom(this.environmentService.environment$); const urls = env.getUrls(); diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index ca04dd24cdc..b7d03dcff30 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -61,7 +61,7 @@ export class RemovePasswordComponent implements OnInit { return this.continuing || this.leaving; } - async convert() { + convert = async () => { this.continuing = true; try { @@ -71,7 +71,6 @@ export class RemovePasswordComponent implements OnInit { variant: "success", message: this.i18nService.t("removedMasterPassword"), }); - await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId); await this.router.navigate([""]); } catch (e) { @@ -79,9 +78,9 @@ export class RemovePasswordComponent implements OnInit { this.handleActionError(e); } - } + }; - async leave() { + leave = async () => { const confirmed = await this.dialogService.openSimpleDialog({ title: this.organization.name, content: { key: "leaveOrganizationConfirmation" }, @@ -95,12 +94,12 @@ export class RemovePasswordComponent implements OnInit { this.leaving = true; try { await this.organizationApiService.leave(this.organization.id); + await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId); this.toastService.showToast({ variant: "success", message: this.i18nService.t("leftOrganization"), }); - await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId); await this.router.navigate([""]); } catch (e) { @@ -108,7 +107,7 @@ export class RemovePasswordComponent implements OnInit { this.handleActionError(e); } - } + }; handleActionError(e: unknown) { let message = ""; diff --git a/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts b/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts index e7f6d54b070..cdfd708c244 100644 --- a/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts @@ -11,8 +11,6 @@ export abstract class KeyConnectorService { abstract migrateUser(userId: UserId): Promise; - abstract userNeedsMigration(userId: UserId, organizations: Organization[]): Promise; - abstract convertNewSsoUserToKeyConnector( tokenResponse: IdentityTokenResponse, orgId: string, @@ -21,8 +19,6 @@ export abstract class KeyConnectorService { abstract setUsesKeyConnector(enabled: boolean, userId: UserId): Promise; - abstract setConvertAccountRequired(status: boolean | null, userId: UserId): Promise; - abstract getConvertAccountRequired(): Promise; abstract removeConvertAccountRequired(userId: UserId): Promise; diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.ts index e5da277f556..0f3b49fb029 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts @@ -1,6 +1,6 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { combineLatest, filter, firstValueFrom, of, switchMap } from "rxjs"; +import { combineLatest, distinctUntilChanged, filter, firstValueFrom, of, switchMap } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -43,6 +43,7 @@ export const USES_KEY_CONNECTOR = new UserKeyDefinition( { deserializer: (usesKeyConnector) => usesKeyConnector, clearOn: ["logout"], + cleanupDelayMs: 0, }, ); @@ -52,6 +53,7 @@ export const CONVERT_ACCOUNT_TO_KEY_CONNECTOR = new UserKeyDefinition convertAccountToKeyConnector, clearOn: ["logout"], + cleanupDelayMs: 0, }, ); @@ -86,24 +88,29 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { this.organizationService .organizations$(account.id) .pipe(filter((organizations) => organizations != null)), + this.stateProvider + .getUserState$(USES_KEY_CONNECTOR, account.id) + .pipe(filter((usesKeyConnector) => usesKeyConnector != null)), tokenService.hasAccessToken$(account.id).pipe(filter((hasToken) => hasToken)), ]), ), - switchMap(async ([userId, organizations]) => { - const needsMigration = await this.userNeedsMigration(userId, organizations); + distinctUntilChanged(), + switchMap(async ([userId, organizations, usesKeyConnector]) => { + const loggedInUsingSso = await this.tokenService.getIsExternal(userId); + const requiredByOrganization = this.findManagingOrganization(organizations) != null; + const userIsNotUsingKeyConnector = !usesKeyConnector; + + const needsMigration = + loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; if (needsMigration) { await this.setConvertAccountRequired(true, userId); + this.messagingService.send("convertAccountToKeyConnector"); } else { await this.removeConvertAccountRequired(userId); } - return needsMigration; }), ) - .subscribe((needsMigration) => { - if (needsMigration) { - this.messagingService.send("convertAccountToKeyConnector"); - } - }); + .subscribe(); } async setUsesKeyConnector(usesKeyConnector: boolean, userId: UserId) { @@ -116,14 +123,6 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { ); } - async userNeedsMigration(userId: UserId, organizations: Organization[]): Promise { - const loggedInUsingSso = await this.tokenService.getIsExternal(userId); - const requiredByOrganization = this.findManagingOrganization(organizations) != null; - const userIsNotUsingKeyConnector = !(await this.getUsesKeyConnector(userId)); - - return loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; - } - async migrateUser(userId: UserId) { const organization = await this.getManagingOrganization(userId); const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId)); @@ -139,6 +138,9 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { } await this.apiService.postConvertToKeyConnector(); + + await this.setUsesKeyConnector(true, userId); + await this.removeConvertAccountRequired(userId); } // TODO: UserKey should be renamed to MasterKey and typed accordingly @@ -210,10 +212,6 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postSetKeyConnectorKey(setPasswordRequest); } - async setConvertAccountRequired(status: boolean | null, userId: UserId) { - await this.stateProvider.setUserState(CONVERT_ACCOUNT_TO_KEY_CONNECTOR, status, userId); - } - getConvertAccountRequired(): Promise { return firstValueFrom(this.convertAccountToKeyConnectorState.state$); } @@ -222,6 +220,10 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.setConvertAccountRequired(null, userId); } + private async setConvertAccountRequired(status: boolean | null, userId: UserId) { + await this.stateProvider.setUserState(CONVERT_ACCOUNT_TO_KEY_CONNECTOR, status, userId); + } + private handleKeyConnectorError(e: any) { this.logService.error(e); if (this.logoutCallback != null) { From 7d1bdb8fe0c8a059828f5d765641be6c9af63b8a Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 19:07:48 +0000 Subject: [PATCH 21/25] introducing convertAccountRequired$ --- apps/cli/src/auth/commands/unlock.command.ts | 2 +- .../convert-to-key-connector.command.ts | 1 - .../components/remove-password.component.ts | 1 - libs/angular/src/auth/guards/auth.guard.ts | 2 +- .../abstractions/key-connector.service.ts | 6 +- .../services/key-connector.service.ts | 106 +++++++----------- 6 files changed, 43 insertions(+), 75 deletions(-) diff --git a/apps/cli/src/auth/commands/unlock.command.ts b/apps/cli/src/auth/commands/unlock.command.ts index 6fe0d7faff6..502355d5786 100644 --- a/apps/cli/src/auth/commands/unlock.command.ts +++ b/apps/cli/src/auth/commands/unlock.command.ts @@ -71,7 +71,7 @@ export class UnlockCommand { const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey, userId); await this.keyService.setUserKey(userKey, userId); - if (await this.keyConnectorService.getConvertAccountRequired()) { + if (await firstValueFrom(this.keyConnectorService.convertAccountRequired$)) { const convertToKeyConnectorCommand = new ConvertToKeyConnectorCommand( userId, this.keyConnectorService, diff --git a/apps/cli/src/commands/convert-to-key-connector.command.ts b/apps/cli/src/commands/convert-to-key-connector.command.ts index 3ba5ec662e4..481c23fc5a0 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.ts @@ -76,7 +76,6 @@ export class ConvertToKeyConnectorCommand { return Response.success(); } else if (answer.convert === "leave") { await this.organizationApiService.leave(organization.id); - await this.keyConnectorService.removeConvertAccountRequired(this.userId); return Response.success(); } else { await this.logout(); diff --git a/libs/angular/src/auth/components/remove-password.component.ts b/libs/angular/src/auth/components/remove-password.component.ts index b7d03dcff30..bc94f7d1177 100644 --- a/libs/angular/src/auth/components/remove-password.component.ts +++ b/libs/angular/src/auth/components/remove-password.component.ts @@ -94,7 +94,6 @@ export class RemovePasswordComponent implements OnInit { this.leaving = true; try { await this.organizationApiService.leave(this.organization.id); - await this.keyConnectorService.removeConvertAccountRequired(this.activeUserId); this.toastService.showToast({ variant: "success", diff --git a/libs/angular/src/auth/guards/auth.guard.ts b/libs/angular/src/auth/guards/auth.guard.ts index 329e365e542..690db37d090 100644 --- a/libs/angular/src/auth/guards/auth.guard.ts +++ b/libs/angular/src/auth/guards/auth.guard.ts @@ -47,7 +47,7 @@ export const authGuard: CanActivateFn = async ( if ( !routerState.url.includes("remove-password") && - (await keyConnectorService.getConvertAccountRequired()) + (await firstValueFrom(keyConnectorService.convertAccountRequired$)) ) { return router.createUrlTree(["/remove-password"]); } diff --git a/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts b/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts index cdfd708c244..131b941f274 100644 --- a/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/abstractions/key-connector.service.ts @@ -1,3 +1,5 @@ +import { Observable } from "rxjs"; + import { Organization } from "../../../admin-console/models/domain/organization"; import { IdentityTokenResponse } from "../../../auth/models/response/identity-token.response"; import { UserId } from "../../../types/guid"; @@ -19,7 +21,5 @@ export abstract class KeyConnectorService { abstract setUsesKeyConnector(enabled: boolean, userId: UserId): Promise; - abstract getConvertAccountRequired(): Promise; - - abstract removeConvertAccountRequired(userId: UserId): Promise; + abstract convertAccountRequired$: Observable; } diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.ts index 0f3b49fb029..9332aec2601 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts @@ -1,6 +1,14 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { combineLatest, distinctUntilChanged, filter, firstValueFrom, of, switchMap } from "rxjs"; +import { + combineLatest, + distinctUntilChanged, + filter, + firstValueFrom, + Observable, + of, + switchMap, +} from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -24,12 +32,7 @@ import { KeyGenerationService } from "../../../platform/abstractions/key-generat import { LogService } from "../../../platform/abstractions/log.service"; import { Utils } from "../../../platform/misc/utils"; import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key"; -import { - ActiveUserState, - KEY_CONNECTOR_DISK, - StateProvider, - UserKeyDefinition, -} from "../../../platform/state"; +import { KEY_CONNECTOR_DISK, StateProvider, UserKeyDefinition } from "../../../platform/state"; import { UserId } from "../../../types/guid"; import { MasterKey } from "../../../types/key"; import { InternalMasterPasswordServiceAbstraction } from "../../master-password/abstractions/master-password.service.abstraction"; @@ -47,19 +50,8 @@ export const USES_KEY_CONNECTOR = new UserKeyDefinition( }, ); -export const CONVERT_ACCOUNT_TO_KEY_CONNECTOR = new UserKeyDefinition( - KEY_CONNECTOR_DISK, - "convertAccountToKeyConnector", - { - deserializer: (convertAccountToKeyConnector) => convertAccountToKeyConnector, - clearOn: ["logout"], - cleanupDelayMs: 0, - }, -); - export class KeyConnectorService implements KeyConnectorServiceAbstraction { - private usesKeyConnectorState: ActiveUserState; - private convertAccountToKeyConnectorState: ActiveUserState; + readonly convertAccountRequired$: Observable; constructor( accountService: AccountService, @@ -74,43 +66,34 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private stateProvider: StateProvider, private messagingService: MessagingService, ) { - this.usesKeyConnectorState = this.stateProvider.getActive(USES_KEY_CONNECTOR); - this.convertAccountToKeyConnectorState = this.stateProvider.getActive( - CONVERT_ACCOUNT_TO_KEY_CONNECTOR, + this.convertAccountRequired$ = accountService.activeAccount$.pipe( + filter((account) => account != null), + switchMap((account) => + combineLatest([ + of(account.id), + this.organizationService + .organizations$(account.id) + .pipe(filter((organizations) => organizations != null)), + this.stateProvider + .getUserState$(USES_KEY_CONNECTOR, account.id) + .pipe(filter((usesKeyConnector) => usesKeyConnector != null)), + tokenService.hasAccessToken$(account.id).pipe(filter((hasToken) => hasToken)), + ]), + ), + distinctUntilChanged(), + switchMap(async ([userId, organizations, usesKeyConnector]) => { + const loggedInUsingSso = await this.tokenService.getIsExternal(userId); + const requiredByOrganization = this.findManagingOrganization(organizations) != null; + const userIsNotUsingKeyConnector = !usesKeyConnector; + + const needsMigration = + loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; + if (needsMigration) { + this.messagingService.send("convertAccountToKeyConnector"); + } + return needsMigration; + }), ); - - accountService.activeAccount$ - .pipe( - filter((account) => account != null), - switchMap((account) => - combineLatest([ - of(account.id), - this.organizationService - .organizations$(account.id) - .pipe(filter((organizations) => organizations != null)), - this.stateProvider - .getUserState$(USES_KEY_CONNECTOR, account.id) - .pipe(filter((usesKeyConnector) => usesKeyConnector != null)), - tokenService.hasAccessToken$(account.id).pipe(filter((hasToken) => hasToken)), - ]), - ), - distinctUntilChanged(), - switchMap(async ([userId, organizations, usesKeyConnector]) => { - const loggedInUsingSso = await this.tokenService.getIsExternal(userId); - const requiredByOrganization = this.findManagingOrganization(organizations) != null; - const userIsNotUsingKeyConnector = !usesKeyConnector; - - const needsMigration = - loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; - if (needsMigration) { - await this.setConvertAccountRequired(true, userId); - this.messagingService.send("convertAccountToKeyConnector"); - } else { - await this.removeConvertAccountRequired(userId); - } - }), - ) - .subscribe(); } async setUsesKeyConnector(usesKeyConnector: boolean, userId: UserId) { @@ -140,7 +123,6 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postConvertToKeyConnector(); await this.setUsesKeyConnector(true, userId); - await this.removeConvertAccountRequired(userId); } // TODO: UserKey should be renamed to MasterKey and typed accordingly @@ -212,18 +194,6 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postSetKeyConnectorKey(setPasswordRequest); } - getConvertAccountRequired(): Promise { - return firstValueFrom(this.convertAccountToKeyConnectorState.state$); - } - - async removeConvertAccountRequired(userId: UserId) { - await this.setConvertAccountRequired(null, userId); - } - - private async setConvertAccountRequired(status: boolean | null, userId: UserId) { - await this.stateProvider.setUserState(CONVERT_ACCOUNT_TO_KEY_CONNECTOR, status, userId); - } - private handleKeyConnectorError(e: any) { this.logService.error(e); if (this.logoutCallback != null) { From dd623972268c220a14125d6d02386cb43ac779ce Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 19:14:30 +0000 Subject: [PATCH 22/25] cli build fix --- apps/cli/src/commands/convert-to-key-connector.command.spec.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/apps/cli/src/commands/convert-to-key-connector.command.spec.ts b/apps/cli/src/commands/convert-to-key-connector.command.spec.ts index f86fcafa551..59be187c1c1 100644 --- a/apps/cli/src/commands/convert-to-key-connector.command.spec.ts +++ b/apps/cli/src/commands/convert-to-key-connector.command.spec.ts @@ -103,8 +103,6 @@ describe("ConvertToKeyConnectorCommand", () => { expect(response).not.toBeNull(); expect(response.success).toEqual(true); expect(keyConnectorService.migrateUser).toHaveBeenCalledWith(userId); - expect(keyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); - expect(keyConnectorService.setUsesKeyConnector).toHaveBeenCalledWith(true, userId); expect(environmentService.setEnvironment).toHaveBeenCalledWith(Region.SelfHosted, { keyConnector: organization.keyConnectorUrl, } as Urls); @@ -137,7 +135,6 @@ describe("ConvertToKeyConnectorCommand", () => { expect(response).not.toBeNull(); expect(response.success).toEqual(true); expect(organizationApiService.leave).toHaveBeenCalledWith(organization.id); - expect(keyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); }); }); }); From 7bed47b6d1e96b2370798cc5be2c9d3d77a57b54 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 20:06:47 +0000 Subject: [PATCH 23/25] moving message sending side effect to sync --- .../browser/src/background/main.background.ts | 1 - .../service-container/service-container.ts | 1 - .../src/services/jslib-services.module.ts | 1 - .../services/key-connector.service.ts | 20 ++----------------- .../src/platform/sync/default-sync.service.ts | 4 ++++ 5 files changed, 6 insertions(+), 21 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index b49cea2f58f..74fa6acdf79 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -743,7 +743,6 @@ export default class MainBackground { this.keyGenerationService, logoutCallback, this.stateProvider, - this.messagingService, ); const sdkClientFactory = flagEnabled("sdk") diff --git a/apps/cli/src/service-container/service-container.ts b/apps/cli/src/service-container/service-container.ts index 131866d722f..b7f423e8ff7 100644 --- a/apps/cli/src/service-container/service-container.ts +++ b/apps/cli/src/service-container/service-container.ts @@ -573,7 +573,6 @@ export class ServiceContainer { this.keyGenerationService, logoutCallback, this.stateProvider, - this.messagingService, ); this.twoFactorService = new TwoFactorService( diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 5b1d0047c9c..9cb39a35856 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -987,7 +987,6 @@ const safeProviders: SafeProvider[] = [ KeyGenerationServiceAbstraction, LOGOUT_CALLBACK, StateProvider, - MessagingServiceAbstraction, ], }), safeProvider({ diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.ts index 9332aec2601..546b991132e 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.ts @@ -1,18 +1,9 @@ // FIXME: Update this file to be type safe and remove this and next line // @ts-strict-ignore -import { - combineLatest, - distinctUntilChanged, - filter, - firstValueFrom, - Observable, - of, - switchMap, -} from "rxjs"; +import { combineLatest, filter, firstValueFrom, Observable, of, switchMap } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; -import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { Argon2KdfConfig, KdfConfig, @@ -64,7 +55,6 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private keyGenerationService: KeyGenerationService, private logoutCallback: (logoutReason: LogoutReason, userId?: string) => Promise, private stateProvider: StateProvider, - private messagingService: MessagingService, ) { this.convertAccountRequired$ = accountService.activeAccount$.pipe( filter((account) => account != null), @@ -80,18 +70,12 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { tokenService.hasAccessToken$(account.id).pipe(filter((hasToken) => hasToken)), ]), ), - distinctUntilChanged(), switchMap(async ([userId, organizations, usesKeyConnector]) => { const loggedInUsingSso = await this.tokenService.getIsExternal(userId); const requiredByOrganization = this.findManagingOrganization(organizations) != null; const userIsNotUsingKeyConnector = !usesKeyConnector; - const needsMigration = - loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; - if (needsMigration) { - this.messagingService.send("convertAccountToKeyConnector"); - } - return needsMigration; + return loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; }), ); } diff --git a/libs/common/src/platform/sync/default-sync.service.ts b/libs/common/src/platform/sync/default-sync.service.ts index 8b551678e2e..54441514e88 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -214,6 +214,10 @@ export class DefaultSyncService extends CoreSyncService { await this.providerService.save(providers, response.id); await this.syncProfileOrganizations(response, response.id); + + if (await firstValueFrom(this.keyConnectorService.convertAccountRequired$)) { + this.messageSender.send("convertAccountToKeyConnector"); + } } private async setForceSetPasswordReasonIfNeeded(profileResponse: ProfileResponse) { From f066313354ec19e60ac4be13938edcf8e1986c09 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 20:34:58 +0000 Subject: [PATCH 24/25] key connector service unit tests --- .../services/key-connector.service.spec.ts | 243 ++++++++++-------- 1 file changed, 132 insertions(+), 111 deletions(-) diff --git a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts index 21df1cbbd69..568470725a9 100644 --- a/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts +++ b/libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts @@ -1,9 +1,8 @@ import { mock } from "jest-mock-extended"; -import { firstValueFrom, of } from "rxjs"; +import { firstValueFrom, of, timeout, TimeoutError } from "rxjs"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; -import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { KeyService } from "@bitwarden/key-management"; import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../../spec"; @@ -22,11 +21,7 @@ import { MasterKey } from "../../../types/key"; import { FakeMasterPasswordService } from "../../master-password/services/fake-master-password.service"; import { KeyConnectorUserKeyRequest } from "../models/key-connector-user-key.request"; -import { - USES_KEY_CONNECTOR, - CONVERT_ACCOUNT_TO_KEY_CONNECTOR, - KeyConnectorService, -} from "./key-connector.service"; +import { USES_KEY_CONNECTOR, KeyConnectorService } from "./key-connector.service"; describe("KeyConnectorService", () => { let keyConnectorService: KeyConnectorService; @@ -37,7 +32,6 @@ describe("KeyConnectorService", () => { const logService = mock(); const organizationService = mock(); const keyGenerationService = mock(); - const messagingService = mock(); let stateProvider: FakeStateProvider; @@ -69,7 +63,6 @@ describe("KeyConnectorService", () => { keyGenerationService, async () => {}, stateProvider, - messagingService, ); }); @@ -233,108 +226,6 @@ describe("KeyConnectorService", () => { }); }); - describe("setConvertAccountRequired", () => { - it("should update the convertAccountToKeyConnectorState with the provided value", async () => { - const state = stateProvider.activeUser.getFake(CONVERT_ACCOUNT_TO_KEY_CONNECTOR); - state.nextState(false); - - const newValue = true; - - await keyConnectorService.setConvertAccountRequired(newValue, mockUserId); - - expect(await firstValueFrom(state.state$)).toBe(newValue); - }); - - it("should remove the convertAccountToKeyConnectorState", async () => { - const state = stateProvider.activeUser.getFake(CONVERT_ACCOUNT_TO_KEY_CONNECTOR); - state.nextState(false); - - const newValue: boolean | null = null; - - await keyConnectorService.setConvertAccountRequired(newValue, mockUserId); - - expect(await firstValueFrom(state.state$)).toBe(newValue); - }); - }); - - describe("userNeedsMigration", () => { - it("should return true if the user needs migration", async () => { - // token - tokenService.getIsExternal.mockResolvedValue(true); - - // create organization object - const data = organizationData( - true, - true, - "https://key-connector-url.com", - OrganizationUserType.User, - false, - ); - - // uses KeyConnector - const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); - state.nextState(false); - - const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); - - expect(result).toBe(true); - }); - - it("should return false when not logged in with sso", async () => { - tokenService.getIsExternal.mockResolvedValue(false); - const data = organizationData( - true, - true, - "https://key-connector-url.com", - OrganizationUserType.User, - false, - ); - - const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); - state.nextState(false); - - const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); - - expect(result).toBe(false); - }); - - it("should return false when key connector not enabled in organization", async () => { - tokenService.getIsExternal.mockResolvedValue(true); - const data = organizationData( - true, - false, - "https://key-connector-url.com", - OrganizationUserType.User, - false, - ); - - const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); - state.nextState(false); - - const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); - - expect(result).toBe(false); - }); - - it("should return false when user does not need key connector", async () => { - tokenService.getIsExternal.mockResolvedValue(true); - const data = organizationData( - true, - true, - "https://key-connector-url.com", - OrganizationUserType.User, - false, - ); - - const state = stateProvider.singleUser.getFake(mockUserId, USES_KEY_CONNECTOR); - state.nextState(true); - - const result = await keyConnectorService.userNeedsMigration(mockUserId, [data]); - - expect(result).toBe(false); - }); - }); - describe("setMasterKeyFromUrl", () => { it("should set the master key from the provided URL", async () => { // Arrange @@ -436,6 +327,136 @@ describe("KeyConnectorService", () => { }); }); + describe("convertAccountRequired$", () => { + beforeEach(async () => { + const organization = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); + organizationService.organizations$.mockReturnValue(of([organization])); + await stateProvider.getUser(mockUserId, USES_KEY_CONNECTOR).update(() => false); + tokenService.getIsExternal.mockResolvedValue(true); + tokenService.hasAccessToken$.mockReturnValue(of(true)); + }); + + it("should return true when user logged in with sso, belong to organization using key connector and user does not use key connector", async () => { + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(true); + }); + + it("should return false when user logged in with password", async () => { + tokenService.getIsExternal.mockResolvedValue(false); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(false); + }); + + it("should return false when organization's key connector disabled", async () => { + const organization = organizationData( + true, + false, + "https://key-connector-url.com", + OrganizationUserType.User, + false, + ); + organizationService.organizations$.mockReturnValue(of([organization])); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(false); + }); + + it("should return false when user is admin of the organization", async () => { + const organization = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.Admin, + false, + ); + organizationService.organizations$.mockReturnValue(of([organization])); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(false); + }); + + it("should return false when user is owner of the organization", async () => { + const organization = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.Owner, + false, + ); + organizationService.organizations$.mockReturnValue(of([organization])); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(false); + }); + + it("should return false when user is provider user of the organization", async () => { + const organization = organizationData( + true, + true, + "https://key-connector-url.com", + OrganizationUserType.User, + true, + ); + organizationService.organizations$.mockReturnValue(of([organization])); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(false); + }); + + it("should return false when user already uses key connector", async () => { + await stateProvider.getUser(mockUserId, USES_KEY_CONNECTOR).update(() => true); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).resolves.toEqual(false); + }); + + it("should not return any value when user not logged in", async () => { + await accountService.switchAccount(null as unknown as UserId); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).rejects.toBeInstanceOf(TimeoutError); + }); + + it("should not return any value when organization state is empty", async () => { + organizationService.organizations$.mockReturnValue(of(null as unknown as Organization[])); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).rejects.toBeInstanceOf(TimeoutError); + }); + + it("should not return any value when user is not using key connector", async () => { + await stateProvider.getUser(mockUserId, USES_KEY_CONNECTOR).update(() => null); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).rejects.toBeInstanceOf(TimeoutError); + }); + + it("should not return any value when user does not have access token", async () => { + tokenService.hasAccessToken$.mockReturnValue(of(false)); + + await expect( + firstValueFrom(keyConnectorService.convertAccountRequired$.pipe(timeout(100))), + ).rejects.toBeInstanceOf(TimeoutError); + }); + }); + function organizationData( usesKeyConnector: boolean, keyConnectorEnabled: boolean, From af7b8d891d3a84a3f724217f4e177e2ac92a5032 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk Date: Tue, 25 Mar 2025 20:41:46 +0000 Subject: [PATCH 25/25] fix unit tests --- .../src/auth/components/remove-password.component.spec.ts | 7 ------- libs/angular/src/auth/guards/auth.guard.spec.ts | 6 ++---- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/libs/angular/src/auth/components/remove-password.component.spec.ts b/libs/angular/src/auth/components/remove-password.component.spec.ts index 973f5e7c0f1..4c4f8299659 100644 --- a/libs/angular/src/auth/components/remove-password.component.spec.ts +++ b/libs/angular/src/auth/components/remove-password.component.spec.ts @@ -121,7 +121,6 @@ describe("RemovePasswordComponent", () => { expect(component.continuing).toBe(true); expect(mockKeyConnectorService.migrateUser).toHaveBeenCalledWith(userId); - expect(mockKeyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); expect(mockToastService.showToast).toHaveBeenCalledWith({ variant: "success", message: "removed master password", @@ -143,7 +142,6 @@ describe("RemovePasswordComponent", () => { title: "error occurred", message: errorMessage, }); - expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); expect(mockRouter.navigate).not.toHaveBeenCalled(); }); @@ -168,7 +166,6 @@ describe("RemovePasswordComponent", () => { title: "error occurred", message: errorMessage, }); - expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); expect(mockRouter.navigate).not.toHaveBeenCalled(); }); }); @@ -192,7 +189,6 @@ describe("RemovePasswordComponent", () => { variant: "success", message: "left organization", }); - expect(mockKeyConnectorService.removeConvertAccountRequired).toHaveBeenCalledWith(userId); expect(mockRouter.navigate).toHaveBeenCalledWith([""]); }); @@ -211,7 +207,6 @@ describe("RemovePasswordComponent", () => { title: "error occurred", message: errorMessage, }); - expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); expect(mockRouter.navigate).not.toHaveBeenCalled(); }); @@ -237,7 +232,6 @@ describe("RemovePasswordComponent", () => { title: "error occurred", message: errorMessage, }); - expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); expect(mockRouter.navigate).not.toHaveBeenCalled(); }); @@ -248,7 +242,6 @@ describe("RemovePasswordComponent", () => { expect(component.leaving).toBe(false); expect(mockOrganizationApiService.leave).not.toHaveBeenCalled(); - expect(mockKeyConnectorService.removeConvertAccountRequired).not.toHaveBeenCalled(); expect(mockRouter.navigate).not.toHaveBeenCalled(); }); }); diff --git a/libs/angular/src/auth/guards/auth.guard.spec.ts b/libs/angular/src/auth/guards/auth.guard.spec.ts index 4ed72baf284..db08553749a 100644 --- a/libs/angular/src/auth/guards/auth.guard.spec.ts +++ b/libs/angular/src/auth/guards/auth.guard.spec.ts @@ -2,7 +2,7 @@ import { TestBed } from "@angular/core/testing"; import { Router } from "@angular/router"; import { RouterTestingModule } from "@angular/router/testing"; import { MockProxy, mock } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; +import { BehaviorSubject, of } from "rxjs"; import { EmptyComponent } from "@bitwarden/angular/platform/guard/feature-flag.guard.spec"; import { @@ -30,9 +30,7 @@ describe("AuthGuard", () => { authService.getAuthStatus.mockResolvedValue(authStatus); const messagingService: MockProxy = mock(); const keyConnectorService: MockProxy = mock(); - keyConnectorService.getConvertAccountRequired.mockResolvedValue( - keyConnectorServiceRequiresAccountConversion, - ); + keyConnectorService.convertAccountRequired$ = of(keyConnectorServiceRequiresAccountConversion); const accountService: MockProxy = mock(); const activeAccountSubject = new BehaviorSubject(null); accountService.activeAccount$ = activeAccountSubject;