From 91f1d9fb86142805d0774182c3ee6234e13946e3 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Fri, 19 Apr 2024 16:44:24 -0400 Subject: [PATCH 01/16] Auth/PM-6689 - Migrate Security Stamp to Token Service and State Provider (#8792) * PM-6689 - Add security stamp to Token state * PM-6689 - Remove Security Stamp from account and state service * PM-6689 - Add security stamp get and set to token service + abstraction + tests * PM-6689 - Add migration for security stamp, test it, and register it with migrator * PM-6689 - Update sync service + deps to use token service. * PM-6689 - Cleanup missed usages of account tokens which has been removed. * PM-6689 - Per PR feedback, remove unnecessary data migration as the security stamp is only in memory and doesn't need to be migrated. --- .../browser/src/background/main.background.ts | 1 + apps/cli/src/bw.ts | 1 + .../src/services/jslib-services.module.ts | 1 + .../login-strategies/login.strategy.spec.ts | 4 - .../common/login-strategies/login.strategy.ts | 9 +-- .../src/auth/abstractions/token.service.ts | 6 ++ .../src/auth/services/token.service.spec.ts | 79 +++++++++++++++++++ .../common/src/auth/services/token.service.ts | 25 ++++++ .../src/auth/services/token.state.spec.ts | 2 + libs/common/src/auth/services/token.state.ts | 5 ++ .../platform/abstractions/state.service.ts | 2 - .../models/domain/account-tokens.spec.ts | 9 --- .../platform/models/domain/account.spec.ts | 4 +- .../src/platform/models/domain/account.ts | 18 ----- .../src/platform/services/state.service.ts | 17 ---- .../src/vault/services/sync/sync.service.ts | 6 +- 16 files changed, 126 insertions(+), 63 deletions(-) delete mode 100644 libs/common/src/platform/models/domain/account-tokens.spec.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 622a115067c..7d3471e598b 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -788,6 +788,7 @@ export default class MainBackground { this.avatarService, logoutCallback, this.billingAccountProfileStateService, + this.tokenService, ); this.eventUploadService = new EventUploadService( this.apiService, diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index e784997d824..c3c4042adff 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -633,6 +633,7 @@ export class Main { this.avatarService, async (expired: boolean) => await this.logout(), this.billingAccountProfileStateService, + this.tokenService, ); this.totpService = new TotpService(this.cryptoFunctionService, this.logService); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 204ff5a294b..9d311d34af5 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -628,6 +628,7 @@ const safeProviders: SafeProvider[] = [ AvatarServiceAbstraction, LOGOUT_CALLBACK, BillingAccountProfileStateService, + TokenServiceAbstraction, ], }), safeProvider({ diff --git a/libs/auth/src/common/login-strategies/login.strategy.spec.ts b/libs/auth/src/common/login-strategies/login.strategy.spec.ts index 431f736e949..e0833342ce3 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.spec.ts @@ -27,7 +27,6 @@ import { Utils } from "@bitwarden/common/platform/misc/utils"; import { Account, AccountProfile, - AccountTokens, AccountKeys, } from "@bitwarden/common/platform/models/domain/account"; import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; @@ -213,9 +212,6 @@ describe("LoginStrategy", () => { kdfType: kdf, }, }, - tokens: { - ...new AccountTokens(), - }, keys: new AccountKeys(), }), ); diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index a6dc1931839..a73c32e1208 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -27,11 +27,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; -import { - Account, - AccountProfile, - AccountTokens, -} from "@bitwarden/common/platform/models/domain/account"; +import { Account, AccountProfile } from "@bitwarden/common/platform/models/domain/account"; import { UserId } from "@bitwarden/common/types/guid"; import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction"; @@ -192,9 +188,6 @@ export abstract class LoginStrategy { kdfType: tokenResponse.kdf, }, }, - tokens: { - ...new AccountTokens(), - }, }), ); diff --git a/libs/common/src/auth/abstractions/token.service.ts b/libs/common/src/auth/abstractions/token.service.ts index 75bb3838828..fc3bd317f47 100644 --- a/libs/common/src/auth/abstractions/token.service.ts +++ b/libs/common/src/auth/abstractions/token.service.ts @@ -213,4 +213,10 @@ export abstract class TokenService { * @returns A promise that resolves with a boolean representing the user's external authN status. */ getIsExternal: () => Promise; + + /** Gets the active or passed in user's security stamp */ + getSecurityStamp: (userId?: UserId) => Promise; + + /** Sets the security stamp for the active or passed in user */ + setSecurityStamp: (securityStamp: string, userId?: UserId) => Promise; } diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index d32c4d8e1cd..3e92053d2f7 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/libs/common/src/auth/services/token.service.spec.ts @@ -23,6 +23,7 @@ import { EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, REFRESH_TOKEN_DISK, REFRESH_TOKEN_MEMORY, + SECURITY_STAMP_MEMORY, } from "./token.state"; describe("TokenService", () => { @@ -2191,6 +2192,84 @@ describe("TokenService", () => { }); }); + describe("Security Stamp methods", () => { + const mockSecurityStamp = "securityStamp"; + + describe("setSecurityStamp", () => { + it("should throw an error if no user id is provided and there is no active user in global state", async () => { + // Act + // note: don't await here because we want to test the error + const result = tokenService.setSecurityStamp(mockSecurityStamp); + // Assert + await expect(result).rejects.toThrow("User id not found. Cannot set security stamp."); + }); + + it("should set the security stamp in memory when there is an active user in global state", async () => { + // Arrange + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + + // Act + await tokenService.setSecurityStamp(mockSecurityStamp); + + // Assert + expect( + singleUserStateProvider.getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY).nextMock, + ).toHaveBeenCalledWith(mockSecurityStamp); + }); + + it("should set the security stamp in memory for the specified user id", async () => { + // Act + await tokenService.setSecurityStamp(mockSecurityStamp, userIdFromAccessToken); + + // Assert + expect( + singleUserStateProvider.getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY).nextMock, + ).toHaveBeenCalledWith(mockSecurityStamp); + }); + }); + + describe("getSecurityStamp", () => { + it("should throw an error if no user id is provided and there is no active user in global state", async () => { + // Act + // note: don't await here because we want to test the error + const result = tokenService.getSecurityStamp(); + // Assert + await expect(result).rejects.toThrow("User id not found. Cannot get security stamp."); + }); + + it("should return the security stamp from memory with no user id specified (uses global active user)", async () => { + // Arrange + globalStateProvider + .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) + .stateSubject.next(userIdFromAccessToken); + + singleUserStateProvider + .getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY) + .stateSubject.next([userIdFromAccessToken, mockSecurityStamp]); + + // Act + const result = await tokenService.getSecurityStamp(); + + // Assert + expect(result).toEqual(mockSecurityStamp); + }); + + it("should return the security stamp from memory for the specified user id", async () => { + // Arrange + singleUserStateProvider + .getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY) + .stateSubject.next([userIdFromAccessToken, mockSecurityStamp]); + + // Act + const result = await tokenService.getSecurityStamp(userIdFromAccessToken); + // Assert + expect(result).toEqual(mockSecurityStamp); + }); + }); + }); + // Helpers function createTokenService(supportsSecureStorage: boolean) { return new TokenService( diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index c24a2c186b8..40036a8453c 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -32,6 +32,7 @@ import { EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, REFRESH_TOKEN_DISK, REFRESH_TOKEN_MEMORY, + SECURITY_STAMP_MEMORY, } from "./token.state"; export enum TokenStorageLocation { @@ -850,6 +851,30 @@ export class TokenService implements TokenServiceAbstraction { return Array.isArray(decoded.amr) && decoded.amr.includes("external"); } + async getSecurityStamp(userId?: UserId): Promise { + userId ??= await firstValueFrom(this.activeUserIdGlobalState.state$); + + if (!userId) { + throw new Error("User id not found. Cannot get security stamp."); + } + + const securityStamp = await this.getStateValueByUserIdAndKeyDef(userId, SECURITY_STAMP_MEMORY); + + return securityStamp; + } + + async setSecurityStamp(securityStamp: string, userId?: UserId): Promise { + userId ??= await firstValueFrom(this.activeUserIdGlobalState.state$); + + if (!userId) { + throw new Error("User id not found. Cannot set security stamp."); + } + + await this.singleUserStateProvider + .get(userId, SECURITY_STAMP_MEMORY) + .update((_) => securityStamp); + } + private async getStateValueByUserIdAndKeyDef( userId: UserId, storageLocation: UserKeyDefinition, diff --git a/libs/common/src/auth/services/token.state.spec.ts b/libs/common/src/auth/services/token.state.spec.ts index dc00fec383c..bb82410fac1 100644 --- a/libs/common/src/auth/services/token.state.spec.ts +++ b/libs/common/src/auth/services/token.state.spec.ts @@ -10,6 +10,7 @@ import { EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, REFRESH_TOKEN_DISK, REFRESH_TOKEN_MEMORY, + SECURITY_STAMP_MEMORY, } from "./token.state"; describe.each([ @@ -22,6 +23,7 @@ describe.each([ [API_KEY_CLIENT_ID_MEMORY, "apiKeyClientIdMemory"], [API_KEY_CLIENT_SECRET_DISK, "apiKeyClientSecretDisk"], [API_KEY_CLIENT_SECRET_MEMORY, "apiKeyClientSecretMemory"], + [SECURITY_STAMP_MEMORY, "securityStamp"], ])( "deserializes state key definitions", ( diff --git a/libs/common/src/auth/services/token.state.ts b/libs/common/src/auth/services/token.state.ts index 458d6846c17..57d85f2a559 100644 --- a/libs/common/src/auth/services/token.state.ts +++ b/libs/common/src/auth/services/token.state.ts @@ -69,3 +69,8 @@ export const API_KEY_CLIENT_SECRET_MEMORY = new UserKeyDefinition( clearOn: [], // Manually handled }, ); + +export const SECURITY_STAMP_MEMORY = new UserKeyDefinition(TOKEN_MEMORY, "securityStamp", { + deserializer: (securityStamp) => securityStamp, + clearOn: ["logout"], +}); diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index 051604f0ae5..f1d4b3848ef 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -181,8 +181,6 @@ export abstract class StateService { * Sets the user's Pin, encrypted by the user key */ setProtectedPin: (value: string, options?: StorageOptions) => Promise; - getSecurityStamp: (options?: StorageOptions) => Promise; - setSecurityStamp: (value: string, options?: StorageOptions) => Promise; getUserId: (options?: StorageOptions) => Promise; getVaultTimeout: (options?: StorageOptions) => Promise; setVaultTimeout: (value: number, options?: StorageOptions) => Promise; diff --git a/libs/common/src/platform/models/domain/account-tokens.spec.ts b/libs/common/src/platform/models/domain/account-tokens.spec.ts deleted file mode 100644 index 733b3908e9a..00000000000 --- a/libs/common/src/platform/models/domain/account-tokens.spec.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { AccountTokens } from "./account"; - -describe("AccountTokens", () => { - describe("fromJSON", () => { - it("should deserialize to an instance of itself", () => { - expect(AccountTokens.fromJSON({})).toBeInstanceOf(AccountTokens); - }); - }); -}); diff --git a/libs/common/src/platform/models/domain/account.spec.ts b/libs/common/src/platform/models/domain/account.spec.ts index 0c76c16cc2d..77c242b6ff5 100644 --- a/libs/common/src/platform/models/domain/account.spec.ts +++ b/libs/common/src/platform/models/domain/account.spec.ts @@ -1,4 +1,4 @@ -import { Account, AccountKeys, AccountProfile, AccountSettings, AccountTokens } from "./account"; +import { Account, AccountKeys, AccountProfile, AccountSettings } from "./account"; describe("Account", () => { describe("fromJSON", () => { @@ -10,14 +10,12 @@ describe("Account", () => { const keysSpy = jest.spyOn(AccountKeys, "fromJSON"); const profileSpy = jest.spyOn(AccountProfile, "fromJSON"); const settingsSpy = jest.spyOn(AccountSettings, "fromJSON"); - const tokensSpy = jest.spyOn(AccountTokens, "fromJSON"); Account.fromJSON({}); expect(keysSpy).toHaveBeenCalled(); expect(profileSpy).toHaveBeenCalled(); expect(settingsSpy).toHaveBeenCalled(); - expect(tokensSpy).toHaveBeenCalled(); }); }); }); diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index 5a9a7646962..cd416ec1f94 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -171,24 +171,11 @@ export class AccountSettings { } } -export class AccountTokens { - securityStamp?: string; - - static fromJSON(obj: Jsonify): AccountTokens { - if (obj == null) { - return null; - } - - return Object.assign(new AccountTokens(), obj); - } -} - export class Account { data?: AccountData = new AccountData(); keys?: AccountKeys = new AccountKeys(); profile?: AccountProfile = new AccountProfile(); settings?: AccountSettings = new AccountSettings(); - tokens?: AccountTokens = new AccountTokens(); constructor(init: Partial) { Object.assign(this, { @@ -208,10 +195,6 @@ export class Account { ...new AccountSettings(), ...init?.settings, }, - tokens: { - ...new AccountTokens(), - ...init?.tokens, - }, }); } @@ -225,7 +208,6 @@ export class Account { data: AccountData.fromJSON(json?.data), profile: AccountProfile.fromJSON(json?.profile), settings: AccountSettings.fromJSON(json?.settings), - tokens: AccountTokens.fromJSON(json?.tokens), }); } } diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index f660cd7a342..d0a55d7a47c 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -839,23 +839,6 @@ export class StateService< ); } - async getSecurityStamp(options?: StorageOptions): Promise { - return ( - await this.getAccount(this.reconcileOptions(options, await this.defaultInMemoryOptions())) - )?.tokens?.securityStamp; - } - - async setSecurityStamp(value: string, options?: StorageOptions): Promise { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - account.tokens.securityStamp = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultInMemoryOptions()), - ); - } - async getUserId(options?: StorageOptions): Promise { return ( await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())) diff --git a/libs/common/src/vault/services/sync/sync.service.ts b/libs/common/src/vault/services/sync/sync.service.ts index ff8e9f1f4f5..73869ff488e 100644 --- a/libs/common/src/vault/services/sync/sync.service.ts +++ b/libs/common/src/vault/services/sync/sync.service.ts @@ -15,6 +15,7 @@ import { AccountService } from "../../../auth/abstractions/account.service"; import { AvatarService } from "../../../auth/abstractions/avatar.service"; import { KeyConnectorService } from "../../../auth/abstractions/key-connector.service"; import { InternalMasterPasswordServiceAbstraction } from "../../../auth/abstractions/master-password.service.abstraction"; +import { TokenService } from "../../../auth/abstractions/token.service"; import { ForceSetPasswordReason } from "../../../auth/models/domain/force-set-password-reason"; import { DomainSettingsService } from "../../../autofill/services/domain-settings.service"; import { BillingAccountProfileStateService } from "../../../billing/abstractions/account/billing-account-profile-state.service"; @@ -73,6 +74,7 @@ export class SyncService implements SyncServiceAbstraction { private avatarService: AvatarService, private logoutCallback: (expired: boolean) => Promise, private billingAccountProfileStateService: BillingAccountProfileStateService, + private tokenService: TokenService, ) {} async getLastSync(): Promise { @@ -309,7 +311,7 @@ export class SyncService implements SyncServiceAbstraction { } private async syncProfile(response: ProfileResponse) { - const stamp = await this.stateService.getSecurityStamp(); + const stamp = await this.tokenService.getSecurityStamp(response.id as UserId); if (stamp != null && stamp !== response.securityStamp) { if (this.logoutCallback != null) { await this.logoutCallback(true); @@ -323,7 +325,7 @@ export class SyncService implements SyncServiceAbstraction { await this.cryptoService.setProviderKeys(response.providers); await this.cryptoService.setOrgKeys(response.organizations, response.providerOrganizations); await this.avatarService.setSyncAvatarColor(response.id as UserId, response.avatarColor); - await this.stateService.setSecurityStamp(response.securityStamp); + await this.tokenService.setSecurityStamp(response.securityStamp, response.id as UserId); await this.stateService.setEmailVerified(response.emailVerified); await this.billingAccountProfileStateService.setHasPremium( From f829cdd8a724dae42ab93aaadf930d433dd2cc4d Mon Sep 17 00:00:00 2001 From: aj-rosado <109146700+aj-rosado@users.noreply.github.com> Date: Mon, 22 Apr 2024 10:18:11 +0100 Subject: [PATCH 02/16] [PM-7603] Fix individual vault export not appearing on Event Logs (#8829) * Added validation to update User_ClientExportedVault on events even with no organization id or cipher id * Fixed missing data and validation --- .../common/src/services/event/event-collection.service.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libs/common/src/services/event/event-collection.service.ts b/libs/common/src/services/event/event-collection.service.ts index 641c1b4d44b..1482bb8b61e 100644 --- a/libs/common/src/services/event/event-collection.service.ts +++ b/libs/common/src/services/event/event-collection.service.ts @@ -36,7 +36,7 @@ export class EventCollectionService implements EventCollectionServiceAbstraction const userId = await firstValueFrom(this.stateProvider.activeUserId$); const eventStore = this.stateProvider.getUser(userId, EVENT_COLLECTION); - if (!(await this.shouldUpdate(cipherId, organizationId))) { + if (!(await this.shouldUpdate(cipherId, organizationId, eventType))) { return; } @@ -64,6 +64,7 @@ export class EventCollectionService implements EventCollectionServiceAbstraction private async shouldUpdate( cipherId: string = null, organizationId: string = null, + eventType: EventType = null, ): Promise { const orgIds$ = this.organizationService.organizations$.pipe( map((orgs) => orgs?.filter((o) => o.useEvents)?.map((x) => x.id) ?? []), @@ -85,6 +86,11 @@ export class EventCollectionService implements EventCollectionServiceAbstraction return false; } + // Individual vault export doesn't need cipher id or organization id. + if (eventType == EventType.User_ClientExportedVault) { + return true; + } + // If the cipher is null there must be an organization id provided if (cipher == null && organizationId == null) { return false; From b5362ca1ce6f4260b3ae52eecc6b0e9ace325945 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Mon, 22 Apr 2024 08:55:19 -0400 Subject: [PATCH 03/16] Browser MV3: Default store values to session storage (#8844) * Introduce browser large object storage location. This location is encrypted and serialized to disk in order to allow for storage of uncountable things like vault items that take a significant amount of time to prepare, but are not guaranteed to fit within session storage. however, limit the need to write to disk is a big benefit, so _most_ things are written to storage.session instead, where things specifically flagged as large will be moved to disk-backed memory * Store derived values in large object store for browser * Fix AbstractMemoryStorageService implementation --- .../browser/src/background/main.background.ts | 18 ++++++---- .../derived-state-provider.factory.ts | 10 +++--- .../browser-memory-storage.service.ts | 11 +++++- .../background-derived-state.provider.ts | 9 ++++- .../state/background-derived-state.ts | 2 +- .../state/derived-state-interactions.spec.ts | 28 +++++++++------ .../foreground-derived-state.provider.ts | 9 +++-- .../state/foreground-derived-state.spec.ts | 3 +- .../state/foreground-derived-state.ts | 3 +- .../browser-storage-service.provider.ts | 35 +++++++++++++++++++ .../src/popup/services/services.module.ts | 32 ++++++++++++++++- apps/cli/src/bw.ts | 4 +-- apps/desktop/src/main.ts | 2 +- .../src/services/jslib-services.module.ts | 2 +- libs/common/spec/fake-state-provider.ts | 4 +-- .../src/platform/state/derive-definition.ts | 4 +-- .../default-derived-state.provider.ts | 17 ++++++--- .../default-derived-state.spec.ts | 16 ++++----- .../src/platform/state/state-definition.ts | 4 ++- .../src/platform/state/state-definitions.ts | 16 ++++++--- 20 files changed, 171 insertions(+), 58 deletions(-) create mode 100644 apps/browser/src/platform/storage/browser-storage-service.provider.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 7d3471e598b..0a9ad44962c 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -111,7 +111,6 @@ import { KeyGenerationService } from "@bitwarden/common/platform/services/key-ge import { MemoryStorageService } from "@bitwarden/common/platform/services/memory-storage.service"; import { MigrationBuilderService } from "@bitwarden/common/platform/services/migration-builder.service"; 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"; @@ -226,6 +225,7 @@ import { BackgroundPlatformUtilsService } from "../platform/services/platform-ut import { BrowserPlatformUtilsService } from "../platform/services/platform-utils/browser-platform-utils.service"; import { BackgroundDerivedStateProvider } from "../platform/state/background-derived-state.provider"; import { BackgroundMemoryStorageService } from "../platform/storage/background-memory-storage.service"; +import { BrowserStorageServiceProvider } from "../platform/storage/browser-storage-service.provider"; import { ForegroundMemoryStorageService } from "../platform/storage/foreground-memory-storage.service"; import { fromChromeRuntimeMessaging } from "../platform/utils/from-chrome-runtime-messaging"; import VaultTimeoutService from "../services/vault-timeout/vault-timeout.service"; @@ -246,6 +246,8 @@ export default class MainBackground { secureStorageService: AbstractStorageService; memoryStorageService: AbstractMemoryStorageService; memoryStorageForStateProviders: AbstractMemoryStorageService & ObservableStorageService; + largeObjectMemoryStorageForStateProviders: AbstractMemoryStorageService & + ObservableStorageService; i18nService: I18nServiceAbstraction; platformUtilsService: PlatformUtilsServiceAbstraction; logService: LogServiceAbstraction; @@ -424,12 +426,16 @@ export default class MainBackground { ? mv3MemoryStorageCreator("stateService") : new MemoryStorageService(); this.memoryStorageForStateProviders = BrowserApi.isManifestVersion(3) - ? mv3MemoryStorageCreator("stateProviders") - : new BackgroundMemoryStorageService(); + ? new BrowserMemoryStorageService() // mv3 stores to storage.session + : new BackgroundMemoryStorageService(); // mv2 stores to memory + this.largeObjectMemoryStorageForStateProviders = BrowserApi.isManifestVersion(3) + ? mv3MemoryStorageCreator("stateProviders") // mv3 stores to local-backed session storage + : this.memoryStorageForStateProviders; // mv2 stores to the same location - const storageServiceProvider = new StorageServiceProvider( + const storageServiceProvider = new BrowserStorageServiceProvider( this.storageService, this.memoryStorageForStateProviders, + this.largeObjectMemoryStorageForStateProviders, ); this.globalStateProvider = new DefaultGlobalStateProvider(storageServiceProvider); @@ -466,9 +472,7 @@ export default class MainBackground { this.accountService, this.singleUserStateProvider, ); - this.derivedStateProvider = new BackgroundDerivedStateProvider( - this.memoryStorageForStateProviders, - ); + this.derivedStateProvider = new BackgroundDerivedStateProvider(storageServiceProvider); this.stateProvider = new DefaultStateProvider( this.activeUserStateProvider, this.singleUserStateProvider, diff --git a/apps/browser/src/platform/background/service-factories/derived-state-provider.factory.ts b/apps/browser/src/platform/background/service-factories/derived-state-provider.factory.ts index 4f329c93d5e..4025d01950f 100644 --- a/apps/browser/src/platform/background/service-factories/derived-state-provider.factory.ts +++ b/apps/browser/src/platform/background/service-factories/derived-state-provider.factory.ts @@ -4,14 +4,14 @@ import { BackgroundDerivedStateProvider } from "../../state/background-derived-s import { CachedServices, FactoryOptions, factory } from "./factory-options"; import { - MemoryStorageServiceInitOptions, - observableMemoryStorageServiceFactory, -} from "./storage-service.factory"; + StorageServiceProviderInitOptions, + storageServiceProviderFactory, +} from "./storage-service-provider.factory"; type DerivedStateProviderFactoryOptions = FactoryOptions; export type DerivedStateProviderInitOptions = DerivedStateProviderFactoryOptions & - MemoryStorageServiceInitOptions; + StorageServiceProviderInitOptions; export async function derivedStateProviderFactory( cache: { derivedStateProvider?: DerivedStateProvider } & CachedServices, @@ -22,6 +22,6 @@ export async function derivedStateProviderFactory( "derivedStateProvider", opts, async () => - new BackgroundDerivedStateProvider(await observableMemoryStorageServiceFactory(cache, opts)), + new BackgroundDerivedStateProvider(await storageServiceProviderFactory(cache, opts)), ); } diff --git a/apps/browser/src/platform/services/browser-memory-storage.service.ts b/apps/browser/src/platform/services/browser-memory-storage.service.ts index f824a1df0de..b067dc5a128 100644 --- a/apps/browser/src/platform/services/browser-memory-storage.service.ts +++ b/apps/browser/src/platform/services/browser-memory-storage.service.ts @@ -1,7 +1,16 @@ +import { AbstractMemoryStorageService } from "@bitwarden/common/platform/abstractions/storage.service"; + import AbstractChromeStorageService from "./abstractions/abstract-chrome-storage-api.service"; -export default class BrowserMemoryStorageService extends AbstractChromeStorageService { +export default class BrowserMemoryStorageService + extends AbstractChromeStorageService + implements AbstractMemoryStorageService +{ constructor() { super(chrome.storage.session); } + type = "MemoryStorageService" as const; + getBypassCache(key: string): Promise { + return this.get(key); + } } diff --git a/apps/browser/src/platform/state/background-derived-state.provider.ts b/apps/browser/src/platform/state/background-derived-state.provider.ts index 95eec711132..f3d217789ed 100644 --- a/apps/browser/src/platform/state/background-derived-state.provider.ts +++ b/apps/browser/src/platform/state/background-derived-state.provider.ts @@ -1,5 +1,9 @@ import { Observable } from "rxjs"; +import { + AbstractStorageService, + ObservableStorageService, +} from "@bitwarden/common/platform/abstractions/storage.service"; import { DeriveDefinition, DerivedState } from "@bitwarden/common/platform/state"; // eslint-disable-next-line import/no-restricted-paths -- extending this class for this client import { DefaultDerivedStateProvider } from "@bitwarden/common/platform/state/implementations/default-derived-state.provider"; @@ -12,11 +16,14 @@ export class BackgroundDerivedStateProvider extends DefaultDerivedStateProvider parentState$: Observable, deriveDefinition: DeriveDefinition, dependencies: TDeps, + storageLocation: [string, AbstractStorageService & ObservableStorageService], ): DerivedState { + const [cacheKey, storageService] = storageLocation; return new BackgroundDerivedState( parentState$, deriveDefinition, - this.memoryStorage, + storageService, + cacheKey, dependencies, ); } diff --git a/apps/browser/src/platform/state/background-derived-state.ts b/apps/browser/src/platform/state/background-derived-state.ts index 7a7146aa886..c62795acdcd 100644 --- a/apps/browser/src/platform/state/background-derived-state.ts +++ b/apps/browser/src/platform/state/background-derived-state.ts @@ -23,10 +23,10 @@ export class BackgroundDerivedState< parentState$: Observable, deriveDefinition: DeriveDefinition, memoryStorage: AbstractStorageService & ObservableStorageService, + portName: string, dependencies: TDeps, ) { super(parentState$, deriveDefinition, memoryStorage, dependencies); - const portName = deriveDefinition.buildCacheKey(); // listen for foreground derived states to connect BrowserApi.addListener(chrome.runtime.onConnect, (port) => { diff --git a/apps/browser/src/platform/state/derived-state-interactions.spec.ts b/apps/browser/src/platform/state/derived-state-interactions.spec.ts index d709c401af0..a5df01bc989 100644 --- a/apps/browser/src/platform/state/derived-state-interactions.spec.ts +++ b/apps/browser/src/platform/state/derived-state-interactions.spec.ts @@ -38,14 +38,21 @@ describe("foreground background derived state interactions", () => { let memoryStorage: FakeStorageService; const initialParent = "2020-01-01"; const ngZone = mock(); + const portName = "testPort"; beforeEach(() => { mockPorts(); parentState$ = new Subject(); memoryStorage = new FakeStorageService(); - background = new BackgroundDerivedState(parentState$, deriveDefinition, memoryStorage, {}); - foreground = new ForegroundDerivedState(deriveDefinition, memoryStorage, ngZone); + background = new BackgroundDerivedState( + parentState$, + deriveDefinition, + memoryStorage, + portName, + {}, + ); + foreground = new ForegroundDerivedState(deriveDefinition, memoryStorage, portName, ngZone); }); afterEach(() => { @@ -65,7 +72,12 @@ describe("foreground background derived state interactions", () => { }); it("should initialize a late-connected foreground", async () => { - const newForeground = new ForegroundDerivedState(deriveDefinition, memoryStorage, ngZone); + const newForeground = new ForegroundDerivedState( + deriveDefinition, + memoryStorage, + portName, + ngZone, + ); const backgroundEmissions = trackEmissions(background.state$); parentState$.next(initialParent); await awaitAsync(); @@ -82,8 +94,6 @@ describe("foreground background derived state interactions", () => { const dateString = "2020-12-12"; const emissions = trackEmissions(background.state$); - // 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 await foreground.forceValue(new Date(dateString)); await awaitAsync(); @@ -99,9 +109,7 @@ describe("foreground background derived state interactions", () => { expect(foreground["port"]).toBeDefined(); const newDate = new Date(); - // 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 - foreground.forceValue(newDate); + await foreground.forceValue(newDate); await awaitAsync(); expect(connectMock.mock.calls.length).toBe(initialConnectCalls); @@ -114,9 +122,7 @@ describe("foreground background derived state interactions", () => { expect(foreground["port"]).toBeUndefined(); const newDate = new Date(); - // 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 - foreground.forceValue(newDate); + await foreground.forceValue(newDate); await awaitAsync(); expect(connectMock.mock.calls.length).toBe(initialConnectCalls + 1); diff --git a/apps/browser/src/platform/state/foreground-derived-state.provider.ts b/apps/browser/src/platform/state/foreground-derived-state.provider.ts index ccefb1157c1..d9262e3b6e7 100644 --- a/apps/browser/src/platform/state/foreground-derived-state.provider.ts +++ b/apps/browser/src/platform/state/foreground-derived-state.provider.ts @@ -5,6 +5,7 @@ import { AbstractStorageService, ObservableStorageService, } from "@bitwarden/common/platform/abstractions/storage.service"; +import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; import { DeriveDefinition, DerivedState } from "@bitwarden/common/platform/state"; // eslint-disable-next-line import/no-restricted-paths -- extending this class for this client import { DefaultDerivedStateProvider } from "@bitwarden/common/platform/state/implementations/default-derived-state.provider"; @@ -14,16 +15,18 @@ import { ForegroundDerivedState } from "./foreground-derived-state"; export class ForegroundDerivedStateProvider extends DefaultDerivedStateProvider { constructor( - memoryStorage: AbstractStorageService & ObservableStorageService, + storageServiceProvider: StorageServiceProvider, private ngZone: NgZone, ) { - super(memoryStorage); + super(storageServiceProvider); } override buildDerivedState( _parentState$: Observable, deriveDefinition: DeriveDefinition, _dependencies: TDeps, + storageLocation: [string, AbstractStorageService & ObservableStorageService], ): DerivedState { - return new ForegroundDerivedState(deriveDefinition, this.memoryStorage, this.ngZone); + const [cacheKey, storageService] = storageLocation; + return new ForegroundDerivedState(deriveDefinition, storageService, cacheKey, this.ngZone); } } diff --git a/apps/browser/src/platform/state/foreground-derived-state.spec.ts b/apps/browser/src/platform/state/foreground-derived-state.spec.ts index fce672a5ef5..2c29f39bc12 100644 --- a/apps/browser/src/platform/state/foreground-derived-state.spec.ts +++ b/apps/browser/src/platform/state/foreground-derived-state.spec.ts @@ -33,13 +33,14 @@ jest.mock("../browser/run-inside-angular.operator", () => { describe("ForegroundDerivedState", () => { let sut: ForegroundDerivedState; let memoryStorage: FakeStorageService; + const portName = "testPort"; const ngZone = mock(); beforeEach(() => { memoryStorage = new FakeStorageService(); memoryStorage.internalUpdateValuesRequireDeserialization(true); mockPorts(); - sut = new ForegroundDerivedState(deriveDefinition, memoryStorage, ngZone); + sut = new ForegroundDerivedState(deriveDefinition, memoryStorage, portName, ngZone); }); afterEach(() => { diff --git a/apps/browser/src/platform/state/foreground-derived-state.ts b/apps/browser/src/platform/state/foreground-derived-state.ts index b005697be89..b9dda763dfd 100644 --- a/apps/browser/src/platform/state/foreground-derived-state.ts +++ b/apps/browser/src/platform/state/foreground-derived-state.ts @@ -35,6 +35,7 @@ export class ForegroundDerivedState implements DerivedState { constructor( private deriveDefinition: DeriveDefinition, private memoryStorage: AbstractStorageService & ObservableStorageService, + private portName: string, private ngZone: NgZone, ) { this.storageKey = deriveDefinition.storageKey; @@ -88,7 +89,7 @@ export class ForegroundDerivedState implements DerivedState { return; } - this.port = chrome.runtime.connect({ name: this.deriveDefinition.buildCacheKey() }); + this.port = chrome.runtime.connect({ name: this.portName }); this.backgroundResponses$ = fromChromeEvent(this.port.onMessage).pipe( map(([message]) => message as DerivedStateMessage), diff --git a/apps/browser/src/platform/storage/browser-storage-service.provider.ts b/apps/browser/src/platform/storage/browser-storage-service.provider.ts new file mode 100644 index 00000000000..e0214baef44 --- /dev/null +++ b/apps/browser/src/platform/storage/browser-storage-service.provider.ts @@ -0,0 +1,35 @@ +import { + AbstractStorageService, + ObservableStorageService, +} from "@bitwarden/common/platform/abstractions/storage.service"; +import { + PossibleLocation, + StorageServiceProvider, +} from "@bitwarden/common/platform/services/storage-service.provider"; +// eslint-disable-next-line import/no-restricted-paths +import { ClientLocations } from "@bitwarden/common/platform/state/state-definition"; + +export class BrowserStorageServiceProvider extends StorageServiceProvider { + constructor( + diskStorageService: AbstractStorageService & ObservableStorageService, + limitedMemoryStorageService: AbstractStorageService & ObservableStorageService, + private largeObjectMemoryStorageService: AbstractStorageService & ObservableStorageService, + ) { + super(diskStorageService, limitedMemoryStorageService); + } + + override get( + defaultLocation: PossibleLocation, + overrides: Partial, + ): [location: PossibleLocation, service: AbstractStorageService & ObservableStorageService] { + const location = overrides["browser"] ?? defaultLocation; + switch (location) { + case "memory-large-object": + return ["memory-large-object", this.largeObjectMemoryStorageService]; + default: + // Pass in computed location to super because they could have + // override default "disk" with web "memory". + return super.get(location, overrides); + } + } +} diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 123e901e4e3..a7da6b76127 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -71,6 +71,7 @@ import { GlobalState } from "@bitwarden/common/platform/models/domain/global-sta import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service"; import { ContainerService } from "@bitwarden/common/platform/services/container.service"; import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; +import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { DerivedStateProvider, @@ -108,6 +109,7 @@ import { DefaultBrowserStateService } from "../../platform/services/default-brow import I18nService from "../../platform/services/i18n.service"; import { ForegroundPlatformUtilsService } from "../../platform/services/platform-utils/foreground-platform-utils.service"; import { ForegroundDerivedStateProvider } from "../../platform/state/foreground-derived-state.provider"; +import { BrowserStorageServiceProvider } from "../../platform/storage/browser-storage-service.provider"; import { ForegroundMemoryStorageService } from "../../platform/storage/foreground-memory-storage.service"; import { fromChromeRuntimeMessaging } from "../../platform/utils/from-chrome-runtime-messaging"; import { BrowserSendStateService } from "../../tools/popup/services/browser-send-state.service"; @@ -120,6 +122,10 @@ import { InitService } from "./init.service"; import { PopupCloseWarningService } from "./popup-close-warning.service"; import { PopupSearchService } from "./popup-search.service"; +const OBSERVABLE_LARGE_OBJECT_MEMORY_STORAGE = new SafeInjectionToken< + AbstractStorageService & ObservableStorageService +>("OBSERVABLE_LARGE_OBJECT_MEMORY_STORAGE"); + const needsBackgroundInit = BrowserPopupUtils.backgroundInitializationRequired(); const isPrivateMode = BrowserPopupUtils.inPrivateMode(); const mainBackground: MainBackground = needsBackgroundInit @@ -380,6 +386,21 @@ const safeProviders: SafeProvider[] = [ }, deps: [], }), + safeProvider({ + provide: OBSERVABLE_LARGE_OBJECT_MEMORY_STORAGE, + useFactory: ( + regularMemoryStorageService: AbstractMemoryStorageService & ObservableStorageService, + ) => { + if (BrowserApi.isManifestVersion(2)) { + return regularMemoryStorageService; + } + + return getBgService( + "largeObjectMemoryStorageForStateProviders", + )(); + }, + deps: [OBSERVABLE_MEMORY_STORAGE], + }), safeProvider({ provide: OBSERVABLE_DISK_STORAGE, useExisting: AbstractStorageService, @@ -466,7 +487,7 @@ const safeProviders: SafeProvider[] = [ safeProvider({ provide: DerivedStateProvider, useClass: ForegroundDerivedStateProvider, - deps: [OBSERVABLE_MEMORY_STORAGE, NgZone], + deps: [StorageServiceProvider, NgZone], }), safeProvider({ provide: AutofillSettingsServiceAbstraction, @@ -542,6 +563,15 @@ const safeProviders: SafeProvider[] = [ }, deps: [], }), + safeProvider({ + provide: StorageServiceProvider, + useClass: BrowserStorageServiceProvider, + deps: [ + OBSERVABLE_DISK_STORAGE, + OBSERVABLE_MEMORY_STORAGE, + OBSERVABLE_LARGE_OBJECT_MEMORY_STORAGE, + ], + }), ]; @NgModule({ diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index c3c4042adff..437f807bc61 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -309,9 +309,7 @@ export class Main { this.singleUserStateProvider, ); - this.derivedStateProvider = new DefaultDerivedStateProvider( - this.memoryStorageForStateProviders, - ); + this.derivedStateProvider = new DefaultDerivedStateProvider(storageServiceProvider); this.stateProvider = new DefaultStateProvider( this.activeUserStateProvider, diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts index 0655e5600d2..bffd2002ff1 100644 --- a/apps/desktop/src/main.ts +++ b/apps/desktop/src/main.ts @@ -157,7 +157,7 @@ export class Main { activeUserStateProvider, singleUserStateProvider, globalStateProvider, - new DefaultDerivedStateProvider(this.memoryStorageForStateProviders), + new DefaultDerivedStateProvider(storageServiceProvider), ); this.environmentService = new DefaultEnvironmentService(stateProvider, accountService); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 9d311d34af5..27b182de5dc 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -1047,7 +1047,7 @@ const safeProviders: SafeProvider[] = [ safeProvider({ provide: DerivedStateProvider, useClass: DefaultDerivedStateProvider, - deps: [OBSERVABLE_MEMORY_STORAGE], + deps: [StorageServiceProvider], }), safeProvider({ provide: StateProvider, diff --git a/libs/common/spec/fake-state-provider.ts b/libs/common/spec/fake-state-provider.ts index 2078fe3abde..306ae00c215 100644 --- a/libs/common/spec/fake-state-provider.ts +++ b/libs/common/spec/fake-state-provider.ts @@ -249,11 +249,11 @@ export class FakeDerivedStateProvider implements DerivedStateProvider { deriveDefinition: DeriveDefinition, dependencies: TDeps, ): DerivedState { - let result = this.states.get(deriveDefinition.buildCacheKey()) as DerivedState; + let result = this.states.get(deriveDefinition.buildCacheKey("memory")) as DerivedState; if (result == null) { result = new FakeDerivedState(parentState$, deriveDefinition, dependencies); - this.states.set(deriveDefinition.buildCacheKey(), result); + this.states.set(deriveDefinition.buildCacheKey("memory"), result); } return result; } diff --git a/libs/common/src/platform/state/derive-definition.ts b/libs/common/src/platform/state/derive-definition.ts index 8f62d3a342c..9cb5eff3e8c 100644 --- a/libs/common/src/platform/state/derive-definition.ts +++ b/libs/common/src/platform/state/derive-definition.ts @@ -171,8 +171,8 @@ export class DeriveDefinition> = {}; - constructor(protected memoryStorage: AbstractStorageService & ObservableStorageService) {} + constructor(protected storageServiceProvider: StorageServiceProvider) {} get( parentState$: Observable, deriveDefinition: DeriveDefinition, dependencies: TDeps, ): DerivedState { - const cacheKey = deriveDefinition.buildCacheKey(); + // TODO: we probably want to support optional normal memory storage for browser + const [location, storageService] = this.storageServiceProvider.get("memory", { + browser: "memory-large-object", + }); + const cacheKey = deriveDefinition.buildCacheKey(location); const existingDerivedState = this.cache[cacheKey]; if (existingDerivedState != null) { // I have to cast out of the unknown generic but this should be safe if rules @@ -29,7 +34,10 @@ export class DefaultDerivedStateProvider implements DerivedStateProvider { return existingDerivedState as DefaultDerivedState; } - const newDerivedState = this.buildDerivedState(parentState$, deriveDefinition, dependencies); + const newDerivedState = this.buildDerivedState(parentState$, deriveDefinition, dependencies, [ + location, + storageService, + ]); this.cache[cacheKey] = newDerivedState; return newDerivedState; } @@ -38,11 +46,12 @@ export class DefaultDerivedStateProvider implements DerivedStateProvider { parentState$: Observable, deriveDefinition: DeriveDefinition, dependencies: TDeps, + storageLocation: [string, AbstractStorageService & ObservableStorageService], ): DerivedState { return new DefaultDerivedState( parentState$, deriveDefinition, - this.memoryStorage, + storageLocation[1], dependencies, ); } diff --git a/libs/common/src/platform/state/implementations/default-derived-state.spec.ts b/libs/common/src/platform/state/implementations/default-derived-state.spec.ts index 958a9386114..e3b1587e3a1 100644 --- a/libs/common/src/platform/state/implementations/default-derived-state.spec.ts +++ b/libs/common/src/platform/state/implementations/default-derived-state.spec.ts @@ -72,12 +72,12 @@ describe("DefaultDerivedState", () => { parentState$.next(dateString); await awaitAsync(); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toEqual( + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual( derivedValue(new Date(dateString)), ); const calls = memoryStorage.mock.save.mock.calls; expect(calls.length).toBe(1); - expect(calls[0][0]).toBe(deriveDefinition.buildCacheKey()); + expect(calls[0][0]).toBe(deriveDefinition.storageKey); expect(calls[0][1]).toEqual(derivedValue(new Date(dateString))); }); @@ -94,7 +94,7 @@ describe("DefaultDerivedState", () => { it("should store the forced value", async () => { await sut.forceValue(forced); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toEqual( + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual( derivedValue(forced), ); }); @@ -109,7 +109,7 @@ describe("DefaultDerivedState", () => { it("should store the forced value", async () => { await sut.forceValue(forced); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toEqual( + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual( derivedValue(forced), ); }); @@ -153,7 +153,7 @@ describe("DefaultDerivedState", () => { parentState$.next(newDate); await awaitAsync(); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toEqual( + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual( derivedValue(new Date(newDate)), ); @@ -161,7 +161,7 @@ describe("DefaultDerivedState", () => { // Wait for cleanup await awaitAsync(cleanupDelayMs * 2); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toBeUndefined(); + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toBeUndefined(); }); it("should not clear state after cleanup if clearOnCleanup is false", async () => { @@ -171,7 +171,7 @@ describe("DefaultDerivedState", () => { parentState$.next(newDate); await awaitAsync(); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toEqual( + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual( derivedValue(new Date(newDate)), ); @@ -179,7 +179,7 @@ describe("DefaultDerivedState", () => { // Wait for cleanup await awaitAsync(cleanupDelayMs * 2); - expect(memoryStorage.internalStore[deriveDefinition.buildCacheKey()]).toEqual( + expect(memoryStorage.internalStore[deriveDefinition.storageKey]).toEqual( derivedValue(new Date(newDate)), ); }); diff --git a/libs/common/src/platform/state/state-definition.ts b/libs/common/src/platform/state/state-definition.ts index 15dc9ff7574..f1e7dc80abd 100644 --- a/libs/common/src/platform/state/state-definition.ts +++ b/libs/common/src/platform/state/state-definition.ts @@ -24,8 +24,10 @@ export type ClientLocations = { web: StorageLocation | "disk-local"; /** * Overriding storage location for browser clients. + * + * "memory-large-object" is used to store non-countable objects in memory. This exists due to limited persistent memory available to browser extensions. */ - //browser: StorageLocation; + browser: StorageLocation | "memory-large-object"; /** * Overriding storage location for desktop clients. */ diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index 18df252062f..e04110f28b2 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -116,7 +116,9 @@ export const EVENT_COLLECTION_DISK = new StateDefinition("eventCollection", "dis export const SEND_DISK = new StateDefinition("encryptedSend", "disk", { web: "memory", }); -export const SEND_MEMORY = new StateDefinition("decryptedSend", "memory"); +export const SEND_MEMORY = new StateDefinition("decryptedSend", "memory", { + browser: "memory-large-object", +}); // Vault @@ -133,10 +135,16 @@ export const VAULT_ONBOARDING = new StateDefinition("vaultOnboarding", "disk", { export const VAULT_SETTINGS_DISK = new StateDefinition("vaultSettings", "disk", { web: "disk-local", }); -export const VAULT_BROWSER_MEMORY = new StateDefinition("vaultBrowser", "memory"); -export const VAULT_SEARCH_MEMORY = new StateDefinition("vaultSearch", "memory"); +export const VAULT_BROWSER_MEMORY = new StateDefinition("vaultBrowser", "memory", { + browser: "memory-large-object", +}); +export const VAULT_SEARCH_MEMORY = new StateDefinition("vaultSearch", "memory", { + browser: "memory-large-object", +}); export const CIPHERS_DISK = new StateDefinition("ciphers", "disk", { web: "memory" }); export const CIPHERS_DISK_LOCAL = new StateDefinition("ciphersLocal", "disk", { web: "disk-local", }); -export const CIPHERS_MEMORY = new StateDefinition("ciphersMemory", "memory"); +export const CIPHERS_MEMORY = new StateDefinition("ciphersMemory", "memory", { + browser: "memory-large-object", +}); From 300b17aaeb327d6593d8e90e4943ecd703e9fcdb Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Mon, 22 Apr 2024 10:14:38 -0400 Subject: [PATCH 04/16] [PM-7653] Do not store disk-backed sessions as single blobs (#8852) * Implement a lazy value class This will be used as a source for composing key-protected storage from a single key source. * Simplify local-backed-session-storage The new implementation stores each value to a unique location, prefixed with `session_` to help indicate the purpose. I've also removed the complexity around session keys, favoring passing in a pre-defined value that is determined lazily once for the service worker. This is more in line with how I expect a key-protected storage would work. * Remove decrypted session flag This has been nothing but an annoyance. If it's ever added back, it needs to have some way to determine if the session key matches the one it was written with * Remove unnecessary string interpolation * Remove sync Lazy This is better done as a separate class. * Handle async through type * prefer two factory calls to incorrect value on races. * Fix type * Remove log * Update libs/common/src/platform/misc/lazy.ts Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --------- Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --- apps/browser/config/development.json | 1 - .../browser/src/background/main.background.ts | 47 +- .../storage-service.factory.ts | 26 +- .../decorators/dev-flag.decorator.spec.ts | 2 +- apps/browser/src/platform/flags.ts | 1 - ...cal-backed-session-storage.service.spec.ts | 508 +++++------------- .../local-backed-session-storage.service.ts | 225 +++----- libs/common/spec/index.ts | 1 + libs/common/src/platform/misc/lazy.spec.ts | 85 +++ libs/common/src/platform/misc/lazy.ts | 20 + 10 files changed, 380 insertions(+), 536 deletions(-) create mode 100644 libs/common/src/platform/misc/lazy.spec.ts create mode 100644 libs/common/src/platform/misc/lazy.ts diff --git a/apps/browser/config/development.json b/apps/browser/config/development.json index 1b628c173ce..aba10eb25b2 100644 --- a/apps/browser/config/development.json +++ b/apps/browser/config/development.json @@ -1,6 +1,5 @@ { "devFlags": { - "storeSessionDecrypted": false, "managedEnvironment": { "base": "https://localhost:8080" } diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 0a9ad44962c..fa1add06028 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -98,7 +98,9 @@ import { StateFactory } from "@bitwarden/common/platform/factories/state-factory import { Message, MessageListener, MessageSender } from "@bitwarden/common/platform/messaging"; // eslint-disable-next-line no-restricted-imports -- Used for dependency creation import { SubjectMessageSender } from "@bitwarden/common/platform/messaging/internal"; +import { Lazy } from "@bitwarden/common/platform/misc/lazy"; import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state"; +import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { AppIdService } from "@bitwarden/common/platform/services/app-id.service"; import { ConfigApiService } from "@bitwarden/common/platform/services/config/config-api.service"; import { DefaultConfigService } from "@bitwarden/common/platform/services/config/default-config.service"; @@ -404,32 +406,51 @@ export default class MainBackground { self, ); - const mv3MemoryStorageCreator = (partitionName: string) => { - if (this.popupOnlyContext) { - return new ForegroundMemoryStorageService(partitionName); + // Creates a session key for mv3 storage of large memory items + const sessionKey = new Lazy(async () => { + // Key already in session storage + const sessionStorage = new BrowserMemoryStorageService(); + const existingKey = await sessionStorage.get("session-key"); + if (existingKey) { + if (sessionStorage.valuesRequireDeserialization) { + return SymmetricCryptoKey.fromJSON(existingKey); + } + return existingKey; + } + + // New key + const { derivedKey } = await this.keyGenerationService.createKeyWithPurpose( + 128, + "ephemeral", + "bitwarden-ephemeral", + ); + await sessionStorage.save("session-key", derivedKey); + return derivedKey; + }); + + const mv3MemoryStorageCreator = () => { + if (this.popupOnlyContext) { + return new ForegroundMemoryStorageService(); } - // TODO: Consider using multithreaded encrypt service in popup only context return new LocalBackedSessionStorageService( - this.logService, + sessionKey, + this.storageService, new EncryptServiceImplementation(this.cryptoFunctionService, this.logService, false), - this.keyGenerationService, - new BrowserLocalStorageService(), - new BrowserMemoryStorageService(), this.platformUtilsService, - partitionName, + this.logService, ); }; this.secureStorageService = this.storageService; // secure storage is not supported in browsers, so we use local storage and warn users when it is used - this.memoryStorageService = BrowserApi.isManifestVersion(3) - ? mv3MemoryStorageCreator("stateService") - : new MemoryStorageService(); this.memoryStorageForStateProviders = BrowserApi.isManifestVersion(3) ? new BrowserMemoryStorageService() // mv3 stores to storage.session : new BackgroundMemoryStorageService(); // mv2 stores to memory + this.memoryStorageService = BrowserApi.isManifestVersion(3) + ? this.memoryStorageForStateProviders // manifest v3 can reuse the same storage. They are split for v2 due to lacking a good sync mechanism, which isn't true for v3 + : new MemoryStorageService(); this.largeObjectMemoryStorageForStateProviders = BrowserApi.isManifestVersion(3) - ? mv3MemoryStorageCreator("stateProviders") // mv3 stores to local-backed session storage + ? mv3MemoryStorageCreator() // mv3 stores to local-backed session storage : this.memoryStorageForStateProviders; // mv2 stores to the same location const storageServiceProvider = new BrowserStorageServiceProvider( diff --git a/apps/browser/src/platform/background/service-factories/storage-service.factory.ts b/apps/browser/src/platform/background/service-factories/storage-service.factory.ts index 83e8a780a6d..e63e39944d3 100644 --- a/apps/browser/src/platform/background/service-factories/storage-service.factory.ts +++ b/apps/browser/src/platform/background/service-factories/storage-service.factory.ts @@ -3,6 +3,8 @@ import { AbstractStorageService, ObservableStorageService, } from "@bitwarden/common/platform/abstractions/storage.service"; +import { Lazy } from "@bitwarden/common/platform/misc/lazy"; +import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { MemoryStorageService } from "@bitwarden/common/platform/services/memory-storage.service"; import { BrowserApi } from "../../browser/browser-api"; @@ -17,10 +19,10 @@ import { KeyGenerationServiceInitOptions, keyGenerationServiceFactory, } from "./key-generation-service.factory"; -import { logServiceFactory, LogServiceInitOptions } from "./log-service.factory"; +import { LogServiceInitOptions, logServiceFactory } from "./log-service.factory"; import { - platformUtilsServiceFactory, PlatformUtilsServiceInitOptions, + platformUtilsServiceFactory, } from "./platform-utils-service.factory"; export type DiskStorageServiceInitOptions = FactoryOptions; @@ -70,13 +72,23 @@ export function memoryStorageServiceFactory( return factory(cache, "memoryStorageService", opts, async () => { if (BrowserApi.isManifestVersion(3)) { return new LocalBackedSessionStorageService( - await logServiceFactory(cache, opts), - await encryptServiceFactory(cache, opts), - await keyGenerationServiceFactory(cache, opts), + new Lazy(async () => { + const existingKey = await ( + await sessionStorageServiceFactory(cache, opts) + ).get("session-key"); + if (existingKey) { + return existingKey; + } + const { derivedKey } = await ( + await keyGenerationServiceFactory(cache, opts) + ).createKeyWithPurpose(128, "ephemeral", "bitwarden-ephemeral"); + await (await sessionStorageServiceFactory(cache, opts)).save("session-key", derivedKey); + return derivedKey; + }), await diskStorageServiceFactory(cache, opts), - await sessionStorageServiceFactory(cache, opts), + await encryptServiceFactory(cache, opts), await platformUtilsServiceFactory(cache, opts), - "serviceFactories", + await logServiceFactory(cache, opts), ); } return new MemoryStorageService(); diff --git a/apps/browser/src/platform/decorators/dev-flag.decorator.spec.ts b/apps/browser/src/platform/decorators/dev-flag.decorator.spec.ts index c5401f8a097..da00bc6fe30 100644 --- a/apps/browser/src/platform/decorators/dev-flag.decorator.spec.ts +++ b/apps/browser/src/platform/decorators/dev-flag.decorator.spec.ts @@ -9,7 +9,7 @@ jest.mock("../flags", () => ({ })); class TestClass { - @devFlag("storeSessionDecrypted") test() { + @devFlag("managedEnvironment") test() { return "test"; } } diff --git a/apps/browser/src/platform/flags.ts b/apps/browser/src/platform/flags.ts index 36aa698a7bc..383e982f065 100644 --- a/apps/browser/src/platform/flags.ts +++ b/apps/browser/src/platform/flags.ts @@ -19,7 +19,6 @@ export type Flags = { // required to avoid linting errors when there are no flags // eslint-disable-next-line @typescript-eslint/ban-types export type DevFlags = { - storeSessionDecrypted?: boolean; managedEnvironment?: GroupPolicyEnvironment; } & SharedDevFlags; diff --git a/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts b/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts index a4581e6ac1a..7114bda06e3 100644 --- a/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts +++ b/apps/browser/src/platform/services/local-backed-session-storage.service.spec.ts @@ -1,412 +1,200 @@ import { mock, MockProxy } from "jest-mock-extended"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; -import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; -import { - AbstractMemoryStorageService, - AbstractStorageService, - StorageUpdate, -} from "@bitwarden/common/platform/abstractions/storage.service"; +import { Lazy } from "@bitwarden/common/platform/misc/lazy"; import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; - -import { BrowserApi } from "../browser/browser-api"; +import { FakeStorageService, makeEncString } from "@bitwarden/common/spec"; import { LocalBackedSessionStorageService } from "./local-backed-session-storage.service"; -describe.skip("LocalBackedSessionStorage", () => { - const sendMessageWithResponseSpy: jest.SpyInstance = jest.spyOn( - BrowserApi, - "sendMessageWithResponse", +describe("LocalBackedSessionStorage", () => { + const sessionKey = new SymmetricCryptoKey( + Utils.fromUtf8ToArray("00000000000000000000000000000000"), ); - + let localStorage: FakeStorageService; let encryptService: MockProxy; - let keyGenerationService: MockProxy; - let localStorageService: MockProxy; - let sessionStorageService: MockProxy; - let logService: MockProxy; let platformUtilsService: MockProxy; - - let cache: Record; - const testObj = { a: 1, b: 2 }; - const stringifiedTestObj = JSON.stringify(testObj); - - const key = new SymmetricCryptoKey(Utils.fromUtf8ToArray("00000000000000000000000000000000")); - let getSessionKeySpy: jest.SpyInstance; - let sendUpdateSpy: jest.SpyInstance; - const mockEnc = (input: string) => Promise.resolve(new EncString("ENCRYPTED" + input)); + let logService: MockProxy; let sut: LocalBackedSessionStorageService; - const mockExistingSessionKey = (key: SymmetricCryptoKey) => { - sessionStorageService.get.mockImplementation((storageKey) => { - if (storageKey === "localEncryptionKey_test") { - return Promise.resolve(key?.toJSON()); - } - - return Promise.reject("No implementation for " + storageKey); - }); - }; - beforeEach(() => { - sendMessageWithResponseSpy.mockResolvedValue(null); - logService = mock(); + localStorage = new FakeStorageService(); encryptService = mock(); - keyGenerationService = mock(); - localStorageService = mock(); - sessionStorageService = mock(); + platformUtilsService = mock(); + logService = mock(); sut = new LocalBackedSessionStorageService( - logService, + new Lazy(async () => sessionKey), + localStorage, encryptService, - keyGenerationService, - localStorageService, - sessionStorageService, platformUtilsService, - "test", + logService, ); - - cache = sut["cachedSession"]; - - keyGenerationService.createKeyWithPurpose.mockResolvedValue({ - derivedKey: key, - salt: "bitwarden-ephemeral", - material: null, // Not used - }); - - getSessionKeySpy = jest.spyOn(sut, "getSessionEncKey"); - getSessionKeySpy.mockResolvedValue(key); - - // sendUpdateSpy = jest.spyOn(sut, "sendUpdate"); - // sendUpdateSpy.mockReturnValue(); }); describe("get", () => { - describe("in local cache or external context cache", () => { - it("should return from local cache", async () => { - cache["test"] = stringifiedTestObj; - const result = await sut.get("test"); - expect(result).toStrictEqual(testObj); - }); - - it("should return from external context cache when local cache is not available", async () => { - sendMessageWithResponseSpy.mockResolvedValue(stringifiedTestObj); - const result = await sut.get("test"); - expect(result).toStrictEqual(testObj); - }); + it("return the cached value when one is cached", async () => { + sut["cache"]["test"] = "cached"; + const result = await sut.get("test"); + expect(result).toEqual("cached"); }); - describe("not in cache", () => { - const session = { test: stringifiedTestObj }; + it("returns a decrypted value when one is stored in local storage", async () => { + const encrypted = makeEncString("encrypted"); + localStorage.internalStore["session_test"] = encrypted.encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + const result = await sut.get("test"); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encrypted, sessionKey); + expect(result).toEqual("decrypted"); + }); - beforeEach(() => { - mockExistingSessionKey(key); - }); + it("caches the decrypted value when one is stored in local storage", async () => { + const encrypted = makeEncString("encrypted"); + localStorage.internalStore["session_test"] = encrypted.encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + await sut.get("test"); + expect(sut["cache"]["test"]).toEqual("decrypted"); + }); + }); - describe("no session retrieved", () => { - let result: any; - let spy: jest.SpyInstance; - beforeEach(async () => { - spy = jest.spyOn(sut, "getLocalSession").mockResolvedValue(null); - localStorageService.get.mockResolvedValue(null); - result = await sut.get("test"); - }); + describe("getBypassCache", () => { + it("ignores cached values", async () => { + sut["cache"]["test"] = "cached"; + const encrypted = makeEncString("encrypted"); + localStorage.internalStore["session_test"] = encrypted.encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + const result = await sut.getBypassCache("test"); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encrypted, sessionKey); + expect(result).toEqual("decrypted"); + }); - it("should grab from session if not in cache", async () => { - expect(spy).toHaveBeenCalledWith(key); - }); + it("returns a decrypted value when one is stored in local storage", async () => { + const encrypted = makeEncString("encrypted"); + localStorage.internalStore["session_test"] = encrypted.encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + const result = await sut.getBypassCache("test"); + expect(encryptService.decryptToUtf8).toHaveBeenCalledWith(encrypted, sessionKey); + expect(result).toEqual("decrypted"); + }); - it("should return null if session is null", async () => { - expect(result).toBeNull(); - }); - }); + it("caches the decrypted value when one is stored in local storage", async () => { + const encrypted = makeEncString("encrypted"); + localStorage.internalStore["session_test"] = encrypted.encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + await sut.getBypassCache("test"); + expect(sut["cache"]["test"]).toEqual("decrypted"); + }); - describe("session retrieved from storage", () => { - beforeEach(() => { - jest.spyOn(sut, "getLocalSession").mockResolvedValue(session); - }); - - it("should return null if session does not have the key", async () => { - const result = await sut.get("DNE"); - expect(result).toBeNull(); - }); - - it("should return the value retrieved from session", async () => { - const result = await sut.get("test"); - expect(result).toEqual(session.test); - }); - - it("should set retrieved values in cache", async () => { - await sut.get("test"); - expect(cache["test"]).toBeTruthy(); - expect(cache["test"]).toEqual(session.test); - }); - - it("should use a deserializer if provided", async () => { - const deserializer = jest.fn().mockReturnValue(testObj); - const result = await sut.get("test", { deserializer: deserializer }); - expect(deserializer).toHaveBeenCalledWith(session.test); - expect(result).toEqual(testObj); - }); - }); + it("deserializes when a deserializer is provided", async () => { + const encrypted = makeEncString("encrypted"); + localStorage.internalStore["session_test"] = encrypted.encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + const deserializer = jest.fn().mockReturnValue("deserialized"); + const result = await sut.getBypassCache("test", { deserializer }); + expect(deserializer).toHaveBeenCalledWith("decrypted"); + expect(result).toEqual("deserialized"); }); }); describe("has", () => { - it("should be false if `get` returns null", async () => { - const spy = jest.spyOn(sut, "get"); - spy.mockResolvedValue(null); - expect(await sut.has("test")).toBe(false); + it("returns false when the key is not in cache", async () => { + const result = await sut.has("test"); + expect(result).toBe(false); + }); + + it("returns true when the key is in cache", async () => { + sut["cache"]["test"] = "cached"; + const result = await sut.has("test"); + expect(result).toBe(true); + }); + + it("returns true when the key is in local storage", async () => { + localStorage.internalStore["session_test"] = makeEncString("encrypted").encryptedString; + encryptService.decryptToUtf8.mockResolvedValue(JSON.stringify("decrypted")); + const result = await sut.has("test"); + expect(result).toBe(true); + }); + + it.each([null, undefined])("returns false when %s is cached", async (nullish) => { + sut["cache"]["test"] = nullish; + await expect(sut.has("test")).resolves.toBe(false); + }); + + it.each([null, undefined])( + "returns false when null is stored in local storage", + async (nullish) => { + localStorage.internalStore["session_test"] = nullish; + await expect(sut.has("test")).resolves.toBe(false); + expect(encryptService.decryptToUtf8).not.toHaveBeenCalled(); + }, + ); + }); + + describe("save", () => { + const encString = makeEncString("encrypted"); + beforeEach(() => { + encryptService.encrypt.mockResolvedValue(encString); + }); + + it("logs a warning when saving the same value twice and in a dev environment", async () => { + platformUtilsService.isDev.mockReturnValue(true); + sut["cache"]["test"] = "cached"; + await sut.save("test", "cached"); + expect(logService.warning).toHaveBeenCalled(); + }); + + it("does not log when saving the same value twice and not in a dev environment", async () => { + platformUtilsService.isDev.mockReturnValue(false); + sut["cache"]["test"] = "cached"; + await sut.save("test", "cached"); + expect(logService.warning).not.toHaveBeenCalled(); + }); + + it("removes the key when saving a null value", async () => { + const spy = jest.spyOn(sut, "remove"); + await sut.save("test", null); expect(spy).toHaveBeenCalledWith("test"); }); - it("should be true if `get` returns non-null", async () => { - const spy = jest.spyOn(sut, "get"); - spy.mockResolvedValue({}); - expect(await sut.has("test")).toBe(true); - expect(spy).toHaveBeenCalledWith("test"); + it("saves the value to cache", async () => { + await sut.save("test", "value"); + expect(sut["cache"]["test"]).toEqual("value"); + }); + + it("encrypts and saves the value to local storage", async () => { + await sut.save("test", "value"); + expect(encryptService.encrypt).toHaveBeenCalledWith(JSON.stringify("value"), sessionKey); + expect(localStorage.internalStore["session_test"]).toEqual(encString.encryptedString); + }); + + it("emits an update", async () => { + const spy = jest.spyOn(sut["updatesSubject"], "next"); + await sut.save("test", "value"); + expect(spy).toHaveBeenCalledWith({ key: "test", updateType: "save" }); }); }); describe("remove", () => { - describe("existing cache value is null", () => { - it("should not save null if the local cached value is already null", async () => { - cache["test"] = null; - await sut.remove("test"); - expect(sendUpdateSpy).not.toHaveBeenCalled(); - }); - - it("should not save null if the externally cached value is already null", async () => { - sendMessageWithResponseSpy.mockResolvedValue(null); - await sut.remove("test"); - expect(sendUpdateSpy).not.toHaveBeenCalled(); - }); - }); - - it("should save null", async () => { - cache["test"] = stringifiedTestObj; - + it("nulls the value in cache", async () => { + sut["cache"]["test"] = "cached"; await sut.remove("test"); - expect(sendUpdateSpy).toHaveBeenCalledWith({ key: "test", updateType: "remove" }); - }); - }); - - describe("save", () => { - describe("currently cached", () => { - it("does not save the value a local cached value exists which is an exact match", async () => { - cache["test"] = stringifiedTestObj; - await sut.save("test", testObj); - expect(sendUpdateSpy).not.toHaveBeenCalled(); - }); - - it("does not save the value if a local cached value exists, even if the keys not in the same order", async () => { - cache["test"] = JSON.stringify({ b: 2, a: 1 }); - await sut.save("test", testObj); - expect(sendUpdateSpy).not.toHaveBeenCalled(); - }); - - it("does not save the value a externally cached value exists which is an exact match", async () => { - sendMessageWithResponseSpy.mockResolvedValue(stringifiedTestObj); - await sut.save("test", testObj); - expect(sendUpdateSpy).not.toHaveBeenCalled(); - expect(cache["test"]).toBe(stringifiedTestObj); - }); - - it("saves the value if the currently cached string value evaluates to a falsy value", async () => { - cache["test"] = "null"; - await sut.save("test", testObj); - expect(sendUpdateSpy).toHaveBeenCalledWith({ key: "test", updateType: "save" }); - }); + expect(sut["cache"]["test"]).toBeNull(); }); - describe("caching", () => { - beforeEach(() => { - localStorageService.get.mockResolvedValue(null); - sessionStorageService.get.mockResolvedValue(null); - - localStorageService.save.mockResolvedValue(); - sessionStorageService.save.mockResolvedValue(); - - encryptService.encrypt.mockResolvedValue(mockEnc("{}")); - }); - - it("should remove key from cache if value is null", async () => { - cache["test"] = {}; - // const cacheSetSpy = jest.spyOn(cache, "set"); - expect(cache["test"]).toBe(true); - await sut.save("test", null); - // Don't remove from cache, just replace with null - expect(cache["test"]).toBe(null); - // expect(cacheSetSpy).toHaveBeenCalledWith("test", null); - }); - - it("should set cache if value is non-null", async () => { - expect(cache["test"]).toBe(false); - // const setSpy = jest.spyOn(cache, "set"); - await sut.save("test", testObj); - expect(cache["test"]).toBe(stringifiedTestObj); - // expect(setSpy).toHaveBeenCalledWith("test", stringifiedTestObj); - }); + it("removes the key from local storage", async () => { + localStorage.internalStore["session_test"] = makeEncString("encrypted").encryptedString; + await sut.remove("test"); + expect(localStorage.internalStore["session_test"]).toBeUndefined(); }); - describe("local storing", () => { - let setSpy: jest.SpyInstance; - - beforeEach(() => { - setSpy = jest.spyOn(sut, "setLocalSession").mockResolvedValue(); - }); - - it("should store a new session", async () => { - jest.spyOn(sut, "getLocalSession").mockResolvedValue(null); - await sut.save("test", testObj); - - expect(setSpy).toHaveBeenCalledWith({ test: testObj }, key); - }); - - it("should update an existing session", async () => { - const existingObj = { test: testObj }; - jest.spyOn(sut, "getLocalSession").mockResolvedValue(existingObj); - await sut.save("test2", testObj); - - expect(setSpy).toHaveBeenCalledWith({ test2: testObj, ...existingObj }, key); - }); - - it("should overwrite an existing item in session", async () => { - const existingObj = { test: {} }; - jest.spyOn(sut, "getLocalSession").mockResolvedValue(existingObj); - await sut.save("test", testObj); - - expect(setSpy).toHaveBeenCalledWith({ test: testObj }, key); - }); - }); - }); - - describe("getSessionKey", () => { - beforeEach(() => { - getSessionKeySpy.mockRestore(); - }); - - it("should return the stored symmetric crypto key", async () => { - sessionStorageService.get.mockResolvedValue({ ...key }); - const result = await sut.getSessionEncKey(); - - expect(result).toStrictEqual(key); - }); - - describe("new key creation", () => { - beforeEach(() => { - keyGenerationService.createKeyWithPurpose.mockResolvedValue({ - salt: "salt", - material: null, - derivedKey: key, - }); - jest.spyOn(sut, "setSessionEncKey").mockResolvedValue(); - }); - - it("should create a symmetric crypto key", async () => { - const result = await sut.getSessionEncKey(); - - expect(result).toStrictEqual(key); - expect(keyGenerationService.createKeyWithPurpose).toHaveBeenCalledTimes(1); - }); - - it("should store a symmetric crypto key if it makes one", async () => { - const spy = jest.spyOn(sut, "setSessionEncKey").mockResolvedValue(); - await sut.getSessionEncKey(); - - expect(spy).toHaveBeenCalledWith(key); - }); - }); - }); - - describe("getLocalSession", () => { - it("should return null if session is null", async () => { - const result = await sut.getLocalSession(key); - - expect(result).toBeNull(); - expect(localStorageService.get).toHaveBeenCalledWith("session_test"); - }); - - describe("non-null sessions", () => { - const session = { test: "test" }; - const encSession = new EncString(JSON.stringify(session)); - const decryptedSession = JSON.stringify(session); - - beforeEach(() => { - localStorageService.get.mockResolvedValue(encSession.encryptedString); - }); - - it("should decrypt returned sessions", async () => { - encryptService.decryptToUtf8 - .calledWith(expect.anything(), key) - .mockResolvedValue(decryptedSession); - await sut.getLocalSession(key); - expect(encryptService.decryptToUtf8).toHaveBeenNthCalledWith(1, encSession, key); - }); - - it("should parse session", async () => { - encryptService.decryptToUtf8 - .calledWith(expect.anything(), key) - .mockResolvedValue(decryptedSession); - const result = await sut.getLocalSession(key); - expect(result).toEqual(session); - }); - - it("should remove state if decryption fails", async () => { - encryptService.decryptToUtf8.mockResolvedValue(null); - const setSessionEncKeySpy = jest.spyOn(sut, "setSessionEncKey").mockResolvedValue(); - - const result = await sut.getLocalSession(key); - - expect(result).toBeNull(); - expect(setSessionEncKeySpy).toHaveBeenCalledWith(null); - expect(localStorageService.remove).toHaveBeenCalledWith("session_test"); - }); - }); - }); - - describe("setLocalSession", () => { - const testSession = { test: "a" }; - const testJSON = JSON.stringify(testSession); - - it("should encrypt a stringified session", async () => { - encryptService.encrypt.mockImplementation(mockEnc); - localStorageService.save.mockResolvedValue(); - await sut.setLocalSession(testSession, key); - - expect(encryptService.encrypt).toHaveBeenNthCalledWith(1, testJSON, key); - }); - - it("should remove local session if null", async () => { - encryptService.encrypt.mockResolvedValue(null); - await sut.setLocalSession(null, key); - - expect(localStorageService.remove).toHaveBeenCalledWith("session_test"); - }); - - it("should save encrypted string", async () => { - encryptService.encrypt.mockImplementation(mockEnc); - await sut.setLocalSession(testSession, key); - - expect(localStorageService.save).toHaveBeenCalledWith( - "session_test", - (await mockEnc(testJSON)).encryptedString, - ); - }); - }); - - describe("setSessionKey", () => { - it("should remove if null", async () => { - await sut.setSessionEncKey(null); - expect(sessionStorageService.remove).toHaveBeenCalledWith("localEncryptionKey_test"); - }); - - it("should save key when not null", async () => { - await sut.setSessionEncKey(key); - expect(sessionStorageService.save).toHaveBeenCalledWith("localEncryptionKey_test", key); + it("emits an update", async () => { + const spy = jest.spyOn(sut["updatesSubject"], "next"); + await sut.remove("test"); + expect(spy).toHaveBeenCalledWith({ key: "test", updateType: "remove" }); }); }); }); diff --git a/apps/browser/src/platform/services/local-backed-session-storage.service.ts b/apps/browser/src/platform/services/local-backed-session-storage.service.ts index 146eb11b2bd..5432e8d918b 100644 --- a/apps/browser/src/platform/services/local-backed-session-storage.service.ts +++ b/apps/browser/src/platform/services/local-backed-session-storage.service.ts @@ -2,7 +2,6 @@ import { Subject } from "rxjs"; import { Jsonify } from "type-fest"; import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service"; -import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { @@ -11,13 +10,12 @@ import { ObservableStorageService, StorageUpdate, } from "@bitwarden/common/platform/abstractions/storage.service"; +import { Lazy } from "@bitwarden/common/platform/misc/lazy"; import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; import { MemoryStorageOptions } from "@bitwarden/common/platform/models/domain/storage-options"; import { SymmetricCryptoKey } from "@bitwarden/common/platform/models/domain/symmetric-crypto-key"; import { BrowserApi } from "../browser/browser-api"; -import { devFlag } from "../decorators/dev-flag.decorator"; -import { devFlagEnabled } from "../flags"; import { MemoryStoragePortMessage } from "../storage/port-messages"; import { portName } from "../storage/port-name"; @@ -25,85 +23,64 @@ export class LocalBackedSessionStorageService extends AbstractMemoryStorageService implements ObservableStorageService { + private ports: Set = new Set([]); + private cache: Record = {}; private updatesSubject = new Subject(); - private commandName = `localBackedSessionStorage_${this.partitionName}`; - private encKey = `localEncryptionKey_${this.partitionName}`; - private sessionKey = `session_${this.partitionName}`; - private cachedSession: Record = {}; - private _ports: Set = new Set([]); - private knownNullishCacheKeys: Set = new Set([]); + readonly valuesRequireDeserialization = true; + updates$ = this.updatesSubject.asObservable(); constructor( - private logService: LogService, - private encryptService: EncryptService, - private keyGenerationService: KeyGenerationService, - private localStorage: AbstractStorageService, - private sessionStorage: AbstractStorageService, - private platformUtilsService: PlatformUtilsService, - private partitionName: string, + private readonly sessionKey: Lazy>, + private readonly localStorage: AbstractStorageService, + private readonly encryptService: EncryptService, + private readonly platformUtilsService: PlatformUtilsService, + private readonly logService: LogService, ) { super(); BrowserApi.addListener(chrome.runtime.onConnect, (port) => { - if (port.name !== `${portName(chrome.storage.session)}_${partitionName}`) { + if (port.name !== portName(chrome.storage.session)) { return; } - this._ports.add(port); + this.ports.add(port); const listenerCallback = this.onMessageFromForeground.bind(this); port.onDisconnect.addListener(() => { - this._ports.delete(port); + this.ports.delete(port); port.onMessage.removeListener(listenerCallback); }); port.onMessage.addListener(listenerCallback); // Initialize the new memory storage service with existing data this.sendMessageTo(port, { action: "initialization", - data: Array.from(Object.keys(this.cachedSession)), + data: Array.from(Object.keys(this.cache)), + }); + this.updates$.subscribe((update) => { + this.broadcastMessage({ + action: "subject_update", + data: update, + }); }); }); - this.updates$.subscribe((update) => { - this.broadcastMessage({ - action: "subject_update", - data: update, - }); - }); - } - - get valuesRequireDeserialization(): boolean { - return true; - } - - get updates$() { - return this.updatesSubject.asObservable(); } async get(key: string, options?: MemoryStorageOptions): Promise { - if (this.cachedSession[key] != null) { - return this.cachedSession[key] as T; - } - - if (this.knownNullishCacheKeys.has(key)) { - return null; + if (this.cache[key] !== undefined) { + return this.cache[key] as T; } return await this.getBypassCache(key, options); } async getBypassCache(key: string, options?: MemoryStorageOptions): Promise { - const session = await this.getLocalSession(await this.getSessionEncKey()); - if (session[key] == null) { - this.knownNullishCacheKeys.add(key); - return null; - } + let value = await this.getLocalSessionValue(await this.sessionKey.get(), key); - let value = session[key]; if (options?.deserializer != null) { value = options.deserializer(value as Jsonify); } - void this.save(key, value); + this.cache[key] = value; return value as T; } @@ -114,7 +91,7 @@ export class LocalBackedSessionStorageService async save(key: string, obj: T): Promise { // This is for observation purposes only. At some point, we don't want to write to local session storage if the value is the same. if (this.platformUtilsService.isDev()) { - const existingValue = this.cachedSession[key] as T; + const existingValue = this.cache[key] as T; if (this.compareValues(existingValue, obj)) { this.logService.warning(`Possible unnecessary write to local session storage. Key: ${key}`); this.logService.warning(obj as any); @@ -125,128 +102,42 @@ export class LocalBackedSessionStorageService return await this.remove(key); } - this.knownNullishCacheKeys.delete(key); - this.cachedSession[key] = obj; + this.cache[key] = obj; await this.updateLocalSessionValue(key, obj); this.updatesSubject.next({ key, updateType: "save" }); } async remove(key: string): Promise { - this.knownNullishCacheKeys.add(key); - delete this.cachedSession[key]; + this.cache[key] = null; await this.updateLocalSessionValue(key, null); this.updatesSubject.next({ key, updateType: "remove" }); } - private async updateLocalSessionValue(key: string, obj: T) { - const sessionEncKey = await this.getSessionEncKey(); - const localSession = (await this.getLocalSession(sessionEncKey)) ?? {}; - localSession[key] = obj; - void this.setLocalSession(localSession, sessionEncKey); - } - - async getLocalSession(encKey: SymmetricCryptoKey): Promise> { - if (Object.keys(this.cachedSession).length > 0) { - return this.cachedSession; - } - - this.cachedSession = {}; - const local = await this.localStorage.get(this.sessionKey); + private async getLocalSessionValue(encKey: SymmetricCryptoKey, key: string): Promise { + const local = await this.localStorage.get(this.sessionStorageKey(key)); if (local == null) { - return this.cachedSession; + return null; } - if (devFlagEnabled("storeSessionDecrypted")) { - return local as any as Record; + const valueJson = await this.encryptService.decryptToUtf8(new EncString(local), encKey); + if (valueJson == null) { + // error with decryption, value is lost, delete state and start over + await this.localStorage.remove(this.sessionStorageKey(key)); + return null; } - const sessionJson = await this.encryptService.decryptToUtf8(new EncString(local), encKey); - if (sessionJson == null) { - // Error with decryption -- session is lost, delete state and key and start over - await this.setSessionEncKey(null); - await this.localStorage.remove(this.sessionKey); - return this.cachedSession; - } - - this.cachedSession = JSON.parse(sessionJson); - return this.cachedSession; + return JSON.parse(valueJson); } - async setLocalSession(session: Record, key: SymmetricCryptoKey) { - if (devFlagEnabled("storeSessionDecrypted")) { - await this.setDecryptedLocalSession(session); - } else { - await this.setEncryptedLocalSession(session, key); - } - } - - @devFlag("storeSessionDecrypted") - async setDecryptedLocalSession(session: Record): Promise { - // Make sure we're storing the jsonified version of the session - const jsonSession = JSON.parse(JSON.stringify(session)); - if (session == null) { - await this.localStorage.remove(this.sessionKey); - } else { - await this.localStorage.save(this.sessionKey, jsonSession); - } - } - - async setEncryptedLocalSession(session: Record, key: SymmetricCryptoKey) { - const jsonSession = JSON.stringify(session); - const encSession = await this.encryptService.encrypt(jsonSession, key); - - if (encSession == null) { - return await this.localStorage.remove(this.sessionKey); - } - await this.localStorage.save(this.sessionKey, encSession.encryptedString); - } - - async getSessionEncKey(): Promise { - let storedKey = await this.sessionStorage.get(this.encKey); - if (storedKey == null || Object.keys(storedKey).length == 0) { - const generatedKey = await this.keyGenerationService.createKeyWithPurpose( - 128, - "ephemeral", - "bitwarden-ephemeral", - ); - storedKey = generatedKey.derivedKey; - await this.setSessionEncKey(storedKey); - return storedKey; - } else { - return SymmetricCryptoKey.fromJSON(storedKey); - } - } - - async setSessionEncKey(input: SymmetricCryptoKey): Promise { - if (input == null) { - await this.sessionStorage.remove(this.encKey); - } else { - await this.sessionStorage.save(this.encKey, input); - } - } - - private compareValues(value1: T, value2: T): boolean { - if (value1 == null && value2 == null) { - return true; + private async updateLocalSessionValue(key: string, value: unknown): Promise { + if (value == null) { + await this.localStorage.remove(this.sessionStorageKey(key)); + return; } - if (value1 && value2 == null) { - return false; - } - - if (value1 == null && value2) { - return false; - } - - if (typeof value1 !== "object" || typeof value2 !== "object") { - return value1 === value2; - } - - if (JSON.stringify(value1) === JSON.stringify(value2)) { - return true; - } - - return Object.entries(value1).sort().toString() === Object.entries(value2).sort().toString(); + const valueJson = JSON.stringify(value); + const encValue = await this.encryptService.encrypt(valueJson, await this.sessionKey.get()); + await this.localStorage.save(this.sessionStorageKey(key), encValue.encryptedString); } private async onMessageFromForeground( @@ -282,7 +173,7 @@ export class LocalBackedSessionStorageService } protected broadcastMessage(data: Omit) { - this._ports.forEach((port) => { + this.ports.forEach((port) => { this.sendMessageTo(port, data); }); } @@ -296,4 +187,32 @@ export class LocalBackedSessionStorageService originator: "background", }); } + + private sessionStorageKey(key: string) { + return `session_${key}`; + } + + private compareValues(value1: T, value2: T): boolean { + if (value1 == null && value2 == null) { + return true; + } + + if (value1 && value2 == null) { + return false; + } + + if (value1 == null && value2) { + return false; + } + + if (typeof value1 !== "object" || typeof value2 !== "object") { + return value1 === value2; + } + + if (JSON.stringify(value1) === JSON.stringify(value2)) { + return true; + } + + return Object.entries(value1).sort().toString() === Object.entries(value2).sort().toString(); + } } diff --git a/libs/common/spec/index.ts b/libs/common/spec/index.ts index 72bd28aca4f..6e9af8400ee 100644 --- a/libs/common/spec/index.ts +++ b/libs/common/spec/index.ts @@ -4,3 +4,4 @@ export * from "./matchers"; export * from "./fake-state-provider"; export * from "./fake-state"; export * from "./fake-account-service"; +export * from "./fake-storage.service"; diff --git a/libs/common/src/platform/misc/lazy.spec.ts b/libs/common/src/platform/misc/lazy.spec.ts new file mode 100644 index 00000000000..76ee085d3da --- /dev/null +++ b/libs/common/src/platform/misc/lazy.spec.ts @@ -0,0 +1,85 @@ +import { Lazy } from "./lazy"; + +describe("Lazy", () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("async", () => { + let factory: jest.Mock>; + let lazy: Lazy>; + + beforeEach(() => { + factory = jest.fn(); + lazy = new Lazy(factory); + }); + + describe("get", () => { + it("should call the factory once", async () => { + await lazy.get(); + await lazy.get(); + + expect(factory).toHaveBeenCalledTimes(1); + }); + + it("should return the value from the factory", async () => { + factory.mockResolvedValue(42); + + const value = await lazy.get(); + + expect(value).toBe(42); + }); + }); + + describe("factory throws", () => { + it("should throw the error", async () => { + factory.mockRejectedValue(new Error("factory error")); + + await expect(lazy.get()).rejects.toThrow("factory error"); + }); + }); + + describe("factory returns undefined", () => { + it("should return undefined", async () => { + factory.mockResolvedValue(undefined); + + const value = await lazy.get(); + + expect(value).toBeUndefined(); + }); + }); + + describe("factory returns null", () => { + it("should return null", async () => { + factory.mockResolvedValue(null); + + const value = await lazy.get(); + + expect(value).toBeNull(); + }); + }); + }); + + describe("sync", () => { + const syncFactory = jest.fn(); + let lazy: Lazy; + + beforeEach(() => { + syncFactory.mockReturnValue(42); + lazy = new Lazy(syncFactory); + }); + + it("should return the value from the factory", () => { + const value = lazy.get(); + + expect(value).toBe(42); + }); + + it("should call the factory once", () => { + lazy.get(); + lazy.get(); + + expect(syncFactory).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/libs/common/src/platform/misc/lazy.ts b/libs/common/src/platform/misc/lazy.ts new file mode 100644 index 00000000000..fb85b93678d --- /dev/null +++ b/libs/common/src/platform/misc/lazy.ts @@ -0,0 +1,20 @@ +export class Lazy { + private _value: T | undefined = undefined; + private _isCreated = false; + + constructor(private readonly factory: () => T) {} + + /** + * Resolves the factory and returns the result. Guaranteed to resolve the value only once. + * + * @returns The value produced by your factory. + */ + get(): T { + if (!this._isCreated) { + this._value = this.factory(); + this._isCreated = true; + } + + return this._value as T; + } +} From 100b43dd8f7ac23cb888b0f031353aa68beffb82 Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Mon, 22 Apr 2024 12:06:43 -0400 Subject: [PATCH 05/16] =?UTF-8?q?Revert=20"Auth/PM-6689=20-=20Migrate=20Se?= =?UTF-8?q?curity=20Stamp=20to=20Token=20Service=20and=20State=20Prov?= =?UTF-8?q?=E2=80=A6"=20(#8860)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 91f1d9fb86142805d0774182c3ee6234e13946e3. --- .../browser/src/background/main.background.ts | 1 - apps/cli/src/bw.ts | 1 - .../src/services/jslib-services.module.ts | 1 - .../login-strategies/login.strategy.spec.ts | 4 + .../common/login-strategies/login.strategy.ts | 9 ++- .../src/auth/abstractions/token.service.ts | 6 -- .../src/auth/services/token.service.spec.ts | 79 ------------------- .../common/src/auth/services/token.service.ts | 25 ------ .../src/auth/services/token.state.spec.ts | 2 - libs/common/src/auth/services/token.state.ts | 5 -- .../platform/abstractions/state.service.ts | 2 + .../models/domain/account-tokens.spec.ts | 9 +++ .../platform/models/domain/account.spec.ts | 4 +- .../src/platform/models/domain/account.ts | 18 +++++ .../src/platform/services/state.service.ts | 17 ++++ .../src/vault/services/sync/sync.service.ts | 6 +- 16 files changed, 63 insertions(+), 126 deletions(-) create mode 100644 libs/common/src/platform/models/domain/account-tokens.spec.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index fa1add06028..15f21b25014 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -813,7 +813,6 @@ export default class MainBackground { this.avatarService, logoutCallback, this.billingAccountProfileStateService, - this.tokenService, ); this.eventUploadService = new EventUploadService( this.apiService, diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index 437f807bc61..58329128b8a 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -631,7 +631,6 @@ export class Main { this.avatarService, async (expired: boolean) => await this.logout(), this.billingAccountProfileStateService, - this.tokenService, ); this.totpService = new TotpService(this.cryptoFunctionService, this.logService); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 27b182de5dc..f31bcb1c513 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -628,7 +628,6 @@ const safeProviders: SafeProvider[] = [ AvatarServiceAbstraction, LOGOUT_CALLBACK, BillingAccountProfileStateService, - TokenServiceAbstraction, ], }), safeProvider({ diff --git a/libs/auth/src/common/login-strategies/login.strategy.spec.ts b/libs/auth/src/common/login-strategies/login.strategy.spec.ts index e0833342ce3..431f736e949 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.spec.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.spec.ts @@ -27,6 +27,7 @@ import { Utils } from "@bitwarden/common/platform/misc/utils"; import { Account, AccountProfile, + AccountTokens, AccountKeys, } from "@bitwarden/common/platform/models/domain/account"; import { EncString } from "@bitwarden/common/platform/models/domain/enc-string"; @@ -212,6 +213,9 @@ describe("LoginStrategy", () => { kdfType: kdf, }, }, + tokens: { + ...new AccountTokens(), + }, keys: new AccountKeys(), }), ); diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index a73c32e1208..a6dc1931839 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/libs/auth/src/common/login-strategies/login.strategy.ts @@ -27,7 +27,11 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service" import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; -import { Account, AccountProfile } from "@bitwarden/common/platform/models/domain/account"; +import { + Account, + AccountProfile, + AccountTokens, +} from "@bitwarden/common/platform/models/domain/account"; import { UserId } from "@bitwarden/common/types/guid"; import { InternalUserDecryptionOptionsServiceAbstraction } from "../abstractions/user-decryption-options.service.abstraction"; @@ -188,6 +192,9 @@ export abstract class LoginStrategy { kdfType: tokenResponse.kdf, }, }, + tokens: { + ...new AccountTokens(), + }, }), ); diff --git a/libs/common/src/auth/abstractions/token.service.ts b/libs/common/src/auth/abstractions/token.service.ts index fc3bd317f47..75bb3838828 100644 --- a/libs/common/src/auth/abstractions/token.service.ts +++ b/libs/common/src/auth/abstractions/token.service.ts @@ -213,10 +213,4 @@ export abstract class TokenService { * @returns A promise that resolves with a boolean representing the user's external authN status. */ getIsExternal: () => Promise; - - /** Gets the active or passed in user's security stamp */ - getSecurityStamp: (userId?: UserId) => Promise; - - /** Sets the security stamp for the active or passed in user */ - setSecurityStamp: (securityStamp: string, userId?: UserId) => Promise; } diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index 3e92053d2f7..d32c4d8e1cd 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/libs/common/src/auth/services/token.service.spec.ts @@ -23,7 +23,6 @@ import { EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, REFRESH_TOKEN_DISK, REFRESH_TOKEN_MEMORY, - SECURITY_STAMP_MEMORY, } from "./token.state"; describe("TokenService", () => { @@ -2192,84 +2191,6 @@ describe("TokenService", () => { }); }); - describe("Security Stamp methods", () => { - const mockSecurityStamp = "securityStamp"; - - describe("setSecurityStamp", () => { - it("should throw an error if no user id is provided and there is no active user in global state", async () => { - // Act - // note: don't await here because we want to test the error - const result = tokenService.setSecurityStamp(mockSecurityStamp); - // Assert - await expect(result).rejects.toThrow("User id not found. Cannot set security stamp."); - }); - - it("should set the security stamp in memory when there is an active user in global state", async () => { - // Arrange - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); - - // Act - await tokenService.setSecurityStamp(mockSecurityStamp); - - // Assert - expect( - singleUserStateProvider.getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY).nextMock, - ).toHaveBeenCalledWith(mockSecurityStamp); - }); - - it("should set the security stamp in memory for the specified user id", async () => { - // Act - await tokenService.setSecurityStamp(mockSecurityStamp, userIdFromAccessToken); - - // Assert - expect( - singleUserStateProvider.getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY).nextMock, - ).toHaveBeenCalledWith(mockSecurityStamp); - }); - }); - - describe("getSecurityStamp", () => { - it("should throw an error if no user id is provided and there is no active user in global state", async () => { - // Act - // note: don't await here because we want to test the error - const result = tokenService.getSecurityStamp(); - // Assert - await expect(result).rejects.toThrow("User id not found. Cannot get security stamp."); - }); - - it("should return the security stamp from memory with no user id specified (uses global active user)", async () => { - // Arrange - globalStateProvider - .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID) - .stateSubject.next(userIdFromAccessToken); - - singleUserStateProvider - .getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY) - .stateSubject.next([userIdFromAccessToken, mockSecurityStamp]); - - // Act - const result = await tokenService.getSecurityStamp(); - - // Assert - expect(result).toEqual(mockSecurityStamp); - }); - - it("should return the security stamp from memory for the specified user id", async () => { - // Arrange - singleUserStateProvider - .getFake(userIdFromAccessToken, SECURITY_STAMP_MEMORY) - .stateSubject.next([userIdFromAccessToken, mockSecurityStamp]); - - // Act - const result = await tokenService.getSecurityStamp(userIdFromAccessToken); - // Assert - expect(result).toEqual(mockSecurityStamp); - }); - }); - }); - // Helpers function createTokenService(supportsSecureStorage: boolean) { return new TokenService( diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index 40036a8453c..c24a2c186b8 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -32,7 +32,6 @@ import { EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, REFRESH_TOKEN_DISK, REFRESH_TOKEN_MEMORY, - SECURITY_STAMP_MEMORY, } from "./token.state"; export enum TokenStorageLocation { @@ -851,30 +850,6 @@ export class TokenService implements TokenServiceAbstraction { return Array.isArray(decoded.amr) && decoded.amr.includes("external"); } - async getSecurityStamp(userId?: UserId): Promise { - userId ??= await firstValueFrom(this.activeUserIdGlobalState.state$); - - if (!userId) { - throw new Error("User id not found. Cannot get security stamp."); - } - - const securityStamp = await this.getStateValueByUserIdAndKeyDef(userId, SECURITY_STAMP_MEMORY); - - return securityStamp; - } - - async setSecurityStamp(securityStamp: string, userId?: UserId): Promise { - userId ??= await firstValueFrom(this.activeUserIdGlobalState.state$); - - if (!userId) { - throw new Error("User id not found. Cannot set security stamp."); - } - - await this.singleUserStateProvider - .get(userId, SECURITY_STAMP_MEMORY) - .update((_) => securityStamp); - } - private async getStateValueByUserIdAndKeyDef( userId: UserId, storageLocation: UserKeyDefinition, diff --git a/libs/common/src/auth/services/token.state.spec.ts b/libs/common/src/auth/services/token.state.spec.ts index bb82410fac1..dc00fec383c 100644 --- a/libs/common/src/auth/services/token.state.spec.ts +++ b/libs/common/src/auth/services/token.state.spec.ts @@ -10,7 +10,6 @@ import { EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, REFRESH_TOKEN_DISK, REFRESH_TOKEN_MEMORY, - SECURITY_STAMP_MEMORY, } from "./token.state"; describe.each([ @@ -23,7 +22,6 @@ describe.each([ [API_KEY_CLIENT_ID_MEMORY, "apiKeyClientIdMemory"], [API_KEY_CLIENT_SECRET_DISK, "apiKeyClientSecretDisk"], [API_KEY_CLIENT_SECRET_MEMORY, "apiKeyClientSecretMemory"], - [SECURITY_STAMP_MEMORY, "securityStamp"], ])( "deserializes state key definitions", ( diff --git a/libs/common/src/auth/services/token.state.ts b/libs/common/src/auth/services/token.state.ts index 57d85f2a559..458d6846c17 100644 --- a/libs/common/src/auth/services/token.state.ts +++ b/libs/common/src/auth/services/token.state.ts @@ -69,8 +69,3 @@ export const API_KEY_CLIENT_SECRET_MEMORY = new UserKeyDefinition( clearOn: [], // Manually handled }, ); - -export const SECURITY_STAMP_MEMORY = new UserKeyDefinition(TOKEN_MEMORY, "securityStamp", { - deserializer: (securityStamp) => securityStamp, - clearOn: ["logout"], -}); diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index f1d4b3848ef..051604f0ae5 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -181,6 +181,8 @@ export abstract class StateService { * Sets the user's Pin, encrypted by the user key */ setProtectedPin: (value: string, options?: StorageOptions) => Promise; + getSecurityStamp: (options?: StorageOptions) => Promise; + setSecurityStamp: (value: string, options?: StorageOptions) => Promise; getUserId: (options?: StorageOptions) => Promise; getVaultTimeout: (options?: StorageOptions) => Promise; setVaultTimeout: (value: number, options?: StorageOptions) => Promise; diff --git a/libs/common/src/platform/models/domain/account-tokens.spec.ts b/libs/common/src/platform/models/domain/account-tokens.spec.ts new file mode 100644 index 00000000000..733b3908e9a --- /dev/null +++ b/libs/common/src/platform/models/domain/account-tokens.spec.ts @@ -0,0 +1,9 @@ +import { AccountTokens } from "./account"; + +describe("AccountTokens", () => { + describe("fromJSON", () => { + it("should deserialize to an instance of itself", () => { + expect(AccountTokens.fromJSON({})).toBeInstanceOf(AccountTokens); + }); + }); +}); diff --git a/libs/common/src/platform/models/domain/account.spec.ts b/libs/common/src/platform/models/domain/account.spec.ts index 77c242b6ff5..0c76c16cc2d 100644 --- a/libs/common/src/platform/models/domain/account.spec.ts +++ b/libs/common/src/platform/models/domain/account.spec.ts @@ -1,4 +1,4 @@ -import { Account, AccountKeys, AccountProfile, AccountSettings } from "./account"; +import { Account, AccountKeys, AccountProfile, AccountSettings, AccountTokens } from "./account"; describe("Account", () => { describe("fromJSON", () => { @@ -10,12 +10,14 @@ describe("Account", () => { const keysSpy = jest.spyOn(AccountKeys, "fromJSON"); const profileSpy = jest.spyOn(AccountProfile, "fromJSON"); const settingsSpy = jest.spyOn(AccountSettings, "fromJSON"); + const tokensSpy = jest.spyOn(AccountTokens, "fromJSON"); Account.fromJSON({}); expect(keysSpy).toHaveBeenCalled(); expect(profileSpy).toHaveBeenCalled(); expect(settingsSpy).toHaveBeenCalled(); + expect(tokensSpy).toHaveBeenCalled(); }); }); }); diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index cd416ec1f94..5a9a7646962 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -171,11 +171,24 @@ export class AccountSettings { } } +export class AccountTokens { + securityStamp?: string; + + static fromJSON(obj: Jsonify): AccountTokens { + if (obj == null) { + return null; + } + + return Object.assign(new AccountTokens(), obj); + } +} + export class Account { data?: AccountData = new AccountData(); keys?: AccountKeys = new AccountKeys(); profile?: AccountProfile = new AccountProfile(); settings?: AccountSettings = new AccountSettings(); + tokens?: AccountTokens = new AccountTokens(); constructor(init: Partial) { Object.assign(this, { @@ -195,6 +208,10 @@ export class Account { ...new AccountSettings(), ...init?.settings, }, + tokens: { + ...new AccountTokens(), + ...init?.tokens, + }, }); } @@ -208,6 +225,7 @@ export class Account { data: AccountData.fromJSON(json?.data), profile: AccountProfile.fromJSON(json?.profile), settings: AccountSettings.fromJSON(json?.settings), + tokens: AccountTokens.fromJSON(json?.tokens), }); } } diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index d0a55d7a47c..f660cd7a342 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -839,6 +839,23 @@ export class StateService< ); } + async getSecurityStamp(options?: StorageOptions): Promise { + return ( + await this.getAccount(this.reconcileOptions(options, await this.defaultInMemoryOptions())) + )?.tokens?.securityStamp; + } + + async setSecurityStamp(value: string, options?: StorageOptions): Promise { + const account = await this.getAccount( + this.reconcileOptions(options, await this.defaultInMemoryOptions()), + ); + account.tokens.securityStamp = value; + await this.saveAccount( + account, + this.reconcileOptions(options, await this.defaultInMemoryOptions()), + ); + } + async getUserId(options?: StorageOptions): Promise { return ( await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())) diff --git a/libs/common/src/vault/services/sync/sync.service.ts b/libs/common/src/vault/services/sync/sync.service.ts index 73869ff488e..ff8e9f1f4f5 100644 --- a/libs/common/src/vault/services/sync/sync.service.ts +++ b/libs/common/src/vault/services/sync/sync.service.ts @@ -15,7 +15,6 @@ import { AccountService } from "../../../auth/abstractions/account.service"; import { AvatarService } from "../../../auth/abstractions/avatar.service"; import { KeyConnectorService } from "../../../auth/abstractions/key-connector.service"; import { InternalMasterPasswordServiceAbstraction } from "../../../auth/abstractions/master-password.service.abstraction"; -import { TokenService } from "../../../auth/abstractions/token.service"; import { ForceSetPasswordReason } from "../../../auth/models/domain/force-set-password-reason"; import { DomainSettingsService } from "../../../autofill/services/domain-settings.service"; import { BillingAccountProfileStateService } from "../../../billing/abstractions/account/billing-account-profile-state.service"; @@ -74,7 +73,6 @@ export class SyncService implements SyncServiceAbstraction { private avatarService: AvatarService, private logoutCallback: (expired: boolean) => Promise, private billingAccountProfileStateService: BillingAccountProfileStateService, - private tokenService: TokenService, ) {} async getLastSync(): Promise { @@ -311,7 +309,7 @@ export class SyncService implements SyncServiceAbstraction { } private async syncProfile(response: ProfileResponse) { - const stamp = await this.tokenService.getSecurityStamp(response.id as UserId); + const stamp = await this.stateService.getSecurityStamp(); if (stamp != null && stamp !== response.securityStamp) { if (this.logoutCallback != null) { await this.logoutCallback(true); @@ -325,7 +323,7 @@ export class SyncService implements SyncServiceAbstraction { await this.cryptoService.setProviderKeys(response.providers); await this.cryptoService.setOrgKeys(response.organizations, response.providerOrganizations); await this.avatarService.setSyncAvatarColor(response.id as UserId, response.avatarColor); - await this.tokenService.setSecurityStamp(response.securityStamp, response.id as UserId); + await this.stateService.setSecurityStamp(response.securityStamp); await this.stateService.setEmailVerified(response.emailVerified); await this.billingAccountProfileStateService.setHasPremium( From b395cb40a7a0e35d836009028f4f32242858d0f7 Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Mon, 22 Apr 2024 09:32:44 -0700 Subject: [PATCH 06/16] [AC-1999] Fix deleting collections from collection dialog (#8647) * [AC-1999] Fix null check this.collection can be both null or unassigned and `!= null` will handle both cases. * [AC-1999] Navigate away when selected collection is deleted --------- Co-authored-by: bnagawiecki <107435978+bnagawiecki@users.noreply.github.com> --- .../vault/individual-vault/vault.component.ts | 12 +++++++++--- .../vault-header/vault-header.component.ts | 2 +- .../src/app/vault/org-vault/vault.component.ts | 18 ++++++++++++++---- 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/apps/web/src/app/vault/individual-vault/vault.component.ts b/apps/web/src/app/vault/individual-vault/vault.component.ts index a25ba6edbcf..2c20328336b 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -679,6 +679,14 @@ export class VaultComponent implements OnInit, OnDestroy { } else if (result.action === CollectionDialogAction.Deleted) { await this.collectionService.delete(result.collection?.id); this.refresh(); + // Navigate away if we deleted the collection we were viewing + if (this.selectedCollection?.node.id === c?.id) { + void this.router.navigate([], { + queryParams: { collectionId: this.selectedCollection.parent?.node.id ?? null }, + queryParamsHandling: "merge", + replaceUrl: true, + }); + } } } @@ -710,9 +718,7 @@ export class VaultComponent implements OnInit, OnDestroy { ); // Navigate away if we deleted the collection we were viewing if (this.selectedCollection?.node.id === collection.id) { - // 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.router.navigate([], { + void this.router.navigate([], { queryParams: { collectionId: this.selectedCollection.parent?.node.id ?? null }, queryParamsHandling: "merge", replaceUrl: true, diff --git a/apps/web/src/app/vault/org-vault/vault-header/vault-header.component.ts b/apps/web/src/app/vault/org-vault/vault-header/vault-header.component.ts index c4c67759c7f..a5cd4680081 100644 --- a/apps/web/src/app/vault/org-vault/vault-header/vault-header.component.ts +++ b/apps/web/src/app/vault/org-vault/vault-header/vault-header.component.ts @@ -80,7 +80,7 @@ export class VaultHeaderComponent implements OnInit { ? this.i18nService.t("collections").toLowerCase() : this.i18nService.t("vault").toLowerCase(); - if (this.collection !== undefined) { + if (this.collection != null) { return this.collection.node.name; } diff --git a/apps/web/src/app/vault/org-vault/vault.component.ts b/apps/web/src/app/vault/org-vault/vault.component.ts index 587758dda13..9de404e9698 100644 --- a/apps/web/src/app/vault/org-vault/vault.component.ts +++ b/apps/web/src/app/vault/org-vault/vault.component.ts @@ -958,11 +958,9 @@ export class VaultComponent implements OnInit, OnDestroy { this.i18nService.t("deletedCollectionId", collection.name), ); - // Navigate away if we deleted the colletion we were viewing + // Navigate away if we deleted the collection we were viewing if (this.selectedCollection?.node.id === collection.id) { - // 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.router.navigate([], { + void this.router.navigate([], { queryParams: { collectionId: this.selectedCollection.parent?.node.id ?? null }, queryParamsHandling: "merge", replaceUrl: true, @@ -1095,6 +1093,18 @@ export class VaultComponent implements OnInit, OnDestroy { result.action === CollectionDialogAction.Deleted ) { this.refresh(); + + // If we deleted the selected collection, navigate up/away + if ( + result.action === CollectionDialogAction.Deleted && + this.selectedCollection?.node.id === c?.id + ) { + void this.router.navigate([], { + queryParams: { collectionId: this.selectedCollection.parent?.node.id ?? null }, + queryParamsHandling: "merge", + replaceUrl: true, + }); + } } } From fb211c5feea0594ccdd992c2729154424376bc57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Garc=C3=ADa?= Date: Mon, 22 Apr 2024 18:58:29 +0200 Subject: [PATCH 07/16] Add rust analyzer support for desktop-native (#8830) --- .vscode/settings.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 27e3a9b293a..3a70af3481d 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -5,5 +5,6 @@ "**/locales/*[^n]/messages.json": true, "**/_locales/[^e]*/messages.json": true, "**/_locales/*[^n]/messages.json": true - } + }, + "rust-analyzer.linkedProjects": ["apps/desktop/desktop_native/Cargo.toml"] } From 29d4f1aad5fade6c85749e002a4d0b1a05f2fdbb Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Mon, 22 Apr 2024 12:58:20 -0500 Subject: [PATCH 08/16] [PM-7660] Master Password Re-Prompt from Autofill Not Working (#8862) --- .../browser/src/background/main.background.ts | 30 ++++++++++--------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 15f21b25014..a64ee2b8a00 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -1105,20 +1105,22 @@ export default class MainBackground { await (this.eventUploadService as EventUploadService).init(true); this.twoFactorService.init(); - if (!this.popupOnlyContext) { - await this.vaultTimeoutService.init(true); - this.fido2Background.init(); - await this.runtimeBackground.init(); - await this.notificationBackground.init(); - this.filelessImporterBackground.init(); - await this.commandsBackground.init(); - await this.overlayBackground.init(); - await this.tabsBackground.init(); - this.contextMenusBackground?.init(); - await this.idleBackground.init(); - if (BrowserApi.isManifestVersion(2)) { - await this.webRequestBackground.init(); - } + if (this.popupOnlyContext) { + return; + } + + await this.vaultTimeoutService.init(true); + this.fido2Background.init(); + await this.runtimeBackground.init(); + await this.notificationBackground.init(); + this.filelessImporterBackground.init(); + await this.commandsBackground.init(); + await this.overlayBackground.init(); + await this.tabsBackground.init(); + this.contextMenusBackground?.init(); + await this.idleBackground.init(); + if (BrowserApi.isManifestVersion(2)) { + await this.webRequestBackground.init(); } if (this.platformUtilsService.isFirefox() && !this.isPrivateMode) { From e29779875797cb8a508d3857aa25ecefca5aa5c1 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 22 Apr 2024 16:54:41 -0400 Subject: [PATCH 09/16] Stop CryptoService from using `getBgService` (#8843) --- .../src/popup/services/services.module.ts | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index a7da6b76127..b344a18492e 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -31,6 +31,7 @@ import { AuthService as AuthServiceAbstraction } from "@bitwarden/common/auth/ab import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { DevicesServiceAbstraction } from "@bitwarden/common/auth/abstractions/devices/devices.service.abstraction"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; +import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; import { SsoLoginServiceAbstraction } from "@bitwarden/common/auth/abstractions/sso-login.service.abstraction"; import { TokenService } from "@bitwarden/common/auth/abstractions/token.service"; import { TwoFactorService } from "@bitwarden/common/auth/abstractions/two-factor.service"; @@ -55,6 +56,7 @@ import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt. import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { FileDownloadService } from "@bitwarden/common/platform/abstractions/file-download/file-download.service"; import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService as BaseStateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service"; @@ -63,6 +65,7 @@ import { AbstractStorageService, ObservableStorageService, } from "@bitwarden/common/platform/abstractions/storage.service"; +import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { Message, MessageListener, MessageSender } from "@bitwarden/common/platform/messaging"; // eslint-disable-next-line no-restricted-imports -- Used for dependency injection @@ -102,6 +105,7 @@ import BrowserPopupUtils from "../../platform/popup/browser-popup-utils"; import { BrowserFileDownloadService } from "../../platform/popup/services/browser-file-download.service"; import { BrowserStateService as StateServiceAbstraction } from "../../platform/services/abstractions/browser-state.service"; import { ScriptInjectorService } from "../../platform/services/abstractions/script-injector.service"; +import { BrowserCryptoService } from "../../platform/services/browser-crypto.service"; import { BrowserEnvironmentService } from "../../platform/services/browser-environment.service"; import BrowserLocalStorageService from "../../platform/services/browser-local-storage.service"; import { BrowserScriptInjectorService } from "../../platform/services/browser-script-injector.service"; @@ -232,12 +236,45 @@ const safeProviders: SafeProvider[] = [ }), safeProvider({ provide: CryptoService, - useFactory: (encryptService: EncryptService) => { - const cryptoService = getBgService("cryptoService")(); + useFactory: ( + masterPasswordService: InternalMasterPasswordServiceAbstraction, + keyGenerationService: KeyGenerationService, + cryptoFunctionService: CryptoFunctionService, + encryptService: EncryptService, + platformUtilsService: PlatformUtilsService, + logService: LogService, + stateService: StateServiceAbstraction, + accountService: AccountServiceAbstraction, + stateProvider: StateProvider, + biometricStateService: BiometricStateService, + ) => { + const cryptoService = new BrowserCryptoService( + masterPasswordService, + keyGenerationService, + cryptoFunctionService, + encryptService, + platformUtilsService, + logService, + stateService, + accountService, + stateProvider, + biometricStateService, + ); new ContainerService(cryptoService, encryptService).attachToGlobal(self); return cryptoService; }, - deps: [EncryptService], + deps: [ + InternalMasterPasswordServiceAbstraction, + KeyGenerationService, + CryptoFunctionService, + EncryptService, + PlatformUtilsService, + LogService, + StateServiceAbstraction, + AccountServiceAbstraction, + StateProvider, + BiometricStateService, + ], }), safeProvider({ provide: TotpServiceAbstraction, From 33dae77a4d9caab3a0ea88ec89d10efb38097a84 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 22 Apr 2024 17:11:30 -0400 Subject: [PATCH 10/16] Revert "Stop CryptoService from using `getBgService` (#8843)" (#8867) This reverts commit e29779875797cb8a508d3857aa25ecefca5aa5c1. --- .../src/popup/services/services.module.ts | 43 ++----------------- 1 file changed, 3 insertions(+), 40 deletions(-) diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index b344a18492e..a7da6b76127 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -31,7 +31,6 @@ import { AuthService as AuthServiceAbstraction } from "@bitwarden/common/auth/ab import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; import { DevicesServiceAbstraction } from "@bitwarden/common/auth/abstractions/devices/devices.service.abstraction"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; -import { InternalMasterPasswordServiceAbstraction } from "@bitwarden/common/auth/abstractions/master-password.service.abstraction"; import { SsoLoginServiceAbstraction } from "@bitwarden/common/auth/abstractions/sso-login.service.abstraction"; import { TokenService } from "@bitwarden/common/auth/abstractions/token.service"; import { TwoFactorService } from "@bitwarden/common/auth/abstractions/two-factor.service"; @@ -56,7 +55,6 @@ import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt. import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { FileDownloadService } from "@bitwarden/common/platform/abstractions/file-download/file-download.service"; import { I18nService as I18nServiceAbstraction } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { KeyGenerationService } from "@bitwarden/common/platform/abstractions/key-generation.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService as BaseStateServiceAbstraction } from "@bitwarden/common/platform/abstractions/state.service"; @@ -65,7 +63,6 @@ import { AbstractStorageService, ObservableStorageService, } from "@bitwarden/common/platform/abstractions/storage.service"; -import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; import { StateFactory } from "@bitwarden/common/platform/factories/state-factory"; import { Message, MessageListener, MessageSender } from "@bitwarden/common/platform/messaging"; // eslint-disable-next-line no-restricted-imports -- Used for dependency injection @@ -105,7 +102,6 @@ import BrowserPopupUtils from "../../platform/popup/browser-popup-utils"; import { BrowserFileDownloadService } from "../../platform/popup/services/browser-file-download.service"; import { BrowserStateService as StateServiceAbstraction } from "../../platform/services/abstractions/browser-state.service"; import { ScriptInjectorService } from "../../platform/services/abstractions/script-injector.service"; -import { BrowserCryptoService } from "../../platform/services/browser-crypto.service"; import { BrowserEnvironmentService } from "../../platform/services/browser-environment.service"; import BrowserLocalStorageService from "../../platform/services/browser-local-storage.service"; import { BrowserScriptInjectorService } from "../../platform/services/browser-script-injector.service"; @@ -236,45 +232,12 @@ const safeProviders: SafeProvider[] = [ }), safeProvider({ provide: CryptoService, - useFactory: ( - masterPasswordService: InternalMasterPasswordServiceAbstraction, - keyGenerationService: KeyGenerationService, - cryptoFunctionService: CryptoFunctionService, - encryptService: EncryptService, - platformUtilsService: PlatformUtilsService, - logService: LogService, - stateService: StateServiceAbstraction, - accountService: AccountServiceAbstraction, - stateProvider: StateProvider, - biometricStateService: BiometricStateService, - ) => { - const cryptoService = new BrowserCryptoService( - masterPasswordService, - keyGenerationService, - cryptoFunctionService, - encryptService, - platformUtilsService, - logService, - stateService, - accountService, - stateProvider, - biometricStateService, - ); + useFactory: (encryptService: EncryptService) => { + const cryptoService = getBgService("cryptoService")(); new ContainerService(cryptoService, encryptService).attachToGlobal(self); return cryptoService; }, - deps: [ - InternalMasterPasswordServiceAbstraction, - KeyGenerationService, - CryptoFunctionService, - EncryptService, - PlatformUtilsService, - LogService, - StateServiceAbstraction, - AccountServiceAbstraction, - StateProvider, - BiometricStateService, - ], + deps: [EncryptService], }), safeProvider({ provide: TotpServiceAbstraction, From 4afb5d04f00c4a4a883d3dfc20a8752f929880d6 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 22 Apr 2024 17:14:14 -0400 Subject: [PATCH 11/16] Remove `alarms` Permission (#8866) --- apps/browser/src/manifest.v3.json | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/browser/src/manifest.v3.json b/apps/browser/src/manifest.v3.json index cdd0869fc52..26efbbbf9ba 100644 --- a/apps/browser/src/manifest.v3.json +++ b/apps/browser/src/manifest.v3.json @@ -59,7 +59,6 @@ "clipboardRead", "clipboardWrite", "idle", - "alarms", "scripting", "offscreen" ], From 714ca66f331ec4ffde3a3e40fc767b27a4967903 Mon Sep 17 00:00:00 2001 From: Bitwarden DevOps <106330231+bitwarden-devops-bot@users.noreply.github.com> Date: Tue, 23 Apr 2024 07:32:09 -0400 Subject: [PATCH 12/16] Bumped browser,cli,desktop,web version to (#8875) --- apps/browser/package.json | 2 +- apps/browser/src/manifest.json | 2 +- apps/browser/src/manifest.v3.json | 2 +- apps/cli/package.json | 2 +- apps/desktop/package.json | 2 +- apps/desktop/src/package-lock.json | 4 ++-- apps/desktop/src/package.json | 2 +- apps/web/package.json | 2 +- package-lock.json | 8 ++++---- 9 files changed, 13 insertions(+), 13 deletions(-) diff --git a/apps/browser/package.json b/apps/browser/package.json index ee6d100572d..506f19f279b 100644 --- a/apps/browser/package.json +++ b/apps/browser/package.json @@ -1,6 +1,6 @@ { "name": "@bitwarden/browser", - "version": "2024.4.1", + "version": "2024.4.2", "scripts": { "build": "webpack", "build:mv3": "cross-env MANIFEST_VERSION=3 webpack", diff --git a/apps/browser/src/manifest.json b/apps/browser/src/manifest.json index 78f1e2cc419..18bfaf8acbd 100644 --- a/apps/browser/src/manifest.json +++ b/apps/browser/src/manifest.json @@ -2,7 +2,7 @@ "manifest_version": 2, "name": "__MSG_extName__", "short_name": "__MSG_appName__", - "version": "2024.4.1", + "version": "2024.4.2", "description": "__MSG_extDesc__", "default_locale": "en", "author": "Bitwarden Inc.", diff --git a/apps/browser/src/manifest.v3.json b/apps/browser/src/manifest.v3.json index 26efbbbf9ba..c0c88706b80 100644 --- a/apps/browser/src/manifest.v3.json +++ b/apps/browser/src/manifest.v3.json @@ -3,7 +3,7 @@ "minimum_chrome_version": "102.0", "name": "__MSG_extName__", "short_name": "__MSG_appName__", - "version": "2024.4.1", + "version": "2024.4.2", "description": "__MSG_extDesc__", "default_locale": "en", "author": "Bitwarden Inc.", diff --git a/apps/cli/package.json b/apps/cli/package.json index 690842d831d..b06caacfd48 100644 --- a/apps/cli/package.json +++ b/apps/cli/package.json @@ -1,7 +1,7 @@ { "name": "@bitwarden/cli", "description": "A secure and free password manager for all of your devices.", - "version": "2024.3.1", + "version": "2024.4.0", "keywords": [ "bitwarden", "password", diff --git a/apps/desktop/package.json b/apps/desktop/package.json index 4bb0ab2d931..5e098eb2135 100644 --- a/apps/desktop/package.json +++ b/apps/desktop/package.json @@ -1,7 +1,7 @@ { "name": "@bitwarden/desktop", "description": "A secure and free password manager for all of your devices.", - "version": "2024.4.2", + "version": "2024.4.3", "keywords": [ "bitwarden", "password", diff --git a/apps/desktop/src/package-lock.json b/apps/desktop/src/package-lock.json index 11b38bd2738..d6945fd16e1 100644 --- a/apps/desktop/src/package-lock.json +++ b/apps/desktop/src/package-lock.json @@ -1,12 +1,12 @@ { "name": "@bitwarden/desktop", - "version": "2024.4.2", + "version": "2024.4.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@bitwarden/desktop", - "version": "2024.4.2", + "version": "2024.4.3", "license": "GPL-3.0", "dependencies": { "@bitwarden/desktop-native": "file:../desktop_native", diff --git a/apps/desktop/src/package.json b/apps/desktop/src/package.json index a65dab016ca..fa190af9a69 100644 --- a/apps/desktop/src/package.json +++ b/apps/desktop/src/package.json @@ -2,7 +2,7 @@ "name": "@bitwarden/desktop", "productName": "Bitwarden", "description": "A secure and free password manager for all of your devices.", - "version": "2024.4.2", + "version": "2024.4.3", "author": "Bitwarden Inc. (https://bitwarden.com)", "homepage": "https://bitwarden.com", "license": "GPL-3.0", diff --git a/apps/web/package.json b/apps/web/package.json index 55fe0987d72..434712cdf4a 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -1,6 +1,6 @@ { "name": "@bitwarden/web-vault", - "version": "2024.4.1", + "version": "2024.4.2", "scripts": { "build:oss": "webpack", "build:bit": "webpack -c ../../bitwarden_license/bit-web/webpack.config.js", diff --git a/package-lock.json b/package-lock.json index d72ba9cb19b..f5932a25b80 100644 --- a/package-lock.json +++ b/package-lock.json @@ -193,11 +193,11 @@ }, "apps/browser": { "name": "@bitwarden/browser", - "version": "2024.4.1" + "version": "2024.4.2" }, "apps/cli": { "name": "@bitwarden/cli", - "version": "2024.3.1", + "version": "2024.4.0", "license": "GPL-3.0-only", "dependencies": { "@koa/multer": "3.0.2", @@ -233,7 +233,7 @@ }, "apps/desktop": { "name": "@bitwarden/desktop", - "version": "2024.4.2", + "version": "2024.4.3", "hasInstallScript": true, "license": "GPL-3.0" }, @@ -247,7 +247,7 @@ }, "apps/web": { "name": "@bitwarden/web-vault", - "version": "2024.4.1" + "version": "2024.4.2" }, "libs/admin-console": { "name": "@bitwarden/admin-console", From e4ebf4aeccb4c214e0b483113d9b9b53cc6f7438 Mon Sep 17 00:00:00 2001 From: Todd Martin <106564991+trmartin4@users.noreply.github.com> Date: Tue, 23 Apr 2024 09:18:49 -0400 Subject: [PATCH 13/16] [PM-7349] Update snap description with new URL to help docs (#8703) * Updated snap summary with new URL to help docs. * Updated to use summary and description. --- apps/desktop/electron-builder.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/apps/desktop/electron-builder.json b/apps/desktop/electron-builder.json index 4f0d05581c0..960d56b0362 100644 --- a/apps/desktop/electron-builder.json +++ b/apps/desktop/electron-builder.json @@ -228,7 +228,8 @@ "artifactName": "${productName}-${version}-${arch}.${ext}" }, "snap": { - "summary": "After installation enable required `password-manager-service` by running `sudo snap connect bitwarden:password-manager-service`.", + "summary": "Bitwarden is a secure and free password manager for all of your devices.", + "description": "**Installation**\nBitwarden requires access to the `password-manager-service`. Please enable it through permissions or by running `sudo snap connect bitwarden:password-manager-service` after installation. See https://btwrdn.com/install-snap for details.", "autoStart": true, "base": "core22", "confinement": "strict", From 73d0782b6ce4a0be1cf0480d0a9658420b5ef438 Mon Sep 17 00:00:00 2001 From: Will Martin Date: Tue, 23 Apr 2024 09:45:11 -0400 Subject: [PATCH 14/16] [CL-110] fix code block text color in Storybook (#8868) --- libs/components/src/styles.scss | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/libs/components/src/styles.scss b/libs/components/src/styles.scss index f03d2dd3409..ae97838e09f 100644 --- a/libs/components/src/styles.scss +++ b/libs/components/src/styles.scss @@ -47,3 +47,8 @@ $card-icons-base: "../../src/billing/images/cards/"; @import "bootstrap/scss/_print"; @import "multi-select/scss/bw.theme.scss"; + +// Workaround for https://bitwarden.atlassian.net/browse/CL-110 +#storybook-docs pre.prismjs { + color: white; +} From 7d58b21856c5551dadd7127c725be2882076cf20 Mon Sep 17 00:00:00 2001 From: vinith-kovan <156108204+vinith-kovan@users.noreply.github.com> Date: Tue, 23 Apr 2024 19:20:15 +0530 Subject: [PATCH 15/16] migrating activate autofill component (#8782) --- .../policies/activate-autofill.component.html | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/bitwarden_license/bit-web/src/app/admin-console/policies/activate-autofill.component.html b/bitwarden_license/bit-web/src/app/admin-console/policies/activate-autofill.component.html index 9f08e98daea..94f2e8a422d 100644 --- a/bitwarden_license/bit-web/src/app/admin-console/policies/activate-autofill.component.html +++ b/bitwarden_license/bit-web/src/app/admin-console/policies/activate-autofill.component.html @@ -5,15 +5,7 @@ }} -
-
- - -
-
+ + + {{ "turnOn" | i18n }} + From 38ea110755a4256f96ba7946b680070b5c32cdc6 Mon Sep 17 00:00:00 2001 From: vinith-kovan <156108204+vinith-kovan@users.noreply.github.com> Date: Tue, 23 Apr 2024 19:22:26 +0530 Subject: [PATCH 16/16] migrating two factor authentication component (#8760) --- .../two-factor-authentication.component.html | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/policies/two-factor-authentication.component.html b/apps/web/src/app/admin-console/organizations/policies/two-factor-authentication.component.html index 3286c086894..d0be72a52e6 100644 --- a/apps/web/src/app/admin-console/organizations/policies/two-factor-authentication.component.html +++ b/apps/web/src/app/admin-console/organizations/policies/two-factor-authentication.component.html @@ -2,15 +2,7 @@ {{ "twoStepLoginPolicyWarning" | i18n }} -
-
- - -
-
+ + + {{ "turnOn" | i18n }} +