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

[PM-10607] Require userId for getKeyForCipherKeyDecryption (#10509)

Browse Source 
* updated cipher service to stop using the deprecated getUserKeyWithLegacySupport and use the version that requires a user id

* Added account service mock

* fixed cipher test

* Fixed test

* removed async from encryptCipher

* updated encryptSharedCipher to pass userId to the encrypt function

* Pass userId to getUserKeyWithLegacySupport on encryptSharedCipher

* pass in userid when setting masterKeyEncryptedUserKey

* Added activer usedId to new web refresh function
This commit is contained in:
SmithThe4th
2024-08-20 12:00:48 -04:00
committed by GitHub
parent ed719f835a
commit dedd7f1b5c
67 changed files with 534 additions and 118 deletions
Download Patch File Download Diff File Expand all files Collapse all files

21
libs/common/src/platform/services/fido2/fido2-authenticator.service.spec.ts
View File

@@ -1,7 +1,10 @@
import { TextEncoder } from "util";
import { mock, MockProxy } from "jest-mock-extended";
import { BehaviorSubject } from "rxjs";
import { AccountInfo, AccountService } from "../../../auth/abstractions/account.service";
import { UserId } from "../../../types/guid";
import { CipherService } from "../../../vault/abstractions/cipher.service";
import { SyncService } from "../../../vault/abstractions/sync/sync.service.abstraction";
import { CipherRepromptType } from "../../../vault/enums/cipher-reprompt-type";
@@ -30,10 +33,18 @@ import { guidToRawFormat } from "./guid-utils";
const RpId = "bitwarden.com";
describe("FidoAuthenticatorService", () => {
const activeAccountSubject = new BehaviorSubject<{ id: UserId } & AccountInfo>({
id: "testId" as UserId,
email: "test@example.com",
emailVerified: true,
name: "Test User",
});
let cipherService!: MockProxy<CipherService>;
let userInterface!: MockProxy<Fido2UserInterfaceService>;
let userInterfaceSession!: MockProxy<Fido2UserInterfaceSession>;
let syncService!: MockProxy<SyncService>;
let accountService!: MockProxy<AccountService>;
let authenticator!: Fido2AuthenticatorService;
let tab!: chrome.tabs.Tab;
@@ -43,8 +54,15 @@ describe("FidoAuthenticatorService", () => {
userInterfaceSession = mock<Fido2UserInterfaceSession>();
userInterface.newSession.mockResolvedValue(userInterfaceSession);
syncService = mock<SyncService>();
authenticator = new Fido2AuthenticatorService(cipherService, userInterface, syncService);
accountService = mock<AccountService>();
authenticator = new Fido2AuthenticatorService(
cipherService,
userInterface,
syncService,
accountService,
);
tab = { id: 123, windowId: 456 } as chrome.tabs.Tab;
accountService.activeAccount$ = activeAccountSubject;
});
describe("makeCredential", () => {
@@ -677,6 +695,7 @@ describe("FidoAuthenticatorService", () => {
],
}),
}),
"testId",
);
});

17
libs/common/src/platform/services/fido2/fido2-authenticator.service.ts
View File

@@ -1,3 +1,6 @@
import { firstValueFrom, map } from "rxjs";
import { AccountService } from "../../../auth/abstractions/account.service";
import { CipherService } from "../../../vault/abstractions/cipher.service";
import { SyncService } from "../../../vault/abstractions/sync/sync.service.abstraction";
import { CipherRepromptType } from "../../../vault/enums/cipher-reprompt-type";
@@ -42,6 +45,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
private cipherService: CipherService,
private userInterface: Fido2UserInterfaceService,
private syncService: SyncService,
private accountService: AccountService,
private logService?: LogService,
) {}
@@ -130,8 +134,12 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
keyPair = await createKeyPair();
pubKeyDer = await crypto.subtle.exportKey("spki", keyPair.publicKey);
const encrypted = await this.cipherService.get(cipherId);
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
cipher = await encrypted.decrypt(
await this.cipherService.getKeyForCipherKeyDecryption(encrypted),
await this.cipherService.getKeyForCipherKeyDecryption(encrypted, activeUserId),
);
if (
@@ -150,7 +158,7 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
if (Utils.isNullOrEmpty(cipher.login.username)) {
cipher.login.username = fido2Credential.userName;
}
const reencrypted = await this.cipherService.encrypt(cipher);
const reencrypted = await this.cipherService.encrypt(cipher, activeUserId);
await this.cipherService.updateWithServer(reencrypted);
credentialId = fido2Credential.credentialId;
} catch (error) {
@@ -277,7 +285,10 @@ export class Fido2AuthenticatorService implements Fido2AuthenticatorServiceAbstr
};
if (selectedFido2Credential.counter > 0) {
const encrypted = await this.cipherService.encrypt(selectedCipher);
const activeUserId = await firstValueFrom(
this.accountService.activeAccount$.pipe(map((a) => a?.id)),
);
const encrypted = await this.cipherService.encrypt(selectedCipher, activeUserId);
await this.cipherService.updateWithServer(encrypted);
}

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

