Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update decryptUserKeyWithMasterKey to requireUserId #11560

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,7 @@ export default class MainBackground {
this.stateService,
this.keyGenerationService,
this.encryptService,
this.logService,
);

this.i18nService = new I18nService(BrowserApi.getUILanguage(), this.globalStateProvider);
Expand Down
2 changes: 1 addition & 1 deletion apps/cli/src/auth/commands/unlock.command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
return Response.error(e.message);
}

const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey, userId);

Check warning on line 71 in apps/cli/src/auth/commands/unlock.command.ts

View check run for this annotation

Codecov / codecov/patch

apps/cli/src/auth/commands/unlock.command.ts#L71

Added line #L71 was not covered by tests
await this.cryptoService.setUserKey(userKey, userId);

if (await this.keyConnectorService.getConvertAccountRequired()) {
Expand Down
1 change: 1 addition & 0 deletions apps/cli/src/service-container/service-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -395,6 +395,7 @@ export class ServiceContainer {
this.stateService,
this.keyGenerationService,
this.encryptService,
this.logService,
);

this.kdfConfigService = new KdfConfigService(this.stateProvider);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@
HashPurpose.LocalAuthorization,
);

const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey, userId);

Check warning on line 197 in apps/web/src/app/auth/settings/change-password.component.ts

View check run for this annotation

Codecov / codecov/patch

apps/web/src/app/auth/settings/change-password.component.ts#L197

