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

Auth/PM-7811 - Refactor User Auto Unlock Key Hydration Process To Remove Race Conditions (#8979)

Browse Source 
* PM-7811 - Refactor UserKeyInitService to UserAutoUnlockKeyService - remove active account listening logic as it introduced race conditions with user key memory retrieval happening before the user auto unlock key was set into memory.

* PM-7811 - CLI - (1) Fix deps (2) On CLI init (pre command execution), if there is an active account, then set the user key in memory from the user auto unlock key.

* PM-7811 - Browser Extension / desktop - (1) Update deps (2) Sets user key in memory if the auto unlock key is set on account switch and background init (must act on all accounts so that account switcher displays unlock status properly).

* PM-7811 - Web - (1) Update deps (2) Sets user key in memory if the auto unlock key is set on init

* PM-7811 - Fix account switcher service changes not being necessary.
This commit is contained in:
Jared Snider
2024-04-29 17:43:14 -04:00
committed by GitHub
parent 82ae2fe62c
commit 20de053770
9 changed files with 165 additions and 246 deletions
Download Patch File Download Diff File Expand all files Collapse all files

71
libs/common/src/platform/services/user-auto-unlock-key.service.spec.ts Normal file
View File

@@ -0,0 +1,71 @@
import { mock } from "jest-mock-extended";
import { CsprngArray } from "../../types/csprng";
import { UserId } from "../../types/guid";
import { UserKey } from "../../types/key";
import { KeySuffixOptions } from "../enums";
import { Utils } from "../misc/utils";
import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
import { CryptoService } from "./crypto.service";
import { UserAutoUnlockKeyService } from "./user-auto-unlock-key.service";
describe("UserAutoUnlockKeyService", () => {
let userAutoUnlockKeyService: UserAutoUnlockKeyService;
const mockUserId = Utils.newGuid() as UserId;
const cryptoService = mock<CryptoService>();
beforeEach(() => {
userAutoUnlockKeyService = new UserAutoUnlockKeyService(cryptoService);
});
describe("setUserKeyInMemoryIfAutoUserKeySet", () => {
it("does nothing if the userId is null", async () => {
// Act
await (userAutoUnlockKeyService as any).setUserKeyInMemoryIfAutoUserKeySet(null);
// Assert
expect(cryptoService.getUserKeyFromStorage).not.toHaveBeenCalled();
expect(cryptoService.setUserKey).not.toHaveBeenCalled();
});
it("does nothing if the autoUserKey is null", async () => {
// Arrange
const userId = mockUserId;
cryptoService.getUserKeyFromStorage.mockResolvedValue(null);
// Act
await (userAutoUnlockKeyService as any).setUserKeyInMemoryIfAutoUserKeySet(userId);
// Assert
expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith(
KeySuffixOptions.Auto,
userId,
);
expect(cryptoService.setUserKey).not.toHaveBeenCalled();
});
it("sets the user key in memory if the autoUserKey is not null", async () => {
// Arrange
const userId = mockUserId;
const mockRandomBytes = new Uint8Array(64) as CsprngArray;
const mockAutoUserKey: UserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey;
cryptoService.getUserKeyFromStorage.mockResolvedValue(mockAutoUserKey);
// Act
await (userAutoUnlockKeyService as any).setUserKeyInMemoryIfAutoUserKeySet(userId);
// Assert
expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith(
KeySuffixOptions.Auto,
userId,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockAutoUserKey, userId);
});
});
});

36
libs/common/src/platform/services/user-auto-unlock-key.service.ts Normal file
View File

@@ -0,0 +1,36 @@
import { UserId } from "../../types/guid";
import { CryptoService } from "../abstractions/crypto.service";
import { KeySuffixOptions } from "../enums";
// TODO: this is a half measure improvement which allows us to reduce some side effects today (cryptoService.getUserKey setting user key in memory if auto key exists)
// but ideally, in the future, we would be able to put this logic into the cryptoService
// after the vault timeout settings service is transitioned to state provider so that
// the getUserKey logic can simply go to the correct location based on the vault timeout settings
// similar to the TokenService (it would either go to secure storage for the auto user key or memory for the user key)
export class UserAutoUnlockKeyService {
constructor(private cryptoService: CryptoService) {}
/**
* The presence of the user key in memory dictates whether the user's vault is locked or unlocked.
* However, for users that have the auto unlock user key set, we need to set the user key in memory
* on application bootstrap and on active account changes so that the user's vault loads unlocked.
* @param userId - The user id to check for an auto user key.
*/
async setUserKeyInMemoryIfAutoUserKeySet(userId: UserId): Promise<void> {
if (userId == null) {
return;
}
const autoUserKey = await this.cryptoService.getUserKeyFromStorage(
KeySuffixOptions.Auto,
userId,
);
if (autoUserKey == null) {
return;
}
await this.cryptoService.setUserKey(autoUserKey, userId);
}
}

162
libs/common/src/platform/services/user-key-init.service.spec.ts
View File

@@ -1,162 +0,0 @@
import { mock } from "jest-mock-extended";
import { FakeAccountService, mockAccountServiceWith } from "../../../spec";
import { CsprngArray } from "../../types/csprng";
import { UserId } from "../../types/guid";
import { UserKey } from "../../types/key";
import { LogService } from "../abstractions/log.service";
import { KeySuffixOptions } from "../enums";
import { Utils } from "../misc/utils";
import { SymmetricCryptoKey } from "../models/domain/symmetric-crypto-key";
import { CryptoService } from "./crypto.service";
import { UserKeyInitService } from "./user-key-init.service";
describe("UserKeyInitService", () => {
let userKeyInitService: UserKeyInitService;
const mockUserId = Utils.newGuid() as UserId;
const accountService: FakeAccountService = mockAccountServiceWith(mockUserId);
const cryptoService = mock<CryptoService>();
const logService = mock<LogService>();
beforeEach(() => {
userKeyInitService = new UserKeyInitService(accountService, cryptoService, logService);
});
describe("listenForActiveUserChangesToSetUserKey()", () => {
it("calls setUserKeyInMemoryIfAutoUserKeySet if there is an active user", () => {
// Arrange
accountService.activeAccountSubject.next({
id: mockUserId,
name: "name",
email: "email",
});
(userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet = jest.fn();
// Act
const subscription = userKeyInitService.listenForActiveUserChangesToSetUserKey();
// Assert
expect(subscription).not.toBeFalsy();
expect((userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet).toHaveBeenCalledWith(
mockUserId,
);
});
it("calls setUserKeyInMemoryIfAutoUserKeySet if there is an active user and tracks subsequent emissions", () => {
// Arrange
accountService.activeAccountSubject.next({
id: mockUserId,
name: "name",
email: "email",
});
const mockUser2Id = Utils.newGuid() as UserId;
jest
.spyOn(userKeyInitService as any, "setUserKeyInMemoryIfAutoUserKeySet")
.mockImplementation(() => Promise.resolve());
// Act
const subscription = userKeyInitService.listenForActiveUserChangesToSetUserKey();
accountService.activeAccountSubject.next({
id: mockUser2Id,
name: "name",
email: "email",
});
// Assert
expect(subscription).not.toBeFalsy();
expect((userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet).toHaveBeenCalledTimes(
2,
);
expect(
(userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet,
).toHaveBeenNthCalledWith(1, mockUserId);
expect(
(userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet,
).toHaveBeenNthCalledWith(2, mockUser2Id);
subscription.unsubscribe();
});
it("does not call setUserKeyInMemoryIfAutoUserKeySet if there is not an active user", () => {
// Arrange
accountService.activeAccountSubject.next(null);
(userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet = jest.fn();
// Act
const subscription = userKeyInitService.listenForActiveUserChangesToSetUserKey();
// Assert
expect(subscription).not.toBeFalsy();
expect(
(userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet,
).not.toHaveBeenCalledWith(mockUserId);
});
});
describe("setUserKeyInMemoryIfAutoUserKeySet", () => {
it("does nothing if the userId is null", async () => {
// Act
await (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet(null);
// Assert
expect(cryptoService.getUserKeyFromStorage).not.toHaveBeenCalled();
expect(cryptoService.setUserKey).not.toHaveBeenCalled();
});
it("does nothing if the autoUserKey is null", async () => {
// Arrange
const userId = mockUserId;
cryptoService.getUserKeyFromStorage.mockResolvedValue(null);
// Act
await (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet(userId);
// Assert
expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith(
KeySuffixOptions.Auto,
userId,
);
expect(cryptoService.setUserKey).not.toHaveBeenCalled();
});
it("sets the user key in memory if the autoUserKey is not null", async () => {
// Arrange
const userId = mockUserId;
const mockRandomBytes = new Uint8Array(64) as CsprngArray;
const mockAutoUserKey: UserKey = new SymmetricCryptoKey(mockRandomBytes) as UserKey;
cryptoService.getUserKeyFromStorage.mockResolvedValue(mockAutoUserKey);
// Act
await (userKeyInitService as any).setUserKeyInMemoryIfAutoUserKeySet(userId);
// Assert
expect(cryptoService.getUserKeyFromStorage).toHaveBeenCalledWith(
KeySuffixOptions.Auto,
userId,
);
expect(cryptoService.setUserKey).toHaveBeenCalledWith(mockAutoUserKey, userId);
});
});
});

57
libs/common/src/platform/services/user-key-init.service.ts
View File

@@ -1,57 +0,0 @@
import { EMPTY, Subscription, catchError, filter, from, switchMap } from "rxjs";
import { AccountService } from "../../auth/abstractions/account.service";
import { UserId } from "../../types/guid";
import { CryptoService } from "../abstractions/crypto.service";
import { LogService } from "../abstractions/log.service";
import { KeySuffixOptions } from "../enums";
// TODO: this is a half measure improvement which allows us to reduce some side effects today (cryptoService.getUserKey setting user key in memory if auto key exists)
// but ideally, in the future, we would be able to put this logic into the cryptoService
// after the vault timeout settings service is transitioned to state provider so that
// the getUserKey logic can simply go to the correct location based on the vault timeout settings
// similar to the TokenService (it would either go to secure storage for the auto user key or memory for the user key)
export class UserKeyInitService {
constructor(
private accountService: AccountService,
private cryptoService: CryptoService,
private logService: LogService,
) {}
// Note: must listen for changes to support account switching
listenForActiveUserChangesToSetUserKey(): Subscription {
return this.accountService.activeAccount$
.pipe(
filter((activeAccount) => activeAccount != null),
switchMap((activeAccount) =>
from(this.setUserKeyInMemoryIfAutoUserKeySet(activeAccount?.id)).pipe(
catchError((err: unknown) => {
this.logService.warning(
`setUserKeyInMemoryIfAutoUserKeySet failed with error: ${err}`,
);
// Returning EMPTY to protect observable chain from cancellation in case of error
return EMPTY;
}),
),
),
)
.subscribe();
}
private async setUserKeyInMemoryIfAutoUserKeySet(userId: UserId) {
if (userId == null) {
return;
}
const autoUserKey = await this.cryptoService.getUserKeyFromStorage(
KeySuffixOptions.Auto,
userId,
);
if (autoUserKey == null) {
return;
}
await this.cryptoService.setUserKey(autoUserKey, userId);
}
}