diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 5eaac4033eb..27fb0ae2a5f 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -1154,6 +1154,7 @@ const safeProviders: SafeProvider[] = [ RegisterSdkService, SecurityStateService, AccountCryptographicStateService, + InternalUserDecryptionOptionsServiceAbstraction, ], }), 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 b8ee5d7df64..7a159762c22 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,6 +1,9 @@ import { mock } from "jest-mock-extended"; import { firstValueFrom, of, timeout, TimeoutError } from "rxjs"; +// This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. +// eslint-disable-next-line no-restricted-imports +import { InternalUserDecryptionOptionsServiceAbstraction } from "@bitwarden/auth/common"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { SetKeyConnectorKeyRequest } from "@bitwarden/common/key-management/key-connector/models/set-key-connector-key.request"; @@ -53,6 +56,7 @@ describe("KeyConnectorService", () => { const registerSdkService = mock(); const securityStateService = mock(); const accountCryptographicStateService = mock(); + const userDecryptionOptionsService = mock(); let stateProvider: FakeStateProvider; @@ -97,6 +101,7 @@ describe("KeyConnectorService", () => { registerSdkService, securityStateService, accountCryptographicStateService, + userDecryptionOptionsService, ); }); @@ -265,7 +270,15 @@ describe("KeyConnectorService", () => { Utils.fromBufferToB64(masterKey.inner().encryptionKey), ); + const mockUserDecryptionOptions = { + hasMasterPassword: true, + keyConnectorOption: undefined, + }; + jest.spyOn(apiService, "postUserKeyToKeyConnector").mockResolvedValue(); + userDecryptionOptionsService.userDecryptionOptionsById$.mockReturnValue( + of(mockUserDecryptionOptions), + ); // Act await keyConnectorService.migrateUser(keyConnectorUrl, mockUserId); @@ -276,6 +289,22 @@ describe("KeyConnectorService", () => { keyConnectorRequest, ); expect(apiService.postConvertToKeyConnector).toHaveBeenCalled(); + expect(masterPasswordService.mock.clearMasterKeyHash).toHaveBeenCalledWith(mockUserId); + expect(masterPasswordService.mock.clearMasterPasswordUnlockData).toHaveBeenCalledWith( + mockUserId, + ); + expect(userDecryptionOptionsService.userDecryptionOptionsById$).toHaveBeenCalledWith( + mockUserId, + ); + expect(userDecryptionOptionsService.setUserDecryptionOptionsById).toHaveBeenCalledWith( + mockUserId, + { + hasMasterPassword: false, + keyConnectorOption: { + keyConnectorUrl, + }, + }, + ); }); it("should handle errors thrown during migration", async () => { @@ -299,6 +328,10 @@ describe("KeyConnectorService", () => { keyConnectorUrl, keyConnectorRequest, ); + // Verify state clearing operations were not called due to error + expect(masterPasswordService.mock.clearMasterKeyHash).not.toHaveBeenCalled(); + expect(masterPasswordService.mock.clearMasterPasswordUnlockData).not.toHaveBeenCalled(); + expect(userDecryptionOptionsService.setUserDecryptionOptionsById).not.toHaveBeenCalled(); } }); }); 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 751f1ec8594..ef9e1d810ec 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 @@ -4,7 +4,10 @@ import { combineLatest, filter, firstValueFrom, map, Observable, of, switchMap } // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. // eslint-disable-next-line no-restricted-imports -import { LogoutReason } from "@bitwarden/auth/common"; +import { + InternalUserDecryptionOptionsServiceAbstraction, + LogoutReason, +} from "@bitwarden/auth/common"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { NewSsoUserKeyConnectorConversion } from "@bitwarden/common/key-management/key-connector/models/new-sso-user-key-connector-conversion"; // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. @@ -93,6 +96,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { private registerSdkService: RegisterSdkService, private securityStateService: SecurityStateService, private accountCryptographicStateService: AccountCryptographicStateService, + private userDecryptionOptionsService: InternalUserDecryptionOptionsServiceAbstraction, ) { this.convertAccountRequired$ = accountService.activeAccount$.pipe( filter((account) => account != null), @@ -143,6 +147,22 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction { await this.apiService.postConvertToKeyConnector(); await this.setUsesKeyConnector(true, userId); + + // Clear master password unlock from state + await this.masterPasswordService.clearMasterKeyHash(userId); + await this.masterPasswordService.clearMasterPasswordUnlockData(userId); + + const userDecryptionOptions = await firstValueFrom( + this.userDecryptionOptionsService.userDecryptionOptionsById$(userId), + ); + userDecryptionOptions.hasMasterPassword = false; + userDecryptionOptions.keyConnectorOption = { + keyConnectorUrl, + }; + await this.userDecryptionOptionsService.setUserDecryptionOptionsById( + userId, + userDecryptionOptions, + ); } // TODO: UserKey should be renamed to MasterKey and typed accordingly diff --git a/libs/common/src/key-management/master-password/abstractions/master-password.service.abstraction.ts b/libs/common/src/key-management/master-password/abstractions/master-password.service.abstraction.ts index 9a5b39993e8..75e7fa5862c 100644 --- a/libs/common/src/key-management/master-password/abstractions/master-password.service.abstraction.ts +++ b/libs/common/src/key-management/master-password/abstractions/master-password.service.abstraction.ts @@ -196,6 +196,13 @@ export abstract class InternalMasterPasswordServiceAbstraction extends MasterPas userId: UserId, ): Promise; + /** + * Clears the master password unlock data for the user. + * @param userId The user ID. + * @throws Error If the user ID is missing. + */ + abstract clearMasterPasswordUnlockData(userId: UserId): Promise; + /** * An observable that emits the master password unlock data for the target user. * @param userId The user ID. diff --git a/libs/common/src/key-management/master-password/services/fake-master-password.service.ts b/libs/common/src/key-management/master-password/services/fake-master-password.service.ts index 40be7025d89..3f6d749fca3 100644 --- a/libs/common/src/key-management/master-password/services/fake-master-password.service.ts +++ b/libs/common/src/key-management/master-password/services/fake-master-password.service.ts @@ -124,6 +124,10 @@ export class FakeMasterPasswordService implements InternalMasterPasswordServiceA return this.mock.setMasterPasswordUnlockData(masterPasswordUnlockData, userId); } + clearMasterPasswordUnlockData(userId: UserId): Promise { + return this.mock.clearMasterPasswordUnlockData(userId); + } + masterPasswordUnlockData$(userId: UserId): Observable { return this.mock.masterPasswordUnlockData$(userId); } diff --git a/libs/common/src/key-management/master-password/services/master-password.service.spec.ts b/libs/common/src/key-management/master-password/services/master-password.service.spec.ts index f72ae0e7c5e..b6089ca7d81 100644 --- a/libs/common/src/key-management/master-password/services/master-password.service.spec.ts +++ b/libs/common/src/key-management/master-password/services/master-password.service.spec.ts @@ -416,6 +416,36 @@ describe("MasterPasswordService", () => { ); }); + describe("clearMasterPasswordUnlockData", () => { + it("clears the master password unlock data from state", async () => { + const masterKeyWrappedUserKey = makeEncString().toSdk() as MasterKeyWrappedUserKey; + const masterPasswordUnlockData = new MasterPasswordUnlockData( + salt, + kdfPBKDF2, + masterKeyWrappedUserKey, + ); + stateProvider.singleUser + .getFake(userId, MASTER_PASSWORD_UNLOCK_KEY) + .nextState(masterPasswordUnlockData.toJSON()); + + await sut.clearMasterPasswordUnlockData(userId); + + const state = await firstValueFrom( + stateProvider.getUser(userId, MASTER_PASSWORD_UNLOCK_KEY).state$, + ); + expect(state).toBeNull(); + }); + + test.each([null as unknown as UserId, undefined as unknown as UserId])( + "throws when the provided userId is %s", + async (userId) => { + await expect(sut.clearMasterPasswordUnlockData(userId)).rejects.toThrow( + "userId is null or undefined.", + ); + }, + ); + }); + describe("setLegacyMasterKeyFromUnlockData", () => { const password = "test-password"; diff --git a/libs/common/src/key-management/master-password/services/master-password.service.ts b/libs/common/src/key-management/master-password/services/master-password.service.ts index 28d4f58d7dc..d6ec10c1660 100644 --- a/libs/common/src/key-management/master-password/services/master-password.service.ts +++ b/libs/common/src/key-management/master-password/services/master-password.service.ts @@ -338,6 +338,12 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr .update(() => masterPasswordUnlockData.toJSON()); } + async clearMasterPasswordUnlockData(userId: UserId): Promise { + assertNonNullish(userId, "userId"); + + await this.stateProvider.getUser(userId, MASTER_PASSWORD_UNLOCK_KEY).update(() => null); + } + masterPasswordUnlockData$(userId: UserId): Observable { assertNonNullish(userId, "userId");