Added line #L197 was not covered by tests
if (userKey == null) {
this.toastService.showToast({
variant: "error",
Expand Down
1 change: 1 addition & 0 deletions libs/angular/src/auth/components/lock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,7 @@ export class LockComponent implements OnInit, OnDestroy {

const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(
response.masterKey,
userId,
);
await this.setUserKeyAndContinue(userKey, userId, true);
}
Expand Down
8 changes: 7 additions & 1 deletion libs/angular/src/services/jslib-services.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,13 @@ const safeProviders: SafeProvider[] = [
safeProvider({
provide: InternalMasterPasswordServiceAbstraction,
useClass: MasterPasswordService,
deps: [StateProvider, StateServiceAbstraction, KeyGenerationServiceAbstraction, EncryptService],
deps: [
StateProvider,
StateServiceAbstraction,
KeyGenerationServiceAbstraction,
EncryptService,
LogService,
],
}),
safeProvider({
provide: MasterPasswordServiceAbstraction,
Expand Down
1 change: 1 addition & 0 deletions libs/auth/src/angular/lock/lock.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ export class LockV2Component implements OnInit, OnDestroy {

const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(
masterPasswordVerificationResponse.masterKey,
this.activeAccount.id,
);
await this.setUserKeyAndContinue(userKey, true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,10 @@ export class AuthRequestLoginStrategy extends LoginStrategy {
private async trySetUserKeyWithMasterKey(userId: UserId): Promise<void> {
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(
masterKey,
userId,
);
await this.cryptoService.setUserKey(userKey, userId);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,10 @@ export class PasswordLoginStrategy extends LoginStrategy {

const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(
masterKey,
userId,
);
await this.cryptoService.setUserKey(userKey, userId);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,7 @@ describe("SsoLoginStrategy", () => {

expect(masterPasswordService.mock.decryptUserKeyWithMasterKey).toHaveBeenCalledWith(
masterKey,
undefined,
userId,
undefined,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey, userId);
Expand Down Expand Up @@ -555,7 +555,7 @@ describe("SsoLoginStrategy", () => {

expect(masterPasswordService.mock.decryptUserKeyWithMasterKey).toHaveBeenCalledWith(
masterKey,
undefined,
userId,
undefined,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey, userId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ export class SsoLoginStrategy extends LoginStrategy {
return;
}

const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey, userId);
await this.cryptoService.setUserKey(userKey, userId);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ describe("UserApiLoginStrategy", () => {

expect(masterPasswordService.mock.decryptUserKeyWithMasterKey).toHaveBeenCalledWith(
masterKey,
undefined,
userId,
undefined,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(userKey, userId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ export class UserApiLoginStrategy extends LoginStrategy {
if (response.apiUseKeyConnector) {
const masterKey = await firstValueFrom(this.masterPasswordService.masterKey$(userId));
if (masterKey) {
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(
masterKey,
userId,
);
await this.cryptoService.setUserKey(userKey, userId);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ describe("AuthRequestService", () => {
);
expect(masterPasswordService.mock.decryptUserKeyWithMasterKey).toHaveBeenCalledWith(
mockDecryptedMasterKey,
undefined,
mockUserId,
undefined,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockDecryptedUserKey, mockUserId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export class AuthRequestService implements AuthRequestServiceAbstraction {
);

// Decrypt and set user key in state
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey);
const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(masterKey, userId);

// Set masterKey + masterKeyHash in state after decryption (in case decryption fails)
await this.masterPasswordService.setMasterKey(masterKey, userId);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,7 @@ export class PinService implements PinServiceAbstraction {

const userKey = await this.masterPasswordService.decryptUserKeyWithMasterKey(
masterKey,
userId,
encUserKey ? new EncString(encUserKey) : undefined,
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,16 @@ export abstract class MasterPasswordServiceAbstraction {
/**
* Decrypts the user key with the provided master key
* @param masterKey The user's master key
* * @param userId The desired user
* @param userKey The user's encrypted symmetric key
* @param userId The desired user
* @throws If either the MasterKey or UserKey are not resolved, or if the UserKey encryption type
* is neither AesCbc256_B64 nor AesCbc256_HmacSha256_B64
* @returns The user key
*/
abstract decryptUserKeyWithMasterKey: (
masterKey: MasterKey,
userId: string,
userKey?: EncString,
userId?: string,
) => Promise<UserKey>;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,9 @@

decryptUserKeyWithMasterKey(
masterKey: MasterKey,
userId: string,
userKey?: EncString,
userId?: string,
): Promise<UserKey> {
return this.mock.decryptUserKeyWithMasterKey(masterKey, userKey, userId);
return this.mock.decryptUserKeyWithMasterKey(masterKey, userId, userKey);

Check warning on line 70 in libs/common/src/auth/services/master-password/fake-master-password.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/auth/services/master-password/fake-master-password.service.ts#L70

Added line #L70 was not covered by tests
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { firstValueFrom, map, Observable } from "rxjs";

import { LogService } from "@bitwarden/common/platform/abstractions/log.service";

import { EncryptService } from "../../../platform/abstractions/encrypt.service";
import { KeyGenerationService } from "../../../platform/abstractions/key-generation.service";
import { StateService } from "../../../platform/abstractions/state.service";
Expand Down Expand Up @@ -55,6 +57,7 @@
private stateService: StateService,
private keyGenerationService: KeyGenerationService,
private encryptService: EncryptService,
private logService: LogService,

Check warning on line 60 in libs/common/src/auth/services/master-password/master-password.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/auth/services/master-password/master-password.service.ts#L60

Added line #L60 was not covered by tests
) {}

masterKey$(userId: UserId): Observable<MasterKey> {
Expand Down Expand Up @@ -149,10 +152,9 @@

async decryptUserKeyWithMasterKey(
masterKey: MasterKey,
userId: UserId,
userKey?: EncString,
userId?: UserId,
): Promise<UserKey> {
userId ??= await firstValueFrom(this.stateProvider.activeUserId$);
userKey ??= await this.getMasterKeyEncryptedUserKey(userId);
masterKey ??= await firstValueFrom(this.masterKey$(userId));

Expand Down Expand Up @@ -185,6 +187,7 @@
}

if (decUserKey == null) {
this.logService.warning("Failed to decrypt user key with master key.");

Check warning on line 190 in libs/common/src/auth/services/master-password/master-password.service.ts

View check run for this annotation

Codecov / codecov/patch

libs/common/src/auth/services/master-password/master-password.service.ts#L190

Added line #L190 was not covered by tests
return null;
}

Expand Down
Loading