1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-15 07:43:35 +00:00

Ps/pm 2910/add browser storage services (#6849)

* Allow for update logic in state update callbacks

* Prefer reading updates to sending in stream

* Inform state providers when they must deserialize

* Update DefaultGlobalState to act more like DefaultUserState

* Fully Implement AbstractStorageService

* Add KeyDefinitionOptions

* Address PR feedback

* Prefer testing interactions for ports

* Synced memory storage for browser

* Fix port handling

* Do not stringify port message data

* Use messaging storage

* Initialize new foreground memory storage services

This will need to be rethought for short-lived background pages, but for
now the background is the source of truth for memory storage

* Use global state for account service

* Use BrowserApi listener to avoid safari memory leaks

* Fix build errors: debugging and missed impls

* Prefer bound arrow functions

* JSON Stringify Messages

* Prefer `useClass`

* Use noop services

* extract storage observable to new interface

This also reverts changes for the existing services to use
foreground/background services. Those are now used only in state
providers

* Fix web DI

* Prefer initializing observable in constructor

* Do not use jsonify as equality operator

* Remove port listener to avoid memory leaks

* Fix logic and type issues

---------

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>
This commit is contained in:
Matt Gibson
2023-11-21 16:35:37 -05:00
committed by GitHub
parent 1ecf019397
commit 24c240d0d4
36 changed files with 744 additions and 160 deletions

View File

@@ -3,12 +3,20 @@ import { Observable } from "rxjs";
import { UserId } from "../../types/guid";
import { AuthenticationStatus } from "../enums/authentication-status";
/**
* Holds information about an account for use in the AccountService
* if more information is added, be sure to update the equality method.
*/
export type AccountInfo = {
status: AuthenticationStatus;
email: string;
name: string | undefined;
};
export function accountInfoEqual(a: AccountInfo, b: AccountInfo) {
return a.status == b.status && a.email == b.email && a.name == b.name;
}
export abstract class AccountService {
accounts$: Observable<Record<UserId, AccountInfo>>;
activeAccount$: Observable<{ id: UserId | undefined } & AccountInfo>;

View File

@@ -1,9 +1,15 @@
import { MockProxy, mock } from "jest-mock-extended";
import { firstValueFrom } from "rxjs";
import { BehaviorSubject, firstValueFrom } from "rxjs";
import { trackEmissions } from "../../../spec/utils";
import { LogService } from "../../platform/abstractions/log.service";
import { MessagingService } from "../../platform/abstractions/messaging.service";
import {
ACCOUNT_ACCOUNTS,
ACCOUNT_ACTIVE_ACCOUNT_ID,
GlobalState,
GlobalStateProvider,
} from "../../platform/state";
import { UserId } from "../../types/guid";
import { AccountInfo } from "../abstractions/account.service";
import { AuthenticationStatus } from "../enums/authentication-status";
@@ -13,6 +19,11 @@ import { AccountServiceImplementation } from "./account.service";
describe("accountService", () => {
let messagingService: MockProxy<MessagingService>;
let logService: MockProxy<LogService>;
let globalStateProvider: MockProxy<GlobalStateProvider>;
let accountsState: MockProxy<GlobalState<Record<UserId, AccountInfo>>>;
let accountsSubject: BehaviorSubject<Record<UserId, AccountInfo>>;
let activeAccountIdState: MockProxy<GlobalState<UserId>>;
let activeAccountIdSubject: BehaviorSubject<UserId>;
let sut: AccountServiceImplementation;
const userId = "userId" as UserId;
function userInfo(status: AuthenticationStatus): AccountInfo {
@@ -20,10 +31,29 @@ describe("accountService", () => {
}
beforeEach(() => {
messagingService = mock<MessagingService>();
logService = mock<LogService>();
messagingService = mock();
logService = mock();
globalStateProvider = mock();
accountsState = mock();
activeAccountIdState = mock();
sut = new AccountServiceImplementation(messagingService, logService);
accountsSubject = new BehaviorSubject<Record<UserId, AccountInfo>>(null);
accountsState.state$ = accountsSubject.asObservable();
activeAccountIdSubject = new BehaviorSubject<UserId>(null);
activeAccountIdState.state$ = activeAccountIdSubject.asObservable();
globalStateProvider.get.mockImplementation((keyDefinition) => {
switch (keyDefinition) {
case ACCOUNT_ACCOUNTS:
return accountsState;
case ACCOUNT_ACTIVE_ACCOUNT_ID:
return activeAccountIdState;
default:
throw new Error("Unknown key definition");
}
});
sut = new AccountServiceImplementation(messagingService, logService, globalStateProvider);
});
afterEach(() => {
@@ -39,8 +69,8 @@ describe("accountService", () => {
it("should emit the active account and status", async () => {
const emissions = trackEmissions(sut.activeAccount$);
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
sut.switchAccount(userId);
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
activeAccountIdSubject.next(userId);
expect(emissions).toEqual([
undefined, // initial value
@@ -48,9 +78,21 @@ describe("accountService", () => {
]);
});
it("should update the status if the account status changes", async () => {
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
activeAccountIdSubject.next(userId);
const emissions = trackEmissions(sut.activeAccount$);
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Locked) });
expect(emissions).toEqual([
{ id: userId, ...userInfo(AuthenticationStatus.Unlocked) },
{ id: userId, ...userInfo(AuthenticationStatus.Locked) },
]);
});
it("should remember the last emitted value", async () => {
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
sut.switchAccount(userId);
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
activeAccountIdSubject.next(userId);
expect(await firstValueFrom(sut.activeAccount$)).toEqual({
id: userId,
@@ -59,77 +101,98 @@ describe("accountService", () => {
});
});
describe("accounts$", () => {
it("should maintain an accounts cache", async () => {
expect(await firstValueFrom(sut.accounts$)).toEqual({});
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
expect(await firstValueFrom(sut.accounts$)).toEqual({
[userId]: userInfo(AuthenticationStatus.Unlocked),
});
});
});
describe("addAccount", () => {
it("should emit the new account", () => {
const emissions = trackEmissions(sut.accounts$);
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
expect(emissions).toEqual([
{}, // initial value
{ [userId]: userInfo(AuthenticationStatus.Unlocked) },
]);
expect(accountsState.update).toHaveBeenCalledTimes(1);
const callback = accountsState.update.mock.calls[0][0];
expect(callback({}, null)).toEqual({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
});
});
describe("setAccountName", () => {
beforeEach(() => {
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
});
it("should emit the updated account", () => {
const emissions = trackEmissions(sut.accounts$);
it("should update the account", async () => {
sut.setAccountName(userId, "new name");
expect(emissions).toEqual([
{ [userId]: { ...userInfo(AuthenticationStatus.Unlocked), name: "name" } },
{ [userId]: { ...userInfo(AuthenticationStatus.Unlocked), name: "new name" } },
]);
const callback = accountsState.update.mock.calls[0][0];
expect(callback(accountsSubject.value, null)).toEqual({
[userId]: { ...userInfo(AuthenticationStatus.Unlocked), name: "new name" },
});
});
it("should not update if the name is the same", async () => {
sut.setAccountName(userId, "name");
const callback = accountsState.update.mock.calls[0][1].shouldUpdate;
expect(callback(accountsSubject.value, null)).toBe(false);
});
});
describe("setAccountEmail", () => {
beforeEach(() => {
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
});
it("should emit the updated account", () => {
const emissions = trackEmissions(sut.accounts$);
it("should update the account", () => {
sut.setAccountEmail(userId, "new email");
expect(emissions).toEqual([
{ [userId]: { ...userInfo(AuthenticationStatus.Unlocked), email: "email" } },
{ [userId]: { ...userInfo(AuthenticationStatus.Unlocked), email: "new email" } },
]);
const callback = accountsState.update.mock.calls[0][0];
expect(callback(accountsSubject.value, null)).toEqual({
[userId]: { ...userInfo(AuthenticationStatus.Unlocked), email: "new email" },
});
});
it("should not update if the email is the same", () => {
sut.setAccountEmail(userId, "email");
const callback = accountsState.update.mock.calls[0][1].shouldUpdate;
expect(callback(accountsSubject.value, null)).toBe(false);
});
});
describe("setAccountStatus", () => {
beforeEach(() => {
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
});
it("should not emit if the status is the same", async () => {
const emissions = trackEmissions(sut.accounts$);
sut.setAccountStatus(userId, AuthenticationStatus.Unlocked);
sut.setAccountStatus(userId, AuthenticationStatus.Unlocked);
it("should update the account", () => {
sut.setAccountStatus(userId, AuthenticationStatus.Locked);
expect(emissions).toEqual([{ userId: userInfo(AuthenticationStatus.Unlocked) }]);
});
const callback = accountsState.update.mock.calls[0][0];
it("should maintain an accounts cache", async () => {
expect(await firstValueFrom(sut.accounts$)).toEqual({
[userId]: userInfo(AuthenticationStatus.Unlocked),
expect(callback(accountsSubject.value, null)).toEqual({
[userId]: {
...userInfo(AuthenticationStatus.Unlocked),
status: AuthenticationStatus.Locked,
},
});
});
it("should emit if the status is different", () => {
const emissions = trackEmissions(sut.accounts$);
sut.setAccountStatus(userId, AuthenticationStatus.Locked);
it("should not update if the status is the same", () => {
sut.setAccountStatus(userId, AuthenticationStatus.Unlocked);
expect(emissions).toEqual([
{ userId: userInfo(AuthenticationStatus.Unlocked) }, // initial value from beforeEach
{ userId: userInfo(AuthenticationStatus.Locked) },
]);
const callback = accountsState.update.mock.calls[0][1].shouldUpdate;
expect(callback(accountsSubject.value, null)).toBe(false);
});
it("should emit logout if the status is logged out", () => {
@@ -148,34 +211,20 @@ describe("accountService", () => {
});
describe("switchAccount", () => {
let emissions: { id: string; status: AuthenticationStatus }[];
beforeEach(() => {
emissions = [];
sut.activeAccount$.subscribe((value) => emissions.push(value));
accountsSubject.next({ [userId]: userInfo(AuthenticationStatus.Unlocked) });
});
it("should emit undefined if no account is provided", () => {
sut.switchAccount(undefined);
expect(emissions).toEqual([undefined]);
sut.switchAccount(null);
const callback = activeAccountIdState.update.mock.calls[0][0];
expect(callback(userId, accountsSubject.value)).toBeUndefined();
});
it("should emit the active account and status", () => {
sut.addAccount(userId, userInfo(AuthenticationStatus.Unlocked));
sut.switchAccount(userId);
sut.setAccountStatus(userId, AuthenticationStatus.Locked);
sut.switchAccount(undefined);
sut.switchAccount(undefined);
expect(emissions).toEqual([
undefined, // initial value
{ id: userId, ...userInfo(AuthenticationStatus.Unlocked) },
{ id: userId, ...userInfo(AuthenticationStatus.Locked) },
]);
});
it("should throw if switched to an unknown account", () => {
expect(() => sut.switchAccount(userId)).toThrowError("Account does not exist");
it("should throw if the account does not exist", () => {
sut.switchAccount("unknown" as UserId);
const callback = activeAccountIdState.update.mock.calls[0][0];
expect(() => callback(userId, accountsSubject.value)).toThrowError("Account does not exist");
});
});
});

View File

@@ -1,50 +1,80 @@
import {
BehaviorSubject,
Subject,
combineLatestWith,
map,
distinctUntilChanged,
shareReplay,
} from "rxjs";
import { Subject, combineLatestWith, map, distinctUntilChanged, shareReplay } from "rxjs";
import { Jsonify } from "type-fest";
import { AccountInfo, InternalAccountService } from "../../auth/abstractions/account.service";
import {
AccountInfo,
InternalAccountService,
accountInfoEqual,
} from "../../auth/abstractions/account.service";
import { LogService } from "../../platform/abstractions/log.service";
import { MessagingService } from "../../platform/abstractions/messaging.service";
import {
ACCOUNT_ACCOUNTS,
ACCOUNT_ACTIVE_ACCOUNT_ID,
GlobalState,
GlobalStateProvider,
} from "../../platform/state";
import { UserId } from "../../types/guid";
import { AuthenticationStatus } from "../enums/authentication-status";
export function AccountsDeserializer(
accounts: Jsonify<Record<UserId, AccountInfo> | null>
): Record<UserId, AccountInfo> {
if (accounts == null) {
return {};
}
return accounts;
}
export class AccountServiceImplementation implements InternalAccountService {
private accounts = new BehaviorSubject<Record<UserId, AccountInfo>>({});
private activeAccountId = new BehaviorSubject<UserId | undefined>(undefined);
private lock = new Subject<UserId>();
private logout = new Subject<UserId>();
private accountsState: GlobalState<Record<UserId, AccountInfo>>;
private activeAccountIdState: GlobalState<UserId | undefined>;
accounts$ = this.accounts.asObservable();
activeAccount$ = this.activeAccountId.pipe(
combineLatestWith(this.accounts$),
map(([id, accounts]) => (id ? { id, ...accounts[id] } : undefined)),
distinctUntilChanged(),
shareReplay({ bufferSize: 1, refCount: false })
);
accounts$;
activeAccount$;
accountLock$ = this.lock.asObservable();
accountLogout$ = this.logout.asObservable();
constructor(private messagingService: MessagingService, private logService: LogService) {}
constructor(
private messagingService: MessagingService,
private logService: LogService,
private globalStateProvider: GlobalStateProvider
) {
this.accountsState = this.globalStateProvider.get(ACCOUNT_ACCOUNTS);
this.activeAccountIdState = this.globalStateProvider.get(ACCOUNT_ACTIVE_ACCOUNT_ID);
this.accounts$ = this.accountsState.state$.pipe(
map((accounts) => (accounts == null ? {} : accounts))
);
this.activeAccount$ = this.activeAccountIdState.state$.pipe(
combineLatestWith(this.accounts$),
map(([id, accounts]) => (id ? { id, ...accounts[id] } : undefined)),
distinctUntilChanged(),
shareReplay({ bufferSize: 1, refCount: false })
);
}
addAccount(userId: UserId, accountData: AccountInfo): void {
this.accounts.value[userId] = accountData;
this.accounts.next(this.accounts.value);
this.accountsState.update((accounts) => {
accounts ||= {};
accounts[userId] = accountData;
return accounts;
});
}
setAccountName(userId: UserId, name: string): void {
this.setAccountInfo(userId, { ...this.accounts.value[userId], name });
this.setAccountInfo(userId, { name });
}
setAccountEmail(userId: UserId, email: string): void {
this.setAccountInfo(userId, { ...this.accounts.value[userId], email });
this.setAccountInfo(userId, { email });
}
setAccountStatus(userId: UserId, status: AuthenticationStatus): void {
this.setAccountInfo(userId, { ...this.accounts.value[userId], status });
this.setAccountInfo(userId, { status });
if (status === AuthenticationStatus.LoggedOut) {
this.logout.next(userId);
@@ -54,16 +84,22 @@ export class AccountServiceImplementation implements InternalAccountService {
}
switchAccount(userId: UserId) {
if (userId == null) {
// indicates no account is active
this.activeAccountId.next(undefined);
return;
}
this.activeAccountIdState.update(
(_, accounts) => {
if (userId == null) {
// indicates no account is active
return undefined;
}
if (this.accounts.value[userId] == null) {
throw new Error("Account does not exist");
}
this.activeAccountId.next(userId);
if (accounts?.[userId] == null) {
throw new Error("Account does not exist");
}
return userId;
},
{
combineLatestWith: this.accounts$,
}
);
}
// TODO: update to use our own account status settings. Requires inverting direction of state service accounts flow
@@ -76,18 +112,26 @@ export class AccountServiceImplementation implements InternalAccountService {
}
}
private setAccountInfo(userId: UserId, accountInfo: AccountInfo) {
if (this.accounts.value[userId] == null) {
throw new Error("Account does not exist");
private setAccountInfo(userId: UserId, update: Partial<AccountInfo>) {
function newAccountInfo(oldAccountInfo: AccountInfo): AccountInfo {
return { ...oldAccountInfo, ...update };
}
this.accountsState.update(
(accounts) => {
accounts[userId] = newAccountInfo(accounts[userId]);
return accounts;
},
{
// Avoid unnecessary updates
// TODO: Faster comparison, maybe include a hash on the objects?
shouldUpdate: (accounts) => {
if (accounts?.[userId] == null) {
throw new Error("Account does not exist");
}
// Avoid unnecessary updates
// TODO: Faster comparison, maybe include a hash on the objects?
if (JSON.stringify(this.accounts.value[userId]) === JSON.stringify(accountInfo)) {
return;
}
this.accounts.value[userId] = accountInfo;
this.accounts.next(this.accounts.value);
return !accountInfoEqual(accounts[userId], newAccountInfo(accounts[userId]));
},
}
);
}
}

View File

@@ -8,14 +8,17 @@ export type StorageUpdate = {
updateType: StorageUpdateType;
};
export abstract class AbstractStorageService {
abstract get valuesRequireDeserialization(): boolean;
export interface ObservableStorageService {
/**
* Provides an {@link Observable} that represents a stream of updates that
* have happened in this storage service or in the storage this service provides
* an interface to.
*/
abstract get updates$(): Observable<StorageUpdate>;
get updates$(): Observable<StorageUpdate>;
}
export abstract class AbstractStorageService {
abstract get valuesRequireDeserialization(): boolean;
abstract get<T>(key: string, options?: StorageOptions): Promise<T>;
abstract has(key: string, options?: StorageOptions): Promise<boolean>;
abstract save<T>(key: string, obj: T, options?: StorageOptions): Promise<void>;

View File

@@ -3,7 +3,7 @@ import { Subject } from "rxjs";
import { AbstractMemoryStorageService, StorageUpdate } from "../abstractions/storage.service";
export class MemoryStorageService extends AbstractMemoryStorageService {
private store = new Map<string, unknown>();
protected store = new Map<string, unknown>();
private updatesSubject = new Subject<StorageUpdate>();
get valuesRequireDeserialization(): boolean {

View File

@@ -1,6 +1,7 @@
import {
AbstractMemoryStorageService,
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { GlobalState } from "../global-state";
import { GlobalStateProvider } from "../global-state.provider";
@@ -13,8 +14,8 @@ export class DefaultGlobalStateProvider implements GlobalStateProvider {
private globalStateCache: Record<string, GlobalState<unknown>> = {};
constructor(
private memoryStorage: AbstractMemoryStorageService,
private diskStorage: AbstractStorageService
private memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
private diskStorage: AbstractStorageService & ObservableStorageService
) {}
get<T>(keyDefinition: KeyDefinition<T>): GlobalState<T> {

View File

@@ -10,7 +10,10 @@ import {
timeout,
} from "rxjs";
import { AbstractStorageService } from "../../abstractions/storage.service";
import {
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { GlobalState } from "../global-state";
import { KeyDefinition, globalKeyBuilder } from "../key-definition";
import { StateUpdateOptions, populateOptionsWithDefault } from "../state-update-options";
@@ -29,7 +32,7 @@ export class DefaultGlobalState<T> implements GlobalState<T> {
constructor(
private keyDefinition: KeyDefinition<T>,
private chosenLocation: AbstractStorageService
private chosenLocation: AbstractStorageService & ObservableStorageService
) {
this.storageKey = globalKeyBuilder(this.keyDefinition);

View File

@@ -3,6 +3,7 @@ import { EncryptService } from "../../abstractions/encrypt.service";
import {
AbstractMemoryStorageService,
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { KeyDefinition } from "../key-definition";
import { StorageLocation } from "../state-definition";
@@ -17,8 +18,8 @@ export class DefaultUserStateProvider implements UserStateProvider {
constructor(
protected accountService: AccountService,
protected encryptService: EncryptService,
protected memoryStorage: AbstractMemoryStorageService,
protected diskStorage: AbstractStorageService
protected memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
protected diskStorage: AbstractStorageService & ObservableStorageService
) {}
get<T>(keyDefinition: KeyDefinition<T>): UserState<T> {

View File

@@ -15,7 +15,10 @@ import {
import { AccountService } from "../../../auth/abstractions/account.service";
import { UserId } from "../../../types/guid";
import { EncryptService } from "../../abstractions/encrypt.service";
import { AbstractStorageService } from "../../abstractions/storage.service";
import {
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { DerivedUserState } from "../derived-user-state";
import { KeyDefinition, userKeyBuilder } from "../key-definition";
import { StateUpdateOptions, populateOptionsWithDefault } from "../state-update-options";
@@ -40,7 +43,7 @@ export class DefaultUserState<T> implements UserState<T> {
protected keyDefinition: KeyDefinition<T>,
private accountService: AccountService,
private encryptService: EncryptService,
private chosenStorageLocation: AbstractStorageService
private chosenStorageLocation: AbstractStorageService & ObservableStorageService
) {
this.formattedKey$ = this.accountService.activeAccount$.pipe(
map((account) =>

View File

@@ -1,3 +1,7 @@
export { DerivedUserState } from "./derived-user-state";
export { DefaultGlobalStateProvider } from "./implementations/default-global-state.provider";
export { DefaultUserStateProvider } from "./implementations/default-user-state.provider";
export { GlobalState } from "./global-state";
export { GlobalStateProvider } from "./global-state.provider";
export { UserState } from "./user-state";
export { UserStateProvider } from "./user-state.provider";
export * from "./key-definitions";

View File

@@ -0,0 +1,18 @@
import { AccountInfo } from "../../auth/abstractions/account.service";
import { AccountsDeserializer } from "../../auth/services/account.service";
import { UserId } from "../../types/guid";
import { KeyDefinition } from "./key-definition";
import { StateDefinition } from "./state-definition";
const ACCOUNT_MEMORY = new StateDefinition("account", "memory");
export const ACCOUNT_ACCOUNTS = new KeyDefinition<Record<UserId, AccountInfo>>(
ACCOUNT_MEMORY,
"accounts",
{
deserializer: (obj) => AccountsDeserializer(obj),
}
);
export const ACCOUNT_ACTIVE_ACCOUNT_ID = new KeyDefinition(ACCOUNT_MEMORY, "activeAccountId", {
deserializer: (id: UserId) => id,
});