vaultwarden
/
bitwarden-estensione-browser
mirror of https://github.com/bitwarden/browser
1
0
Fork 0
Code Issues 1 Releases Wiki Activity

Auth/PM-7235 - Refactor AuthService.getAuthStatus, deprecate everBeenUnlocked, and handle initialization of auto user key on client init (#8590) 

* PM-7235 - AuthSvc - Refactor getAuthStatus to simply use the cryptoService.hasUserKey check to determine the user's auth status.

* PM-7235 - CryptoSvc - getUserKey - remove setUserKey side effect if auto key is stored. Will move to app init

* PM-7235 - For each client init service, add setUserKeyInMemoryIfAutoUserKeySet logic

* PM-7235 - CryptoSvc tests - remove uncessary test.

* PM-7235 - Create UserKeyInitService and inject into all init services with new listening logic to support acct switching.

* PM-7235 - UserKeyInitSvc - minor refactor of setUserKeyInMemoryIfAutoUserKeySet

* PM-7235 - Add test suite for UserKeyInitService

* PM-7235 - Remove everBeenUnlocked as it is no longer needed

* PM-7235 - Fix tests

* PM-7235 - UserKeyInitSvc - per PR feedback, add error handling to protect observable stream from being cancelled in case of an error

* PM-7235 - Fix tests

* Update libs/common/src/platform/services/user-key-init.service.ts

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

* Update libs/common/src/platform/services/user-key-init.service.ts

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>

* PM-7235 - AuthSvc - Per PR review, for getAuthStatus, only check user key existence in memory.

* PM-7235 - remove not useful test per PR feedback.

* PM-7235 - Per PR feedback, update cryptoService.hasUserKey to only check memory for the user key.

* PM-7235 - Per PR feedback, move user key init service listener to main.background instead of init service

* PM-7235 - UserKeyInitSvc tests - fix tests to plass

---------

Co-authored-by: Matt Gibson <mgibson@bitwarden.com>
This commit is contained in:
Jared Snider 2024-04-19 11:20:13 -04:00 committed by GitHub
parent 6cafb1d28f
commit fffef95c5e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
17 changed files with 253 additions and 94 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
apps/browser/src/background/main.background.ts
View File

@ -110,6 +110,7 @@ import { MigrationBuilderService } from "@bitwarden/common/platform/services/mig
import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner";
import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider";
import { SystemService } from "@bitwarden/common/platform/services/system.service";
import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service";
import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service";
import {
ActiveUserStateProvider,
@ -325,6 +326,7 @@ export default class MainBackground {
stateEventRunnerService: StateEventRunnerService;
ssoLoginService: SsoLoginServiceAbstraction;
billingAccountProfileStateService: BillingAccountProfileStateService;
userKeyInitService: UserKeyInitService;
scriptInjectorService: BrowserScriptInjectorService;
onUpdatedRan: boolean;
@ -1046,6 +1048,12 @@ export default class MainBackground {
);
}
}
this.userKeyInitService = new UserKeyInitService(
this.accountService,
this.cryptoService,
this.logService,
);
}
async bootstrap() {
@ -1053,6 +1061,10 @@ export default class MainBackground {
await this.stateService.init({ runMigrations: !this.isPrivateMode });
// This is here instead of in in the InitService b/c we don't plan for
// side effects to run in the Browser InitService.
this.userKeyInitService.listenForActiveUserChangesToSetUserKey();
await (this.i18nService as I18nService).init();
await (this.eventUploadService as EventUploadService).init(true);
this.twoFactorService.init();

1
apps/browser/src/popup/services/init.service.ts
View File

@ -9,7 +9,6 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl
import { BrowserApi } from "../../platform/browser/browser-api";
import BrowserPopupUtils from "../../platform/popup/browser-popup-utils";
import { BrowserStateService as StateServiceAbstraction } from "../../platform/services/abstractions/browser-state.service";
@Injectable()
export class InitService {
constructor(

4
apps/desktop/src/app/services/init.service.ts
View File

@ -12,6 +12,7 @@ import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platfor
import { PlatformUtilsService as PlatformUtilsServiceAbstraction } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service";
import { ContainerService } from "@bitwarden/common/platform/services/container.service";
import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service";
import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service";
import { VaultTimeoutService } from "@bitwarden/common/services/vault-timeout/vault-timeout.service";
import { SyncService as SyncServiceAbstraction } from "@bitwarden/common/vault/abstractions/sync/sync.service.abstraction";
@ -35,6 +36,7 @@ export class InitService {
private nativeMessagingService: NativeMessagingService,
private themingService: AbstractThemingService,
private encryptService: EncryptService,
private userKeyInitService: UserKeyInitService,
@Inject(DOCUMENT) private document: Document,
) {}
@ -42,6 +44,8 @@ export class InitService {
return async () => {
this.nativeMessagingService.init();
await this.stateService.init({ runMigrations: false }); // Desktop will run them in main process
this.userKeyInitService.listenForActiveUserChangesToSetUserKey();
// FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
this.syncService.fullSync(true);

3
apps/web/src/app/core/init.service.ts
View File

@ -11,6 +11,7 @@ import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.
import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service";
import { StateService as StateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service";
import { ContainerService } from "@bitwarden/common/platform/services/container.service";
import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service";
import { EventUploadService } from "@bitwarden/common/services/event/event-upload.service";
import { VaultTimeoutService } from "@bitwarden/common/services/vault-timeout/vault-timeout.service";
@ -27,12 +28,14 @@ export class InitService {
private cryptoService: CryptoServiceAbstraction,
private themingService: AbstractThemingService,
private encryptService: EncryptService,
private userKeyInitService: UserKeyInitService,
@Inject(DOCUMENT) private document: Document,
) {}
init() {
return async () => {
await this.stateService.init();
this.userKeyInitService.listenForActiveUserChangesToSetUserKey();
setTimeout(() => this.notificationsService.init(), 3000);
await this.vaultTimeoutService.init(true);

1
libs/angular/src/auth/components/lock.component.ts
View File

@ -283,7 +283,6 @@ export class LockComponent implements OnInit, OnDestroy {
}
private async doContinue(evaluatePasswordAfterUnlock: boolean) {
await this.stateService.setEverBeenUnlocked(true);
await this.biometricStateService.resetUserPromptCancelled();
this.messagingService.send("unlocked");

7
libs/angular/src/services/jslib-services.module.ts
View File

@ -52,6 +52,7 @@ import { ProviderApiService } from "@bitwarden/common/admin-console/services/pro
import { ProviderService } from "@bitwarden/common/admin-console/services/provider.service";
import { AccountApiService as AccountApiServiceAbstraction } from "@bitwarden/common/auth/abstractions/account-api.service";
import {
AccountService,
AccountService as AccountServiceAbstraction,
InternalAccountService,
} from "@bitwarden/common/auth/abstractions/account.service";
@ -154,6 +155,7 @@ import { MigrationRunner } from "@bitwarden/common/platform/services/migration-r
import { NoopNotificationsService } from "@bitwarden/common/platform/services/noop-notifications.service";
import { StateService } from "@bitwarden/common/platform/services/state.service";
import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider";
import { UserKeyInitService } from "@bitwarden/common/platform/services/user-key-init.service";
import { ValidationService } from "@bitwarden/common/platform/services/validation.service";
import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service";
import {
@ -1115,6 +1117,11 @@ const safeProviders: SafeProvider[] = [
useClass: DefaultOrganizationManagementPreferencesService,
deps: [StateProvider],
}),
safeProvider({
provide: UserKeyInitService,
useClass: UserKeyInitService,
deps: [AccountService, CryptoServiceAbstraction, LogService],
}),
safeProvider({
provide: ErrorHandler,
useClass: LoggingErrorHandler,

29
libs/common/src/auth/services/auth.service.ts
View File

@ -12,7 +12,6 @@ import { ApiService } from "../../abstractions/api.service";
import { CryptoService } from "../../platform/abstractions/crypto.service";
import { MessagingService } from "../../platform/abstractions/messaging.service";
import { StateService } from "../../platform/abstractions/state.service";
import { KeySuffixOptions } from "../../platform/enums";
import { UserId } from "../../types/guid";
import { AccountService } from "../abstractions/account.service";
import { AuthService as AuthServiceAbstraction } from "../abstractions/auth.service";
@ -91,31 +90,11 @@ export class AuthService implements AuthServiceAbstraction {
return AuthenticationStatus.LoggedOut;
}
// If we don't have a user key in memory, we're locked
if (!(await this.cryptoService.hasUserKeyInMemory(userId))) {
// Check if the user has vault timeout set to never and verify that
// they've never unlocked their vault
const neverLock =
(await this.cryptoService.hasUserKeyStored(KeySuffixOptions.Auto, userId)) &&
!(await this.stateService.getEverBeenUnlocked({ userId: userId }));
// Note: since we aggresively set the auto user key to memory if it exists on app init (see InitService)
// we only need to check if the user key is in memory.
const hasUserKey = await this.cryptoService.hasUserKeyInMemory(userId as UserId);
if (neverLock) {
// Attempt to get the key from storage and set it in memory
const userKey = await this.cryptoService.getUserKeyFromStorage(
KeySuffixOptions.Auto,
userId,
);
await this.cryptoService.setUserKey(userKey, userId);
}
}
// We do another check here in case setting the auto key failed
const hasKeyInMemory = await this.cryptoService.hasUserKeyInMemory(userId);
if (!hasKeyInMemory) {
return AuthenticationStatus.Locked;
}
return AuthenticationStatus.Unlocked;
return hasUserKey ? AuthenticationStatus.Unlocked : AuthenticationStatus.Locked;
}
logOut(callback: () => void) {

2
libs/common/src/platform/abstractions/state.service.ts
View File

@ -148,8 +148,6 @@ export abstract class StateService<T extends Account = Account> {
* @deprecated For migration purposes only, use setEncryptedUserKeyPin instead
*/
setEncryptedPinProtected: (value: string, options?: StorageOptions) => Promise<void>;
getEverBeenUnlocked: (options?: StorageOptions) => Promise<boolean>;
setEverBeenUnlocked: (value: boolean, options?: StorageOptions) => Promise<void>;
getIsAuthenticated: (options?: StorageOptions) => Promise<boolean>;
getKdfConfig: (options?: StorageOptions) => Promise<KdfConfig>;
setKdfConfig: (kdfConfig: KdfConfig, options?: StorageOptions) => Promise<void>;

1
libs/common/src/platform/models/domain/account.ts
View File

@ -126,7 +126,6 @@ export class AccountProfile {
name?: string;
email?: string;
emailVerified?: boolean;
everBeenUnlocked?: boolean;
lastSync?: string;
userId?: string;
kdfIterations?: number;

27
libs/common/src/platform/services/crypto.service.spec.ts
View File

@ -91,21 +91,7 @@ describe("cryptoService", () => {
expect(userKey).toEqual(mockUserKey);
});
it("sets from the Auto key if the User Key if not set", async () => {
const autoKeyB64 =
"IT5cA1i5Hncd953pb00E58D2FqJX+fWTj4AvoI67qkGHSQPgulAqKv+LaKRAo9Bg0xzP9Nw00wk4TqjMmGSM+g==";
stateService.getUserKeyAutoUnlock.mockResolvedValue(autoKeyB64);
const setKeySpy = jest.spyOn(cryptoService, "setUserKey");
const userKey = await cryptoService.getUserKey(mockUserId);
expect(setKeySpy).toHaveBeenCalledWith(expect.any(SymmetricCryptoKey), mockUserId);
expect(setKeySpy).toHaveBeenCalledTimes(1);
expect(userKey.keyB64).toEqual(autoKeyB64);
});
it("returns nullish if there is no auto key and the user key is not set", async () => {
it("returns nullish if the user key is not set", async () => {
const userKey = await cryptoService.getUserKey(mockUserId);
expect(userKey).toBeFalsy();
@ -147,17 +133,6 @@ describe("cryptoService", () => {
},
);
describe("hasUserKey", () => {
it.each([true, false])(
"returns %s when the user key is not in memory, but the auto key is set",
async (hasKey) => {
stateProvider.singleUser.getFake(mockUserId, USER_KEY).nextState(null);
cryptoService.hasUserKeyStored = jest.fn().mockResolvedValue(hasKey);
expect(await cryptoService.hasUserKey(mockUserId)).toBe(hasKey);
},
);
});
describe("getUserKeyWithLegacySupport", () => {
let mockUserKey: UserKey;
let mockMasterKey: MasterKey;

20
libs/common/src/platform/services/crypto.service.ts
View File

@ -164,19 +164,8 @@ export class CryptoService implements CryptoServiceAbstraction {
}
async getUserKey(userId?: UserId): Promise<UserKey> {
let userKey = await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId));
if (userKey) {
return userKey;
}
// If the user has set their vault timeout to 'Never', we can load the user key from storage
if (await this.hasUserKeyStored(KeySuffixOptions.Auto, userId)) {
userKey = await this.getKeyFromStorage(KeySuffixOptions.Auto, userId);
if (userKey) {
await this.setUserKey(userKey, userId);
return userKey;
}
}
const userKey = await firstValueFrom(this.stateProvider.getUserState$(USER_KEY, userId));
return userKey;
}
async isLegacyUser(masterKey?: MasterKey, userId?: UserId): Promise<boolean> {
@ -217,10 +206,7 @@ export class CryptoService implements CryptoServiceAbstraction {
if (userId == null) {
return false;
}
return (
(await this.hasUserKeyInMemory(userId)) ||
(await this.hasUserKeyStored(KeySuffixOptions.Auto, userId))
);
return await this.hasUserKeyInMemory(userId);
}
async hasUserKeyInMemory(userId?: UserId): Promise<boolean> {

18
libs/common/src/platform/services/state.service.ts
View File

@ -634,24 +634,6 @@ export class StateService<
);
}
async getEverBeenUnlocked(options?: StorageOptions): Promise<boolean> {
return (
(await this.getAccount(this.reconcileOptions(options, await this.defaultInMemoryOptions())))
?.profile?.everBeenUnlocked ?? false
);
}
async setEverBeenUnlocked(value: boolean, options?: StorageOptions): Promise<void> {
const account = await this.getAccount(
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
account.profile.everBeenUnlocked = value;
await this.saveAccount(
account,
this.reconcileOptions(options, await this.defaultInMemoryOptions()),
);
}
async getIsAuthenticated(options?: StorageOptions): Promise<boolean> {
return (
(await this.tokenService.getAccessToken(options?.userId as UserId)) != null &&

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

@ -0,0 +1,162 @@
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 Normal file
View File

@ -0,0 +1,57 @@
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);
}
}

1
libs/common/src/services/vault-timeout/vault-timeout-settings.service.ts
View File

@ -172,7 +172,6 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
}
async clear(userId?: string): Promise<void> {
await this.stateService.setEverBeenUnlocked(false, { userId: userId });
await this.cryptoService.clearPinKeys(userId);
}

1
libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts
View File

@ -174,7 +174,6 @@ describe("VaultTimeoutService", () => {
// This does NOT assert all the things that the lock process does
expect(stateService.getIsAuthenticated).toHaveBeenCalledWith({ userId: userId });
expect(vaultTimeoutSettingsService.availableVaultTimeoutActions$).toHaveBeenCalledWith(userId);
expect(stateService.setEverBeenUnlocked).toHaveBeenCalledWith(true, { userId: userId });
expect(stateService.setUserKeyAutoUnlock).toHaveBeenCalledWith(null, { userId: userId });
expect(masterPasswordService.mock.clearMasterKey).toHaveBeenCalledWith(userId);
expect(cipherService.clearCache).toHaveBeenCalledWith(userId);

1
libs/common/src/services/vault-timeout/vault-timeout.service.ts
View File

@ -98,7 +98,6 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction {
await this.masterPasswordService.clearMasterKey((userId ?? currentUserId) as UserId);
await this.stateService.setEverBeenUnlocked(true, { userId: userId });
await this.stateService.setUserKeyAutoUnlock(null, { userId: userId });
await this.stateService.setCryptoMasterKeyAuto(null, { userId: userId });