@@ -175,7 +175,7 @@ export class DefaultSyncService extends CoreSyncService {
throw new Error("Stamp has changed");
}
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key);
await this.cryptoService.setMasterKeyEncryptedUserKey(response.key, response.id);
await this.cryptoService.setPrivateKey(response.privateKey, response.id);
await this.cryptoService.setProviderKeys(response.providers, response.id);
await this.cryptoService.setOrgKeys(

7
libs/common/src/vault/abstractions/cipher.service.ts
View File

@@ -27,6 +27,7 @@ export abstract class CipherService implements UserKeyRotationDataProvider<Ciphe
clearCache: (userId?: string) => Promise<void>;
encrypt: (
model: CipherView,
userId: UserId,
keyForEncryption?: SymmetricCryptoKey,
keyForCipherKeyDecryption?: SymmetricCryptoKey,
originalCipher?: Cipher,
@@ -83,21 +84,25 @@ export abstract class CipherService implements UserKeyRotationDataProvider<Ciphe
cipher: CipherView,
organizationId: string,
collectionIds: string[],
userId: UserId,
) => Promise<any>;
shareManyWithServer: (
ciphers: CipherView[],
organizationId: string,
collectionIds: string[],
userId: UserId,
) => Promise<any>;
saveAttachmentWithServer: (
cipher: Cipher,
unencryptedFile: any,
userId: UserId,
admin?: boolean,
) => Promise<Cipher>;
saveAttachmentRawWithServer: (
cipher: Cipher,
filename: string,
data: ArrayBuffer,
userId: UserId,
admin?: boolean,
) => Promise<Cipher>;
/**
@@ -147,7 +152,7 @@ export abstract class CipherService implements UserKeyRotationDataProvider<Ciphe
) => Promise<any>;
restoreWithServer: (id: string, asAdmin?: boolean) => Promise<any>;
restoreManyWithServer: (ids: string[], orgId?: string) => Promise<void>;
getKeyForCipherKeyDecryption: (cipher: Cipher) => Promise<any>;
getKeyForCipherKeyDecryption: (cipher: Cipher, userId: UserId) => Promise<any>;
setAddEditCipherInfo: (value: AddEditCipherInfo) => Promise<void>;
/**
* Returns user ciphers re-encrypted with the new user key.

12
libs/common/src/vault/models/domain/cipher.spec.ts
View File

@@ -1,6 +1,8 @@
import { mock } from "jest-mock-extended";
import { Jsonify } from "type-fest";
import { UserId } from "@bitwarden/common/types/guid";
import { makeStaticByteArray, mockEnc, mockFromJson } from "../../../../spec/utils";
import { UriMatchStrategy } from "../../../models/domain/domain-service";
import { CryptoService } from "../../../platform/abstractions/crypto.service";
@@ -247,7 +249,7 @@ describe("Cipher DTO", () => {
);
const cipherView = await cipher.decrypt(
await cipherService.getKeyForCipherKeyDecryption(cipher),
await cipherService.getKeyForCipherKeyDecryption(cipher, mockUserId),
);
expect(cipherView).toMatchObject({
@@ -367,7 +369,7 @@ describe("Cipher DTO", () => {
);
const cipherView = await cipher.decrypt(
await cipherService.getKeyForCipherKeyDecryption(cipher),
await cipherService.getKeyForCipherKeyDecryption(cipher, mockUserId),
);
expect(cipherView).toMatchObject({
@@ -505,7 +507,7 @@ describe("Cipher DTO", () => {
);
const cipherView = await cipher.decrypt(
await cipherService.getKeyForCipherKeyDecryption(cipher),
await cipherService.getKeyForCipherKeyDecryption(cipher, mockUserId),
);
expect(cipherView).toMatchObject({
@@ -667,7 +669,7 @@ describe("Cipher DTO", () => {
);
const cipherView = await cipher.decrypt(
await cipherService.getKeyForCipherKeyDecryption(cipher),
await cipherService.getKeyForCipherKeyDecryption(cipher, mockUserId),
);
expect(cipherView).toMatchObject({
@@ -754,3 +756,5 @@ describe("Cipher DTO", () => {
});
});
});
const mockUserId = "TestUserId" as UserId;

14
libs/common/src/vault/services/cipher.service.spec.ts
View File

@@ -121,6 +121,8 @@ describe("Cipher Service", () => {
accountService = mockAccountServiceWith(mockUserId);
const stateProvider = new FakeStateProvider(accountService);
const userId = "TestUserId" as UserId;
let cipherService: CipherService;
let cipherObj: Cipher;
@@ -168,7 +170,7 @@ describe("Cipher Service", () => {
const spy = jest.spyOn(cipherFileUploadService, "upload");
await cipherService.saveAttachmentRawWithServer(new Cipher(), fileName, fileData);
await cipherService.saveAttachmentRawWithServer(new Cipher(), fileName, fileData, userId);
expect(spy).toHaveBeenCalled();
});
@@ -283,7 +285,7 @@ describe("Cipher Service", () => {
{ uri: "uri", match: UriMatchStrategy.RegularExpression } as LoginUriView,
];
const domain = await cipherService.encrypt(cipherView);
const domain = await cipherService.encrypt(cipherView, userId);
expect(domain.login.uris).toEqual([
{
@@ -299,7 +301,7 @@ describe("Cipher Service", () => {
it("is null when enableCipherKeyEncryption flag is false", async () => {
setEncryptionKeyFlag(false);
const cipher = await cipherService.encrypt(cipherView);
const cipher = await cipherService.encrypt(cipherView, userId);
expect(cipher.key).toBeNull();
});
@@ -307,7 +309,7 @@ describe("Cipher Service", () => {
it("is defined when enableCipherKeyEncryption flag is true", async () => {
setEncryptionKeyFlag(true);
const cipher = await cipherService.encrypt(cipherView);
const cipher = await cipherService.encrypt(cipherView, userId);
expect(cipher.key).toBeDefined();
});
@@ -321,7 +323,7 @@ describe("Cipher Service", () => {
it("is not called when enableCipherKeyEncryption is false", async () => {
setEncryptionKeyFlag(false);
await cipherService.encrypt(cipherView);
await cipherService.encrypt(cipherView, userId);
expect(cipherService["encryptCipherWithCipherKey"]).not.toHaveBeenCalled();
});
@@ -329,7 +331,7 @@ describe("Cipher Service", () => {
it("is called when enableCipherKeyEncryption is true", async () => {
setEncryptionKeyFlag(true);
await cipherService.encrypt(cipherView);
await cipherService.encrypt(cipherView, userId);
expect(cipherService["encryptCipherWithCipherKey"]).toHaveBeenCalled();
});

41
libs/common/src/vault/services/cipher.service.ts
View File

@@ -165,6 +165,7 @@ export class CipherService implements CipherServiceAbstraction {
async encrypt(
model: CipherView,
userId: UserId,
keyForEncryption?: SymmetricCryptoKey,
keyForCipherKeyDecryption?: SymmetricCryptoKey,
originalCipher: Cipher = null,
@@ -174,7 +175,7 @@ export class CipherService implements CipherServiceAbstraction {
originalCipher = await this.get(model.id);
}
if (originalCipher != null) {
await this.updateModelfromExistingCipher(model, originalCipher);
await this.updateModelfromExistingCipher(model, originalCipher, userId);
}
this.adjustPasswordHistoryLength(model);
}
@@ -192,7 +193,7 @@ export class CipherService implements CipherServiceAbstraction {
if (await this.getCipherKeyEncryptionEnabled()) {
cipher.key = originalCipher?.key ?? null;
const userOrOrgKey = await this.getKeyForCipherKeyDecryption(cipher);
const userOrOrgKey = await this.getKeyForCipherKeyDecryption(cipher, userId);
// The keyForEncryption is only used for encrypting the cipher key, not the cipher itself, since cipher key encryption is enabled.
// If the caller has provided a key for cipher key encryption, use it. Otherwise, use the user or org key.
keyForEncryption ||= userOrOrgKey;
@@ -718,6 +719,7 @@ export class CipherService implements CipherServiceAbstraction {
cipher: CipherView,
organizationId: string,
collectionIds: string[],
userId: UserId,
): Promise<any> {
const attachmentPromises: Promise<any>[] = [];
if (cipher.attachments != null) {
@@ -733,7 +735,7 @@ export class CipherService implements CipherServiceAbstraction {
cipher.organizationId = organizationId;
cipher.collectionIds = collectionIds;
const encCipher = await this.encryptSharedCipher(cipher);
const encCipher = await this.encryptSharedCipher(cipher, userId);
const request = new CipherShareRequest(encCipher);
const response = await this.apiService.putShareCipher(cipher.id, request);
const data = new CipherData(response, collectionIds);
@@ -744,6 +746,7 @@ export class CipherService implements CipherServiceAbstraction {
ciphers: CipherView[],
organizationId: string,
collectionIds: string[],
userId: UserId,
): Promise<any> {
const promises: Promise<any>[] = [];
const encCiphers: Cipher[] = [];
@@ -751,7 +754,7 @@ export class CipherService implements CipherServiceAbstraction {
cipher.organizationId = organizationId;
cipher.collectionIds = collectionIds;
promises.push(
this.encryptSharedCipher(cipher).then((c) => {
this.encryptSharedCipher(cipher, userId).then((c) => {
encCiphers.push(c);
}),
);
@@ -770,7 +773,12 @@ export class CipherService implements CipherServiceAbstraction {
await this.upsert(encCiphers.map((c) => c.toCipherData()));
}
saveAttachmentWithServer(cipher: Cipher, unencryptedFile: any, admin = false): Promise<Cipher> {
saveAttachmentWithServer(
cipher: Cipher,
unencryptedFile: any,
userId: UserId,
admin = false,
): Promise<Cipher> {
return new Promise((resolve, reject) => {
const reader = new FileReader();
reader.readAsArrayBuffer(unencryptedFile);
@@ -780,6 +788,7 @@ export class CipherService implements CipherServiceAbstraction {
cipher,
unencryptedFile.name,
evt.target.result,
userId,
admin,
);
resolve(cData);
@@ -797,9 +806,10 @@ export class CipherService implements CipherServiceAbstraction {
cipher: Cipher,
filename: string,
data: Uint8Array,
userId: UserId,
admin = false,
): Promise<Cipher> {
const encKey = await this.getKeyForCipherKeyDecryption(cipher);
const encKey = await this.getKeyForCipherKeyDecryption(cipher, userId);
const cipherKeyEncryptionEnabled = await this.getCipherKeyEncryptionEnabled();
const cipherEncKey =
@@ -813,8 +823,8 @@ export class CipherService implements CipherServiceAbstraction {
//then we rollback to using the user key as the main key of encryption of the item
//in order to keep item and it's attachments with the same encryption level
if (cipher.key != null && !cipherKeyEncryptionEnabled) {
const model = await cipher.decrypt(await this.getKeyForCipherKeyDecryption(cipher));
cipher = await this.encrypt(model);
const model = await cipher.decrypt(await this.getKeyForCipherKeyDecryption(cipher, userId));
cipher = await this.encrypt(model, userId);
await this.updateWithServer(cipher);
}
@@ -1209,10 +1219,10 @@ export class CipherService implements CipherServiceAbstraction {
await this.restore(restores);
}
async getKeyForCipherKeyDecryption(cipher: Cipher): Promise<UserKey | OrgKey> {
async getKeyForCipherKeyDecryption(cipher: Cipher, userId: UserId): Promise<UserKey | OrgKey> {
return (
(await this.cryptoService.getOrgKey(cipher.organizationId)) ||
((await this.cryptoService.getUserKeyWithLegacySupport()) as UserKey)
((await this.cryptoService.getUserKeyWithLegacySupport(userId)) as UserKey)
);
}
@@ -1247,7 +1257,7 @@ export class CipherService implements CipherServiceAbstraction {
}
encryptedCiphers = await Promise.all(
userCiphers.map(async (cipher) => {
const encryptedCipher = await this.encrypt(cipher, newUserKey, originalUserKey);
const encryptedCipher = await this.encrypt(cipher, userId, newUserKey, originalUserKey);
return new CipherWithIdRequest(encryptedCipher);
}),
);
@@ -1259,17 +1269,18 @@ export class CipherService implements CipherServiceAbstraction {
// In the case of a cipher that is being shared with an organization, we want to decrypt the
// cipher key with the user's key and then re-encrypt it with the organization's key.
private async encryptSharedCipher(model: CipherView): Promise<Cipher> {
const keyForCipherKeyDecryption = await this.cryptoService.getUserKeyWithLegacySupport();
return await this.encrypt(model, null, keyForCipherKeyDecryption);
private async encryptSharedCipher(model: CipherView, userId: UserId): Promise<Cipher> {
const keyForCipherKeyDecryption = await this.cryptoService.getUserKeyWithLegacySupport(userId);
return await this.encrypt(model, userId, null, keyForCipherKeyDecryption);
}
private async updateModelfromExistingCipher(
model: CipherView,
originalCipher: Cipher,
userId: UserId,
): Promise<void> {
const existingCipher = await originalCipher.decrypt(
await this.getKeyForCipherKeyDecryption(originalCipher),
await this.getKeyForCipherKeyDecryption(originalCipher, userId),
);
model.passwordHistory = existingCipher.passwordHistory || [];
if (model.type === CipherType.Login && existingCipher.type === CipherType.Login) {