From e73f902aee23f2576e2b0409df2bd8a04ac76423 Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> Date: Mon, 19 May 2025 08:51:46 +0200 Subject: [PATCH] [PM-18576] Fix missing user id on remove password (#13777) * Passed in userId on RemovePasswordComponent. * Added userId on other references to KeyConnectorService methods * remove password component refactor, test coverage, enabled strict * explicit user id provided to key connector service * redirect to / instead when user not logged in or not managing organization * key connector service explicit user id * key connector service no longer requires account service * key connector service missing null type * cli convert to key connector unit tests * remove unnecessary SyncService * error toast not showing on ErrorResponse * bad import due to merge conflict * bad import due to merge conflict * missing loading in remove password component for browser extension * error handling in remove password component * organization observable race condition in key-connector * usesKeyConnector always returns boolean * unit test coverage * key connector reactive * reactive key connector service * introducing convertAccountRequired$ * cli build fix * moving message sending side effect to sync * key connector service unit tests * fix unit tests * unit tests in wrong place after KM code ownership move * infinite page reload * failing unit tests * failing unit tests --------- Co-authored-by: Todd Martin --- .../remove-password.component.html | 16 +- apps/cli/src/auth/commands/unlock.command.ts | 5 +- apps/cli/src/base-program.ts | 1 - .../convert-to-key-connector.command.spec.ts | 140 ++++++++ .../convert-to-key-connector.command.ts | 12 +- .../src/models/response/message.response.ts | 4 +- apps/cli/src/oss-serve-configurator.ts | 1 - apps/cli/src/program.ts | 1 - .../remove-password.component.html | 14 +- .../src/auth/guards/auth.guard.spec.ts | 6 +- libs/angular/src/auth/guards/auth.guard.ts | 2 +- .../abstractions/key-connector.service.ts | 29 +- .../services/key-connector.service.spec.ts | 338 +++++++++++++----- .../services/key-connector.service.ts | 104 +++--- .../src/platform/sync/default-sync.service.ts | 7 +- .../remove-password.component.spec.ts | 252 +++++++++++++ .../remove-password.component.ts | 95 +++-- 17 files changed, 782 insertions(+), 245 deletions(-) create mode 100644 apps/cli/src/key-management/convert-to-key-connector.command.spec.ts create mode 100644 libs/key-management-ui/src/key-connector/remove-password.component.spec.ts diff --git a/apps/browser/src/key-management/key-connector/remove-password.component.html b/apps/browser/src/key-management/key-connector/remove-password.component.html index 793bcff3e0..272f727a7b 100644 --- a/apps/browser/src/key-management/key-connector/remove-password.component.html +++ b/apps/browser/src/key-management/key-connector/remove-password.component.html @@ -8,17 +8,17 @@
-
+
+
+ +
+
+

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

- - diff --git a/libs/angular/src/auth/guards/auth.guard.spec.ts b/libs/angular/src/auth/guards/auth.guard.spec.ts index 4ed72baf28..db08553749 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; diff --git a/libs/angular/src/auth/guards/auth.guard.ts b/libs/angular/src/auth/guards/auth.guard.ts index 329e365e54..690db37d09 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 b88f7bc7cd..131b941f27 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,22 +1,25 @@ -// FIXME: Update this file to be type safe and remove this and next line -// @ts-strict-ignore +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"; 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 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 convertAccountRequired$: Observable; } 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 eff3fc7f47..a2e994ad9e 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, 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 { KeyService } from "@bitwarden/key-management"; import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../../spec"; @@ -20,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; @@ -73,32 +70,87 @@ 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)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toEqual(orgs[0]); @@ -107,13 +159,25 @@ 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)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toBeUndefined(); @@ -128,7 +192,7 @@ describe("KeyConnectorService", () => { organizationService.organizations$.mockReturnValue(of(orgs)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toBeUndefined(); @@ -137,74 +201,31 @@ 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)); // Act - const result = await keyConnectorService.getManagingOrganization(); + const result = await keyConnectorService.getManagingOrganization(mockUserId); // Assert expect(result).toBeUndefined(); }); }); - 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); - - expect(await keyConnectorService.getConvertAccountRequired()).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; - - await keyConnectorService.setConvertAccountRequired(newValue); - - expect(await keyConnectorService.getConvertAccountRequired()).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", 2, false); - organizationService.organizations$.mockReturnValue(of([data])); - - // uses KeyConnector - const state = stateProvider.activeUser.getFake(USES_KEY_CONNECTOR); - state.nextState(false); - - const result = await keyConnectorService.userNeedsMigration(mockUserId); - - expect(result).toBe(true); - }); - - it("should return false if the user does not need migration", async () => { - tokenService.getIsExternal.mockResolvedValue(false); - const data = organizationData(false, false, "https://key-connector-url.com", 2, false); - organizationService.organizations$.mockReturnValue(of([data])); - - const state = stateProvider.activeUser.getFake(USES_KEY_CONNECTOR); - state.nextState(true); - const result = await keyConnectorService.userNeedsMigration(mockUserId); - - expect(result).toBe(false); - }); - }); - describe("setMasterKeyFromUrl", () => { it("should set the master key from the provided URL", async () => { // Arrange @@ -221,10 +242,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 () => { @@ -246,10 +264,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( @@ -260,7 +284,7 @@ describe("KeyConnectorService", () => { jest.spyOn(apiService, "postUserKeyToKeyConnector").mockResolvedValue(); // Act - await keyConnectorService.migrateUser(); + await keyConnectorService.migrateUser(mockUserId); // Assert expect(keyConnectorService.getManagingOrganization).toHaveBeenCalled(); @@ -273,7 +297,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( Utils.fromBufferToB64(masterKey.inner().encryptionKey), @@ -288,7 +318,7 @@ describe("KeyConnectorService", () => { try { // Act - await keyConnectorService.migrateUser(); + await keyConnectorService.migrateUser(mockUserId); } catch { // Assert expect(logService.error).toHaveBeenCalledWith(error); @@ -301,6 +331,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, 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 9799f06f64..744b7d6608 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,9 @@ // 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, Observable, of, switchMap } from "rxjs"; import { LogoutReason } from "@bitwarden/auth/common"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { Argon2KdfConfig, KdfConfig, @@ -15,7 +16,6 @@ import { ApiService } from "../../../abstractions/api.service"; import { OrganizationService } from "../../../admin-console/abstractions/organization/organization.service.abstraction"; import { OrganizationUserType } from "../../../admin-console/enums"; import { Organization } from "../../../admin-console/models/domain/organization"; -import { AccountService } from "../../../auth/abstractions/account.service"; import { TokenService } from "../../../auth/abstractions/token.service"; import { IdentityTokenResponse } from "../../../auth/models/response/identity-token.response"; import { KeysRequest } from "../../../models/request/keys.request"; @@ -23,12 +23,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"; @@ -42,23 +37,15 @@ export const USES_KEY_CONNECTOR = new UserKeyDefinition( { deserializer: (usesKeyConnector) => usesKeyConnector, clearOn: ["logout"], - }, -); - -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( - private accountService: AccountService, + accountService: AccountService, private masterPasswordService: InternalMasterPasswordServiceAbstraction, private keyService: KeyService, private apiService: ApiService, @@ -69,9 +56,27 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private logoutCallback: (logoutReason: LogoutReason, userId?: string) => Promise, private stateProvider: StateProvider, ) { - 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)), + ]), + ), + switchMap(async ([userId, organizations, usesKeyConnector]) => { + const loggedInUsingSso = await this.tokenService.getIsExternal(userId); + const requiredByOrganization = this.findManagingOrganization(organizations) != null; + const userIsNotUsingKeyConnector = !usesKeyConnector; + + return loggedInUsingSso && requiredByOrganization && userIsNotUsingKeyConnector; + }), ); } @@ -79,20 +84,13 @@ 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) { - const loggedInUsingSso = await this.tokenService.getIsExternal(userId); - const requiredByOrganization = (await this.getManagingOrganization(userId)) != null; - const userIsNotUsingKeyConnector = !(await this.getUsesKeyConnector(userId)); - - 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( @@ -109,6 +107,8 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { } await this.apiService.postConvertToKeyConnector(); + + await this.setUsesKeyConnector(true, userId); } // TODO: UserKey should be renamed to MasterKey and typed accordingly @@ -123,15 +123,9 @@ 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, - ); + async getManagingOrganization(userId: UserId): Promise { + const organizations = await firstValueFrom(this.organizationService.organizations$(userId)); + return this.findManagingOrganization(organizations); } async convertNewSsoUserToKeyConnector( @@ -188,18 +182,6 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postSetKeyConnectorKey(setPasswordRequest); } - async setConvertAccountRequired(status: boolean, userId?: UserId) { - await this.stateProvider.setUserState(CONVERT_ACCOUNT_TO_KEY_CONNECTOR, status, userId); - } - - getConvertAccountRequired(): Promise { - return firstValueFrom(this.convertAccountToKeyConnectorState.state$); - } - - async removeConvertAccountRequired(userId?: UserId) { - await this.setConvertAccountRequired(null, userId); - } - private handleKeyConnectorError(e: any) { this.logService.error(e); if (this.logoutCallback != null) { @@ -209,4 +191,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 faf54f1191..e9f6c60af6 100644 --- a/libs/common/src/platform/sync/default-sync.service.ts +++ b/libs/common/src/platform/sync/default-sync.service.ts @@ -224,13 +224,8 @@ export class DefaultSyncService extends CoreSyncService { await this.syncProfileOrganizations(response, response.id); - if (await this.keyConnectorService.userNeedsMigration(response.id)) { - await this.keyConnectorService.setConvertAccountRequired(true, response.id); + if (await firstValueFrom(this.keyConnectorService.convertAccountRequired$)) { 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); } } diff --git a/libs/key-management-ui/src/key-connector/remove-password.component.spec.ts b/libs/key-management-ui/src/key-connector/remove-password.component.spec.ts new file mode 100644 index 0000000000..a3ab70a618 --- /dev/null +++ b/libs/key-management-ui/src/key-connector/remove-password.component.spec.ts @@ -0,0 +1,252 @@ +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/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"; +import { SyncService } from "@bitwarden/common/platform/sync"; +import { UserId } from "@bitwarden/common/types/guid"; +import { DialogService, ToastService } from "@bitwarden/components"; + +import { mockAccountServiceWith } from "../../../common/spec"; + +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( + mock(), + 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); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + + it("should redirect to login when no active account is found", async () => { + await accountService.switchAccount(null as unknown as UserId); + + await component.ngOnInit(); + + expect(mockRouter.navigate).toHaveBeenCalledWith(["/"]); + }); + + it("should redirect to login when no organization is found", async () => { + mockKeyConnectorService.getManagingOrganization.mockResolvedValue( + null as unknown as Organization, + ); + + await component.ngOnInit(); + + expect(mockKeyConnectorService.getManagingOrganization).toHaveBeenCalledWith(userId); + expect(mockSyncService.fullSync).toHaveBeenCalledWith(false); + expect(mockRouter.navigate).toHaveBeenCalledWith(["/"]); + }); + }); + + 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(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(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( + { + message: errorMessage, + }, + 404, + ), + ); + 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(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(mockRouter.navigate).toHaveBeenCalledWith(["/"]); + }); + + 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(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( + new ErrorResponse( + { + message: errorMessage, + }, + 404, + ), + ); + 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(mockRouter.navigate).not.toHaveBeenCalled(); + }); + + it("should not call leave when dialog is canceled", async () => { + mockDialogService.openSimpleDialog.mockResolvedValue(false); + + await component.leave(); + + expect(component.leaving).toBe(false); + expect(mockOrganizationApiService.leave).not.toHaveBeenCalled(); + expect(mockRouter.navigate).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/libs/key-management-ui/src/key-connector/remove-password.component.ts b/libs/key-management-ui/src/key-connector/remove-password.component.ts index d68644f588..967e34223d 100644 --- a/libs/key-management-ui/src/key-connector/remove-password.component.ts +++ b/libs/key-management-ui/src/key-connector/remove-password.component.ts @@ -1,33 +1,32 @@ -// 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, 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 { 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 { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.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"; @Directive() export class RemovePasswordComponent implements OnInit { - actionPromise: Promise; continuing = false; leaving = false; loading = true; - organization: Organization; - email: string; + organization!: Organization; + private activeUserId!: UserId; constructor( + private logService: LogService, private router: Router, private accountService: AccountService, private syncService: SyncService, - private platformUtilsService: PlatformUtilsService, private i18nService: I18nService, private keyConnectorService: KeyConnectorService, private organizationApiService: OrganizationApiServiceAbstraction, @@ -36,35 +35,49 @@ 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)), - ); + const activeAccount = await firstValueFrom(this.accountService.activeAccount$); + if (activeAccount == null) { + this.logService.info( + "[Key Connector remove password] No active account found, redirecting to login.", + ); + await this.router.navigate(["/"]); + return; + } + this.activeUserId = activeAccount.id; + await this.syncService.fullSync(false); + + this.organization = await this.keyConnectorService.getManagingOrganization(this.activeUserId); + if (this.organization == null) { + this.logService.info( + "[Key Connector remove password] No organization found, redirecting to login.", + ); + await this.router.navigate(["/"]); + return; + } this.loading = false; } + get action() { + return this.continuing || this.leaving; + } + convert = async () => { this.continuing = true; - this.actionPromise = this.keyConnectorService.migrateUser(); 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(); - // 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.router.navigate(["/"]); } catch (e) { - this.toastService.showToast({ - variant: "error", - title: this.i18nService.t("errorOccurred"), - message: e.message, - }); + this.continuing = false; + + this.handleActionError(e); } }; @@ -79,25 +92,33 @@ 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(); - // 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.router.navigate(["/"]); } catch (e) { - this.toastService.showToast({ - variant: "error", - title: this.i18nService.t("errorOccurred"), - message: e, - }); + this.leaving = false; + + 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, + }); + } }