bitwarden/browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser synced 2025-12-06 00:13:28 +00:00
Code Issues Releases Wiki Activity

[PM-22408] Remove setMasterKeyEncryptedUserKey from KeyService (#15087)

Browse Source 
* Swap consumers to masterPasswordService.setMasterKeyEncryptedUserKey

* Remove setMasterKeyEncryptedUserKey from keyService

* unit tests
This commit is contained in:
Thomas Avery
2025-06-11 15:48:18 -05:00
committed by GitHub
parent f30d6f0105
commit c52e6a3f2c
19 changed files with 195 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
libs/auth/src/common/login-strategies/auth-request-login.strategy.spec.ts
View File

@@ -166,7 +166,7 @@ describe("AuthRequestLoginStrategy", () => {
decMasterKeyHash,
mockUserId,
);
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
mockUserId,
);
@@ -194,7 +194,7 @@ describe("AuthRequestLoginStrategy", () => {
expect(masterPasswordService.mock.setMasterKeyHash).not.toHaveBeenCalled();
// setMasterKeyEncryptedUserKey, setUserKey, and setPrivateKey should still be called
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
mockUserId,
);

4
libs/auth/src/common/login-strategies/auth-request-login.strategy.ts
View File

@@ -95,7 +95,9 @@ export class AuthRequestLoginStrategy extends LoginStrategy {
const authRequestCredentials = this.cache.value.authRequestCredentials;
// User now may or may not have a master password
// but set the master key encrypted user key if it exists regardless
await this.keyService.setMasterKeyEncryptedUserKey(response.key, userId);
if (response.key) {
await this.masterPasswordService.setMasterKeyEncryptedUserKey(response.key, userId);
}
if (authRequestCredentials.decryptedUserKey) {
await this.keyService.setUserKey(authRequestCredentials.decryptedUserKey, userId);

5
libs/auth/src/common/login-strategies/password-login.strategy.spec.ts
View File

@@ -202,7 +202,10 @@ describe("PasswordLoginStrategy", () => {
localHashedPassword,
userId,
);
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key, userId);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
userId,
);
expect(keyService.setUserKey).toHaveBeenCalledWith(userKey, userId);
expect(keyService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey, userId);
});

5
libs/auth/src/common/login-strategies/password-login.strategy.ts
View File

@@ -126,7 +126,10 @@ export class PasswordLoginStrategy extends LoginStrategy {
if (this.encryptionKeyMigrationRequired(response)) {
return;
}
await this.keyService.setMasterKeyEncryptedUserKey(response.key, userId);
if (response.key) {
await this.masterPasswordService.setMasterKeyEncryptedUserKey(response.key, userId);
}
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {

7
libs/auth/src/common/login-strategies/sso-login.strategy.spec.ts
View File

@@ -196,8 +196,11 @@ describe("SsoLoginStrategy", () => {
await ssoLoginStrategy.logIn(credentials);
// Assert
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledTimes(1);
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key, userId);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledTimes(1);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
userId,
);
});
describe("Trusted Device Decryption", () => {

5
libs/auth/src/common/login-strategies/sso-login.strategy.ts
View File

@@ -185,7 +185,10 @@ export class SsoLoginStrategy extends LoginStrategy {
if (masterKeyEncryptedUserKey) {
// set the master key encrypted user key if it exists
await this.keyService.setMasterKeyEncryptedUserKey(masterKeyEncryptedUserKey, userId);
await this.masterPasswordService.setMasterKeyEncryptedUserKey(
masterKeyEncryptedUserKey,
userId,
);
}
const userDecryptionOptions = tokenResponse?.userDecryptionOptions;

5
libs/auth/src/common/login-strategies/user-api-login.strategy.spec.ts
View File

@@ -176,7 +176,10 @@ describe("UserApiLoginStrategy", () => {
await apiLogInStrategy.logIn(credentials);
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(tokenResponse.key, userId);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
tokenResponse.key,
userId,
);
expect(keyService.setPrivateKey).toHaveBeenCalledWith(tokenResponse.privateKey, userId);
});

4
libs/auth/src/common/login-strategies/user-api-login.strategy.ts
View File

@@ -63,7 +63,9 @@ export class UserApiLoginStrategy extends LoginStrategy {
response: IdentityTokenResponse,
userId: UserId,
): Promise<void> {
await this.keyService.setMasterKeyEncryptedUserKey(response.key, userId);
if (response.key) {
await this.masterPasswordService.setMasterKeyEncryptedUserKey(response.key, userId);
}
if (response.apiUseKeyConnector) {
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));

4
libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts
View File

@@ -237,8 +237,8 @@ describe("WebAuthnLoginStrategy", () => {
// Assert
// Master key encrypted user key should be set
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledTimes(1);
expect(keyService.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledTimes(1);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
idTokenResponse.key,
userId,
);

5
libs/auth/src/common/login-strategies/webauthn-login.strategy.ts
View File

@@ -66,7 +66,10 @@ export class WebAuthnLoginStrategy extends LoginStrategy {
if (masterKeyEncryptedUserKey) {
// set the master key encrypted user key if it exists
await this.keyService.setMasterKeyEncryptedUserKey(masterKeyEncryptedUserKey, userId);
await this.masterPasswordService.setMasterKeyEncryptedUserKey(
masterKeyEncryptedUserKey,
userId,
);
}
const userDecryptionOptions = idTokenResponse?.userDecryptionOptions;

8
libs/common/src/auth/models/response/identity-token.response.ts
View File

@@ -5,6 +5,7 @@
import { KdfType } from "@bitwarden/key-management";
import { BaseResponse } from "../../../models/response/base.response";
import { EncString } from "../../../platform/models/domain/enc-string";
import { MasterPasswordPolicyResponse } from "./master-password-policy.response";
import { UserDecryptionOptionsResponse } from "./user-decryption-options/user-decryption-options.response";
@@ -17,7 +18,7 @@ export class IdentityTokenResponse extends BaseResponse {
resetMasterPassword: boolean;
privateKey: string;
key: string;
key?: EncString;
twoFactorToken: string;
kdf: KdfType;
kdfIterations: number;
@@ -39,7 +40,10 @@ export class IdentityTokenResponse extends BaseResponse {
this.resetMasterPassword = this.getResponseProperty("ResetMasterPassword");
this.privateKey = this.getResponseProperty("PrivateKey");
this.key = this.getResponseProperty("Key");
const key = this.getResponseProperty("Key");
if (key) {
this.key = new EncString(key);
}
this.twoFactorToken = this.getResponseProperty("TwoFactorToken");
this.kdf = this.getResponseProperty("Kdf");
this.kdfIterations = this.getResponseProperty("KdfIterations");

108
libs/common/src/key-management/key-connector/services/key-connector.service.spec.ts
View File

@@ -5,21 +5,23 @@ import { OrganizationService } from "@bitwarden/common/admin-console/abstraction
import { OrganizationUserType } from "@bitwarden/common/admin-console/enums";
// 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 { KeyService } from "@bitwarden/key-management";
import { KdfType, KeyService } from "@bitwarden/key-management";
import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../../spec";
import { ApiService } from "../../../abstractions/api.service";
import { OrganizationData } from "../../../admin-console/models/data/organization.data";
import { Organization } from "../../../admin-console/models/domain/organization";
import { ProfileOrganizationResponse } from "../../../admin-console/models/response/profile-organization.response";
import { IdentityTokenResponse } from "../../../auth/models/response/identity-token.response";
import { KeyConnectorUserKeyResponse } from "../../../auth/models/response/key-connector-user-key.response";
import { TokenService } from "../../../auth/services/token.service";
import { LogService } from "../../../platform/abstractions/log.service";
import { Utils } from "../../../platform/misc/utils";
import { EncString } from "../../../platform/models/domain/enc-string";
import { SymmetricCryptoKey } from "../../../platform/models/domain/symmetric-crypto-key";
import { KeyGenerationService } from "../../../platform/services/key-generation.service";
import { OrganizationId, UserId } from "../../../types/guid";
import { MasterKey } from "../../../types/key";
import { MasterKey, UserKey } from "../../../types/key";
import { FakeMasterPasswordService } from "../../master-password/services/fake-master-password.service";
import { KeyConnectorUserKeyRequest } from "../models/key-connector-user-key.request";
@@ -50,7 +52,7 @@ describe("KeyConnectorService", () => {
const keyConnectorUrl = "https://key-connector-url.com";
beforeEach(() => {
jest.clearAllMocks();
jest.resetAllMocks();
masterPasswordService = new FakeMasterPasswordService();
accountService = mockAccountServiceWith(mockUserId);
@@ -403,6 +405,106 @@ describe("KeyConnectorService", () => {
});
});
describe("convertNewSsoUserToKeyConnector", () => {
const tokenResponse = mock<IdentityTokenResponse>();
const passwordKey = new SymmetricCryptoKey(new Uint8Array(64));
const mockUserKey = new SymmetricCryptoKey(new Uint8Array(64)) as UserKey;
const mockEmail = "test@example.com";
const mockMasterKey = getMockMasterKey();
let mockMakeUserKeyResult: [UserKey, EncString];
beforeEach(() => {
const mockUserKey = new SymmetricCryptoKey(new Uint8Array(64)) as UserKey;
const mockKeyPair = ["mockPubKey", new EncString("mockEncryptedPrivKey")] as [
string,
EncString,
];
const encString = new EncString("mockEncryptedString");
mockMakeUserKeyResult = [mockUserKey, encString] as [UserKey, EncString];
tokenResponse.kdf = KdfType.PBKDF2_SHA256;
tokenResponse.kdfIterations = 100000;
tokenResponse.kdfMemory = 16;
tokenResponse.kdfParallelism = 4;
tokenResponse.keyConnectorUrl = keyConnectorUrl;
keyGenerationService.createKey.mockResolvedValue(passwordKey);
keyService.makeMasterKey.mockResolvedValue(mockMasterKey);
keyService.makeUserKey.mockResolvedValue(mockMakeUserKeyResult);
keyService.makeKeyPair.mockResolvedValue(mockKeyPair);
tokenService.getEmail.mockResolvedValue(mockEmail);
});
it("sets up a new SSO user with key connector", async () => {
await keyConnectorService.convertNewSsoUserToKeyConnector(
tokenResponse,
mockOrgId,
mockUserId,
);
expect(keyGenerationService.createKey).toHaveBeenCalledWith(512);
expect(keyService.makeMasterKey).toHaveBeenCalledWith(
passwordKey.keyB64,
mockEmail,
expect.any(Object),
);
expect(masterPasswordService.mock.setMasterKey).toHaveBeenCalledWith(
mockMasterKey,
mockUserId,
);
expect(keyService.makeUserKey).toHaveBeenCalledWith(mockMasterKey);
expect(keyService.setUserKey).toHaveBeenCalledWith(mockUserKey, mockUserId);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
mockMakeUserKeyResult[1],
mockUserId,
);
expect(keyService.makeKeyPair).toHaveBeenCalledWith(mockMakeUserKeyResult[0]);
expect(apiService.postUserKeyToKeyConnector).toHaveBeenCalledWith(
tokenResponse.keyConnectorUrl,
expect.any(KeyConnectorUserKeyRequest),
);
expect(apiService.postSetKeyConnectorKey).toHaveBeenCalled();
});
it("handles api error", async () => {
apiService.postUserKeyToKeyConnector.mockRejectedValue(new Error("API error"));
try {
await keyConnectorService.convertNewSsoUserToKeyConnector(
tokenResponse,
mockOrgId,
mockUserId,
);
} catch (error: any) {
expect(error).toBeInstanceOf(Error);
expect(error?.message).toBe("Key Connector error");
}
expect(keyGenerationService.createKey).toHaveBeenCalledWith(512);
expect(keyService.makeMasterKey).toHaveBeenCalledWith(
passwordKey.keyB64,
mockEmail,
expect.any(Object),
);
expect(masterPasswordService.mock.setMasterKey).toHaveBeenCalledWith(
mockMasterKey,
mockUserId,
);
expect(keyService.makeUserKey).toHaveBeenCalledWith(mockMasterKey);
expect(keyService.setUserKey).toHaveBeenCalledWith(mockUserKey, mockUserId);
expect(masterPasswordService.mock.setMasterKeyEncryptedUserKey).toHaveBeenCalledWith(
mockMakeUserKeyResult[1],
mockUserId,
);
expect(keyService.makeKeyPair).toHaveBeenCalledWith(mockMakeUserKeyResult[0]);
expect(apiService.postUserKeyToKeyConnector).toHaveBeenCalledWith(
tokenResponse.keyConnectorUrl,
expect.any(KeyConnectorUserKeyRequest),
);
expect(apiService.postSetKeyConnectorKey).not.toHaveBeenCalled();
});
});
function organizationData(
usesKeyConnector: boolean,
keyConnectorEnabled: boolean,

2
libs/common/src/key-management/key-connector/services/key-connector.service.ts
View File

@@ -160,7 +160,7 @@ export class KeyConnectorService implements KeyConnectorServiceAbstraction {
const userKey = await this.keyService.makeUserKey(masterKey);
await this.keyService.setUserKey(userKey[0], userId);
await this.keyService.setMasterKeyEncryptedUserKey(userKey[1].encryptedString, userId);
await this.masterPasswordService.setMasterKeyEncryptedUserKey(userKey[1], userId);
const [pubKey, privKey] = await this.keyService.makeKeyPair(userKey[0]);

37
libs/common/src/key-management/master-password/services/master-password.service.spec.ts
View File

@@ -153,4 +153,41 @@ describe("MasterPasswordService", () => {
expect(result).toBeNull();
});
});
describe("setMasterKeyEncryptedUserKey", () => {
test.each([null as unknown as EncString, undefined as unknown as EncString])(
"throws when the provided encryptedKey is %s",
async (encryptedKey) => {
await expect(sut.setMasterKeyEncryptedUserKey(encryptedKey, userId)).rejects.toThrow(
"Encrypted Key is required.",
);
},
);
it("throws an error if encryptedKey is malformed null", async () => {
await expect(
sut.setMasterKeyEncryptedUserKey(new EncString(null as unknown as string), userId),
).rejects.toThrow("Encrypted Key is required.");
});
test.each([null as unknown as UserId, undefined as unknown as UserId])(
"throws when the provided userId is %s",
async (userId) => {
await expect(
sut.setMasterKeyEncryptedUserKey(new EncString(testMasterKeyEncryptedKey), userId),
).rejects.toThrow("User ID is required.");
},
);
it("calls stateProvider with the provided encryptedKey and user ID", async () => {
const encryptedKey = new EncString(testMasterKeyEncryptedKey);
await sut.setMasterKeyEncryptedUserKey(encryptedKey, userId);
expect(stateProvider.getUser).toHaveBeenCalled();
expect(mockUserState.update).toHaveBeenCalled();
const updateFn = mockUserState.update.mock.calls[0][0];
expect(updateFn(null)).toEqual(encryptedKey.toJSON());
});
});
});

2
libs/common/src/key-management/master-password/services/master-password.service.ts
View File

@@ -130,7 +130,7 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr
}
async setMasterKeyEncryptedUserKey(encryptedKey: EncString, userId: UserId): Promise<void> {
if (encryptedKey == null) {
if (encryptedKey == null || encryptedKey.encryptedString == null) {
throw new Error("Encrypted Key is required.");
}
if (userId == null) {

8
libs/common/src/models/response/profile.response.ts
View File

@@ -1,6 +1,7 @@
import { ProfileOrganizationResponse } from "../../admin-console/models/response/profile-organization.response";
import { ProfileProviderOrganizationResponse } from "../../admin-console/models/response/profile-provider-organization.response";
import { ProfileProviderResponse } from "../../admin-console/models/response/profile-provider.response";
import { EncString } from "../../platform/models/domain/enc-string";
import { UserId } from "../../types/guid";
import { BaseResponse } from "./base.response";
@@ -14,7 +15,7 @@ export class ProfileResponse extends BaseResponse {
premiumFromOrganization: boolean;
culture: string;
twoFactorEnabled: boolean;
key: string;
key?: EncString;
avatarColor: string;
creationDate: string;
privateKey: string;
@@ -36,7 +37,10 @@ export class ProfileResponse extends BaseResponse {
this.premiumFromOrganization = this.getResponseProperty("PremiumFromOrganization");
this.culture = this.getResponseProperty("Culture");
this.twoFactorEnabled = this.getResponseProperty("TwoFactorEnabled");
this.key = this.getResponseProperty("Key");
const key = this.getResponseProperty("Key");
if (key) {
this.key = new EncString(key);
}
this.avatarColor = this.getResponseProperty("AvatarColor");
this.creationDate = this.getResponseProperty("CreationDate");
this.privateKey = this.getResponseProperty("PrivateKey");

5
libs/common/src/platform/sync/default-sync.service.ts
View File

@@ -225,7 +225,10 @@ export class DefaultSyncService extends CoreSyncService {
throw new Error("Stamp has changed");
}
await this.keyService.setMasterKeyEncryptedUserKey(response.key, response.id);
// Users with no master password will not have a key.
if (response?.key) {
await this.masterPasswordService.setMasterKeyEncryptedUserKey(response.key, response.id);
}
await this.keyService.setPrivateKey(response.privateKey, response.id);
await this.keyService.setProviderKeys(response.providers, response.id);
await this.keyService.setOrgKeys(

7
libs/key-management/src/abstractions/key.service.ts
View File

@@ -170,13 +170,6 @@ export abstract class KeyService {
* @throws Error when userId is null or undefined.
*/
abstract clearStoredUserKey(keySuffix: KeySuffixOptions, userId: string): Promise<void>;
/**
* Stores the master key encrypted user key
* @throws Error when userId is null and there is no active user.
* @param userKeyMasterKey The master key encrypted user key to set
* @param userId The desired user
*/
abstract setMasterKeyEncryptedUserKey(userKeyMasterKey: string, userId?: UserId): Promise<void>;
/**
* @throws Error when userId is null and no active user
* @param password The user's master password that will be used to derive a master key if one isn't found

12
libs/key-management/src/key.service.ts
View File

@@ -263,18 +263,6 @@ export class DefaultKeyService implements KeyServiceAbstraction {
}
}
async setMasterKeyEncryptedUserKey(userKeyMasterKey: string, userId?: UserId): Promise<void> {
userId ??= await firstValueFrom(this.stateProvider.activeUserId$);
if (userId == null) {
throw new Error("No active user id found.");
}
await this.masterPasswordService.setMasterKeyEncryptedUserKey(
new EncString(userKeyMasterKey),
userId,
);
}
// TODO: Move to MasterPasswordService
async getOrDeriveMasterKey(password: string, userId?: UserId) {
const [resolvedUserId, email] = await firstValueFrom(