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

[PM-6404] Fully Integrate clearOn Events (#8134)

* Add New KeyDefinitionOption

* Add New Services

* Add WebStorageServiceProvider Tests

* Update Error Message

* Add `UserKeyDefinition`

* Fix Deserialization Helpers

* Fix KeyDefinition

* Add `UserKeyDefinition`

* Fix Deserialization Helpers

* Fix KeyDefinition

* Move `ClearEvent`

* Cleanup

* Fix Imports

* Integrate onClear Events

* Remove Accidental Addition

* Fix Test

* Add VaultTimeoutService Tests

* Only Register When Current State is Null

* Address Feedback
This commit is contained in:
Justin Baur
2024-03-04 14:33:25 -06:00
committed by GitHub
parent 4ba2717eb4
commit c3eba7f2c8
28 changed files with 471 additions and 345 deletions

View File

@@ -1,3 +1,4 @@
import { mock } from "jest-mock-extended";
import { firstValueFrom, timeout } from "rxjs";
import { awaitAsync } from "../../../spec";
@@ -14,9 +15,11 @@ import { DefaultDerivedStateProvider } from "../state/implementations/default-de
import { DefaultGlobalStateProvider } from "../state/implementations/default-global-state.provider";
import { DefaultSingleUserStateProvider } from "../state/implementations/default-single-user-state.provider";
import { DefaultStateProvider } from "../state/implementations/default-state.provider";
/* eslint-disable import/no-restricted-paths */
import { StateEventRegistrarService } from "../state/state-event-registrar.service";
/* eslint-enable import/no-restricted-paths */
import { EnvironmentService } from "./environment.service";
import { StorageServiceProvider } from "./storage-service.provider";
// There are a few main states EnvironmentService could be in when first used
// 1. Not initialized, no active user. Hopefully not to likely but possible
@@ -26,6 +29,8 @@ import { EnvironmentService } from "./environment.service";
describe("EnvironmentService", () => {
let diskStorageService: FakeStorageService;
let memoryStorageService: FakeStorageService;
let storageServiceProvider: StorageServiceProvider;
const stateEventRegistrarService = mock<StateEventRegistrarService>();
let accountService: FakeAccountService;
let stateProvider: StateProvider;
@@ -37,16 +42,17 @@ describe("EnvironmentService", () => {
beforeEach(async () => {
diskStorageService = new FakeStorageService();
memoryStorageService = new FakeStorageService();
storageServiceProvider = new StorageServiceProvider(diskStorageService, memoryStorageService);
accountService = mockAccountServiceWith(undefined);
stateProvider = new DefaultStateProvider(
new DefaultActiveUserStateProvider(
accountService,
memoryStorageService as any,
diskStorageService,
storageServiceProvider,
stateEventRegistrarService,
),
new DefaultSingleUserStateProvider(memoryStorageService as any, diskStorageService),
new DefaultGlobalStateProvider(memoryStorageService as any, diskStorageService),
new DefaultSingleUserStateProvider(storageServiceProvider, stateEventRegistrarService),
new DefaultGlobalStateProvider(storageServiceProvider),
new DefaultDerivedStateProvider(memoryStorageService),
);

View File

@@ -3,17 +3,14 @@ import { mock } from "jest-mock-extended";
import { mockAccountServiceWith, trackEmissions } from "../../../../spec";
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
import { UserId } from "../../../types/guid";
import {
AbstractMemoryStorageService,
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StorageServiceProvider } from "../../services/storage-service.provider";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { DefaultActiveUserStateProvider } from "./default-active-user-state.provider";
describe("DefaultActiveUserStateProvider", () => {
const memoryStorage = mock<AbstractMemoryStorageService & ObservableStorageService>();
const diskStorage = mock<AbstractStorageService & ObservableStorageService>();
const storageServiceProvider = mock<StorageServiceProvider>();
const stateEventRegistrarService = mock<StateEventRegistrarService>();
const userId = "userId" as UserId;
const accountInfo = {
id: userId,
@@ -25,7 +22,11 @@ describe("DefaultActiveUserStateProvider", () => {
let sut: DefaultActiveUserStateProvider;
beforeEach(() => {
sut = new DefaultActiveUserStateProvider(accountService, memoryStorage, diskStorage);
sut = new DefaultActiveUserStateProvider(
accountService,
storageServiceProvider,
stateEventRegistrarService,
);
});
afterEach(() => {

View File

@@ -2,13 +2,9 @@ import { Observable, map } from "rxjs";
import { AccountService } from "../../../auth/abstractions/account.service";
import { UserId } from "../../../types/guid";
import {
AbstractMemoryStorageService,
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StorageServiceProvider } from "../../services/storage-service.provider";
import { KeyDefinition } from "../key-definition";
import { StateDefinition } from "../state-definition";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { UserKeyDefinition, isUserKeyDefinition } from "../user-key-definition";
import { ActiveUserState } from "../user-state";
import { ActiveUserStateProvider } from "../user-state.provider";
@@ -21,9 +17,9 @@ export class DefaultActiveUserStateProvider implements ActiveUserStateProvider {
activeUserId$: Observable<UserId | undefined>;
constructor(
protected readonly accountService: AccountService,
protected readonly memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
protected readonly diskStorage: AbstractStorageService & ObservableStorageService,
private readonly accountService: AccountService,
private readonly storageServiceProvider: StorageServiceProvider,
private readonly stateEventRegistrarService: StateEventRegistrarService,
) {
this.activeUserId$ = this.accountService.activeAccount$.pipe(map((account) => account?.id));
}
@@ -32,7 +28,11 @@ export class DefaultActiveUserStateProvider implements ActiveUserStateProvider {
if (!isUserKeyDefinition(keyDefinition)) {
keyDefinition = UserKeyDefinition.fromBaseKeyDefinition(keyDefinition);
}
const cacheKey = this.buildCacheKey(keyDefinition);
const [location, storageService] = this.storageServiceProvider.get(
keyDefinition.stateDefinition.defaultStorageLocation,
keyDefinition.stateDefinition.storageLocationOverrides,
);
const cacheKey = this.buildCacheKey(location, keyDefinition);
const existingUserState = this.cache[cacheKey];
if (existingUserState != null) {
// I have to cast out of the unknown generic but this should be safe if rules
@@ -40,36 +40,17 @@ export class DefaultActiveUserStateProvider implements ActiveUserStateProvider {
return existingUserState as ActiveUserState<T>;
}
const newUserState = this.buildActiveUserState(keyDefinition);
const newUserState = new DefaultActiveUserState<T>(
keyDefinition,
this.accountService,
storageService,
this.stateEventRegistrarService,
);
this.cache[cacheKey] = newUserState;
return newUserState;
}
private buildCacheKey(keyDefinition: UserKeyDefinition<unknown>) {
return `${this.getLocationString(keyDefinition)}_${keyDefinition.fullName}`;
}
protected buildActiveUserState<T>(keyDefinition: UserKeyDefinition<T>): ActiveUserState<T> {
return new DefaultActiveUserState<T>(
keyDefinition,
this.accountService,
this.getLocation(keyDefinition.stateDefinition),
);
}
protected getLocationString(keyDefinition: UserKeyDefinition<unknown>): string {
return keyDefinition.stateDefinition.defaultStorageLocation;
}
protected getLocation(stateDefinition: StateDefinition) {
// The default implementations don't support the client overrides
// it is up to the client to extend this class and add that support
const location = stateDefinition.defaultStorageLocation;
switch (location) {
case "disk":
return this.diskStorage;
case "memory":
return this.memoryStorage;
}
private buildCacheKey(location: string, keyDefinition: UserKeyDefinition<unknown>) {
return `${location}_${keyDefinition.fullName}`;
}
}

View File

@@ -12,6 +12,7 @@ import { AccountInfo, AccountService } from "../../../auth/abstractions/account.
import { AuthenticationStatus } from "../../../auth/enums/authentication-status";
import { UserId } from "../../../types/guid";
import { StateDefinition } from "../state-definition";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { UserKeyDefinition } from "../user-key-definition";
import { DefaultActiveUserState } from "./default-active-user-state";
@@ -42,6 +43,7 @@ const testKeyDefinition = new UserKeyDefinition<TestState>(testStateDefinition,
describe("DefaultActiveUserState", () => {
const accountService = mock<AccountService>();
let diskStorageService: FakeStorageService;
const stateEventRegistrarService = mock<StateEventRegistrarService>();
let activeAccountSubject: BehaviorSubject<{ id: UserId } & AccountInfo>;
let userState: DefaultActiveUserState<TestState>;
@@ -50,7 +52,12 @@ describe("DefaultActiveUserState", () => {
accountService.activeAccount$ = activeAccountSubject;
diskStorageService = new FakeStorageService();
userState = new DefaultActiveUserState(testKeyDefinition, accountService, diskStorageService);
userState = new DefaultActiveUserState(
testKeyDefinition,
accountService,
diskStorageService,
stateEventRegistrarService,
);
});
const makeUserId = (id: string) => {
@@ -391,6 +398,48 @@ describe("DefaultActiveUserState", () => {
"No active user at this time.",
);
});
it.each([null, undefined])(
"should register user key definition when state transitions from null-ish (%s) to non-null",
async (startingValue: TestState | null) => {
diskStorageService.internalUpdateStore({
"user_00000000-0000-1000-a000-000000000001_fake_fake": startingValue,
});
await userState.update(() => ({ array: ["one"], date: new Date() }));
expect(stateEventRegistrarService.registerEvents).toHaveBeenCalledWith(testKeyDefinition);
},
);
it("should not register user key definition when state has preexisting value", async () => {
diskStorageService.internalUpdateStore({
"user_00000000-0000-1000-a000-000000000001_fake_fake": {
date: new Date(2019, 1),
array: [],
},
});
await userState.update(() => ({ array: ["one"], date: new Date() }));
expect(stateEventRegistrarService.registerEvents).not.toHaveBeenCalled();
});
it.each([null, undefined])(
"should not register user key definition when setting value to null-ish (%s) value",
async (updatedValue: TestState | null) => {
diskStorageService.internalUpdateStore({
"user_00000000-0000-1000-a000-000000000001_fake_fake": {
date: new Date(2019, 1),
array: [],
},
});
await userState.update(() => updatedValue);
expect(stateEventRegistrarService.registerEvents).not.toHaveBeenCalled();
},
);
});
describe("update races", () => {

View File

@@ -21,6 +21,7 @@ import {
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { StateUpdateOptions, populateOptionsWithDefault } from "../state-update-options";
import { UserKeyDefinition } from "../user-key-definition";
import { ActiveUserState, CombinedState, activeMarker } from "../user-state";
@@ -42,6 +43,7 @@ export class DefaultActiveUserState<T> implements ActiveUserState<T> {
protected keyDefinition: UserKeyDefinition<T>,
private accountService: AccountService,
private chosenStorageLocation: AbstractStorageService & ObservableStorageService,
private stateEventRegistrarService: StateEventRegistrarService,
) {
this.activeUserId$ = this.accountService.activeAccount$.pipe(
// We only care about the UserId but we do want to know about no user as well.
@@ -150,6 +152,11 @@ export class DefaultActiveUserState<T> implements ActiveUserState<T> {
const newState = configureState(currentState, combinedDependencies);
await this.saveToStorage(key, newState);
if (newState != null && currentState == null) {
// Only register this state as something clearable on the first time it saves something
// worth deleting. This is helpful in making sure there is less of a race to adding events.
await this.stateEventRegistrarService.registerEvents(this.keyDefinition);
}
return [userId, newState];
}

View File

@@ -1,25 +1,21 @@
import {
AbstractMemoryStorageService,
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StorageServiceProvider } from "../../services/storage-service.provider";
import { GlobalState } from "../global-state";
import { GlobalStateProvider } from "../global-state.provider";
import { KeyDefinition } from "../key-definition";
import { StateDefinition } from "../state-definition";
import { DefaultGlobalState } from "./default-global-state";
export class DefaultGlobalStateProvider implements GlobalStateProvider {
private globalStateCache: Record<string, GlobalState<unknown>> = {};
constructor(
protected readonly memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
protected readonly diskStorage: AbstractStorageService & ObservableStorageService,
) {}
constructor(private storageServiceProvider: StorageServiceProvider) {}
get<T>(keyDefinition: KeyDefinition<T>): GlobalState<T> {
const cacheKey = this.buildCacheKey(keyDefinition);
const [location, storageService] = this.storageServiceProvider.get(
keyDefinition.stateDefinition.defaultStorageLocation,
keyDefinition.stateDefinition.storageLocationOverrides,
);
const cacheKey = this.buildCacheKey(location, keyDefinition);
const existingGlobalState = this.globalStateCache[cacheKey];
if (existingGlobalState != null) {
// The cast into the actual generic is safe because of rules around key definitions
@@ -27,30 +23,13 @@ export class DefaultGlobalStateProvider implements GlobalStateProvider {
return existingGlobalState as DefaultGlobalState<T>;
}
const newGlobalState = new DefaultGlobalState<T>(
keyDefinition,
this.getLocation(keyDefinition.stateDefinition),
);
const newGlobalState = new DefaultGlobalState<T>(keyDefinition, storageService);
this.globalStateCache[cacheKey] = newGlobalState;
return newGlobalState;
}
private buildCacheKey(keyDefinition: KeyDefinition<unknown>) {
return `${this.getLocationString(keyDefinition)}_${keyDefinition.fullName}`;
}
protected getLocationString(keyDefinition: KeyDefinition<unknown>): string {
return keyDefinition.stateDefinition.defaultStorageLocation;
}
protected getLocation(stateDefinition: StateDefinition) {
const location = stateDefinition.defaultStorageLocation;
switch (location) {
case "disk":
return this.diskStorage;
case "memory":
return this.memoryStorage;
}
private buildCacheKey(location: string, keyDefinition: KeyDefinition<unknown>) {
return `${location}_${keyDefinition.fullName}`;
}
}

View File

@@ -1,11 +1,7 @@
import { UserId } from "../../../types/guid";
import {
AbstractMemoryStorageService,
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StorageServiceProvider } from "../../services/storage-service.provider";
import { KeyDefinition } from "../key-definition";
import { StateDefinition } from "../state-definition";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { UserKeyDefinition, isUserKeyDefinition } from "../user-key-definition";
import { SingleUserState } from "../user-state";
import { SingleUserStateProvider } from "../user-state.provider";
@@ -16,8 +12,8 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
private cache: Record<string, SingleUserState<unknown>> = {};
constructor(
protected readonly memoryStorage: AbstractMemoryStorageService & ObservableStorageService,
protected readonly diskStorage: AbstractStorageService & ObservableStorageService,
private readonly storageServiceProvider: StorageServiceProvider,
private readonly stateEventRegistrarService: StateEventRegistrarService,
) {}
get<T>(
@@ -27,7 +23,11 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
if (!isUserKeyDefinition(keyDefinition)) {
keyDefinition = UserKeyDefinition.fromBaseKeyDefinition(keyDefinition);
}
const cacheKey = this.buildCacheKey(userId, keyDefinition);
const [location, storageService] = this.storageServiceProvider.get(
keyDefinition.stateDefinition.defaultStorageLocation,
keyDefinition.stateDefinition.storageLocationOverrides,
);
const cacheKey = this.buildCacheKey(location, userId, keyDefinition);
const existingUserState = this.cache[cacheKey];
if (existingUserState != null) {
// I have to cast out of the unknown generic but this should be safe if rules
@@ -35,38 +35,21 @@ export class DefaultSingleUserStateProvider implements SingleUserStateProvider {
return existingUserState as SingleUserState<T>;
}
const newUserState = this.buildSingleUserState(userId, keyDefinition);
const newUserState = new DefaultSingleUserState<T>(
userId,
keyDefinition,
storageService,
this.stateEventRegistrarService,
);
this.cache[cacheKey] = newUserState;
return newUserState;
}
private buildCacheKey(userId: UserId, keyDefinition: UserKeyDefinition<unknown>) {
return `${this.getLocationString(keyDefinition)}_${keyDefinition.fullName}_${userId}`;
}
protected buildSingleUserState<T>(
private buildCacheKey(
location: string,
userId: UserId,
keyDefinition: UserKeyDefinition<T>,
): SingleUserState<T> {
return new DefaultSingleUserState<T>(
userId,
keyDefinition,
this.getLocation(keyDefinition.stateDefinition),
);
}
protected getLocationString(keyDefinition: UserKeyDefinition<unknown>): string {
return keyDefinition.stateDefinition.defaultStorageLocation;
}
protected getLocation(stateDefinition: StateDefinition) {
// The default implementations don't support the client overrides
// it is up to the client to extend this class and add that support
switch (stateDefinition.defaultStorageLocation) {
case "disk":
return this.diskStorage;
case "memory":
return this.memoryStorage;
}
keyDefinition: UserKeyDefinition<unknown>,
) {
return `${location}_${keyDefinition.fullName}_${userId}`;
}
}

View File

@@ -3,6 +3,7 @@
* @jest-environment ../shared/test.environment.ts
*/
import { mock } from "jest-mock-extended";
import { firstValueFrom, of } from "rxjs";
import { Jsonify } from "type-fest";
@@ -11,6 +12,7 @@ import { FakeStorageService } from "../../../../spec/fake-storage.service";
import { UserId } from "../../../types/guid";
import { Utils } from "../../misc/utils";
import { StateDefinition } from "../state-definition";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { UserKeyDefinition } from "../user-key-definition";
import { DefaultSingleUserState } from "./default-single-user-state";
@@ -42,11 +44,17 @@ const userKey = testKeyDefinition.buildKey(userId);
describe("DefaultSingleUserState", () => {
let diskStorageService: FakeStorageService;
let userState: DefaultSingleUserState<TestState>;
const stateEventRegistrarService = mock<StateEventRegistrarService>();
const newData = { date: new Date() };
beforeEach(() => {
diskStorageService = new FakeStorageService();
userState = new DefaultSingleUserState(userId, testKeyDefinition, diskStorageService);
userState = new DefaultSingleUserState(
userId,
testKeyDefinition,
diskStorageService,
stateEventRegistrarService,
);
});
afterEach(() => {
@@ -255,6 +263,49 @@ describe("DefaultSingleUserState", () => {
expect(emissions).toHaveLength(2);
expect(emissions).toEqual(expect.arrayContaining([initialState, newState]));
});
it.each([null, undefined])(
"should register user key definition when state transitions from null-ish (%s) to non-null",
async (startingValue: TestState | null) => {
const initialState: Record<string, TestState> = {};
initialState[userKey] = startingValue;
diskStorageService.internalUpdateStore(initialState);
await userState.update(() => ({ array: ["one"], date: new Date() }));
expect(stateEventRegistrarService.registerEvents).toHaveBeenCalledWith(testKeyDefinition);
},
);
it("should not register user key definition when state has preexisting value", async () => {
const initialState: Record<string, TestState> = {};
initialState[userKey] = {
date: new Date(2019, 1),
};
diskStorageService.internalUpdateStore(initialState);
await userState.update(() => ({ array: ["one"], date: new Date() }));
expect(stateEventRegistrarService.registerEvents).not.toHaveBeenCalled();
});
it.each([null, undefined])(
"should not register user key definition when setting value to null-ish (%s) value",
async (updatedValue: TestState | null) => {
const initialState: Record<string, TestState> = {};
initialState[userKey] = {
date: new Date(2019, 1),
};
diskStorageService.internalUpdateStore(initialState);
await userState.update(() => updatedValue);
expect(stateEventRegistrarService.registerEvents).not.toHaveBeenCalled();
},
);
});
describe("update races", () => {

View File

@@ -18,6 +18,7 @@ import {
AbstractStorageService,
ObservableStorageService,
} from "../../abstractions/storage.service";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { StateUpdateOptions, populateOptionsWithDefault } from "../state-update-options";
import { UserKeyDefinition } from "../user-key-definition";
import { CombinedState, SingleUserState } from "../user-state";
@@ -35,6 +36,7 @@ export class DefaultSingleUserState<T> implements SingleUserState<T> {
readonly userId: UserId,
private keyDefinition: UserKeyDefinition<T>,
private chosenLocation: AbstractStorageService & ObservableStorageService,
private stateEventRegistrarService: StateEventRegistrarService,
) {
this.storageKey = this.keyDefinition.buildKey(this.userId);
const initialStorageGet$ = defer(() => {
@@ -100,6 +102,11 @@ export class DefaultSingleUserState<T> implements SingleUserState<T> {
const newState = configureState(currentState, combinedDependencies);
await this.chosenLocation.save(this.storageKey, newState);
if (newState != null && currentState == null) {
// Only register this state as something clearable on the first time it saves something
// worth deleting. This is helpful in making sure there is less of a race to adding events.
await this.stateEventRegistrarService.registerEvents(this.keyDefinition);
}
return newState;
}

View File

@@ -1,8 +1,12 @@
import { mock } from "jest-mock-extended";
import { mockAccountServiceWith } from "../../../../spec/fake-account-service";
import { FakeStorageService } from "../../../../spec/fake-storage.service";
import { UserId } from "../../../types/guid";
import { StorageServiceProvider } from "../../services/storage-service.provider";
import { KeyDefinition } from "../key-definition";
import { StateDefinition } from "../state-definition";
import { StateEventRegistrarService } from "../state-event-registrar.service";
import { DefaultActiveUserState } from "./default-active-user-state";
import { DefaultActiveUserStateProvider } from "./default-active-user-state.provider";
@@ -12,6 +16,9 @@ import { DefaultSingleUserState } from "./default-single-user-state";
import { DefaultSingleUserStateProvider } from "./default-single-user-state.provider";
describe("Specific State Providers", () => {
const storageServiceProvider = mock<StorageServiceProvider>();
const stateEventRegistrarService = mock<StateEventRegistrarService>();
let singleSut: DefaultSingleUserStateProvider;
let activeSut: DefaultActiveUserStateProvider;
let globalSut: DefaultGlobalStateProvider;
@@ -19,19 +26,20 @@ describe("Specific State Providers", () => {
const fakeUser1 = "00000000-0000-1000-a000-000000000001" as UserId;
beforeEach(() => {
storageServiceProvider.get.mockImplementation((location) => {
return [location, new FakeStorageService()];
});
singleSut = new DefaultSingleUserStateProvider(
new FakeStorageService() as any,
new FakeStorageService(),
storageServiceProvider,
stateEventRegistrarService,
);
activeSut = new DefaultActiveUserStateProvider(
mockAccountServiceWith(null),
new FakeStorageService() as any,
new FakeStorageService(),
);
globalSut = new DefaultGlobalStateProvider(
new FakeStorageService() as any,
new FakeStorageService(),
storageServiceProvider,
stateEventRegistrarService,
);
globalSut = new DefaultGlobalStateProvider(storageServiceProvider);
});
const fakeDiskStateDefinition = new StateDefinition("fake", "disk");