From d87a8f927132eaeca2d6ccc92689d55c63650425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E2=9C=A8=20Audrey=20=E2=9C=A8?= Date: Mon, 4 Mar 2024 13:43:38 -0500 Subject: [PATCH 01/14] [PM-6523] generator service tuning (#8155) * rename policy$ to evaluator$ * replace `ActiveUserState` with `SingleUserState` * implement `SingleUserState` on `SecretState` --- libs/common/spec/index.ts | 1 + libs/common/src/platform/state/index.ts | 2 +- .../generator-strategy.abstraction.ts | 13 +- .../generator.service.abstraction.ts | 14 +- .../default-generator.service.spec.ts | 199 +++++++++--------- .../generator/default-generator.service.ts | 68 +++--- .../passphrase-generator-strategy.spec.ts | 35 ++- .../passphrase-generator-strategy.ts | 22 +- .../password-generator-strategy.spec.ts | 35 ++- .../password/password-generator-strategy.ts | 22 +- .../generator/state/secret-state.spec.ts | 25 ++- .../src/tools/generator/state/secret-state.ts | 28 ++- .../catchall-generator-strategy.spec.ts | 30 ++- .../username/catchall-generator-strategy.ts | 17 +- .../eff-username-generator-strategy.spec.ts | 30 ++- .../eff-username-generator-strategy.ts | 17 +- .../subaddress-generator-strategy.spec.ts | 30 ++- .../username/subaddress-generator-strategy.ts | 17 +- 18 files changed, 394 insertions(+), 211 deletions(-) diff --git a/libs/common/spec/index.ts b/libs/common/spec/index.ts index 4ba9f3d3939..72bd28aca4f 100644 --- a/libs/common/spec/index.ts +++ b/libs/common/spec/index.ts @@ -2,4 +2,5 @@ export * from "./utils"; export * from "./intercept-console"; export * from "./matchers"; export * from "./fake-state-provider"; +export * from "./fake-state"; export * from "./fake-account-service"; diff --git a/libs/common/src/platform/state/index.ts b/libs/common/src/platform/state/index.ts index b457e14c2f3..79f5b4172fd 100644 --- a/libs/common/src/platform/state/index.ts +++ b/libs/common/src/platform/state/index.ts @@ -4,7 +4,7 @@ export { DerivedState } from "./derived-state"; export { GlobalState } from "./global-state"; export { StateProvider } from "./state.provider"; export { GlobalStateProvider } from "./global-state.provider"; -export { ActiveUserState, SingleUserState } from "./user-state"; +export { ActiveUserState, SingleUserState, CombinedState } from "./user-state"; export { ActiveUserStateProvider, SingleUserStateProvider } from "./user-state.provider"; export { KeyDefinition } from "./key-definition"; export { StateUpdateOptions } from "./state-update-options"; diff --git a/libs/common/src/tools/generator/abstractions/generator-strategy.abstraction.ts b/libs/common/src/tools/generator/abstractions/generator-strategy.abstraction.ts index 2ec098bc418..f11c1d73009 100644 --- a/libs/common/src/tools/generator/abstractions/generator-strategy.abstraction.ts +++ b/libs/common/src/tools/generator/abstractions/generator-strategy.abstraction.ts @@ -2,14 +2,18 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy as AdminPolicy } from "../../../admin-console/models/domain/policy"; -import { KeyDefinition } from "../../../platform/state"; +import { SingleUserState } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { PolicyEvaluator } from "./policy-evaluator.abstraction"; /** Tailors the generator service to generate a specific kind of credentials */ export abstract class GeneratorStrategy { - /** The key used when storing credentials on disk. */ - disk: KeyDefinition; + /** Retrieve application state that persists across locks. + * @param userId: identifies the user state to retrieve + * @returns the strategy's durable user state + */ + durableState: (userId: UserId) => SingleUserState; /** Identifies the policy enforced by the generator. */ policy: PolicyType; @@ -19,7 +23,8 @@ export abstract class GeneratorStrategy { /** Creates an evaluator from a generator policy. * @param policy The policy being evaluated. - * @returns the policy evaluator. + * @returns the policy evaluator. If `policy` is is `null` or `undefined`, + * then the evaluator defaults to the application's limits. * @throws when the policy's type does not match the generator's policy type. */ evaluator: (policy: AdminPolicy) => PolicyEvaluator; diff --git a/libs/common/src/tools/generator/abstractions/generator.service.abstraction.ts b/libs/common/src/tools/generator/abstractions/generator.service.abstraction.ts index e64e0787792..f1820ed707b 100644 --- a/libs/common/src/tools/generator/abstractions/generator.service.abstraction.ts +++ b/libs/common/src/tools/generator/abstractions/generator.service.abstraction.ts @@ -1,5 +1,7 @@ import { Observable } from "rxjs"; +import { UserId } from "../../../types/guid"; + import { PolicyEvaluator } from "./policy-evaluator.abstraction"; /** Generates credentials used for user authentication @@ -9,19 +11,22 @@ import { PolicyEvaluator } from "./policy-evaluator.abstraction"; export abstract class GeneratorService { /** An observable monitoring the options saved to disk. * The observable updates when the options are saved. + * @param userId: Identifies the user making the request */ - options$: Observable; + options$: (userId: UserId) => Observable; /** An observable monitoring the options used to enforce policy. * The observable updates when the policy changes. + * @param userId: Identifies the user making the request */ - policy$: Observable>; + evaluator$: (userId: UserId) => Observable>; /** Enforces the policy on the given options + * @param userId: Identifies the user making the request * @param options the options to enforce the policy on * @returns a new instance of the options with the policy enforced */ - enforcePolicy: (options: Options) => Promise; + enforcePolicy: (userId: UserId, options: Options) => Promise; /** Generates credentials * @param options the options to generate credentials with @@ -30,8 +35,9 @@ export abstract class GeneratorService { generate: (options: Options) => Promise; /** Saves the given options to disk. + * @param userId: Identifies the user making the request * @param options the options to save * @returns a promise that resolves when the options are saved */ - saveOptions: (options: Options) => Promise; + saveOptions: (userId: UserId, options: Options) => Promise; } diff --git a/libs/common/src/tools/generator/default-generator.service.spec.ts b/libs/common/src/tools/generator/default-generator.service.spec.ts index 4293b0168c0..84b8ff45303 100644 --- a/libs/common/src/tools/generator/default-generator.service.spec.ts +++ b/libs/common/src/tools/generator/default-generator.service.spec.ts @@ -6,42 +6,45 @@ import { mock } from "jest-mock-extended"; import { BehaviorSubject, firstValueFrom } from "rxjs"; -import { FakeActiveUserStateProvider, mockAccountServiceWith } from "../../../spec"; +import { FakeSingleUserState, awaitAsync } from "../../../spec"; import { PolicyService } from "../../admin-console/abstractions/policy/policy.service.abstraction"; import { PolicyType } from "../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../admin-console/models/domain/policy"; -import { Utils } from "../../platform/misc/utils"; -import { ActiveUserState, ActiveUserStateProvider, KeyDefinition } from "../../platform/state"; +import { SingleUserState } from "../../platform/state"; import { UserId } from "../../types/guid"; import { GeneratorStrategy, PolicyEvaluator } from "./abstractions"; -import { PASSPHRASE_SETTINGS, PASSWORD_SETTINGS } from "./key-definitions"; import { PasswordGenerationOptions } from "./password"; import { DefaultGeneratorService } from "."; -function mockPolicyService(config?: { data?: any; policy?: BehaviorSubject }) { - const state = mock({ data: config?.data ?? {} }); - const subject = config?.policy ?? new BehaviorSubject(state); - +function mockPolicyService(config?: { state?: BehaviorSubject }) { const service = mock(); - service.get$.mockReturnValue(subject.asObservable()); + + // FIXME: swap out the mock return value when `getAll$` becomes available + const stateValue = config?.state ?? new BehaviorSubject(null); + service.get$.mockReturnValue(stateValue); + + // const stateValue = config?.state ?? new BehaviorSubject(null); + // service.getAll$.mockReturnValue(stateValue); return service; } function mockGeneratorStrategy(config?: { - disk?: KeyDefinition; + userState?: SingleUserState; policy?: PolicyType; evaluator?: any; }) { + const durableState = + config?.userState ?? new FakeSingleUserState(SomeUser); const strategy = mock>({ // intentionally arbitrary so that tests that need to check // whether they're used properly are guaranteed to test // the value from `config`. - disk: config?.disk ?? {}, + durableState: jest.fn(() => durableState), policy: config?.policy ?? PolicyType.DisableSend, evaluator: jest.fn(() => config?.evaluator ?? mock>()), }); @@ -49,129 +52,123 @@ function mockGeneratorStrategy(config?: { return strategy; } -// FIXME: Use the fake instead, once it's updated to monitor its method calls. -function mockStateProvider(): [ - ActiveUserStateProvider, - ActiveUserState, -] { - const state = mock>(); - const provider = mock(); - provider.get.mockReturnValue(state); - - return [provider, state]; -} - -function fakeStateProvider(key: KeyDefinition, initalValue: any): FakeActiveUserStateProvider { - const userId = Utils.newGuid() as UserId; - const acctService = mockAccountServiceWith(userId); - const provider = new FakeActiveUserStateProvider(acctService); - provider.mockFor(key.key, initalValue); - return provider; -} +const SomeUser = "some user" as UserId; +const AnotherUser = "another user" as UserId; describe("Password generator service", () => { - describe("constructor()", () => { - it("should initialize the password generator policy", () => { - const policy = mockPolicyService(); - const strategy = mockGeneratorStrategy({ policy: PolicyType.PasswordGenerator }); - - new DefaultGeneratorService(strategy, policy, null); - - expect(policy.get$).toHaveBeenCalledWith(PolicyType.PasswordGenerator); - }); - }); - describe("options$", () => { - it("should return the state from strategy.key", () => { + it("should retrieve durable state from the service", () => { const policy = mockPolicyService(); - const strategy = mockGeneratorStrategy({ disk: PASSPHRASE_SETTINGS }); - const [state] = mockStateProvider(); - const service = new DefaultGeneratorService(strategy, policy, state); + const userState = new FakeSingleUserState(SomeUser); + const strategy = mockGeneratorStrategy({ userState }); + const service = new DefaultGeneratorService(strategy, policy); - // invoke the getter. It returns the state but that's not important. - service.options$; + const result = service.options$(SomeUser); - expect(state.get).toHaveBeenCalledWith(PASSPHRASE_SETTINGS); + expect(strategy.durableState).toHaveBeenCalledWith(SomeUser); + expect(result).toBe(userState.state$); }); }); describe("saveOptions()", () => { - it("should update the state at strategy.key", async () => { - const policy = mockPolicyService(); - const [provider, state] = mockStateProvider(); - const strategy = mockGeneratorStrategy({ disk: PASSWORD_SETTINGS }); - const service = new DefaultGeneratorService(strategy, policy, provider); - - await service.saveOptions({}); - - expect(provider.get).toHaveBeenCalledWith(PASSWORD_SETTINGS); - expect(state.update).toHaveBeenCalled(); - }); - it("should trigger an options$ update", async () => { const policy = mockPolicyService(); - const strategy = mockGeneratorStrategy(); - // using the fake here because we're testing that the update and the - // property are wired together. If we were to mock that, we'd be testing - // the mock configuration instead of the wiring. - const provider = fakeStateProvider(strategy.disk, { length: 9 }); - const service = new DefaultGeneratorService(strategy, policy, provider); + const userState = new FakeSingleUserState(SomeUser, { length: 9 }); + const strategy = mockGeneratorStrategy({ userState }); + const service = new DefaultGeneratorService(strategy, policy); - await service.saveOptions({ length: 10 }); + await service.saveOptions(SomeUser, { length: 10 }); + await awaitAsync(); + const options = await firstValueFrom(service.options$(SomeUser)); - const options = await firstValueFrom(service.options$); + expect(strategy.durableState).toHaveBeenCalledWith(SomeUser); expect(options).toEqual({ length: 10 }); }); }); - describe("policy$", () => { + describe("evaluator$", () => { + it("should initialize the password generator policy", async () => { + const policy = mockPolicyService(); + const strategy = mockGeneratorStrategy({ policy: PolicyType.PasswordGenerator }); + const service = new DefaultGeneratorService(strategy, policy); + + await firstValueFrom(service.evaluator$(SomeUser)); + + // FIXME: swap out the expect when `getAll$` becomes available + expect(policy.get$).toHaveBeenCalledWith(PolicyType.PasswordGenerator); + //expect(policy.getAll$).toHaveBeenCalledWith(PolicyType.PasswordGenerator, SomeUser); + }); + it("should map the policy using the generation strategy", async () => { const policyService = mockPolicyService(); const evaluator = mock>(); const strategy = mockGeneratorStrategy({ evaluator }); + const service = new DefaultGeneratorService(strategy, policyService); - const service = new DefaultGeneratorService(strategy, policyService, null); - - const policy = await firstValueFrom(service.policy$); + const policy = await firstValueFrom(service.evaluator$(SomeUser)); expect(policy).toBe(evaluator); }); + + it("should update the evaluator when the password generator policy changes", async () => { + // set up dependencies + const state = new BehaviorSubject(null); + const policy = mockPolicyService({ state }); + const strategy = mockGeneratorStrategy(); + const service = new DefaultGeneratorService(strategy, policy); + + // model responses for the observable update + const firstEvaluator = mock>(); + strategy.evaluator.mockReturnValueOnce(firstEvaluator); + const secondEvaluator = mock>(); + strategy.evaluator.mockReturnValueOnce(secondEvaluator); + + // act + const evaluator$ = service.evaluator$(SomeUser); + const firstResult = await firstValueFrom(evaluator$); + state.next(null); + const secondResult = await firstValueFrom(evaluator$); + + // assert + expect(firstResult).toBe(firstEvaluator); + expect(secondResult).toBe(secondEvaluator); + }); + + it("should cache the password generator policy", async () => { + const policy = mockPolicyService(); + const strategy = mockGeneratorStrategy({ policy: PolicyType.PasswordGenerator }); + const service = new DefaultGeneratorService(strategy, policy); + + await firstValueFrom(service.evaluator$(SomeUser)); + await firstValueFrom(service.evaluator$(SomeUser)); + + // FIXME: swap out the expect when `getAll$` becomes available + expect(policy.get$).toHaveBeenCalledTimes(1); + //expect(policy.getAll$).toHaveBeenCalledTimes(1); + }); + + it("should cache the password generator policy for each user", async () => { + const policy = mockPolicyService(); + const strategy = mockGeneratorStrategy({ policy: PolicyType.PasswordGenerator }); + const service = new DefaultGeneratorService(strategy, policy); + + await firstValueFrom(service.evaluator$(SomeUser)); + await firstValueFrom(service.evaluator$(AnotherUser)); + + // FIXME: enable this test when `getAll$` becomes available + // expect(policy.getAll$).toHaveBeenNthCalledWith(1, PolicyType.PasswordGenerator, SomeUser); + // expect(policy.getAll$).toHaveBeenNthCalledWith(2, PolicyType.PasswordGenerator, AnotherUser); + }); }); describe("enforcePolicy()", () => { - describe("should load the policy", () => { - it("from the cache by default", async () => { - const policy = mockPolicyService(); - const strategy = mockGeneratorStrategy(); - const service = new DefaultGeneratorService(strategy, policy, null); - - await service.enforcePolicy({}); - await service.enforcePolicy({}); - - expect(strategy.evaluator).toHaveBeenCalledTimes(1); - }); - - it("from the policy service when the policy changes", async () => { - const policy = new BehaviorSubject(mock({ data: {} })); - const policyService = mockPolicyService({ policy }); - const strategy = mockGeneratorStrategy(); - const service = new DefaultGeneratorService(strategy, policyService, null); - - await service.enforcePolicy({}); - policy.next(mock({ data: { some: "change" } })); - await service.enforcePolicy({}); - - expect(strategy.evaluator).toHaveBeenCalledTimes(2); - }); - }); - it("should evaluate the policy using the generation strategy", async () => { const policy = mockPolicyService(); const evaluator = mock>(); const strategy = mockGeneratorStrategy({ evaluator }); - const service = new DefaultGeneratorService(strategy, policy, null); + const service = new DefaultGeneratorService(strategy, policy); - await service.enforcePolicy({}); + await service.enforcePolicy(SomeUser, {}); expect(evaluator.applyPolicy).toHaveBeenCalled(); expect(evaluator.sanitize).toHaveBeenCalled(); @@ -182,7 +179,7 @@ describe("Password generator service", () => { it("should invoke the generation strategy", async () => { const strategy = mockGeneratorStrategy(); const policy = mockPolicyService(); - const service = new DefaultGeneratorService(strategy, policy, null); + const service = new DefaultGeneratorService(strategy, policy); await service.generate({}); diff --git a/libs/common/src/tools/generator/default-generator.service.ts b/libs/common/src/tools/generator/default-generator.service.ts index 55c15a23c2a..9c884ccefdc 100644 --- a/libs/common/src/tools/generator/default-generator.service.ts +++ b/libs/common/src/tools/generator/default-generator.service.ts @@ -3,7 +3,7 @@ import { firstValueFrom, map, share, timer, ReplaySubject, Observable } from "rx // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { PolicyService } from "../../admin-console/abstractions/policy/policy.service.abstraction"; -import { ActiveUserStateProvider } from "../../platform/state"; +import { UserId } from "../../types/guid"; import { GeneratorStrategy, GeneratorService, PolicyEvaluator } from "./abstractions"; @@ -13,45 +13,57 @@ export class DefaultGeneratorService implements GeneratorServic * @param strategy tailors the service to a specific generator type * (e.g. password, passphrase) * @param policy provides the policy to enforce - * @param state saves and loads password generation options to the location - * specified by the strategy */ constructor( private strategy: GeneratorStrategy, private policy: PolicyService, - private state: ActiveUserStateProvider, - ) { - this._policy$ = this.policy.get$(this.strategy.policy).pipe( + ) {} + + private _evaluators$ = new Map>>(); + + /** {@link GeneratorService.options$()} */ + options$(userId: UserId) { + return this.strategy.durableState(userId).state$; + } + + /** {@link GeneratorService.saveOptions} */ + async saveOptions(userId: UserId, options: Options): Promise { + await this.strategy.durableState(userId).update(() => options); + } + + /** {@link GeneratorService.evaluator$()} */ + evaluator$(userId: UserId) { + let evaluator$ = this._evaluators$.get(userId); + + if (!evaluator$) { + evaluator$ = this.createEvaluator(userId); + this._evaluators$.set(userId, evaluator$); + } + + return evaluator$; + } + + private createEvaluator(userId: UserId) { + // FIXME: when it becomes possible to get a user-specific policy observable + // (`getAll$`) update this code to call it instead of `get$`. + const policies$ = this.policy.get$(this.strategy.policy); + + // cache evaluator in a replay subject to amortize creation cost + // and reduce GC pressure. + const evaluator$ = policies$.pipe( map((policy) => this.strategy.evaluator(policy)), share({ - // cache evaluator in a replay subject to amortize creation cost - // and reduce GC pressure. connector: () => new ReplaySubject(1), resetOnRefCountZero: () => timer(this.strategy.cache_ms), }), ); + + return evaluator$; } - private _policy$: Observable>; - - /** {@link GeneratorService.options$} */ - get options$() { - return this.state.get(this.strategy.disk).state$; - } - - /** {@link GeneratorService.saveOptions} */ - async saveOptions(options: Options): Promise { - await this.state.get(this.strategy.disk).update(() => options); - } - - /** {@link GeneratorService.policy$} */ - get policy$() { - return this._policy$; - } - - /** {@link GeneratorService.enforcePolicy} */ - async enforcePolicy(options: Options): Promise { - const policy = await firstValueFrom(this._policy$); + /** {@link GeneratorService.enforcePolicy()} */ + async enforcePolicy(userId: UserId, options: Options): Promise { + const policy = await firstValueFrom(this.evaluator$(userId)); const evaluated = policy.applyPolicy(options); const sanitized = policy.sanitize(evaluated); return sanitized; diff --git a/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.spec.ts b/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.spec.ts index d355d2316d3..031ea05f014 100644 --- a/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.spec.ts +++ b/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.spec.ts @@ -9,15 +9,21 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { PASSPHRASE_SETTINGS } from "../key-definitions"; import { PasswordGenerationServiceAbstraction } from "../password/password-generation.service.abstraction"; +import { DisabledPassphraseGeneratorPolicy } from "./passphrase-generator-policy"; + import { PassphraseGeneratorOptionsEvaluator, PassphraseGeneratorStrategy } from "."; +const SomeUser = "some user" as UserId; + describe("Password generation strategy", () => { describe("evaluator()", () => { it("should throw if the policy type is incorrect", () => { - const strategy = new PassphraseGeneratorStrategy(null); + const strategy = new PassphraseGeneratorStrategy(null, null); const policy = mock({ type: PolicyType.DisableSend, }); @@ -26,7 +32,7 @@ describe("Password generation strategy", () => { }); it("should map to the policy evaluator", () => { - const strategy = new PassphraseGeneratorStrategy(null); + const strategy = new PassphraseGeneratorStrategy(null, null); const policy = mock({ type: PolicyType.PasswordGenerator, data: { @@ -45,21 +51,32 @@ describe("Password generation strategy", () => { includeNumber: true, }); }); + + it("should map `null` to a default policy evaluator", () => { + const strategy = new PassphraseGeneratorStrategy(null, null); + const evaluator = strategy.evaluator(null); + + expect(evaluator).toBeInstanceOf(PassphraseGeneratorOptionsEvaluator); + expect(evaluator.policy).toMatchObject(DisabledPassphraseGeneratorPolicy); + }); }); - describe("disk", () => { + describe("durableState", () => { it("should use password settings key", () => { + const provider = mock(); const legacy = mock(); - const strategy = new PassphraseGeneratorStrategy(legacy); + const strategy = new PassphraseGeneratorStrategy(legacy, provider); - expect(strategy.disk).toBe(PASSPHRASE_SETTINGS); + strategy.durableState(SomeUser); + + expect(provider.getUser).toHaveBeenCalledWith(SomeUser, PASSPHRASE_SETTINGS); }); }); describe("cache_ms", () => { it("should be a positive non-zero number", () => { const legacy = mock(); - const strategy = new PassphraseGeneratorStrategy(legacy); + const strategy = new PassphraseGeneratorStrategy(legacy, null); expect(strategy.cache_ms).toBeGreaterThan(0); }); @@ -68,7 +85,7 @@ describe("Password generation strategy", () => { describe("policy", () => { it("should use password generator policy", () => { const legacy = mock(); - const strategy = new PassphraseGeneratorStrategy(legacy); + const strategy = new PassphraseGeneratorStrategy(legacy, null); expect(strategy.policy).toBe(PolicyType.PasswordGenerator); }); @@ -77,7 +94,7 @@ describe("Password generation strategy", () => { describe("generate()", () => { it("should call the legacy service with the given options", async () => { const legacy = mock(); - const strategy = new PassphraseGeneratorStrategy(legacy); + const strategy = new PassphraseGeneratorStrategy(legacy, null); const options = { type: "passphrase", minNumberWords: 1, @@ -92,7 +109,7 @@ describe("Password generation strategy", () => { it("should set the generation type to passphrase", async () => { const legacy = mock(); - const strategy = new PassphraseGeneratorStrategy(legacy); + const strategy = new PassphraseGeneratorStrategy(legacy, null); await strategy.generate({ type: "foo" } as any); diff --git a/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.ts b/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.ts index 8e1d0d45987..d39f54b5765 100644 --- a/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.ts +++ b/libs/common/src/tools/generator/passphrase/passphrase-generator-strategy.ts @@ -3,12 +3,17 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { PASSPHRASE_SETTINGS } from "../key-definitions"; import { PasswordGenerationServiceAbstraction } from "../password/password-generation.service.abstraction"; import { PassphraseGenerationOptions } from "./passphrase-generation-options"; import { PassphraseGeneratorOptionsEvaluator } from "./passphrase-generator-options-evaluator"; -import { PassphraseGeneratorPolicy } from "./passphrase-generator-policy"; +import { + DisabledPassphraseGeneratorPolicy, + PassphraseGeneratorPolicy, +} from "./passphrase-generator-policy"; const ONE_MINUTE = 60 * 1000; @@ -19,11 +24,14 @@ export class PassphraseGeneratorStrategy /** instantiates the password generator strategy. * @param legacy generates the passphrase */ - constructor(private legacy: PasswordGenerationServiceAbstraction) {} + constructor( + private legacy: PasswordGenerationServiceAbstraction, + private stateProvider: StateProvider, + ) {} - /** {@link GeneratorStrategy.disk} */ - get disk() { - return PASSPHRASE_SETTINGS; + /** {@link GeneratorStrategy.durableState} */ + durableState(id: UserId) { + return this.stateProvider.getUser(id, PASSPHRASE_SETTINGS); } /** {@link GeneratorStrategy.policy} */ @@ -37,6 +45,10 @@ export class PassphraseGeneratorStrategy /** {@link GeneratorStrategy.evaluator} */ evaluator(policy: Policy): PassphraseGeneratorOptionsEvaluator { + if (!policy) { + return new PassphraseGeneratorOptionsEvaluator(DisabledPassphraseGeneratorPolicy); + } + if (policy.type !== this.policy) { const details = `Expected: ${this.policy}. Received: ${policy.type}`; throw Error("Mismatched policy type. " + details); diff --git a/libs/common/src/tools/generator/password/password-generator-strategy.spec.ts b/libs/common/src/tools/generator/password/password-generator-strategy.spec.ts index e49d1d56712..6c213f8c543 100644 --- a/libs/common/src/tools/generator/password/password-generator-strategy.spec.ts +++ b/libs/common/src/tools/generator/password/password-generator-strategy.spec.ts @@ -9,18 +9,24 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { PASSWORD_SETTINGS } from "../key-definitions"; +import { DisabledPasswordGeneratorPolicy } from "./password-generator-policy"; + import { PasswordGenerationServiceAbstraction, PasswordGeneratorOptionsEvaluator, PasswordGeneratorStrategy, } from "."; +const SomeUser = "some user" as UserId; + describe("Password generation strategy", () => { describe("evaluator()", () => { it("should throw if the policy type is incorrect", () => { - const strategy = new PasswordGeneratorStrategy(null); + const strategy = new PasswordGeneratorStrategy(null, null); const policy = mock({ type: PolicyType.DisableSend, }); @@ -29,7 +35,7 @@ describe("Password generation strategy", () => { }); it("should map to the policy evaluator", () => { - const strategy = new PasswordGeneratorStrategy(null); + const strategy = new PasswordGeneratorStrategy(null, null); const policy = mock({ type: PolicyType.PasswordGenerator, data: { @@ -56,21 +62,32 @@ describe("Password generation strategy", () => { specialCount: 1, }); }); + + it("should map `null` to a default policy evaluator", () => { + const strategy = new PasswordGeneratorStrategy(null, null); + const evaluator = strategy.evaluator(null); + + expect(evaluator).toBeInstanceOf(PasswordGeneratorOptionsEvaluator); + expect(evaluator.policy).toMatchObject(DisabledPasswordGeneratorPolicy); + }); }); - describe("disk", () => { + describe("durableState", () => { it("should use password settings key", () => { + const provider = mock(); const legacy = mock(); - const strategy = new PasswordGeneratorStrategy(legacy); + const strategy = new PasswordGeneratorStrategy(legacy, provider); - expect(strategy.disk).toBe(PASSWORD_SETTINGS); + strategy.durableState(SomeUser); + + expect(provider.getUser).toHaveBeenCalledWith(SomeUser, PASSWORD_SETTINGS); }); }); describe("cache_ms", () => { it("should be a positive non-zero number", () => { const legacy = mock(); - const strategy = new PasswordGeneratorStrategy(legacy); + const strategy = new PasswordGeneratorStrategy(legacy, null); expect(strategy.cache_ms).toBeGreaterThan(0); }); @@ -79,7 +96,7 @@ describe("Password generation strategy", () => { describe("policy", () => { it("should use password generator policy", () => { const legacy = mock(); - const strategy = new PasswordGeneratorStrategy(legacy); + const strategy = new PasswordGeneratorStrategy(legacy, null); expect(strategy.policy).toBe(PolicyType.PasswordGenerator); }); @@ -88,7 +105,7 @@ describe("Password generation strategy", () => { describe("generate()", () => { it("should call the legacy service with the given options", async () => { const legacy = mock(); - const strategy = new PasswordGeneratorStrategy(legacy); + const strategy = new PasswordGeneratorStrategy(legacy, null); const options = { type: "password", minLength: 1, @@ -107,7 +124,7 @@ describe("Password generation strategy", () => { it("should set the generation type to password", async () => { const legacy = mock(); - const strategy = new PasswordGeneratorStrategy(legacy); + const strategy = new PasswordGeneratorStrategy(legacy, null); await strategy.generate({ type: "foo" } as any); diff --git a/libs/common/src/tools/generator/password/password-generator-strategy.ts b/libs/common/src/tools/generator/password/password-generator-strategy.ts index 70bf3087a86..223470c5869 100644 --- a/libs/common/src/tools/generator/password/password-generator-strategy.ts +++ b/libs/common/src/tools/generator/password/password-generator-strategy.ts @@ -3,12 +3,17 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { PASSWORD_SETTINGS } from "../key-definitions"; import { PasswordGenerationOptions } from "./password-generation-options"; import { PasswordGenerationServiceAbstraction } from "./password-generation.service.abstraction"; import { PasswordGeneratorOptionsEvaluator } from "./password-generator-options-evaluator"; -import { PasswordGeneratorPolicy } from "./password-generator-policy"; +import { + DisabledPasswordGeneratorPolicy, + PasswordGeneratorPolicy, +} from "./password-generator-policy"; const ONE_MINUTE = 60 * 1000; @@ -19,11 +24,14 @@ export class PasswordGeneratorStrategy /** instantiates the password generator strategy. * @param legacy generates the password */ - constructor(private legacy: PasswordGenerationServiceAbstraction) {} + constructor( + private legacy: PasswordGenerationServiceAbstraction, + private stateProvider: StateProvider, + ) {} - /** {@link GeneratorStrategy.disk} */ - get disk() { - return PASSWORD_SETTINGS; + /** {@link GeneratorStrategy.durableState} */ + durableState(id: UserId) { + return this.stateProvider.getUser(id, PASSWORD_SETTINGS); } /** {@link GeneratorStrategy.policy} */ @@ -37,6 +45,10 @@ export class PasswordGeneratorStrategy /** {@link GeneratorStrategy.evaluator} */ evaluator(policy: Policy): PasswordGeneratorOptionsEvaluator { + if (!policy) { + return new PasswordGeneratorOptionsEvaluator(DisabledPasswordGeneratorPolicy); + } + if (policy.type !== this.policy) { const details = `Expected: ${this.policy}. Received: ${policy.type}`; throw Error("Mismatched policy type. " + details); diff --git a/libs/common/src/tools/generator/state/secret-state.spec.ts b/libs/common/src/tools/generator/state/secret-state.spec.ts index fa6288173a7..d4804fdb9b8 100644 --- a/libs/common/src/tools/generator/state/secret-state.spec.ts +++ b/libs/common/src/tools/generator/state/secret-state.spec.ts @@ -83,7 +83,16 @@ describe("UserEncryptor", () => { }); describe("instance", () => { - it("gets a set value", async () => { + it("userId outputs the user input during construction", async () => { + const provider = await fakeStateProvider(); + const encryptor = mockEncryptor(); + + const state = SecretState.from(SomeUser, FOOBAR_KEY, provider, encryptor); + + expect(state.userId).toEqual(SomeUser); + }); + + it("state$ gets a set value", async () => { const provider = await fakeStateProvider(); const encryptor = mockEncryptor(); const state = SecretState.from(SomeUser, FOOBAR_KEY, provider, encryptor); @@ -96,6 +105,20 @@ describe("UserEncryptor", () => { expect(result).toEqual(value); }); + it("combinedState$ gets a set value with the userId", async () => { + const provider = await fakeStateProvider(); + const encryptor = mockEncryptor(); + const state = SecretState.from(SomeUser, FOOBAR_KEY, provider, encryptor); + const value = { foo: true, bar: false }; + + await state.update(() => value); + await awaitAsync(); + const [userId, result] = await firstValueFrom(state.combinedState$); + + expect(result).toEqual(value); + expect(userId).toEqual(SomeUser); + }); + it("round-trips json-serializable values", async () => { const provider = await fakeStateProvider(); const encryptor = mockEncryptor(); diff --git a/libs/common/src/tools/generator/state/secret-state.ts b/libs/common/src/tools/generator/state/secret-state.ts index 88d0d95eaf3..62855c3280b 100644 --- a/libs/common/src/tools/generator/state/secret-state.ts +++ b/libs/common/src/tools/generator/state/secret-state.ts @@ -1,4 +1,4 @@ -import { Observable, concatMap, of, zip } from "rxjs"; +import { Observable, concatMap, of, zip, map } from "rxjs"; import { Jsonify } from "type-fest"; import { EncString } from "../../../platform/models/domain/enc-string"; @@ -9,6 +9,7 @@ import { SingleUserState, StateProvider, StateUpdateOptions, + CombinedState, } from "../../../platform/state"; import { UserId } from "../../../types/guid"; @@ -37,7 +38,9 @@ type ClassifiedFormat = { * * DO NOT USE THIS for synchronized data. */ -export class SecretState { +export class SecretState<Plaintext extends object, Disclosed> + implements SingleUserState<Plaintext> +{ // The constructor is private to avoid creating a circular dependency when // wiring the derived and secret states together. private constructor( @@ -46,8 +49,23 @@ export class SecretState<Plaintext extends object, Disclosed> { private readonly plaintext: DerivedState<Plaintext>, ) { this.state$ = plaintext.state$; + this.combinedState$ = plaintext.state$.pipe(map((state) => [this.encrypted.userId, state])); } + /** {@link SingleUserState.userId} */ + get userId() { + return this.encrypted.userId; + } + + /** Observes changes to the decrypted secret state. The observer + * updates after the secret has been recorded to state storage. + * @returns `undefined` when the account is locked. + */ + readonly state$: Observable<Plaintext>; + + /** {@link SingleUserState.combinedState$} */ + readonly combinedState$: Observable<CombinedState<Plaintext>>; + /** Creates a secret state bound to an account encryptor. The account must be unlocked * when this method is called. * @param userId: the user to which the secret state is bound. @@ -106,12 +124,6 @@ export class SecretState<Plaintext extends object, Disclosed> { return secretState; } - /** Observes changes to the decrypted secret state. The observer - * updates after the secret has been recorded to state storage. - * @returns `undefined` when the account is locked. - */ - readonly state$: Observable<Plaintext>; - /** Updates the secret stored by this state. * @param configureState a callback that returns an updated decrypted * secret state. The callback receives the state's present value as its diff --git a/libs/common/src/tools/generator/username/catchall-generator-strategy.spec.ts b/libs/common/src/tools/generator/username/catchall-generator-strategy.spec.ts index fb5f07520b6..dafb55febab 100644 --- a/libs/common/src/tools/generator/username/catchall-generator-strategy.spec.ts +++ b/libs/common/src/tools/generator/username/catchall-generator-strategy.spec.ts @@ -4,15 +4,19 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { DefaultPolicyEvaluator } from "../default-policy-evaluator"; import { CATCHALL_SETTINGS } from "../key-definitions"; import { CatchallGeneratorStrategy, UsernameGenerationServiceAbstraction } from "."; +const SomeUser = "some user" as UserId; + describe("Email subaddress list generation strategy", () => { describe("evaluator()", () => { it("should throw if the policy type is incorrect", () => { - const strategy = new CatchallGeneratorStrategy(null); + const strategy = new CatchallGeneratorStrategy(null, null); const policy = mock<Policy>({ type: PolicyType.DisableSend, }); @@ -21,7 +25,7 @@ describe("Email subaddress list generation strategy", () => { }); it("should map to the policy evaluator", () => { - const strategy = new CatchallGeneratorStrategy(null); + const strategy = new CatchallGeneratorStrategy(null, null); const policy = mock<Policy>({ type: PolicyType.PasswordGenerator, data: { @@ -34,21 +38,31 @@ describe("Email subaddress list generation strategy", () => { expect(evaluator).toBeInstanceOf(DefaultPolicyEvaluator); expect(evaluator.policy).toMatchObject({}); }); + + it("should map `null` to a default policy evaluator", () => { + const strategy = new CatchallGeneratorStrategy(null, null); + const evaluator = strategy.evaluator(null); + + expect(evaluator).toBeInstanceOf(DefaultPolicyEvaluator); + }); }); - describe("disk", () => { + describe("durableState", () => { it("should use password settings key", () => { + const provider = mock<StateProvider>(); const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new CatchallGeneratorStrategy(legacy); + const strategy = new CatchallGeneratorStrategy(legacy, provider); - expect(strategy.disk).toBe(CATCHALL_SETTINGS); + strategy.durableState(SomeUser); + + expect(provider.getUser).toHaveBeenCalledWith(SomeUser, CATCHALL_SETTINGS); }); }); describe("cache_ms", () => { it("should be a positive non-zero number", () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new CatchallGeneratorStrategy(legacy); + const strategy = new CatchallGeneratorStrategy(legacy, null); expect(strategy.cache_ms).toBeGreaterThan(0); }); @@ -57,7 +71,7 @@ describe("Email subaddress list generation strategy", () => { describe("policy", () => { it("should use password generator policy", () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new CatchallGeneratorStrategy(legacy); + const strategy = new CatchallGeneratorStrategy(legacy, null); expect(strategy.policy).toBe(PolicyType.PasswordGenerator); }); @@ -66,7 +80,7 @@ describe("Email subaddress list generation strategy", () => { describe("generate()", () => { it("should call the legacy service with the given options", async () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new CatchallGeneratorStrategy(legacy); + const strategy = new CatchallGeneratorStrategy(legacy, null); const options = { type: "website-name" as const, domain: "example.com", diff --git a/libs/common/src/tools/generator/username/catchall-generator-strategy.ts b/libs/common/src/tools/generator/username/catchall-generator-strategy.ts index 86a7a01cd99..aadca78b3b4 100644 --- a/libs/common/src/tools/generator/username/catchall-generator-strategy.ts +++ b/libs/common/src/tools/generator/username/catchall-generator-strategy.ts @@ -1,5 +1,7 @@ import { PolicyType } from "../../../admin-console/enums"; import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { GeneratorStrategy } from "../abstractions"; import { DefaultPolicyEvaluator } from "../default-policy-evaluator"; import { CATCHALL_SETTINGS } from "../key-definitions"; @@ -17,11 +19,14 @@ export class CatchallGeneratorStrategy /** Instantiates the generation strategy * @param usernameService generates a catchall address for a domain */ - constructor(private usernameService: UsernameGenerationServiceAbstraction) {} + constructor( + private usernameService: UsernameGenerationServiceAbstraction, + private stateProvider: StateProvider, + ) {} - /** {@link GeneratorStrategy.disk} */ - get disk() { - return CATCHALL_SETTINGS; + /** {@link GeneratorStrategy.durableState} */ + durableState(id: UserId) { + return this.stateProvider.getUser(id, CATCHALL_SETTINGS); } /** {@link GeneratorStrategy.policy} */ @@ -38,6 +43,10 @@ export class CatchallGeneratorStrategy /** {@link GeneratorStrategy.evaluator} */ evaluator(policy: Policy) { + if (!policy) { + return new DefaultPolicyEvaluator<CatchallGenerationOptions>(); + } + if (policy.type !== this.policy) { const details = `Expected: ${this.policy}. Received: ${policy.type}`; throw Error("Mismatched policy type. " + details); diff --git a/libs/common/src/tools/generator/username/eff-username-generator-strategy.spec.ts b/libs/common/src/tools/generator/username/eff-username-generator-strategy.spec.ts index 2433ae34f19..0fb5bf573c0 100644 --- a/libs/common/src/tools/generator/username/eff-username-generator-strategy.spec.ts +++ b/libs/common/src/tools/generator/username/eff-username-generator-strategy.spec.ts @@ -4,15 +4,19 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { DefaultPolicyEvaluator } from "../default-policy-evaluator"; import { EFF_USERNAME_SETTINGS } from "../key-definitions"; import { EffUsernameGeneratorStrategy, UsernameGenerationServiceAbstraction } from "."; +const SomeUser = "some user" as UserId; + describe("EFF long word list generation strategy", () => { describe("evaluator()", () => { it("should throw if the policy type is incorrect", () => { - const strategy = new EffUsernameGeneratorStrategy(null); + const strategy = new EffUsernameGeneratorStrategy(null, null); const policy = mock<Policy>({ type: PolicyType.DisableSend, }); @@ -21,7 +25,7 @@ describe("EFF long word list generation strategy", () => { }); it("should map to the policy evaluator", () => { - const strategy = new EffUsernameGeneratorStrategy(null); + const strategy = new EffUsernameGeneratorStrategy(null, null); const policy = mock<Policy>({ type: PolicyType.PasswordGenerator, data: { @@ -34,21 +38,31 @@ describe("EFF long word list generation strategy", () => { expect(evaluator).toBeInstanceOf(DefaultPolicyEvaluator); expect(evaluator.policy).toMatchObject({}); }); + + it("should map `null` to a default policy evaluator", () => { + const strategy = new EffUsernameGeneratorStrategy(null, null); + const evaluator = strategy.evaluator(null); + + expect(evaluator).toBeInstanceOf(DefaultPolicyEvaluator); + }); }); - describe("disk", () => { + describe("durableState", () => { it("should use password settings key", () => { + const provider = mock<StateProvider>(); const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new EffUsernameGeneratorStrategy(legacy); + const strategy = new EffUsernameGeneratorStrategy(legacy, provider); - expect(strategy.disk).toBe(EFF_USERNAME_SETTINGS); + strategy.durableState(SomeUser); + + expect(provider.getUser).toHaveBeenCalledWith(SomeUser, EFF_USERNAME_SETTINGS); }); }); describe("cache_ms", () => { it("should be a positive non-zero number", () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new EffUsernameGeneratorStrategy(legacy); + const strategy = new EffUsernameGeneratorStrategy(legacy, null); expect(strategy.cache_ms).toBeGreaterThan(0); }); @@ -57,7 +71,7 @@ describe("EFF long word list generation strategy", () => { describe("policy", () => { it("should use password generator policy", () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new EffUsernameGeneratorStrategy(legacy); + const strategy = new EffUsernameGeneratorStrategy(legacy, null); expect(strategy.policy).toBe(PolicyType.PasswordGenerator); }); @@ -66,7 +80,7 @@ describe("EFF long word list generation strategy", () => { describe("generate()", () => { it("should call the legacy service with the given options", async () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new EffUsernameGeneratorStrategy(legacy); + const strategy = new EffUsernameGeneratorStrategy(legacy, null); const options = { wordCapitalize: false, wordIncludeNumber: false, diff --git a/libs/common/src/tools/generator/username/eff-username-generator-strategy.ts b/libs/common/src/tools/generator/username/eff-username-generator-strategy.ts index fb878c16c92..e0179895ae3 100644 --- a/libs/common/src/tools/generator/username/eff-username-generator-strategy.ts +++ b/libs/common/src/tools/generator/username/eff-username-generator-strategy.ts @@ -1,5 +1,7 @@ import { PolicyType } from "../../../admin-console/enums"; import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { GeneratorStrategy } from "../abstractions"; import { DefaultPolicyEvaluator } from "../default-policy-evaluator"; import { EFF_USERNAME_SETTINGS } from "../key-definitions"; @@ -17,11 +19,14 @@ export class EffUsernameGeneratorStrategy /** Instantiates the generation strategy * @param usernameService generates a username from EFF word list */ - constructor(private usernameService: UsernameGenerationServiceAbstraction) {} + constructor( + private usernameService: UsernameGenerationServiceAbstraction, + private stateProvider: StateProvider, + ) {} - /** {@link GeneratorStrategy.disk} */ - get disk() { - return EFF_USERNAME_SETTINGS; + /** {@link GeneratorStrategy.durableState} */ + durableState(id: UserId) { + return this.stateProvider.getUser(id, EFF_USERNAME_SETTINGS); } /** {@link GeneratorStrategy.policy} */ @@ -38,6 +43,10 @@ export class EffUsernameGeneratorStrategy /** {@link GeneratorStrategy.evaluator} */ evaluator(policy: Policy) { + if (!policy) { + return new DefaultPolicyEvaluator<EffUsernameGenerationOptions>(); + } + if (policy.type !== this.policy) { const details = `Expected: ${this.policy}. Received: ${policy.type}`; throw Error("Mismatched policy type. " + details); diff --git a/libs/common/src/tools/generator/username/subaddress-generator-strategy.spec.ts b/libs/common/src/tools/generator/username/subaddress-generator-strategy.spec.ts index 2c0fa896bb7..105edd6b4df 100644 --- a/libs/common/src/tools/generator/username/subaddress-generator-strategy.spec.ts +++ b/libs/common/src/tools/generator/username/subaddress-generator-strategy.spec.ts @@ -4,15 +4,19 @@ import { PolicyType } from "../../../admin-console/enums"; // FIXME: use index.ts imports once policy abstractions and models // implement ADR-0002 import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { DefaultPolicyEvaluator } from "../default-policy-evaluator"; import { SUBADDRESS_SETTINGS } from "../key-definitions"; import { SubaddressGeneratorStrategy, UsernameGenerationServiceAbstraction } from "."; +const SomeUser = "some user" as UserId; + describe("Email subaddress list generation strategy", () => { describe("evaluator()", () => { it("should throw if the policy type is incorrect", () => { - const strategy = new SubaddressGeneratorStrategy(null); + const strategy = new SubaddressGeneratorStrategy(null, null); const policy = mock<Policy>({ type: PolicyType.DisableSend, }); @@ -21,7 +25,7 @@ describe("Email subaddress list generation strategy", () => { }); it("should map to the policy evaluator", () => { - const strategy = new SubaddressGeneratorStrategy(null); + const strategy = new SubaddressGeneratorStrategy(null, null); const policy = mock<Policy>({ type: PolicyType.PasswordGenerator, data: { @@ -34,21 +38,31 @@ describe("Email subaddress list generation strategy", () => { expect(evaluator).toBeInstanceOf(DefaultPolicyEvaluator); expect(evaluator.policy).toMatchObject({}); }); + + it("should map `null` to a default policy evaluator", () => { + const strategy = new SubaddressGeneratorStrategy(null, null); + const evaluator = strategy.evaluator(null); + + expect(evaluator).toBeInstanceOf(DefaultPolicyEvaluator); + }); }); - describe("disk", () => { + describe("durableState", () => { it("should use password settings key", () => { + const provider = mock<StateProvider>(); const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new SubaddressGeneratorStrategy(legacy); + const strategy = new SubaddressGeneratorStrategy(legacy, provider); - expect(strategy.disk).toBe(SUBADDRESS_SETTINGS); + strategy.durableState(SomeUser); + + expect(provider.getUser).toHaveBeenCalledWith(SomeUser, SUBADDRESS_SETTINGS); }); }); describe("cache_ms", () => { it("should be a positive non-zero number", () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new SubaddressGeneratorStrategy(legacy); + const strategy = new SubaddressGeneratorStrategy(legacy, null); expect(strategy.cache_ms).toBeGreaterThan(0); }); @@ -57,7 +71,7 @@ describe("Email subaddress list generation strategy", () => { describe("policy", () => { it("should use password generator policy", () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new SubaddressGeneratorStrategy(legacy); + const strategy = new SubaddressGeneratorStrategy(legacy, null); expect(strategy.policy).toBe(PolicyType.PasswordGenerator); }); @@ -66,7 +80,7 @@ describe("Email subaddress list generation strategy", () => { describe("generate()", () => { it("should call the legacy service with the given options", async () => { const legacy = mock<UsernameGenerationServiceAbstraction>(); - const strategy = new SubaddressGeneratorStrategy(legacy); + const strategy = new SubaddressGeneratorStrategy(legacy, null); const options = { type: "website-name" as const, email: "someone@example.com", diff --git a/libs/common/src/tools/generator/username/subaddress-generator-strategy.ts b/libs/common/src/tools/generator/username/subaddress-generator-strategy.ts index eb364103133..1aba473476d 100644 --- a/libs/common/src/tools/generator/username/subaddress-generator-strategy.ts +++ b/libs/common/src/tools/generator/username/subaddress-generator-strategy.ts @@ -1,5 +1,7 @@ import { PolicyType } from "../../../admin-console/enums"; import { Policy } from "../../../admin-console/models/domain/policy"; +import { StateProvider } from "../../../platform/state"; +import { UserId } from "../../../types/guid"; import { GeneratorStrategy } from "../abstractions"; import { DefaultPolicyEvaluator } from "../default-policy-evaluator"; import { SUBADDRESS_SETTINGS } from "../key-definitions"; @@ -17,11 +19,14 @@ export class SubaddressGeneratorStrategy /** Instantiates the generation strategy * @param usernameService generates an email subaddress from an email address */ - constructor(private usernameService: UsernameGenerationServiceAbstraction) {} + constructor( + private usernameService: UsernameGenerationServiceAbstraction, + private stateProvider: StateProvider, + ) {} - /** {@link GeneratorStrategy.disk} */ - get disk() { - return SUBADDRESS_SETTINGS; + /** {@link GeneratorStrategy.durableState} */ + durableState(id: UserId) { + return this.stateProvider.getUser(id, SUBADDRESS_SETTINGS); } /** {@link GeneratorStrategy.policy} */ @@ -38,6 +43,10 @@ export class SubaddressGeneratorStrategy /** {@link GeneratorStrategy.evaluator} */ evaluator(policy: Policy) { + if (!policy) { + return new DefaultPolicyEvaluator<SubaddressGenerationOptions>(); + } + if (policy.type !== this.policy) { const details = `Expected: ${this.policy}. Received: ${policy.type}`; throw Error("Mismatched policy type. " + details); From 4ba2717eb487ab231497fe0705b4d19693fbd501 Mon Sep 17 00:00:00 2001 From: Jonathan Prusik <jprusik@users.noreply.github.com> Date: Mon, 4 Mar 2024 14:12:23 -0500 Subject: [PATCH 02/14] [PM-5559] Implement User Notification Settings state provider (#8032) * create user notification settings state provider * replace state service get/set disableAddLoginNotification and disableChangedPasswordNotification with user notification settings service equivalents * migrate disableAddLoginNotification and disableChangedPasswordNotification global settings to user notification settings state provider * add content script messaging the background for enableChangedPasswordPrompt setting * Implementing feedback to provide on PR * Implementing feedback to provide on PR * PR suggestions cleanup --------- Co-authored-by: Cesar Gonzalez <cgonzalez@bitwarden.com> --- .../abstractions/notification.background.ts | 2 + .../notification.background.spec.ts | 50 +++++---- .../background/notification.background.ts | 30 ++++- ...r-notification-settings-service.factory.ts | 25 +++++ .../src/autofill/content/notification-bar.ts | 54 +++------ .../services/autofill.service.spec.ts | 11 +- apps/browser/src/autofill/types/index.ts | 5 +- .../browser/src/background/main.background.ts | 17 ++- .../src/popup/services/services.module.ts | 9 ++ .../src/popup/settings/options.component.ts | 19 +++- .../user-notification-settings.service.ts | 60 ++++++++++ .../platform/abstractions/state.service.ts | 7 -- .../platform/models/domain/global-state.ts | 2 - .../src/platform/services/state.service.ts | 39 ------- .../src/platform/state/state-definitions.ts | 4 + libs/common/src/state-migrations/migrate.ts | 6 +- ...ication-settings-to-state-provider.spec.ts | 102 +++++++++++++++++ ...notification-settings-to-state-provider.ts | 105 ++++++++++++++++++ 18 files changed, 409 insertions(+), 138 deletions(-) create mode 100644 apps/browser/src/autofill/background/service_factories/user-notification-settings-service.factory.ts create mode 100644 libs/common/src/autofill/services/user-notification-settings.service.ts create mode 100644 libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.ts diff --git a/apps/browser/src/autofill/background/abstractions/notification.background.ts b/apps/browser/src/autofill/background/abstractions/notification.background.ts index 94ab1a398d9..322ccd732d0 100644 --- a/apps/browser/src/autofill/background/abstractions/notification.background.ts +++ b/apps/browser/src/autofill/background/abstractions/notification.background.ts @@ -109,6 +109,8 @@ type NotificationBackgroundExtensionMessageHandlers = { bgReopenUnlockPopout: ({ sender }: BackgroundSenderParam) => Promise<void>; checkNotificationQueue: ({ sender }: BackgroundSenderParam) => Promise<void>; collectPageDetailsResponse: ({ message }: BackgroundMessageParam) => Promise<void>; + bgGetEnableChangedPasswordPrompt: () => Promise<boolean>; + bgGetEnableAddedLoginPrompt: () => Promise<boolean>; getWebVaultUrlForNotification: () => string; }; diff --git a/apps/browser/src/autofill/background/notification.background.spec.ts b/apps/browser/src/autofill/background/notification.background.spec.ts index 29c8481e5f2..7dde61b6fd7 100644 --- a/apps/browser/src/autofill/background/notification.background.spec.ts +++ b/apps/browser/src/autofill/background/notification.background.spec.ts @@ -4,6 +4,7 @@ import { firstValueFrom } from "rxjs"; import { PolicyService } from "@bitwarden/common/admin-console/services/policy/policy.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; import { AuthService } from "@bitwarden/common/auth/services/auth.service"; +import { UserNotificationSettingsService } from "@bitwarden/common/autofill/services/user-notification-settings.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { EnvironmentService } from "@bitwarden/common/platform/services/environment.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; @@ -45,6 +46,7 @@ describe("NotificationBackground", () => { const policyService = mock<PolicyService>(); const folderService = mock<FolderService>(); const stateService = mock<BrowserStateService>(); + const userNotificationSettingsService = mock<UserNotificationSettingsService>(); const environmentService = mock<EnvironmentService>(); const logService = mock<LogService>(); @@ -56,6 +58,7 @@ describe("NotificationBackground", () => { policyService, folderService, stateService, + userNotificationSettingsService, environmentService, logService, ); @@ -235,8 +238,8 @@ describe("NotificationBackground", () => { let tab: chrome.tabs.Tab; let sender: chrome.runtime.MessageSender; let getAuthStatusSpy: jest.SpyInstance; - let getDisableAddLoginNotificationSpy: jest.SpyInstance; - let getDisableChangedPasswordNotificationSpy: jest.SpyInstance; + let getEnableAddedLoginPromptSpy: jest.SpyInstance; + let getEnableChangedPasswordPromptSpy: jest.SpyInstance; let pushAddLoginToQueueSpy: jest.SpyInstance; let pushChangePasswordToQueueSpy: jest.SpyInstance; let getAllDecryptedForUrlSpy: jest.SpyInstance; @@ -245,13 +248,13 @@ describe("NotificationBackground", () => { tab = createChromeTabMock(); sender = mock<chrome.runtime.MessageSender>({ tab }); getAuthStatusSpy = jest.spyOn(authService, "getAuthStatus"); - getDisableAddLoginNotificationSpy = jest.spyOn( - stateService, - "getDisableAddLoginNotification", + getEnableAddedLoginPromptSpy = jest.spyOn( + notificationBackground as any, + "getEnableAddedLoginPrompt", ); - getDisableChangedPasswordNotificationSpy = jest.spyOn( - stateService, - "getDisableChangedPasswordNotification", + getEnableChangedPasswordPromptSpy = jest.spyOn( + notificationBackground as any, + "getEnableChangedPasswordPrompt", ); pushAddLoginToQueueSpy = jest.spyOn(notificationBackground as any, "pushAddLoginToQueue"); pushChangePasswordToQueueSpy = jest.spyOn( @@ -272,7 +275,7 @@ describe("NotificationBackground", () => { await flushPromises(); expect(getAuthStatusSpy).toHaveBeenCalled(); - expect(getDisableAddLoginNotificationSpy).not.toHaveBeenCalled(); + expect(getEnableAddedLoginPromptSpy).not.toHaveBeenCalled(); expect(pushAddLoginToQueueSpy).not.toHaveBeenCalled(); }); @@ -287,7 +290,7 @@ describe("NotificationBackground", () => { await flushPromises(); expect(getAuthStatusSpy).toHaveBeenCalled(); - expect(getDisableAddLoginNotificationSpy).not.toHaveBeenCalled(); + expect(getEnableAddedLoginPromptSpy).not.toHaveBeenCalled(); expect(pushAddLoginToQueueSpy).not.toHaveBeenCalled(); }); @@ -297,13 +300,13 @@ describe("NotificationBackground", () => { login: { username: "test", password: "password", url: "https://example.com" }, }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Locked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(true); + getEnableAddedLoginPromptSpy.mockReturnValueOnce(false); sendExtensionRuntimeMessage(message, sender); await flushPromises(); expect(getAuthStatusSpy).toHaveBeenCalled(); - expect(getDisableAddLoginNotificationSpy).toHaveBeenCalled(); + expect(getEnableAddedLoginPromptSpy).toHaveBeenCalled(); expect(getAllDecryptedForUrlSpy).not.toHaveBeenCalled(); expect(pushAddLoginToQueueSpy).not.toHaveBeenCalled(); expect(pushChangePasswordToQueueSpy).not.toHaveBeenCalled(); @@ -315,14 +318,14 @@ describe("NotificationBackground", () => { login: { username: "test", password: "password", url: "https://example.com" }, }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Unlocked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(true); + getEnableAddedLoginPromptSpy.mockReturnValueOnce(false); getAllDecryptedForUrlSpy.mockResolvedValueOnce([]); sendExtensionRuntimeMessage(message, sender); await flushPromises(); expect(getAuthStatusSpy).toHaveBeenCalled(); - expect(getDisableAddLoginNotificationSpy).toHaveBeenCalled(); + expect(getEnableAddedLoginPromptSpy).toHaveBeenCalled(); expect(getAllDecryptedForUrlSpy).toHaveBeenCalled(); expect(pushAddLoginToQueueSpy).not.toHaveBeenCalled(); expect(pushChangePasswordToQueueSpy).not.toHaveBeenCalled(); @@ -334,8 +337,8 @@ describe("NotificationBackground", () => { login: { username: "test", password: "password", url: "https://example.com" }, }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Unlocked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(false); - getDisableChangedPasswordNotificationSpy.mockReturnValueOnce(true); + getEnableAddedLoginPromptSpy.mockReturnValueOnce(true); + getEnableChangedPasswordPromptSpy.mockReturnValueOnce(false); getAllDecryptedForUrlSpy.mockResolvedValueOnce([ mock<CipherView>({ login: { username: "test", password: "oldPassword" } }), ]); @@ -344,9 +347,9 @@ describe("NotificationBackground", () => { await flushPromises(); expect(getAuthStatusSpy).toHaveBeenCalled(); - expect(getDisableAddLoginNotificationSpy).toHaveBeenCalled(); + expect(getEnableAddedLoginPromptSpy).toHaveBeenCalled(); expect(getAllDecryptedForUrlSpy).toHaveBeenCalled(); - expect(getDisableChangedPasswordNotificationSpy).toHaveBeenCalled(); + expect(getEnableChangedPasswordPromptSpy).toHaveBeenCalled(); expect(pushAddLoginToQueueSpy).not.toHaveBeenCalled(); expect(pushChangePasswordToQueueSpy).not.toHaveBeenCalled(); }); @@ -357,7 +360,7 @@ describe("NotificationBackground", () => { login: { username: "test", password: "password", url: "https://example.com" }, }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Unlocked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(false); + getEnableAddedLoginPromptSpy.mockReturnValueOnce(true); getAllDecryptedForUrlSpy.mockResolvedValueOnce([ mock<CipherView>({ login: { username: "test", password: "password" } }), ]); @@ -366,7 +369,7 @@ describe("NotificationBackground", () => { await flushPromises(); expect(getAuthStatusSpy).toHaveBeenCalled(); - expect(getDisableAddLoginNotificationSpy).toHaveBeenCalled(); + expect(getEnableAddedLoginPromptSpy).toHaveBeenCalled(); expect(getAllDecryptedForUrlSpy).toHaveBeenCalled(); expect(pushAddLoginToQueueSpy).not.toHaveBeenCalled(); expect(pushChangePasswordToQueueSpy).not.toHaveBeenCalled(); @@ -376,7 +379,7 @@ describe("NotificationBackground", () => { const login = { username: "test", password: "password", url: "https://example.com" }; const message: NotificationBackgroundExtensionMessage = { command: "bgAddLogin", login }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Locked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(false); + getEnableAddedLoginPromptSpy.mockReturnValueOnce(true); sendExtensionRuntimeMessage(message, sender); await flushPromises(); @@ -393,7 +396,7 @@ describe("NotificationBackground", () => { } as any; const message: NotificationBackgroundExtensionMessage = { command: "bgAddLogin", login }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Unlocked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(false); + getEnableAddedLoginPromptSpy.mockReturnValueOnce(true); getAllDecryptedForUrlSpy.mockResolvedValueOnce([ mock<CipherView>({ login: { username: "anotherTestUsername", password: "password" } }), ]); @@ -409,7 +412,8 @@ describe("NotificationBackground", () => { const login = { username: "tEsT", password: "password", url: "https://example.com" }; const message: NotificationBackgroundExtensionMessage = { command: "bgAddLogin", login }; getAuthStatusSpy.mockResolvedValueOnce(AuthenticationStatus.Unlocked); - getDisableAddLoginNotificationSpy.mockReturnValueOnce(false); + getEnableAddedLoginPromptSpy.mockResolvedValueOnce(true); + getEnableChangedPasswordPromptSpy.mockResolvedValueOnce(true); getAllDecryptedForUrlSpy.mockResolvedValueOnce([ mock<CipherView>({ id: "cipher-id", diff --git a/apps/browser/src/autofill/background/notification.background.ts b/apps/browser/src/autofill/background/notification.background.ts index 8ebf1bd26e3..6b5f1a8297f 100644 --- a/apps/browser/src/autofill/background/notification.background.ts +++ b/apps/browser/src/autofill/background/notification.background.ts @@ -4,6 +4,7 @@ import { PolicyService } from "@bitwarden/common/admin-console/abstractions/poli import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; +import { UserNotificationSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/user-notification-settings.service"; import { EnvironmentService } from "@bitwarden/common/platform/abstractions/environment.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; @@ -57,6 +58,8 @@ export default class NotificationBackground { bgUnlockPopoutOpened: ({ message, sender }) => this.unlockVault(message, sender.tab), checkNotificationQueue: ({ sender }) => this.checkNotificationQueue(sender.tab), bgReopenUnlockPopout: ({ sender }) => this.openUnlockPopout(sender.tab), + bgGetEnableChangedPasswordPrompt: () => this.getEnableChangedPasswordPrompt(), + bgGetEnableAddedLoginPrompt: () => this.getEnableAddedLoginPrompt(), getWebVaultUrlForNotification: () => this.getWebVaultUrl(), }; @@ -67,6 +70,7 @@ export default class NotificationBackground { private policyService: PolicyService, private folderService: FolderService, private stateService: BrowserStateService, + private userNotificationSettingsService: UserNotificationSettingsServiceAbstraction, private environmentService: EnvironmentService, private logService: LogService, ) {} @@ -81,6 +85,20 @@ export default class NotificationBackground { this.cleanupNotificationQueue(); } + /** + * Gets the enableChangedPasswordPrompt setting from the user notification settings service. + */ + async getEnableChangedPasswordPrompt(): Promise<boolean> { + return await firstValueFrom(this.userNotificationSettingsService.enableChangedPasswordPrompt$); + } + + /** + * Gets the enableAddedLoginPrompt setting from the user notification settings service. + */ + async getEnableAddedLoginPrompt(): Promise<boolean> { + return await firstValueFrom(this.userNotificationSettingsService.enableAddedLoginPrompt$); + } + /** * Checks the notification queue for any messages that need to be sent to the * specified tab. If no tab is specified, the current tab will be used. @@ -194,9 +212,10 @@ export default class NotificationBackground { return; } - const disabledAddLogin = await this.stateService.getDisableAddLoginNotification(); + const addLoginIsEnabled = await this.getEnableAddedLoginPrompt(); + if (authStatus === AuthenticationStatus.Locked) { - if (!disabledAddLogin) { + if (addLoginIsEnabled) { await this.pushAddLoginToQueue(loginDomain, loginInfo, sender.tab, true); } @@ -207,14 +226,15 @@ export default class NotificationBackground { const usernameMatches = ciphers.filter( (c) => c.login.username != null && c.login.username.toLowerCase() === normalizedUsername, ); - if (!disabledAddLogin && usernameMatches.length === 0) { + if (addLoginIsEnabled && usernameMatches.length === 0) { await this.pushAddLoginToQueue(loginDomain, loginInfo, sender.tab); return; } - const disabledChangePassword = await this.stateService.getDisableChangedPasswordNotification(); + const changePasswordIsEnabled = await this.getEnableChangedPasswordPrompt(); + if ( - !disabledChangePassword && + changePasswordIsEnabled && usernameMatches.length === 1 && usernameMatches[0].login.password !== loginInfo.password ) { diff --git a/apps/browser/src/autofill/background/service_factories/user-notification-settings-service.factory.ts b/apps/browser/src/autofill/background/service_factories/user-notification-settings-service.factory.ts new file mode 100644 index 00000000000..5e19795e0e6 --- /dev/null +++ b/apps/browser/src/autofill/background/service_factories/user-notification-settings-service.factory.ts @@ -0,0 +1,25 @@ +import { UserNotificationSettingsService } from "@bitwarden/common/autofill/services/user-notification-settings.service"; + +import { + CachedServices, + factory, + FactoryOptions, +} from "../../../platform/background/service-factories/factory-options"; +import { + stateProviderFactory, + StateProviderInitOptions, +} from "../../../platform/background/service-factories/state-provider.factory"; + +export type UserNotificationSettingsServiceInitOptions = FactoryOptions & StateProviderInitOptions; + +export function userNotificationSettingsServiceFactory( + cache: { userNotificationSettingsService?: UserNotificationSettingsService } & CachedServices, + opts: UserNotificationSettingsServiceInitOptions, +): Promise<UserNotificationSettingsService> { + return factory( + cache, + "userNotificationSettingsService", + opts, + async () => new UserNotificationSettingsService(await stateProviderFactory(cache, opts)), + ); +} diff --git a/apps/browser/src/autofill/content/notification-bar.ts b/apps/browser/src/autofill/content/notification-bar.ts index 5766017ebe8..f2a74310c29 100644 --- a/apps/browser/src/autofill/content/notification-bar.ts +++ b/apps/browser/src/autofill/content/notification-bar.ts @@ -7,7 +7,11 @@ import { WatchedForm } from "../models/watched-form"; import { NotificationBarIframeInitData } from "../notification/abstractions/notification-bar"; import { FormData } from "../services/abstractions/autofill.service"; import { GlobalSettings, UserSettings } from "../types"; -import { getFromLocalStorage, setupExtensionDisconnectAction } from "../utils"; +import { + getFromLocalStorage, + sendExtensionMessage, + setupExtensionDisconnectAction, +} from "../utils"; interface HTMLElementWithFormOpId extends HTMLElement { formOpId: string; @@ -86,12 +90,11 @@ async function loadNotificationBar() { ]); const changePasswordButtonContainsNames = new Set(["pass", "change", "contras", "senha"]); - // These are preferences for whether to show the notification bar based on the user's settings - // and they are set in the Settings > Options page in the browser extension. - let disabledAddLoginNotification = false; - let disabledChangedPasswordNotification = false; + const enableChangedPasswordPrompt = await sendExtensionMessage( + "bgGetEnableChangedPasswordPrompt", + ); + const enableAddedLoginPrompt = await sendExtensionMessage("bgGetEnableAddedLoginPrompt"); let showNotificationBar = true; - // Look up the active user id from storage const activeUserIdKey = "activeUserId"; const globalStorageKey = "global"; @@ -121,11 +124,7 @@ async function loadNotificationBar() { // Example: '{"bitwarden.com":null}' const excludedDomainsDict = globalSettings.neverDomains; if (!excludedDomainsDict || !(window.location.hostname in excludedDomainsDict)) { - // Set local disabled preferences - disabledAddLoginNotification = globalSettings.disableAddLoginNotification; - disabledChangedPasswordNotification = globalSettings.disableChangedPasswordNotification; - - if (!disabledAddLoginNotification || !disabledChangedPasswordNotification) { + if (enableAddedLoginPrompt || enableChangedPasswordPrompt) { // If the user has not disabled both notifications, then handle the initial page change (null -> actual page) handlePageChange(); } @@ -352,9 +351,7 @@ async function loadNotificationBar() { // to avoid missing any forms that are added after the page loads observeDom(); - sendPlatformMessage({ - command: "checkNotificationQueue", - }); + void sendExtensionMessage("checkNotificationQueue"); } // This is a safeguard in case the observer misses a SPA page change. @@ -392,10 +389,7 @@ async function loadNotificationBar() { * * */ function collectPageDetails() { - sendPlatformMessage({ - command: "bgCollectPageDetails", - sender: "notificationBar", - }); + void sendExtensionMessage("bgCollectPageDetails", { sender: "notificationBar" }); } // End Page Detail Collection Methods @@ -620,10 +614,9 @@ async function loadNotificationBar() { continue; } - const disabledBoth = disabledChangedPasswordNotification && disabledAddLoginNotification; - // if user has not disabled both notifications and we have a username and password field, + // if user has enabled either add login or change password notification, and we have a username and password field if ( - !disabledBoth && + (enableChangedPasswordPrompt || enableAddedLoginPrompt) && watchedForms[i].usernameEl != null && watchedForms[i].passwordEl != null ) { @@ -639,10 +632,7 @@ async function loadNotificationBar() { const passwordPopulated = login.password != null && login.password !== ""; if (userNamePopulated && passwordPopulated) { processedForm(form); - sendPlatformMessage({ - command: "bgAddLogin", - login, - }); + void sendExtensionMessage("bgAddLogin", { login }); break; } else if ( userNamePopulated && @@ -659,7 +649,7 @@ async function loadNotificationBar() { // if user has not disabled the password changed notification and we have multiple password fields, // then check if the user has changed their password - if (!disabledChangedPasswordNotification && watchedForms[i].passwordEls != null) { + if (enableChangedPasswordPrompt && watchedForms[i].passwordEls != null) { // Get the values of the password fields const passwords: string[] = watchedForms[i].passwordEls .filter((el: HTMLInputElement) => el.value != null && el.value !== "") @@ -716,7 +706,7 @@ async function loadNotificationBar() { currentPassword: curPass, url: document.URL, }; - sendPlatformMessage({ command: "bgChangedPassword", data }); + void sendExtensionMessage("bgChangedPassword", { data }); break; } } @@ -954,9 +944,7 @@ async function loadNotificationBar() { switch (barType) { case "add": case "change": - sendPlatformMessage({ - command: "bgRemoveTabFromNotificationQueue", - }); + void sendExtensionMessage("bgRemoveTabFromNotificationQueue"); break; default: break; @@ -981,12 +969,6 @@ async function loadNotificationBar() { // End Notification Bar Functions (open, close, height adjustment, etc.) // Helper Functions - function sendPlatformMessage(msg: any) { - // 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 - chrome.runtime.sendMessage(msg); - } - function isInIframe() { try { return window.self !== window.top; diff --git a/apps/browser/src/autofill/services/autofill.service.spec.ts b/apps/browser/src/autofill/services/autofill.service.spec.ts index 3fd0bf54342..055e92a31c4 100644 --- a/apps/browser/src/autofill/services/autofill.service.spec.ts +++ b/apps/browser/src/autofill/services/autofill.service.spec.ts @@ -166,11 +166,10 @@ describe("AutofillService", () => { jest .spyOn(autofillService, "getOverlayVisibility") .mockResolvedValue(AutofillOverlayVisibility.OnFieldFocus); + jest.spyOn(autofillService, "getAutofillOnPageLoad").mockResolvedValue(true); }); it("accepts an extension message sender and injects the autofill scripts into the tab of the sender", async () => { - jest.spyOn(autofillService, "getAutofillOnPageLoad").mockResolvedValue(true); - await autofillService.injectAutofillScripts(sender.tab, sender.frameId, true); [autofillOverlayBootstrapScript, ...defaultAutofillScripts].forEach((scriptName) => { @@ -195,11 +194,6 @@ describe("AutofillService", () => { }); it("will inject the bootstrap-autofill-overlay script if the user has the autofill overlay enabled", async () => { - jest - .spyOn(autofillService, "getOverlayVisibility") - .mockResolvedValue(AutofillOverlayVisibility.OnFieldFocus); - jest.spyOn(autofillService, "getAutofillOnPageLoad").mockResolvedValue(true); - await autofillService.injectAutofillScripts(sender.tab, sender.frameId); expect(BrowserApi.executeScriptInTab).toHaveBeenCalledWith(tabMock.id, { @@ -218,7 +212,6 @@ describe("AutofillService", () => { jest .spyOn(autofillService, "getOverlayVisibility") .mockResolvedValue(AutofillOverlayVisibility.Off); - jest.spyOn(autofillService, "getAutofillOnPageLoad").mockResolvedValue(true); await autofillService.injectAutofillScripts(sender.tab, sender.frameId); @@ -235,8 +228,6 @@ describe("AutofillService", () => { }); it("injects the content-message-handler script if not injecting on page load", async () => { - jest.spyOn(autofillService, "getAutofillOnPageLoad").mockResolvedValue(true); - await autofillService.injectAutofillScripts(sender.tab, sender.frameId, false); expect(BrowserApi.executeScriptInTab).toHaveBeenCalledWith(tabMock.id, { diff --git a/apps/browser/src/autofill/types/index.ts b/apps/browser/src/autofill/types/index.ts index b3b803b7ab4..d9ae0b16f47 100644 --- a/apps/browser/src/autofill/types/index.ts +++ b/apps/browser/src/autofill/types/index.ts @@ -39,10 +39,7 @@ export type UserSettings = { vaultTimeoutAction: VaultTimeoutAction; }; -export type GlobalSettings = Pick< - GlobalState, - "disableAddLoginNotification" | "disableChangedPasswordNotification" | "neverDomains" ->; +export type GlobalSettings = Pick<GlobalState, "neverDomains">; /** * A HTMLElement (usually a form element) with additional custom properties added by this script diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index 9e7fc3f08b7..b214ba0d390 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -55,6 +55,10 @@ import { BadgeSettingsServiceAbstraction, BadgeSettingsService, } from "@bitwarden/common/autofill/services/badge-settings.service"; +import { + UserNotificationSettingsService, + UserNotificationSettingsServiceAbstraction, +} from "@bitwarden/common/autofill/services/user-notification-settings.service"; import { AppIdService as AppIdServiceAbstraction } from "@bitwarden/common/platform/abstractions/app-id.service"; import { ConfigApiServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config-api.service.abstraction"; import { CryptoFunctionService as CryptoFunctionServiceAbstraction } from "@bitwarden/common/platform/abstractions/crypto-function.service"; @@ -248,6 +252,7 @@ export default class MainBackground { searchService: SearchServiceAbstraction; notificationsService: NotificationsServiceAbstraction; stateService: StateServiceAbstraction; + userNotificationSettingsService: UserNotificationSettingsServiceAbstraction; autofillSettingsService: AutofillSettingsServiceAbstraction; badgeSettingsService: BadgeSettingsServiceAbstraction; systemService: SystemServiceAbstraction; @@ -419,6 +424,7 @@ export default class MainBackground { this.environmentService, migrationRunner, ); + this.userNotificationSettingsService = new UserNotificationSettingsService(this.stateProvider); this.platformUtilsService = new BrowserPlatformUtilsService( this.messagingService, (clipboardValue, clearMs) => { @@ -846,6 +852,7 @@ export default class MainBackground { this.policyService, this.folderService, this.stateService, + this.userNotificationSettingsService, this.environmentService, this.logService, ); @@ -1088,10 +1095,12 @@ export default class MainBackground { this.keyConnectorService.clear(), this.vaultFilterService.clear(), this.biometricStateService.logout(userId), - /* We intentionally do not clear: - * - autofillSettingsService - * - badgeSettingsService - */ + /* + We intentionally do not clear: + - autofillSettingsService + - badgeSettingsService + - userNotificationSettingsService + */ ]); //Needs to be checked before state is cleaned diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 35849a98e1d..dd081b7877f 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -42,6 +42,10 @@ import { AutofillSettingsService, AutofillSettingsServiceAbstraction, } from "@bitwarden/common/autofill/services/autofill-settings.service"; +import { + UserNotificationSettingsService, + UserNotificationSettingsServiceAbstraction, +} from "@bitwarden/common/autofill/services/user-notification-settings.service"; import { AppIdService } from "@bitwarden/common/platform/abstractions/app-id.service"; import { ConfigApiServiceAbstraction } from "@bitwarden/common/platform/abstractions/config/config-api.service.abstraction"; import { CryptoFunctionService } from "@bitwarden/common/platform/abstractions/crypto-function.service"; @@ -551,6 +555,11 @@ function getBgService<T>(service: keyof MainBackground) { useClass: AutofillSettingsService, deps: [StateProvider, PolicyService], }, + { + provide: UserNotificationSettingsServiceAbstraction, + useClass: UserNotificationSettingsService, + deps: [StateProvider], + }, ], }) export class ServicesModule {} diff --git a/apps/browser/src/popup/settings/options.component.ts b/apps/browser/src/popup/settings/options.component.ts index 2a68fe73ac0..d798714b5f4 100644 --- a/apps/browser/src/popup/settings/options.component.ts +++ b/apps/browser/src/popup/settings/options.component.ts @@ -5,6 +5,7 @@ import { AbstractThemingService } from "@bitwarden/angular/platform/services/the import { SettingsService } from "@bitwarden/common/abstractions/settings.service"; import { AutofillSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/autofill-settings.service"; import { BadgeSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/badge-settings.service"; +import { UserNotificationSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/user-notification-settings.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; @@ -47,6 +48,7 @@ export class OptionsComponent implements OnInit { constructor( private messagingService: MessagingService, private stateService: StateService, + private userNotificationSettingsService: UserNotificationSettingsServiceAbstraction, private autofillSettingsService: AutofillSettingsServiceAbstraction, private badgeSettingsService: BadgeSettingsServiceAbstraction, i18nService: I18nService, @@ -95,10 +97,13 @@ export class OptionsComponent implements OnInit { this.autofillSettingsService.autofillOnPageLoadDefault$, ); - this.enableAddLoginNotification = !(await this.stateService.getDisableAddLoginNotification()); + this.enableAddLoginNotification = await firstValueFrom( + this.userNotificationSettingsService.enableAddedLoginPrompt$, + ); - this.enableChangedPasswordNotification = - !(await this.stateService.getDisableChangedPasswordNotification()); + this.enableChangedPasswordNotification = await firstValueFrom( + this.userNotificationSettingsService.enableChangedPasswordPrompt$, + ); this.enableContextMenuItem = !(await this.stateService.getDisableContextMenuItem()); @@ -122,12 +127,14 @@ export class OptionsComponent implements OnInit { } async updateAddLoginNotification() { - await this.stateService.setDisableAddLoginNotification(!this.enableAddLoginNotification); + await this.userNotificationSettingsService.setEnableAddedLoginPrompt( + this.enableAddLoginNotification, + ); } async updateChangedPasswordNotification() { - await this.stateService.setDisableChangedPasswordNotification( - !this.enableChangedPasswordNotification, + await this.userNotificationSettingsService.setEnableChangedPasswordPrompt( + this.enableChangedPasswordNotification, ); } diff --git a/libs/common/src/autofill/services/user-notification-settings.service.ts b/libs/common/src/autofill/services/user-notification-settings.service.ts new file mode 100644 index 00000000000..1a3a387df6b --- /dev/null +++ b/libs/common/src/autofill/services/user-notification-settings.service.ts @@ -0,0 +1,60 @@ +import { map, Observable } from "rxjs"; + +import { + USER_NOTIFICATION_SETTINGS_DISK, + GlobalState, + KeyDefinition, + StateProvider, +} from "../../platform/state"; + +const ENABLE_ADDED_LOGIN_PROMPT = new KeyDefinition( + USER_NOTIFICATION_SETTINGS_DISK, + "enableAddedLoginPrompt", + { + deserializer: (value: boolean) => value ?? true, + }, +); +const ENABLE_CHANGED_PASSWORD_PROMPT = new KeyDefinition( + USER_NOTIFICATION_SETTINGS_DISK, + "enableChangedPasswordPrompt", + { + deserializer: (value: boolean) => value ?? true, + }, +); + +export abstract class UserNotificationSettingsServiceAbstraction { + enableAddedLoginPrompt$: Observable<boolean>; + setEnableAddedLoginPrompt: (newValue: boolean) => Promise<void>; + enableChangedPasswordPrompt$: Observable<boolean>; + setEnableChangedPasswordPrompt: (newValue: boolean) => Promise<void>; +} + +export class UserNotificationSettingsService implements UserNotificationSettingsServiceAbstraction { + private enableAddedLoginPromptState: GlobalState<boolean>; + readonly enableAddedLoginPrompt$: Observable<boolean>; + + private enableChangedPasswordPromptState: GlobalState<boolean>; + readonly enableChangedPasswordPrompt$: Observable<boolean>; + + constructor(private stateProvider: StateProvider) { + this.enableAddedLoginPromptState = this.stateProvider.getGlobal(ENABLE_ADDED_LOGIN_PROMPT); + this.enableAddedLoginPrompt$ = this.enableAddedLoginPromptState.state$.pipe( + map((x) => x ?? true), + ); + + this.enableChangedPasswordPromptState = this.stateProvider.getGlobal( + ENABLE_CHANGED_PASSWORD_PROMPT, + ); + this.enableChangedPasswordPrompt$ = this.enableChangedPasswordPromptState.state$.pipe( + map((x) => x ?? true), + ); + } + + async setEnableAddedLoginPrompt(newValue: boolean): Promise<void> { + await this.enableAddedLoginPromptState.update(() => newValue); + } + + async setEnableChangedPasswordPrompt(newValue: boolean): Promise<void> { + await this.enableChangedPasswordPromptState.update(() => newValue); + } +} diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index 7f079be45f8..338e3b6df3d 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -200,13 +200,6 @@ export abstract class StateService<T extends Account = Account> { setDecryptedSends: (value: SendView[], options?: StorageOptions) => Promise<void>; getDefaultUriMatch: (options?: StorageOptions) => Promise<UriMatchType>; setDefaultUriMatch: (value: UriMatchType, options?: StorageOptions) => Promise<void>; - getDisableAddLoginNotification: (options?: StorageOptions) => Promise<boolean>; - setDisableAddLoginNotification: (value: boolean, options?: StorageOptions) => Promise<void>; - getDisableChangedPasswordNotification: (options?: StorageOptions) => Promise<boolean>; - setDisableChangedPasswordNotification: ( - value: boolean, - options?: StorageOptions, - ) => Promise<void>; getDisableContextMenuItem: (options?: StorageOptions) => Promise<boolean>; setDisableContextMenuItem: (value: boolean, options?: StorageOptions) => Promise<void>; /** diff --git a/libs/common/src/platform/models/domain/global-state.ts b/libs/common/src/platform/models/domain/global-state.ts index 3b7799ee5da..d7c1d93d035 100644 --- a/libs/common/src/platform/models/domain/global-state.ts +++ b/libs/common/src/platform/models/domain/global-state.ts @@ -26,8 +26,6 @@ export class GlobalState { enableBrowserIntegrationFingerprint?: boolean; enableDuckDuckGoBrowserIntegration?: boolean; neverDomains?: { [id: string]: unknown }; - disableAddLoginNotification?: boolean; - disableChangedPasswordNotification?: boolean; disableContextMenuItem?: boolean; deepLinkRedirectUrl?: string; } diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index cb3b3c8c870..22a3c884f15 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -853,45 +853,6 @@ export class StateService< ); } - async getDisableAddLoginNotification(options?: StorageOptions): Promise<boolean> { - return ( - (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) - ?.disableAddLoginNotification ?? false - ); - } - - async setDisableAddLoginNotification(value: boolean, options?: StorageOptions): Promise<void> { - const globals = await this.getGlobals( - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - globals.disableAddLoginNotification = value; - await this.saveGlobals( - globals, - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - } - - async getDisableChangedPasswordNotification(options?: StorageOptions): Promise<boolean> { - return ( - (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) - ?.disableChangedPasswordNotification ?? false - ); - } - - async setDisableChangedPasswordNotification( - value: boolean, - options?: StorageOptions, - ): Promise<void> { - const globals = await this.getGlobals( - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - globals.disableChangedPasswordNotification = value; - await this.saveGlobals( - globals, - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - } - async getDisableContextMenuItem(options?: StorageOptions): Promise<boolean> { return ( (await this.getGlobals(this.reconcileOptions(options, await this.defaultOnDiskOptions()))) diff --git a/libs/common/src/platform/state/state-definitions.ts b/libs/common/src/platform/state/state-definitions.ts index c5be07023e8..fe419f00cbb 100644 --- a/libs/common/src/platform/state/state-definitions.ts +++ b/libs/common/src/platform/state/state-definitions.ts @@ -63,6 +63,10 @@ export const VAULT_FILTER_DISK = new StateDefinition("vaultFilter", "disk", { web: "disk-local", }); +export const USER_NOTIFICATION_SETTINGS_DISK = new StateDefinition( + "userNotificationSettings", + "disk", +); export const CLEAR_EVENT_DISK = new StateDefinition("clearEvent", "disk"); export const NEW_WEB_LAYOUT_BANNER_DISK = new StateDefinition("newWebLayoutBanner", "disk", { diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index 7ed50c4206d..3b217135394 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/libs/common/src/state-migrations/migrate.ts @@ -23,6 +23,7 @@ import { ClearClipboardDelayMigrator } from "./migrations/25-move-clear-clipboar import { RevertLastSyncMigrator } from "./migrations/26-revert-move-last-sync-to-state-provider"; import { BadgeSettingsMigrator } from "./migrations/27-move-badge-settings-to-state-providers"; import { MoveBiometricUnlockToStateProviders } from "./migrations/28-move-biometric-unlock-to-state-providers"; +import { UserNotificationSettingsKeyMigrator } from "./migrations/29-move-user-notification-settings-to-state-provider"; import { FixPremiumMigrator } from "./migrations/3-fix-premium"; import { RemoveEverBeenUnlockedMigrator } from "./migrations/4-remove-ever-been-unlocked"; import { AddKeyTypeToOrgKeysMigrator } from "./migrations/5-add-key-type-to-org-keys"; @@ -33,7 +34,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting import { MinVersionMigrator } from "./migrations/min-version"; export const MIN_VERSION = 2; -export const CURRENT_VERSION = 28; +export const CURRENT_VERSION = 29; export type MinVersion = typeof MIN_VERSION; export function createMigrationBuilder() { @@ -64,7 +65,8 @@ export function createMigrationBuilder() { .with(ClearClipboardDelayMigrator, 24, 25) .with(RevertLastSyncMigrator, 25, 26) .with(BadgeSettingsMigrator, 26, 27) - .with(MoveBiometricUnlockToStateProviders, 27, CURRENT_VERSION); + .with(MoveBiometricUnlockToStateProviders, 27, 28) + .with(UserNotificationSettingsKeyMigrator, 28, CURRENT_VERSION); } export async function currentVersion( diff --git a/libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.spec.ts b/libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.spec.ts new file mode 100644 index 00000000000..3dbf68b35b8 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.spec.ts @@ -0,0 +1,102 @@ +import { MockProxy } from "jest-mock-extended"; + +import { StateDefinitionLike, MigrationHelper } from "../migration-helper"; +import { mockMigrationHelper } from "../migration-helper.spec"; + +import { UserNotificationSettingsKeyMigrator } from "./29-move-user-notification-settings-to-state-provider"; + +function exampleJSON() { + return { + global: { + disableAddLoginNotification: false, + disableChangedPasswordNotification: false, + otherStuff: "otherStuff1", + }, + }; +} + +function rollbackJSON() { + return { + global_userNotificationSettings_enableAddedLoginPrompt: true, + global_userNotificationSettings_enableChangedPasswordPrompt: true, + global: { + otherStuff: "otherStuff1", + }, + }; +} + +const userNotificationSettingsLocalStateDefinition: { + stateDefinition: StateDefinitionLike; +} = { + stateDefinition: { + name: "userNotificationSettings", + }, +}; + +describe("ProviderKeysMigrator", () => { + let helper: MockProxy<MigrationHelper>; + let sut: UserNotificationSettingsKeyMigrator; + + describe("migrate", () => { + beforeEach(() => { + helper = mockMigrationHelper(exampleJSON(), 28); + sut = new UserNotificationSettingsKeyMigrator(28, 29); + }); + + it("should remove disableAddLoginNotification and disableChangedPasswordNotification global setting", async () => { + await sut.migrate(helper); + expect(helper.set).toHaveBeenCalledTimes(2); + expect(helper.set).toHaveBeenCalledWith("global", { otherStuff: "otherStuff1" }); + expect(helper.set).toHaveBeenCalledWith("global", { otherStuff: "otherStuff1" }); + }); + + it("should set global user notification setting values", async () => { + await sut.migrate(helper); + + expect(helper.setToGlobal).toHaveBeenCalledTimes(2); + expect(helper.setToGlobal).toHaveBeenCalledWith( + { ...userNotificationSettingsLocalStateDefinition, key: "enableAddedLoginPrompt" }, + true, + ); + expect(helper.setToGlobal).toHaveBeenCalledWith( + { ...userNotificationSettingsLocalStateDefinition, key: "enableChangedPasswordPrompt" }, + true, + ); + }); + }); + + describe("rollback", () => { + beforeEach(() => { + helper = mockMigrationHelper(rollbackJSON(), 29); + sut = new UserNotificationSettingsKeyMigrator(28, 29); + }); + + it("should null out new global values", async () => { + await sut.rollback(helper); + + expect(helper.setToGlobal).toHaveBeenCalledTimes(2); + expect(helper.setToGlobal).toHaveBeenCalledWith( + { ...userNotificationSettingsLocalStateDefinition, key: "enableAddedLoginPrompt" }, + null, + ); + expect(helper.setToGlobal).toHaveBeenCalledWith( + { ...userNotificationSettingsLocalStateDefinition, key: "enableChangedPasswordPrompt" }, + null, + ); + }); + + it("should add explicit global values back", async () => { + await sut.rollback(helper); + + expect(helper.set).toHaveBeenCalledTimes(2); + expect(helper.set).toHaveBeenCalledWith("global", { + disableAddLoginNotification: false, + otherStuff: "otherStuff1", + }); + expect(helper.set).toHaveBeenCalledWith("global", { + disableChangedPasswordNotification: false, + otherStuff: "otherStuff1", + }); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.ts b/libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.ts new file mode 100644 index 00000000000..6c26882e66a --- /dev/null +++ b/libs/common/src/state-migrations/migrations/29-move-user-notification-settings-to-state-provider.ts @@ -0,0 +1,105 @@ +import { MigrationHelper } from "../migration-helper"; +import { Migrator } from "../migrator"; + +type ExpectedGlobalState = { + disableAddLoginNotification?: boolean; + disableChangedPasswordNotification?: boolean; +}; + +export class UserNotificationSettingsKeyMigrator extends Migrator<28, 29> { + async migrate(helper: MigrationHelper): Promise<void> { + const globalState = await helper.get<ExpectedGlobalState>("global"); + + // disableAddLoginNotification -> enableAddedLoginPrompt + if (globalState?.disableAddLoginNotification != null) { + await helper.setToGlobal( + { + stateDefinition: { + name: "userNotificationSettings", + }, + key: "enableAddedLoginPrompt", + }, + !globalState.disableAddLoginNotification, + ); + + // delete `disableAddLoginNotification` from state global + delete globalState.disableAddLoginNotification; + + await helper.set<ExpectedGlobalState>("global", globalState); + } + + // disableChangedPasswordNotification -> enableChangedPasswordPrompt + if (globalState?.disableChangedPasswordNotification != null) { + await helper.setToGlobal( + { + stateDefinition: { + name: "userNotificationSettings", + }, + key: "enableChangedPasswordPrompt", + }, + !globalState.disableChangedPasswordNotification, + ); + + // delete `disableChangedPasswordNotification` from state global + delete globalState.disableChangedPasswordNotification; + + await helper.set<ExpectedGlobalState>("global", globalState); + } + } + + async rollback(helper: MigrationHelper): Promise<void> { + const globalState = (await helper.get<ExpectedGlobalState>("global")) || {}; + + const enableAddedLoginPrompt: boolean = await helper.getFromGlobal({ + stateDefinition: { + name: "userNotificationSettings", + }, + key: "enableAddedLoginPrompt", + }); + + const enableChangedPasswordPrompt: boolean = await helper.getFromGlobal({ + stateDefinition: { + name: "userNotificationSettings", + }, + key: "enableChangedPasswordPrompt", + }); + + // enableAddedLoginPrompt -> disableAddLoginNotification + if (enableAddedLoginPrompt) { + await helper.set<ExpectedGlobalState>("global", { + ...globalState, + disableAddLoginNotification: !enableAddedLoginPrompt, + }); + + // remove the global state provider framework key for `enableAddedLoginPrompt` + await helper.setToGlobal( + { + stateDefinition: { + name: "userNotificationSettings", + }, + key: "enableAddedLoginPrompt", + }, + null, + ); + } + + // enableChangedPasswordPrompt -> disableChangedPasswordNotification + if (enableChangedPasswordPrompt) { + await helper.set<ExpectedGlobalState>("global", { + ...globalState, + disableChangedPasswordNotification: !enableChangedPasswordPrompt, + }); + + // remove the global state provider framework key for `enableChangedPasswordPrompt` + await helper.setToGlobal( + { + stateDefinition: { + name: "userNotificationSettings", + }, + key: "enableChangedPasswordPrompt", + }, + null, + ); + } + } +} From c3eba7f2c83c2156052ade0275208665378ef47a Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 4 Mar 2024 14:33:25 -0600 Subject: [PATCH 03/14] [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 --- .../browser/src/background/main.background.ts | 33 ++++++-- .../vault-timeout-service.factory.ts | 8 +- .../active-user-state-provider.factory.ts | 20 +++-- .../global-state-provider.factory.ts | 17 ++-- .../single-user-state-provider.factory.ts | 20 +++-- .../state-event-runner-service.factory.ts | 33 ++++++++ apps/cli/src/bw.ts | 32 +++++-- apps/desktop/src/app/app.component.ts | 18 ++-- apps/desktop/src/main.ts | 18 ++-- apps/web/src/app/app.component.ts | 4 + apps/web/src/app/core/core.module.ts | 35 ++------ .../web-active-user-state.provider.ts | 44 ---------- .../app/platform/web-global-state.provider.ts | 42 ---------- .../web-single-user-state.provider.ts | 43 ---------- .../src/services/jslib-services.module.ts | 25 +++++- libs/common/spec/fake-state.ts | 1 + .../services/environment.service.spec.ts | 16 ++-- ...default-active-user-state.provider.spec.ts | 17 ++-- .../default-active-user-state.provider.ts | 55 ++++-------- .../default-active-user-state.spec.ts | 51 +++++++++++- .../default-active-user-state.ts | 7 ++ .../default-global-state.provider.ts | 41 +++------ .../default-single-user-state.provider.ts | 57 +++++-------- .../default-single-user-state.spec.ts | 53 +++++++++++- .../default-single-user-state.ts | 7 ++ .../specific-state.provider.spec.ts | 24 ++++-- .../vault-timeout.service.spec.ts | 83 ++++++++++++++++++- .../vault-timeout/vault-timeout.service.ts | 12 ++- 28 files changed, 471 insertions(+), 345 deletions(-) create mode 100644 apps/browser/src/platform/background/service-factories/state-event-runner-service.factory.ts delete mode 100644 apps/web/src/app/platform/web-active-user-state.provider.ts delete mode 100644 apps/web/src/app/platform/web-global-state.provider.ts delete mode 100644 apps/web/src/app/platform/web-single-user-state.provider.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index b214ba0d390..ffdd2e30898 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -93,6 +93,7 @@ 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 { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { @@ -100,6 +101,7 @@ import { DerivedStateProvider, GlobalStateProvider, SingleUserStateProvider, + StateEventRunnerService, StateProvider, } from "@bitwarden/common/platform/state"; /* eslint-disable import/no-restricted-paths -- We need the implementation to inject, but generally these should not be accessed */ @@ -107,6 +109,7 @@ import { DefaultActiveUserStateProvider } from "@bitwarden/common/platform/state import { DefaultGlobalStateProvider } from "@bitwarden/common/platform/state/implementations/default-global-state.provider"; import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-single-user-state.provider"; import { DefaultStateProvider } from "@bitwarden/common/platform/state/implementations/default-state.provider"; +import { StateEventRegistrarService } from "@bitwarden/common/platform/state/state-event-registrar.service"; /* eslint-enable import/no-restricted-paths */ import { AvatarUpdateService } from "@bitwarden/common/services/account/avatar-update.service"; import { ApiService } from "@bitwarden/common/services/api.service"; @@ -299,6 +302,7 @@ export default class MainBackground { organizationVaultExportService: OrganizationVaultExportServiceAbstraction; vaultSettingsService: VaultSettingsServiceAbstraction; biometricStateService: BiometricStateService; + stateEventRunnerService: StateEventRunnerService; ssoLoginService: SsoLoginServiceAbstraction; // Passed to the popup for Safari to workaround issues with theming, downloading, etc. @@ -366,10 +370,24 @@ export default class MainBackground { this.keyGenerationService, ) : new BackgroundMemoryStorageService(); - this.globalStateProvider = new DefaultGlobalStateProvider( - this.memoryStorageForStateProviders, + + const storageServiceProvider = new StorageServiceProvider( this.storageService as BrowserLocalStorageService, + this.memoryStorageForStateProviders, ); + + this.globalStateProvider = new DefaultGlobalStateProvider(storageServiceProvider); + + const stateEventRegistrarService = new StateEventRegistrarService( + this.globalStateProvider, + storageServiceProvider, + ); + + this.stateEventRunnerService = new StateEventRunnerService( + this.globalStateProvider, + storageServiceProvider, + ); + this.encryptService = flagEnabled("multithreadDecryption") ? new MultithreadEncryptServiceImplementation( this.cryptoFunctionService, @@ -379,8 +397,8 @@ export default class MainBackground { : new EncryptServiceImplementation(this.cryptoFunctionService, this.logService, true); this.singleUserStateProvider = new DefaultSingleUserStateProvider( - this.memoryStorageForStateProviders, - this.storageService as BrowserLocalStorageService, + storageServiceProvider, + stateEventRegistrarService, ); this.accountService = new AccountServiceImplementation( this.messagingService, @@ -389,8 +407,8 @@ export default class MainBackground { ); this.activeUserStateProvider = new DefaultActiveUserStateProvider( this.accountService, - this.memoryStorageForStateProviders, - this.storageService as BrowserLocalStorageService, + storageServiceProvider, + stateEventRegistrarService, ); this.derivedStateProvider = new BackgroundDerivedStateProvider( this.memoryStorageForStateProviders, @@ -666,6 +684,7 @@ export default class MainBackground { this.stateService, this.authService, this.vaultTimeoutSettingsService, + this.stateEventRunnerService, lockedCallback, logoutCallback, ); @@ -1113,6 +1132,8 @@ export default class MainBackground { this.searchService.clearIndex(); } + await this.stateEventRunnerService.handleEvent("logout", currentUserId as UserId); + if (newActiveUser != null) { // we have a new active user, do not continue tearing down application await this.switchAccount(newActiveUser as UserId); diff --git a/apps/browser/src/background/service-factories/vault-timeout-service.factory.ts b/apps/browser/src/background/service-factories/vault-timeout-service.factory.ts index 7a34b6b4b14..0e4d1420da5 100644 --- a/apps/browser/src/background/service-factories/vault-timeout-service.factory.ts +++ b/apps/browser/src/background/service-factories/vault-timeout-service.factory.ts @@ -21,6 +21,10 @@ import { platformUtilsServiceFactory, PlatformUtilsServiceInitOptions, } from "../../platform/background/service-factories/platform-utils-service.factory"; +import { + stateEventRunnerServiceFactory, + StateEventRunnerServiceInitOptions, +} from "../../platform/background/service-factories/state-event-runner-service.factory"; import { StateServiceInitOptions, stateServiceFactory, @@ -62,7 +66,8 @@ export type VaultTimeoutServiceInitOptions = VaultTimeoutServiceFactoryOptions & SearchServiceInitOptions & StateServiceInitOptions & AuthServiceInitOptions & - VaultTimeoutSettingsServiceInitOptions; + VaultTimeoutSettingsServiceInitOptions & + StateEventRunnerServiceInitOptions; export function vaultTimeoutServiceFactory( cache: { vaultTimeoutService?: AbstractVaultTimeoutService } & CachedServices, @@ -84,6 +89,7 @@ export function vaultTimeoutServiceFactory( await stateServiceFactory(cache, opts), await authServiceFactory(cache, opts), await vaultTimeoutSettingsServiceFactory(cache, opts), + await stateEventRunnerServiceFactory(cache, opts), opts.vaultTimeoutServiceOptions.lockedCallback, opts.vaultTimeoutServiceOptions.loggedOutCallback, ), diff --git a/apps/browser/src/platform/background/service-factories/active-user-state-provider.factory.ts b/apps/browser/src/platform/background/service-factories/active-user-state-provider.factory.ts index e2d8746c7ce..6dafd2952e0 100644 --- a/apps/browser/src/platform/background/service-factories/active-user-state-provider.factory.ts +++ b/apps/browser/src/platform/background/service-factories/active-user-state-provider.factory.ts @@ -9,18 +9,20 @@ import { import { CachedServices, FactoryOptions, factory } from "./factory-options"; import { - DiskStorageServiceInitOptions, - MemoryStorageServiceInitOptions, - observableDiskStorageServiceFactory, - observableMemoryStorageServiceFactory, -} from "./storage-service.factory"; + StateEventRegistrarServiceInitOptions, + stateEventRegistrarServiceFactory, +} from "./state-event-registrar-service.factory"; +import { + StorageServiceProviderInitOptions, + storageServiceProviderFactory, +} from "./storage-service-provider.factory"; type ActiveUserStateProviderFactory = FactoryOptions; export type ActiveUserStateProviderInitOptions = ActiveUserStateProviderFactory & AccountServiceInitOptions & - MemoryStorageServiceInitOptions & - DiskStorageServiceInitOptions; + StorageServiceProviderInitOptions & + StateEventRegistrarServiceInitOptions; export async function activeUserStateProviderFactory( cache: { activeUserStateProvider?: ActiveUserStateProvider } & CachedServices, @@ -33,8 +35,8 @@ export async function activeUserStateProviderFactory( async () => new DefaultActiveUserStateProvider( await accountServiceFactory(cache, opts), - await observableMemoryStorageServiceFactory(cache, opts), - await observableDiskStorageServiceFactory(cache, opts), + await storageServiceProviderFactory(cache, opts), + await stateEventRegistrarServiceFactory(cache, opts), ), ); } diff --git a/apps/browser/src/platform/background/service-factories/global-state-provider.factory.ts b/apps/browser/src/platform/background/service-factories/global-state-provider.factory.ts index 3615b2b060d..154c212f046 100644 --- a/apps/browser/src/platform/background/service-factories/global-state-provider.factory.ts +++ b/apps/browser/src/platform/background/service-factories/global-state-provider.factory.ts @@ -4,17 +4,14 @@ import { DefaultGlobalStateProvider } from "@bitwarden/common/platform/state/imp import { CachedServices, FactoryOptions, factory } from "./factory-options"; import { - DiskStorageServiceInitOptions, - MemoryStorageServiceInitOptions, - observableDiskStorageServiceFactory, - observableMemoryStorageServiceFactory, -} from "./storage-service.factory"; + StorageServiceProviderInitOptions, + storageServiceProviderFactory, +} from "./storage-service-provider.factory"; type GlobalStateProviderFactoryOptions = FactoryOptions; export type GlobalStateProviderInitOptions = GlobalStateProviderFactoryOptions & - MemoryStorageServiceInitOptions & - DiskStorageServiceInitOptions; + StorageServiceProviderInitOptions; export async function globalStateProviderFactory( cache: { globalStateProvider?: GlobalStateProvider } & CachedServices, @@ -24,10 +21,6 @@ export async function globalStateProviderFactory( cache, "globalStateProvider", opts, - async () => - new DefaultGlobalStateProvider( - await observableMemoryStorageServiceFactory(cache, opts), - await observableDiskStorageServiceFactory(cache, opts), - ), + async () => new DefaultGlobalStateProvider(await storageServiceProviderFactory(cache, opts)), ); } diff --git a/apps/browser/src/platform/background/service-factories/single-user-state-provider.factory.ts b/apps/browser/src/platform/background/service-factories/single-user-state-provider.factory.ts index 0af6f822148..87eaa8e95a5 100644 --- a/apps/browser/src/platform/background/service-factories/single-user-state-provider.factory.ts +++ b/apps/browser/src/platform/background/service-factories/single-user-state-provider.factory.ts @@ -4,17 +4,19 @@ import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state import { CachedServices, FactoryOptions, factory } from "./factory-options"; import { - DiskStorageServiceInitOptions, - MemoryStorageServiceInitOptions, - observableDiskStorageServiceFactory, - observableMemoryStorageServiceFactory, -} from "./storage-service.factory"; + StateEventRegistrarServiceInitOptions, + stateEventRegistrarServiceFactory, +} from "./state-event-registrar-service.factory"; +import { + StorageServiceProviderInitOptions, + storageServiceProviderFactory, +} from "./storage-service-provider.factory"; type SingleUserStateProviderFactoryOptions = FactoryOptions; export type SingleUserStateProviderInitOptions = SingleUserStateProviderFactoryOptions & - MemoryStorageServiceInitOptions & - DiskStorageServiceInitOptions; + StorageServiceProviderInitOptions & + StateEventRegistrarServiceInitOptions; export async function singleUserStateProviderFactory( cache: { singleUserStateProvider?: SingleUserStateProvider } & CachedServices, @@ -26,8 +28,8 @@ export async function singleUserStateProviderFactory( opts, async () => new DefaultSingleUserStateProvider( - await observableMemoryStorageServiceFactory(cache, opts), - await observableDiskStorageServiceFactory(cache, opts), + await storageServiceProviderFactory(cache, opts), + await stateEventRegistrarServiceFactory(cache, opts), ), ); } diff --git a/apps/browser/src/platform/background/service-factories/state-event-runner-service.factory.ts b/apps/browser/src/platform/background/service-factories/state-event-runner-service.factory.ts new file mode 100644 index 00000000000..c81384cd4ac --- /dev/null +++ b/apps/browser/src/platform/background/service-factories/state-event-runner-service.factory.ts @@ -0,0 +1,33 @@ +import { StateEventRunnerService } from "@bitwarden/common/platform/state"; + +import { CachedServices, FactoryOptions, factory } from "./factory-options"; +import { + GlobalStateProviderInitOptions, + globalStateProviderFactory, +} from "./global-state-provider.factory"; +import { + StorageServiceProviderInitOptions, + storageServiceProviderFactory, +} from "./storage-service-provider.factory"; + +type StateEventRunnerServiceFactoryOptions = FactoryOptions; + +export type StateEventRunnerServiceInitOptions = StateEventRunnerServiceFactoryOptions & + GlobalStateProviderInitOptions & + StorageServiceProviderInitOptions; + +export function stateEventRunnerServiceFactory( + cache: { stateEventRunnerService?: StateEventRunnerService } & CachedServices, + opts: StateEventRunnerServiceInitOptions, +): Promise<StateEventRunnerService> { + return factory( + cache, + "stateEventRunnerService", + opts, + async () => + new StateEventRunnerService( + await globalStateProviderFactory(cache, opts), + await storageServiceProviderFactory(cache, opts), + ), + ); +} diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index 67c1be16f84..7f76bee69af 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -60,11 +60,13 @@ import { MigrationBuilderService } from "@bitwarden/common/platform/services/mig import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { NoopMessagingService } from "@bitwarden/common/platform/services/noop-messaging.service"; import { StateService } from "@bitwarden/common/platform/services/state.service"; +import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; import { ActiveUserStateProvider, DerivedStateProvider, GlobalStateProvider, SingleUserStateProvider, + StateEventRunnerService, StateProvider, } from "@bitwarden/common/platform/state"; /* eslint-disable import/no-restricted-paths -- We need the implementation to inject, but generally these should not be accessed */ @@ -73,6 +75,7 @@ import { DefaultDerivedStateProvider } from "@bitwarden/common/platform/state/im import { DefaultGlobalStateProvider } from "@bitwarden/common/platform/state/implementations/default-global-state.provider"; import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-single-user-state.provider"; import { DefaultStateProvider } from "@bitwarden/common/platform/state/implementations/default-state.provider"; +import { StateEventRegistrarService } from "@bitwarden/common/platform/state/state-event-registrar.service"; import { MemoryStorageService as MemoryStorageServiceForStateProviders } from "@bitwarden/common/platform/state/storage/memory-storage.service"; /* eslint-enable import/no-restricted-paths */ import { AuditService } from "@bitwarden/common/services/audit.service"; @@ -208,6 +211,7 @@ export class Main { derivedStateProvider: DerivedStateProvider; stateProvider: StateProvider; loginStrategyService: LoginStrategyServiceAbstraction; + stateEventRunnerService: StateEventRunnerService; biometricStateService: BiometricStateService; constructor() { @@ -249,14 +253,26 @@ export class Main { this.memoryStorageService = new MemoryStorageService(); this.memoryStorageForStateProviders = new MemoryStorageServiceForStateProviders(); - this.globalStateProvider = new DefaultGlobalStateProvider( - this.memoryStorageForStateProviders, + const storageServiceProvider = new StorageServiceProvider( this.storageService, + this.memoryStorageForStateProviders, + ); + + this.globalStateProvider = new DefaultGlobalStateProvider(storageServiceProvider); + + const stateEventRegistrarService = new StateEventRegistrarService( + this.globalStateProvider, + storageServiceProvider, + ); + + this.stateEventRunnerService = new StateEventRunnerService( + this.globalStateProvider, + storageServiceProvider, ); this.singleUserStateProvider = new DefaultSingleUserStateProvider( - this.memoryStorageForStateProviders, - this.storageService, + storageServiceProvider, + stateEventRegistrarService, ); this.messagingService = new NoopMessagingService(); @@ -269,8 +285,8 @@ export class Main { this.activeUserStateProvider = new DefaultActiveUserStateProvider( this.accountService, - this.memoryStorageForStateProviders, - this.storageService, + storageServiceProvider, + stateEventRegistrarService, ); this.derivedStateProvider = new DefaultDerivedStateProvider( @@ -530,6 +546,7 @@ export class Main { this.stateService, this.authService, this.vaultTimeoutSettingsService, + this.stateEventRunnerService, lockedCallback, null, ); @@ -639,6 +656,9 @@ export class Main { this.policyService.clear(userId), this.passwordGenerationService.clear(), ]); + + await this.stateEventRunnerService.handleEvent("logout", userId as UserId); + await this.stateService.clean(); process.env.BW_SESSION = null; } diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index a42f80fd6f5..51bde2ef1c4 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -39,6 +39,7 @@ import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/pl import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { SystemService } from "@bitwarden/common/platform/abstractions/system.service"; import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; +import { StateEventRunnerService } from "@bitwarden/common/platform/state"; import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password"; import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; @@ -149,6 +150,7 @@ export class AppComponent implements OnInit, OnDestroy { private configService: ConfigServiceAbstraction, private dialogService: DialogService, private biometricStateService: BiometricStateService, + private stateEventRunnerService: StateEventRunnerService, ) {} ngOnInit() { @@ -219,13 +221,13 @@ export class AppComponent implements OnInit, OnDestroy { const currentUser = await this.stateService.getUserId(); const accounts = await firstValueFrom(this.stateService.accounts$); await this.vaultTimeoutService.lock(currentUser); - // 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 - Promise.all( - Object.keys(accounts) - .filter((u) => u !== currentUser) - .map((u) => this.vaultTimeoutService.lock(u)), - ); + for (const account of Object.keys(accounts)) { + if (account === currentUser) { + continue; + } + + await this.vaultTimeoutService.lock(account); + } break; } case "locked": @@ -583,6 +585,8 @@ export class AppComponent implements OnInit, OnDestroy { await this.keyConnectorService.clear(); await this.biometricStateService.logout(userBeingLoggedOut as UserId); + await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut as UserId); + preLogoutActiveUserId = this.activeUserId; await this.stateService.clean({ userId: userBeingLoggedOut }); } finally { diff --git a/apps/desktop/src/main.ts b/apps/desktop/src/main.ts index 87d85f8174d..dbcb85bacd8 100644 --- a/apps/desktop/src/main.ts +++ b/apps/desktop/src/main.ts @@ -13,11 +13,13 @@ import { MigrationBuilderService } from "@bitwarden/common/platform/services/mig import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { NoopMessagingService } from "@bitwarden/common/platform/services/noop-messaging.service"; /* eslint-disable import/no-restricted-paths -- We need the implementation to inject, but generally this should not be accessed */ +import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; import { DefaultActiveUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-active-user-state.provider"; import { DefaultDerivedStateProvider } from "@bitwarden/common/platform/state/implementations/default-derived-state.provider"; import { DefaultGlobalStateProvider } from "@bitwarden/common/platform/state/implementations/default-global-state.provider"; import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-single-user-state.provider"; import { DefaultStateProvider } from "@bitwarden/common/platform/state/implementations/default-state.provider"; +import { StateEventRegistrarService } from "@bitwarden/common/platform/state/state-event-registrar.service"; import { MemoryStorageService as MemoryStorageServiceForStateProviders } from "@bitwarden/common/platform/state/storage/memory-storage.service"; /* eslint-enable import/no-restricted-paths */ @@ -104,10 +106,11 @@ export class Main { this.storageService = new ElectronStorageService(app.getPath("userData"), storageDefaults); this.memoryStorageService = new MemoryStorageService(); this.memoryStorageForStateProviders = new MemoryStorageServiceForStateProviders(); - const globalStateProvider = new DefaultGlobalStateProvider( - this.memoryStorageForStateProviders, + const storageServiceProvider = new StorageServiceProvider( this.storageService, + this.memoryStorageForStateProviders, ); + const globalStateProvider = new DefaultGlobalStateProvider(storageServiceProvider); const accountService = new AccountServiceImplementation( new NoopMessagingService(), @@ -115,13 +118,18 @@ export class Main { globalStateProvider, ); + const stateEventRegistrarService = new StateEventRegistrarService( + globalStateProvider, + storageServiceProvider, + ); + const stateProvider = new DefaultStateProvider( new DefaultActiveUserStateProvider( accountService, - this.memoryStorageForStateProviders, - this.storageService, + storageServiceProvider, + stateEventRegistrarService, ), - new DefaultSingleUserStateProvider(this.memoryStorageForStateProviders, this.storageService), + new DefaultSingleUserStateProvider(storageServiceProvider, stateEventRegistrarService), globalStateProvider, new DefaultDerivedStateProvider(this.memoryStorageForStateProviders), ); diff --git a/apps/web/src/app/app.component.ts b/apps/web/src/app/app.component.ts index 77f6fd6f1ff..73721de7890 100644 --- a/apps/web/src/app/app.component.ts +++ b/apps/web/src/app/app.component.ts @@ -23,6 +23,7 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { StateService } from "@bitwarden/common/platform/abstractions/state.service"; import { BiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service"; +import { StateEventRunnerService } from "@bitwarden/common/platform/state"; import { PasswordGenerationServiceAbstraction } from "@bitwarden/common/tools/generator/password"; import { UserId } from "@bitwarden/common/types/guid"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; @@ -89,6 +90,7 @@ export class AppComponent implements OnDestroy, OnInit { private configService: ConfigServiceAbstraction, private dialogService: DialogService, private biometricStateService: BiometricStateService, + private stateEventRunnerService: StateEventRunnerService, private paymentMethodWarningService: PaymentMethodWarningService, private organizationService: OrganizationService, ) {} @@ -284,6 +286,8 @@ export class AppComponent implements OnDestroy, OnInit { this.paymentMethodWarningService.clear(), ]); + await this.stateEventRunnerService.handleEvent("logout", userId as UserId); + this.searchService.clearIndex(); this.authService.logOut(async () => { if (expired) { diff --git a/apps/web/src/app/core/core.module.ts b/apps/web/src/app/core/core.module.ts index ee6f53ed018..6aded4875eb 100644 --- a/apps/web/src/app/core/core.module.ts +++ b/apps/web/src/app/core/core.module.ts @@ -14,7 +14,6 @@ import { } from "@bitwarden/angular/services/injection-tokens"; import { JslibServicesModule } from "@bitwarden/angular/services/jslib-services.module"; import { ModalService as ModalServiceAbstraction } from "@bitwarden/angular/services/modal.service"; -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { LoginService as LoginServiceAbstraction } from "@bitwarden/common/auth/abstractions/login.service"; import { LoginService } from "@bitwarden/common/auth/services/login.service"; import { FileDownloadService } from "@bitwarden/common/platform/abstractions/file-download/file-download.service"; @@ -28,21 +27,16 @@ import { StateFactory } from "@bitwarden/common/platform/factories/state-factory 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 { - ActiveUserStateProvider, - GlobalStateProvider, - SingleUserStateProvider, -} from "@bitwarden/common/platform/state"; -// eslint-disable-next-line import/no-restricted-paths -- Implementation for memory storage +/* eslint-disable import/no-restricted-paths -- Implementation for memory storage */ +import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; import { MemoryStorageService as MemoryStorageServiceForStateProviders } from "@bitwarden/common/platform/state/storage/memory-storage.service"; +/* eslint-enable import/no-restricted-paths -- Implementation for memory storage */ import { PolicyListService } from "../admin-console/core/policy-list.service"; import { HtmlStorageService } from "../core/html-storage.service"; import { I18nService } from "../core/i18n.service"; -import { WebActiveUserStateProvider } from "../platform/web-active-user-state.provider"; -import { WebGlobalStateProvider } from "../platform/web-global-state.provider"; import { WebMigrationRunner } from "../platform/web-migration-runner"; -import { WebSingleUserStateProvider } from "../platform/web-single-user-state.provider"; +import { WebStorageServiceProvider } from "../platform/web-storage-service.provider"; import { WindowStorageService } from "../platform/window-storage.service"; import { CollectionAdminService } from "../vault/core/collection-admin.service"; @@ -124,24 +118,9 @@ import { WebPlatformUtilsService } from "./web-platform-utils.service"; useFactory: () => new WindowStorageService(window.localStorage), }, { - provide: SingleUserStateProvider, - useClass: WebSingleUserStateProvider, - deps: [OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE], - }, - { - provide: ActiveUserStateProvider, - useClass: WebActiveUserStateProvider, - deps: [ - AccountService, - OBSERVABLE_MEMORY_STORAGE, - OBSERVABLE_DISK_STORAGE, - OBSERVABLE_DISK_LOCAL_STORAGE, - ], - }, - { - provide: GlobalStateProvider, - useClass: WebGlobalStateProvider, - deps: [OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE], + provide: StorageServiceProvider, + useClass: WebStorageServiceProvider, + deps: [OBSERVABLE_DISK_STORAGE, OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_LOCAL_STORAGE], }, { provide: MigrationRunner, diff --git a/apps/web/src/app/platform/web-active-user-state.provider.ts b/apps/web/src/app/platform/web-active-user-state.provider.ts deleted file mode 100644 index e02b134603a..00000000000 --- a/apps/web/src/app/platform/web-active-user-state.provider.ts +++ /dev/null @@ -1,44 +0,0 @@ -import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; -import { - AbstractMemoryStorageService, - AbstractStorageService, - ObservableStorageService, -} from "@bitwarden/common/platform/abstractions/storage.service"; -import { UserKeyDefinition } from "@bitwarden/common/platform/state"; -/* eslint-disable import/no-restricted-paths -- Needed to extend class & in platform owned code */ -import { DefaultActiveUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-active-user-state.provider"; -import { StateDefinition } from "@bitwarden/common/platform/state/state-definition"; -/* eslint-enable import/no-restricted-paths */ - -export class WebActiveUserStateProvider extends DefaultActiveUserStateProvider { - constructor( - accountService: AccountService, - memoryStorage: AbstractMemoryStorageService & ObservableStorageService, - sessionStorage: AbstractStorageService & ObservableStorageService, - private readonly diskLocalStorage: AbstractStorageService & ObservableStorageService, - ) { - super(accountService, memoryStorage, sessionStorage); - } - - protected override getLocationString(keyDefinition: UserKeyDefinition<unknown>): string { - return ( - keyDefinition.stateDefinition.storageLocationOverrides["web"] ?? - keyDefinition.stateDefinition.defaultStorageLocation - ); - } - - protected override getLocation( - stateDefinition: StateDefinition, - ): AbstractStorageService & ObservableStorageService { - const location = - stateDefinition.storageLocationOverrides["web"] ?? stateDefinition.defaultStorageLocation; - switch (location) { - case "disk": - return this.diskStorage; - case "memory": - return this.memoryStorage; - case "disk-local": - return this.diskLocalStorage; - } - } -} diff --git a/apps/web/src/app/platform/web-global-state.provider.ts b/apps/web/src/app/platform/web-global-state.provider.ts deleted file mode 100644 index 07025864cce..00000000000 --- a/apps/web/src/app/platform/web-global-state.provider.ts +++ /dev/null @@ -1,42 +0,0 @@ -import { - AbstractMemoryStorageService, - AbstractStorageService, - ObservableStorageService, -} from "@bitwarden/common/platform/abstractions/storage.service"; -import { KeyDefinition } from "@bitwarden/common/platform/state"; -/* eslint-disable import/no-restricted-paths -- Needed to extend class & in platform owned code*/ -import { DefaultGlobalStateProvider } from "@bitwarden/common/platform/state/implementations/default-global-state.provider"; -import { StateDefinition } from "@bitwarden/common/platform/state/state-definition"; -/* eslint-enable import/no-restricted-paths */ - -export class WebGlobalStateProvider extends DefaultGlobalStateProvider { - constructor( - memoryStorage: AbstractMemoryStorageService & ObservableStorageService, - sessionStorage: AbstractStorageService & ObservableStorageService, - private readonly diskLocalStorage: AbstractStorageService & ObservableStorageService, - ) { - super(memoryStorage, sessionStorage); - } - - protected getLocationString(keyDefinition: KeyDefinition<unknown>): string { - return ( - keyDefinition.stateDefinition.storageLocationOverrides["web"] ?? - keyDefinition.stateDefinition.defaultStorageLocation - ); - } - - protected override getLocation( - stateDefinition: StateDefinition, - ): AbstractStorageService & ObservableStorageService { - const location = - stateDefinition.storageLocationOverrides["web"] ?? stateDefinition.defaultStorageLocation; - switch (location) { - case "disk": - return this.diskStorage; - case "memory": - return this.memoryStorage; - case "disk-local": - return this.diskLocalStorage; - } - } -} diff --git a/apps/web/src/app/platform/web-single-user-state.provider.ts b/apps/web/src/app/platform/web-single-user-state.provider.ts deleted file mode 100644 index 4e4a807c456..00000000000 --- a/apps/web/src/app/platform/web-single-user-state.provider.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { - AbstractMemoryStorageService, - AbstractStorageService, - ObservableStorageService, -} from "@bitwarden/common/platform/abstractions/storage.service"; -import { UserKeyDefinition } from "@bitwarden/common/platform/state"; -/* eslint-disable import/no-restricted-paths -- Needed to extend service & and in platform owned file */ -import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-single-user-state.provider"; -import { StateDefinition } from "@bitwarden/common/platform/state/state-definition"; -/* eslint-enable import/no-restricted-paths */ - -export class WebSingleUserStateProvider extends DefaultSingleUserStateProvider { - constructor( - memoryStorageService: AbstractMemoryStorageService & ObservableStorageService, - sessionStorageService: AbstractStorageService & ObservableStorageService, - private readonly diskLocalStorageService: AbstractStorageService & ObservableStorageService, - ) { - super(memoryStorageService, sessionStorageService); - } - - protected override getLocationString(keyDefinition: UserKeyDefinition<unknown>): string { - return ( - keyDefinition.stateDefinition.storageLocationOverrides["web"] ?? - keyDefinition.stateDefinition.defaultStorageLocation - ); - } - - protected override getLocation( - stateDefinition: StateDefinition, - ): AbstractStorageService & ObservableStorageService { - const location = - stateDefinition.storageLocationOverrides["web"] ?? stateDefinition.defaultStorageLocation; - - switch (location) { - case "disk": - return this.diskStorage; - case "memory": - return this.memoryStorage; - case "disk-local": - return this.diskLocalStorageService; - } - } -} diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index 3803e959115..daf39717583 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -135,6 +135,7 @@ import { MigrationBuilderService } from "@bitwarden/common/platform/services/mig import { MigrationRunner } from "@bitwarden/common/platform/services/migration-runner"; import { NoopNotificationsService } from "@bitwarden/common/platform/services/noop-notifications.service"; import { StateService } from "@bitwarden/common/platform/services/state.service"; +import { StorageServiceProvider } from "@bitwarden/common/platform/services/storage-service.provider"; import { ValidationService } from "@bitwarden/common/platform/services/validation.service"; import { WebCryptoFunctionService } from "@bitwarden/common/platform/services/web-crypto-function.service"; import { @@ -150,6 +151,8 @@ import { DefaultDerivedStateProvider } from "@bitwarden/common/platform/state/im import { DefaultGlobalStateProvider } from "@bitwarden/common/platform/state/implementations/default-global-state.provider"; import { DefaultSingleUserStateProvider } from "@bitwarden/common/platform/state/implementations/default-single-user-state.provider"; import { DefaultStateProvider } from "@bitwarden/common/platform/state/implementations/default-state.provider"; +import { StateEventRegistrarService } from "@bitwarden/common/platform/state/state-event-registrar.service"; +import { StateEventRunnerService } from "@bitwarden/common/platform/state/state-event-runner.service"; /* eslint-enable import/no-restricted-paths */ import { AvatarUpdateService } from "@bitwarden/common/services/account/avatar-update.service"; import { ApiService } from "@bitwarden/common/services/api.service"; @@ -551,6 +554,7 @@ import { ModalService } from "./modal.service"; StateServiceAbstraction, AuthServiceAbstraction, VaultTimeoutSettingsServiceAbstraction, + StateEventRunnerService, LOCKED_CALLBACK, LOGOUT_CALLBACK, ], @@ -890,20 +894,35 @@ import { ModalService } from "./modal.service"; LogService, ], }, + { + provide: StorageServiceProvider, + useClass: StorageServiceProvider, + deps: [OBSERVABLE_DISK_STORAGE, OBSERVABLE_MEMORY_STORAGE], + }, + { + provide: StateEventRegistrarService, + useClass: StateEventRegistrarService, + deps: [GlobalStateProvider, StorageServiceProvider], + }, + { + provide: StateEventRunnerService, + useClass: StateEventRunnerService, + deps: [GlobalStateProvider, StorageServiceProvider], + }, { provide: GlobalStateProvider, useClass: DefaultGlobalStateProvider, - deps: [OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_STORAGE], + deps: [StorageServiceProvider], }, { provide: ActiveUserStateProvider, useClass: DefaultActiveUserStateProvider, - deps: [AccountServiceAbstraction, OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_STORAGE], + deps: [AccountServiceAbstraction, StorageServiceProvider, StateEventRegistrarService], }, { provide: SingleUserStateProvider, useClass: DefaultSingleUserStateProvider, - deps: [OBSERVABLE_MEMORY_STORAGE, OBSERVABLE_DISK_STORAGE], + deps: [StorageServiceProvider, StateEventRegistrarService], }, { provide: DerivedStateProvider, diff --git a/libs/common/spec/fake-state.ts b/libs/common/spec/fake-state.ts index 6b03ef6ef80..0f2a09d9c1b 100644 --- a/libs/common/spec/fake-state.ts +++ b/libs/common/spec/fake-state.ts @@ -65,6 +65,7 @@ export class FakeGlobalState<T> implements GlobalState<T> { this.nextMock(newState); return newState; } + /** Tracks update values resolved by `FakeState.update` */ nextMock = jest.fn<void, [T]>(); diff --git a/libs/common/src/platform/services/environment.service.spec.ts b/libs/common/src/platform/services/environment.service.spec.ts index a474e469a27..c5548959945 100644 --- a/libs/common/src/platform/services/environment.service.spec.ts +++ b/libs/common/src/platform/services/environment.service.spec.ts @@ -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), ); diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts b/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts index d0fe0aa3f45..02300185492 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.provider.spec.ts @@ -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(() => { diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.provider.ts b/libs/common/src/platform/state/implementations/default-active-user-state.provider.ts index ecb7f0fa955..268b22e5197 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.provider.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.provider.ts @@ -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}`; } } diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts index c1b7efa3b2d..feb9530987b 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.spec.ts @@ -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", () => { diff --git a/libs/common/src/platform/state/implementations/default-active-user-state.ts b/libs/common/src/platform/state/implementations/default-active-user-state.ts index 82c705ddbd9..4e7f7a0ff67 100644 --- a/libs/common/src/platform/state/implementations/default-active-user-state.ts +++ b/libs/common/src/platform/state/implementations/default-active-user-state.ts @@ -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]; } diff --git a/libs/common/src/platform/state/implementations/default-global-state.provider.ts b/libs/common/src/platform/state/implementations/default-global-state.provider.ts index f1b03e36a77..42adf3b9766 100644 --- a/libs/common/src/platform/state/implementations/default-global-state.provider.ts +++ b/libs/common/src/platform/state/implementations/default-global-state.provider.ts @@ -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}`; } } diff --git a/libs/common/src/platform/state/implementations/default-single-user-state.provider.ts b/libs/common/src/platform/state/implementations/default-single-user-state.provider.ts index b24a3a58687..9c01c2a76ed 100644 --- a/libs/common/src/platform/state/implementations/default-single-user-state.provider.ts +++ b/libs/common/src/platform/state/implementations/default-single-user-state.provider.ts @@ -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}`; } } diff --git a/libs/common/src/platform/state/implementations/default-single-user-state.spec.ts b/libs/common/src/platform/state/implementations/default-single-user-state.spec.ts index 4e02c1a654c..b1216c0bafa 100644 --- a/libs/common/src/platform/state/implementations/default-single-user-state.spec.ts +++ b/libs/common/src/platform/state/implementations/default-single-user-state.spec.ts @@ -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", () => { diff --git a/libs/common/src/platform/state/implementations/default-single-user-state.ts b/libs/common/src/platform/state/implementations/default-single-user-state.ts index 9523a94965d..b01e0958b5c 100644 --- a/libs/common/src/platform/state/implementations/default-single-user-state.ts +++ b/libs/common/src/platform/state/implementations/default-single-user-state.ts @@ -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; } diff --git a/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts b/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts index d6637516723..f3e5ef7f610 100644 --- a/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts +++ b/libs/common/src/platform/state/implementations/specific-state.provider.spec.ts @@ -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"); diff --git a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts index fe56bde7022..e48f2fe0a3e 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.spec.ts @@ -11,6 +11,7 @@ import { MessagingService } from "../../platform/abstractions/messaging.service" import { PlatformUtilsService } from "../../platform/abstractions/platform-utils.service"; import { StateService } from "../../platform/abstractions/state.service"; import { Account } from "../../platform/models/domain/account"; +import { StateEventRunnerService } from "../../platform/state"; import { CipherService } from "../../vault/abstractions/cipher.service"; import { CollectionService } from "../../vault/abstractions/collection.service"; import { FolderService } from "../../vault/abstractions/folder/folder.service.abstraction"; @@ -28,6 +29,7 @@ describe("VaultTimeoutService", () => { let stateService: MockProxy<StateService>; let authService: MockProxy<AuthService>; let vaultTimeoutSettingsService: MockProxy<VaultTimeoutSettingsService>; + let stateEventRunnerService: MockProxy<StateEventRunnerService>; let lockedCallback: jest.Mock<Promise<void>, [userId: string]>; let loggedOutCallback: jest.Mock<Promise<void>, [expired: boolean, userId?: string]>; @@ -48,6 +50,7 @@ describe("VaultTimeoutService", () => { stateService = mock(); authService = mock(); vaultTimeoutSettingsService = mock(); + stateEventRunnerService = mock(); lockedCallback = jest.fn(); loggedOutCallback = jest.fn(); @@ -73,6 +76,7 @@ describe("VaultTimeoutService", () => { stateService, authService, vaultTimeoutSettingsService, + stateEventRunnerService, lockedCallback, loggedOutCallback, ); @@ -103,7 +107,8 @@ describe("VaultTimeoutService", () => { return Promise.resolve(accounts[userId]?.authStatus); }); stateService.getIsAuthenticated.mockImplementation((options) => { - return Promise.resolve(accounts[options.userId]?.isAuthenticated); + // Just like actual state service, if no userId is given fallback to active userId + return Promise.resolve(accounts[options.userId ?? globalSetups?.userId]?.isAuthenticated); }); vaultTimeoutSettingsService.getVaultTimeout.mockImplementation((userId) => { @@ -337,4 +342,80 @@ describe("VaultTimeoutService", () => { expectNoAction("1"); }); }); + + describe("lock", () => { + const setupLock = () => { + setupAccounts( + { + user1: { + authStatus: AuthenticationStatus.Unlocked, + isAuthenticated: true, + }, + user2: { + authStatus: AuthenticationStatus.Unlocked, + isAuthenticated: true, + }, + }, + { + userId: "user1", + }, + ); + }; + + it("should call state event runner with currently active user if no user passed into lock", async () => { + setupLock(); + + await vaultTimeoutService.lock(); + + expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user1"); + }); + + it("should call messaging service locked message if no user passed into lock", async () => { + setupLock(); + + await vaultTimeoutService.lock(); + + // Currently these pass `undefined` (or what they were given) as the userId back + // but we could change this to give the user that was locked (active) to these methods + // so they don't have to get it their own way, but that is a behavioral change that needs + // to be tested. + expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: undefined }); + }); + + it("should call locked callback if no user passed into lock", async () => { + setupLock(); + + await vaultTimeoutService.lock(); + + // Currently these pass `undefined` (or what they were given) as the userId back + // but we could change this to give the user that was locked (active) to these methods + // so they don't have to get it their own way, but that is a behavioral change that needs + // to be tested. + expect(lockedCallback).toHaveBeenCalledWith(undefined); + }); + + it("should call state event runner with user passed into lock", async () => { + setupLock(); + + await vaultTimeoutService.lock("user2"); + + expect(stateEventRunnerService.handleEvent).toHaveBeenCalledWith("lock", "user2"); + }); + + it("should call messaging service locked message with user passed into lock", async () => { + setupLock(); + + await vaultTimeoutService.lock("user2"); + + expect(messagingService.send).toHaveBeenCalledWith("locked", { userId: "user2" }); + }); + + it("should call locked callback with user passed into lock", async () => { + setupLock(); + + await vaultTimeoutService.lock("user2"); + + expect(lockedCallback).toHaveBeenCalledWith("user2"); + }); + }); }); diff --git a/libs/common/src/services/vault-timeout/vault-timeout.service.ts b/libs/common/src/services/vault-timeout/vault-timeout.service.ts index 6163005367f..c3270ac2b80 100644 --- a/libs/common/src/services/vault-timeout/vault-timeout.service.ts +++ b/libs/common/src/services/vault-timeout/vault-timeout.service.ts @@ -11,6 +11,8 @@ import { CryptoService } from "../../platform/abstractions/crypto.service"; import { MessagingService } from "../../platform/abstractions/messaging.service"; import { PlatformUtilsService } from "../../platform/abstractions/platform-utils.service"; import { StateService } from "../../platform/abstractions/state.service"; +import { StateEventRunnerService } from "../../platform/state"; +import { UserId } from "../../types/guid"; import { CipherService } from "../../vault/abstractions/cipher.service"; import { CollectionService } from "../../vault/abstractions/collection.service"; import { FolderService } from "../../vault/abstractions/folder/folder.service.abstraction"; @@ -29,6 +31,7 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { private stateService: StateService, private authService: AuthService, private vaultTimeoutSettingsService: VaultTimeoutSettingsService, + private stateEventRunnerService: StateEventRunnerService, private lockedCallback: (userId?: string) => Promise<void> = null, private loggedOutCallback: (expired: boolean, userId?: string) => Promise<void> = null, ) {} @@ -81,7 +84,9 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { await this.logOut(userId); } - if (userId == null || userId === (await this.stateService.getUserId())) { + const currentUserId = await this.stateService.getUserId(); + + if (userId == null || userId === currentUserId) { this.searchService.clearIndex(); await this.folderService.clearCache(); await this.collectionService.clearActiveUserCache(); @@ -98,6 +103,11 @@ export class VaultTimeoutService implements VaultTimeoutServiceAbstraction { await this.cipherService.clearCache(userId); + await this.stateEventRunnerService.handleEvent("lock", (userId ?? currentUserId) as UserId); + + // FIXME: We should send the userId of the user that was locked, in the case of this method being passed + // undefined then it should give back the currentUserId. Better yet, this method shouldn't take + // an undefined userId at all. All receivers need to be checked for how they handle getting undefined. this.messagingService.send("locked", { userId: userId }); if (this.lockedCallback != null) { From d0e99782b9a06a859db891b82dbec49da834cda5 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Mon, 4 Mar 2024 16:04:04 -0500 Subject: [PATCH 04/14] [deps] Platform: Update @angular/cdk to v16.2.14 (#8129) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index f6623bee441..9afe97595c1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,7 +16,7 @@ ], "dependencies": { "@angular/animations": "16.2.12", - "@angular/cdk": "16.2.13", + "@angular/cdk": "16.2.14", "@angular/common": "16.2.12", "@angular/compiler": "16.2.12", "@angular/core": "16.2.12", @@ -965,9 +965,9 @@ } }, "node_modules/@angular/cdk": { - "version": "16.2.13", - "resolved": "https://registry.npmjs.org/@angular/cdk/-/cdk-16.2.13.tgz", - "integrity": "sha512-8kn2X2yesvgfIbCUNoS9EDjooIx9LwEglYBbD89Y/do8EeN/CC3Tn02gqSrEfgMhYBLBJmHXbfOhbDDvcvOCeg==", + "version": "16.2.14", + "resolved": "https://registry.npmjs.org/@angular/cdk/-/cdk-16.2.14.tgz", + "integrity": "sha512-n6PrGdiVeSTEmM/HEiwIyg6YQUUymZrb5afaNLGFRM5YL0Y8OBqd+XhCjb0OfD/AfgCUtedVEPwNqrfW8KzgGw==", "dependencies": { "tslib": "^2.3.0" }, diff --git a/package.json b/package.json index 39872eff963..66eef2e299f 100644 --- a/package.json +++ b/package.json @@ -150,7 +150,7 @@ }, "dependencies": { "@angular/animations": "16.2.12", - "@angular/cdk": "16.2.13", + "@angular/cdk": "16.2.14", "@angular/common": "16.2.12", "@angular/compiler": "16.2.12", "@angular/core": "16.2.12", From 5a705f512dccdcbd5997050cca25992528a2ccba Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 5 Mar 2024 13:56:13 +1000 Subject: [PATCH 05/14] [deps] AC: Update sass-loader to v13.3.3 (#8000) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- package-lock.json | 45 +++++++++++++++++++++++++++++++++++++++++---- package.json | 2 +- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9afe97595c1..c731303eb6c 100644 --- a/package-lock.json +++ b/package-lock.json @@ -170,7 +170,7 @@ "remark-gfm": "3.0.1", "rimraf": "5.0.5", "sass": "1.69.5", - "sass-loader": "13.3.2", + "sass-loader": "13.3.3", "storybook": "7.6.17", "style-loader": "3.3.3", "tailwindcss": "3.4.1", @@ -746,6 +746,43 @@ "node": ">=14.0.0" } }, + "node_modules/@angular-devkit/build-angular/node_modules/sass-loader": { + "version": "13.3.2", + "resolved": "https://registry.npmjs.org/sass-loader/-/sass-loader-13.3.2.tgz", + "integrity": "sha512-CQbKl57kdEv+KDLquhC+gE3pXt74LEAzm+tzywcA0/aHZuub8wTErbjAoNI57rPUWRYRNC5WUnNl8eGJNbDdwg==", + "dev": true, + "dependencies": { + "neo-async": "^2.6.2" + }, + "engines": { + "node": ">= 14.15.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/webpack" + }, + "peerDependencies": { + "fibers": ">= 3.1.0", + "node-sass": "^4.0.0 || ^5.0.0 || ^6.0.0 || ^7.0.0 || ^8.0.0 || ^9.0.0", + "sass": "^1.3.0", + "sass-embedded": "*", + "webpack": "^5.0.0" + }, + "peerDependenciesMeta": { + "fibers": { + "optional": true + }, + "node-sass": { + "optional": true + }, + "sass": { + "optional": true + }, + "sass-embedded": { + "optional": true + } + } + }, "node_modules/@angular-devkit/build-angular/node_modules/schema-utils": { "version": "3.3.0", "resolved": "https://registry.npmjs.org/schema-utils/-/schema-utils-3.3.0.tgz", @@ -33122,9 +33159,9 @@ } }, "node_modules/sass-loader": { - "version": "13.3.2", - "resolved": "https://registry.npmjs.org/sass-loader/-/sass-loader-13.3.2.tgz", - "integrity": "sha512-CQbKl57kdEv+KDLquhC+gE3pXt74LEAzm+tzywcA0/aHZuub8wTErbjAoNI57rPUWRYRNC5WUnNl8eGJNbDdwg==", + "version": "13.3.3", + "resolved": "https://registry.npmjs.org/sass-loader/-/sass-loader-13.3.3.tgz", + "integrity": "sha512-mt5YN2F1MOZr3d/wBRcZxeFgwgkH44wVc2zohO2YF6JiOMkiXe4BYRZpSu2sO1g71mo/j16txzUhsKZlqjVGzA==", "dev": true, "dependencies": { "neo-async": "^2.6.2" diff --git a/package.json b/package.json index 66eef2e299f..c6beacb464c 100644 --- a/package.json +++ b/package.json @@ -131,7 +131,7 @@ "remark-gfm": "3.0.1", "rimraf": "5.0.5", "sass": "1.69.5", - "sass-loader": "13.3.2", + "sass-loader": "13.3.3", "storybook": "7.6.17", "style-loader": "3.3.3", "tailwindcss": "3.4.1", From 1aa7341feeed0872de4aefffa9e2d79f3f190164 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 5 Mar 2024 13:56:23 +1000 Subject: [PATCH 06/14] [deps] AC: Update postcss-loader to v7.3.4 (#7999) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- package-lock.json | 98 +++++++++++++++++++++++++++++++++++++++++------ package.json | 2 +- 2 files changed, 87 insertions(+), 13 deletions(-) diff --git a/package-lock.json b/package-lock.json index c731303eb6c..d8f5ffadcb8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -160,7 +160,7 @@ "node-ipc": "9.2.1", "pkg": "5.8.1", "postcss": "8.4.32", - "postcss-loader": "7.3.3", + "postcss-loader": "7.3.4", "prettier": "3.2.2", "prettier-plugin-tailwindcss": "0.5.11", "process": "0.11.10", @@ -576,6 +576,12 @@ "ajv": "^6.9.1" } }, + "node_modules/@angular-devkit/build-angular/node_modules/argparse": { + "version": "2.0.1", + "resolved": "https://registry.npmjs.org/argparse/-/argparse-2.0.1.tgz", + "integrity": "sha512-8+9WqebbFzpX9OR+Wa6O29asIogeRMzcGtAINdpMHHyAg10f05aSFVBbcEqGf/PXw1EjAZ+q2/bEBg3DvurK3Q==", + "dev": true + }, "node_modules/@angular-devkit/build-angular/node_modules/autoprefixer": { "version": "10.4.14", "resolved": "https://registry.npmjs.org/autoprefixer/-/autoprefixer-10.4.14.tgz", @@ -609,6 +615,32 @@ "postcss": "^8.1.0" } }, + "node_modules/@angular-devkit/build-angular/node_modules/cosmiconfig": { + "version": "8.3.6", + "resolved": "https://registry.npmjs.org/cosmiconfig/-/cosmiconfig-8.3.6.tgz", + "integrity": "sha512-kcZ6+W5QzcJ3P1Mt+83OUv/oHFqZHIx8DuxG6eZ5RGMERoLqp4BuGjhHLYGK+Kf5XVkQvqBSmAy/nGWN3qDgEA==", + "dev": true, + "dependencies": { + "import-fresh": "^3.3.0", + "js-yaml": "^4.1.0", + "parse-json": "^5.2.0", + "path-type": "^4.0.0" + }, + "engines": { + "node": ">=14" + }, + "funding": { + "url": "https://github.com/sponsors/d-fischer" + }, + "peerDependencies": { + "typescript": ">=4.9.5" + }, + "peerDependenciesMeta": { + "typescript": { + "optional": true + } + } + }, "node_modules/@angular-devkit/build-angular/node_modules/eslint-scope": { "version": "5.1.1", "resolved": "https://registry.npmjs.org/eslint-scope/-/eslint-scope-5.1.1.tgz", @@ -695,6 +727,18 @@ "tslib": "^2.1.0" } }, + "node_modules/@angular-devkit/build-angular/node_modules/js-yaml": { + "version": "4.1.0", + "resolved": "https://registry.npmjs.org/js-yaml/-/js-yaml-4.1.0.tgz", + "integrity": "sha512-wpxZs9NoxZaJESJGIZTyDEaYpl0FKSA+FB9aJiyemKhMwkxQg63h4T1KJgUGHpTqPDNRcmmYLugrRjJlBtWvRA==", + "dev": true, + "dependencies": { + "argparse": "^2.0.1" + }, + "bin": { + "js-yaml": "bin/js-yaml.js" + } + }, "node_modules/@angular-devkit/build-angular/node_modules/json-schema-traverse": { "version": "0.4.1", "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-0.4.1.tgz", @@ -729,6 +773,28 @@ "node": "^10 || ^12 || >=14" } }, + "node_modules/@angular-devkit/build-angular/node_modules/postcss-loader": { + "version": "7.3.3", + "resolved": "https://registry.npmjs.org/postcss-loader/-/postcss-loader-7.3.3.tgz", + "integrity": "sha512-YgO/yhtevGO/vJePCQmTxiaEwER94LABZN0ZMT4A0vsak9TpO+RvKRs7EmJ8peIlB9xfXCsS7M8LjqncsUZ5HA==", + "dev": true, + "dependencies": { + "cosmiconfig": "^8.2.0", + "jiti": "^1.18.2", + "semver": "^7.3.8" + }, + "engines": { + "node": ">= 14.15.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/webpack" + }, + "peerDependencies": { + "postcss": "^7.0.0 || ^8.0.1", + "webpack": "^5.0.0" + } + }, "node_modules/@angular-devkit/build-angular/node_modules/sass": { "version": "1.64.1", "resolved": "https://registry.npmjs.org/sass/-/sass-1.64.1.tgz", @@ -30840,14 +30906,14 @@ } }, "node_modules/postcss-loader": { - "version": "7.3.3", - "resolved": "https://registry.npmjs.org/postcss-loader/-/postcss-loader-7.3.3.tgz", - "integrity": "sha512-YgO/yhtevGO/vJePCQmTxiaEwER94LABZN0ZMT4A0vsak9TpO+RvKRs7EmJ8peIlB9xfXCsS7M8LjqncsUZ5HA==", + "version": "7.3.4", + "resolved": "https://registry.npmjs.org/postcss-loader/-/postcss-loader-7.3.4.tgz", + "integrity": "sha512-iW5WTTBSC5BfsBJ9daFMPVrLT36MrNiC6fqOZTTaHjBNX6Pfd5p+hSBqe/fEeNd7pc13QiAyGt7VdGMw4eRC4A==", "dev": true, "dependencies": { - "cosmiconfig": "^8.2.0", - "jiti": "^1.18.2", - "semver": "^7.3.8" + "cosmiconfig": "^8.3.5", + "jiti": "^1.20.0", + "semver": "^7.5.4" }, "engines": { "node": ">= 14.15.0" @@ -30868,14 +30934,14 @@ "dev": true }, "node_modules/postcss-loader/node_modules/cosmiconfig": { - "version": "8.2.0", - "resolved": "https://registry.npmjs.org/cosmiconfig/-/cosmiconfig-8.2.0.tgz", - "integrity": "sha512-3rTMnFJA1tCOPwRxtgF4wd7Ab2qvDbL8jX+3smjIbS4HlZBagTlpERbdN7iAbWlrfxE3M8c27kTwTawQ7st+OQ==", + "version": "8.3.6", + "resolved": "https://registry.npmjs.org/cosmiconfig/-/cosmiconfig-8.3.6.tgz", + "integrity": "sha512-kcZ6+W5QzcJ3P1Mt+83OUv/oHFqZHIx8DuxG6eZ5RGMERoLqp4BuGjhHLYGK+Kf5XVkQvqBSmAy/nGWN3qDgEA==", "dev": true, "dependencies": { - "import-fresh": "^3.2.1", + "import-fresh": "^3.3.0", "js-yaml": "^4.1.0", - "parse-json": "^5.0.0", + "parse-json": "^5.2.0", "path-type": "^4.0.0" }, "engines": { @@ -30883,6 +30949,14 @@ }, "funding": { "url": "https://github.com/sponsors/d-fischer" + }, + "peerDependencies": { + "typescript": ">=4.9.5" + }, + "peerDependenciesMeta": { + "typescript": { + "optional": true + } } }, "node_modules/postcss-loader/node_modules/js-yaml": { diff --git a/package.json b/package.json index c6beacb464c..7924dca89d4 100644 --- a/package.json +++ b/package.json @@ -121,7 +121,7 @@ "node-ipc": "9.2.1", "pkg": "5.8.1", "postcss": "8.4.32", - "postcss-loader": "7.3.3", + "postcss-loader": "7.3.4", "prettier": "3.2.2", "prettier-plugin-tailwindcss": "0.5.11", "process": "0.11.10", From 9d4129e8c955e1697b37509eee249b5a82205940 Mon Sep 17 00:00:00 2001 From: "renovate[bot]" <29139614+renovate[bot]@users.noreply.github.com> Date: Tue, 5 Mar 2024 15:39:48 +1000 Subject: [PATCH 07/14] [deps] AC: Update postcss to v8.4.35 (#7998) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index d8f5ffadcb8..1ab94ef4d47 100644 --- a/package-lock.json +++ b/package-lock.json @@ -159,7 +159,7 @@ "mini-css-extract-plugin": "2.7.6", "node-ipc": "9.2.1", "pkg": "5.8.1", - "postcss": "8.4.32", + "postcss": "8.4.35", "postcss-loader": "7.3.4", "prettier": "3.2.2", "prettier-plugin-tailwindcss": "0.5.11", @@ -30804,9 +30804,9 @@ } }, "node_modules/postcss": { - "version": "8.4.32", - "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.32.tgz", - "integrity": "sha512-D/kj5JNu6oo2EIy+XL/26JEDTlIbB8hw85G8StOE6L74RQAVVP5rej6wxCNqyMbR4RkPfqvezVbPw81Ngd6Kcw==", + "version": "8.4.35", + "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.35.tgz", + "integrity": "sha512-u5U8qYpBCpN13BsiEB0CbR1Hhh4Gc0zLFuedrHJKMctHCHAGrMdG0PRM/KErzAL3CU6/eckEtmHNB3x6e3c0vA==", "dev": true, "funding": [ { diff --git a/package.json b/package.json index 7924dca89d4..ea1f7d318f0 100644 --- a/package.json +++ b/package.json @@ -120,7 +120,7 @@ "mini-css-extract-plugin": "2.7.6", "node-ipc": "9.2.1", "pkg": "5.8.1", - "postcss": "8.4.32", + "postcss": "8.4.35", "postcss-loader": "7.3.4", "prettier": "3.2.2", "prettier-plugin-tailwindcss": "0.5.11", From e6a569b153f72b9f97838834c04ec9d7db239b2a Mon Sep 17 00:00:00 2001 From: SmithThe4th <gsmith@bitwarden.com> Date: Tue, 5 Mar 2024 09:38:38 -0500 Subject: [PATCH 08/14] [PM-6584] [PM-6632] [Defects] Vertical Vault Refresh Product Switcher (#8198) * removed org enabled check from acOrg logic * fixed bug where organization doesn't show on admin console for users admin custom permissions --- .../organizations/layouts/organization-layout.component.ts | 3 ++- .../product-switcher/product-switcher-content.component.ts | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts b/apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts index 81b1ca142b5..90010160aa9 100644 --- a/apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts +++ b/apps/web/src/app/admin-console/organizations/layouts/organization-layout.component.ts @@ -8,6 +8,7 @@ import { canAccessBillingTab, canAccessGroupsTab, canAccessMembersTab, + canAccessOrgAdmin, canAccessReportingTab, canAccessSettingsTab, canAccessVaultTab, @@ -43,7 +44,7 @@ import { AdminConsoleLogo } from "../../icons/admin-console-logo"; export class OrganizationLayoutComponent implements OnInit, OnDestroy { protected readonly logo = AdminConsoleLogo; - protected orgFilter = (org: Organization) => org.isAdmin; + protected orgFilter = (org: Organization) => canAccessOrgAdmin(org); organization$: Observable<Organization>; showPaymentAndHistory$: Observable<boolean>; diff --git a/apps/web/src/app/layouts/product-switcher/product-switcher-content.component.ts b/apps/web/src/app/layouts/product-switcher/product-switcher-content.component.ts index 5354dc2b710..0ce98948c9c 100644 --- a/apps/web/src/app/layouts/product-switcher/product-switcher-content.component.ts +++ b/apps/web/src/app/layouts/product-switcher/product-switcher-content.component.ts @@ -58,9 +58,9 @@ export class ProductSwitcherContentComponent { // If the active route org doesn't have access to AC, find the first org that does. const acOrg = - routeOrg != null && canAccessOrgAdmin(routeOrg) && routeOrg.enabled + routeOrg != null && canAccessOrgAdmin(routeOrg) ? routeOrg - : orgs.find((o) => canAccessOrgAdmin(o) && o.enabled); + : orgs.find((o) => canAccessOrgAdmin(o)); // TODO: This should be migrated to an Observable provided by the provider service and moved to the combineLatest above. See AC-2092. const providers = await this.providerService.getAll(); From 7bb24d5fad7e26b209011b402c850d05a77ab3c9 Mon Sep 17 00:00:00 2001 From: Matt Gibson <mgibson@bitwarden.com> Date: Tue, 5 Mar 2024 09:38:49 -0600 Subject: [PATCH 09/14] Remove Unused services (#8210) --- apps/browser/src/popup/services/services.module.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index dd081b7877f..092bfbc4fdd 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -15,7 +15,6 @@ import { LoginStrategyServiceAbstraction, } from "@bitwarden/auth/common"; import { ApiService } from "@bitwarden/common/abstractions/api.service"; -import { AuditService } from "@bitwarden/common/abstractions/audit.service"; import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; import { EventUploadService } from "@bitwarden/common/abstractions/event/event-upload.service"; import { NotificationsService } from "@bitwarden/common/abstractions/notifications.service"; @@ -211,7 +210,6 @@ function getBgService<T>(service: keyof MainBackground) { }, deps: [LogServiceAbstraction, I18nServiceAbstraction], }, - { provide: AuditService, useFactory: getBgService<AuditService>("auditService"), deps: [] }, { provide: CipherFileUploadService, useFactory: getBgService<CipherFileUploadService>("cipherFileUploadService"), From c9b5125f743d8c44119eef8f9b20b560f2b7e6f3 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Tue, 5 Mar 2024 11:30:40 -0600 Subject: [PATCH 10/14] Include Storage Location In Fake Cache Key (#8203) --- libs/common/spec/fake-state-provider.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/libs/common/spec/fake-state-provider.ts b/libs/common/spec/fake-state-provider.ts index ae3233405f3..5b0be6ccf99 100644 --- a/libs/common/spec/fake-state-provider.ts +++ b/libs/common/spec/fake-state-provider.ts @@ -32,7 +32,8 @@ export class FakeGlobalStateProvider implements GlobalStateProvider { states: Map<string, GlobalState<unknown>> = new Map(); get<T>(keyDefinition: KeyDefinition<T>): GlobalState<T> { this.mock.get(keyDefinition); - let result = this.states.get(keyDefinition.fullName); + const cacheKey = `${keyDefinition.fullName}_${keyDefinition.stateDefinition.defaultStorageLocation}`; + let result = this.states.get(cacheKey); if (result == null) { let fake: FakeGlobalState<T>; @@ -44,10 +45,10 @@ export class FakeGlobalStateProvider implements GlobalStateProvider { } fake.keyDefinition = keyDefinition; result = fake; - this.states.set(keyDefinition.fullName, result); + this.states.set(cacheKey, result); result = new FakeGlobalState<T>(); - this.states.set(keyDefinition.fullName, result); + this.states.set(cacheKey, result); } return result as GlobalState<T>; } @@ -76,7 +77,8 @@ export class FakeSingleUserStateProvider implements SingleUserStateProvider { if (keyDefinition instanceof KeyDefinition) { keyDefinition = UserKeyDefinition.fromBaseKeyDefinition(keyDefinition); } - let result = this.states.get(`${keyDefinition.fullName}_${userId}`); + const cacheKey = `${keyDefinition.fullName}_${keyDefinition.stateDefinition.defaultStorageLocation}_${userId}`; + let result = this.states.get(cacheKey); if (result == null) { let fake: FakeSingleUserState<T>; @@ -88,7 +90,7 @@ export class FakeSingleUserStateProvider implements SingleUserStateProvider { } fake.keyDefinition = keyDefinition; result = fake; - this.states.set(`${keyDefinition.fullName}_${userId}`, result); + this.states.set(cacheKey, result); } return result as SingleUserState<T>; } @@ -119,7 +121,8 @@ export class FakeActiveUserStateProvider implements ActiveUserStateProvider { if (keyDefinition instanceof KeyDefinition) { keyDefinition = UserKeyDefinition.fromBaseKeyDefinition(keyDefinition); } - let result = this.states.get(keyDefinition.fullName); + const cacheKey = `${keyDefinition.fullName}_${keyDefinition.stateDefinition.defaultStorageLocation}`; + let result = this.states.get(cacheKey); if (result == null) { // Look for established mock @@ -129,7 +132,7 @@ export class FakeActiveUserStateProvider implements ActiveUserStateProvider { result = new FakeActiveUserState<T>(this.accountService); } result.keyDefinition = keyDefinition; - this.states.set(keyDefinition.fullName, result); + this.states.set(cacheKey, result); } return result as ActiveUserState<T>; } From 5cd53c3a7d6f3e9b4cd18b4fd326fb82135b2e57 Mon Sep 17 00:00:00 2001 From: Bitwarden DevOps <106330231+bitwarden-devops-bot@users.noreply.github.com> Date: Tue, 5 Mar 2024 19:19:01 +0100 Subject: [PATCH 11/14] Bumped web version to 2024.2.4 (#8215) --- apps/web/package.json | 2 +- package-lock.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/apps/web/package.json b/apps/web/package.json index 2596ee05be5..2b6bdf7c1b6 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -1,6 +1,6 @@ { "name": "@bitwarden/web-vault", - "version": "2024.2.3", + "version": "2024.2.4", "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 1ab94ef4d47..684636d99ed 100644 --- a/package-lock.json +++ b/package-lock.json @@ -264,7 +264,7 @@ }, "apps/web": { "name": "@bitwarden/web-vault", - "version": "2024.2.3" + "version": "2024.2.4" }, "libs/admin-console": { "name": "@bitwarden/admin-console", From 101e1a4f2ba666712bff7c8e484608273b5424ee Mon Sep 17 00:00:00 2001 From: Addison Beck <github@addisonbeck.com> Date: Tue, 5 Mar 2024 13:35:12 -0600 Subject: [PATCH 12/14] Migrate provider service to state provider (#8173) * Migrate existing provider data to StateProvider Migrate existing provider data to StateProvider * Rework the ProviderService to call StateProvider * Unit test the ProviderService * Update DI to reflect ProviderService's new args * Add ProviderService to logout chains across products * Remove provider related stateService methods * Update libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> * Cover up a copy/paste job * Compare equality over entire array in a test --------- Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com> --- .../browser/src/background/main.background.ts | 14 +- .../src/popup/services/services.module.ts | 6 - apps/cli/src/bw.ts | 3 +- apps/desktop/src/app/app.component.ts | 3 + .../src/services/jslib-services.module.ts | 2 +- .../abstractions/provider.service.ts | 3 +- .../services/provider.service.spec.ts | 123 ++++++++++++++- .../services/provider.service.ts | 46 +++--- .../platform/abstractions/state.service.ts | 3 - .../src/platform/models/domain/account.ts | 2 - .../src/platform/services/state.service.ts | 22 --- ...e-provider-state-to-state-provider.spec.ts | 145 ++++++++++++++++++ ...8-move-provider-state-to-state-provider.ts | 71 +++++++++ 13 files changed, 378 insertions(+), 65 deletions(-) create mode 100644 libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.ts diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index ffdd2e30898..f4505ee0d95 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -700,7 +700,7 @@ export default class MainBackground { this.fileUploadService, this.sendService, ); - this.providerService = new ProviderService(this.stateService); + this.providerService = new ProviderService(this.stateProvider); this.syncService = new SyncService( this.apiService, this.settingsService, @@ -1114,12 +1114,12 @@ export default class MainBackground { this.keyConnectorService.clear(), this.vaultFilterService.clear(), this.biometricStateService.logout(userId), - /* - We intentionally do not clear: - - autofillSettingsService - - badgeSettingsService - - userNotificationSettingsService - */ + this.providerService.save(null, userId), + /* We intentionally do not clear: + * - autofillSettingsService + * - badgeSettingsService + * - userNotificationSettingsService + */ ]); //Needs to be checked before state is cleaned diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 092bfbc4fdd..2d9d4c68da8 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -24,7 +24,6 @@ import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vaul import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; -import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service"; import { AccountService as AccountServiceAbstraction } from "@bitwarden/common/auth/abstractions/account.service"; import { AuthService as AuthServiceAbstraction } from "@bitwarden/common/auth/abstractions/auth.service"; import { DeviceTrustCryptoServiceAbstraction } from "@bitwarden/common/auth/abstractions/device-trust-crypto.service.abstraction"; @@ -436,11 +435,6 @@ function getBgService<T>(service: keyof MainBackground) { AccountServiceAbstraction, ], }, - { - provide: ProviderService, - useFactory: getBgService<ProviderService>("providerService"), - deps: [], - }, { provide: SECURE_STORAGE, useFactory: getBgService<AbstractStorageService>("secureStorageService"), diff --git a/apps/cli/src/bw.ts b/apps/cli/src/bw.ts index 7f76bee69af..e46937f71f7 100644 --- a/apps/cli/src/bw.ts +++ b/apps/cli/src/bw.ts @@ -388,7 +388,7 @@ export class Main { this.stateProvider, ); - this.providerService = new ProviderService(this.stateService); + this.providerService = new ProviderService(this.stateProvider); this.organizationService = new OrganizationService(this.stateService, this.stateProvider); @@ -655,6 +655,7 @@ export class Main { this.collectionService.clear(userId as UserId), this.policyService.clear(userId), this.passwordGenerationService.clear(), + this.providerService.save(null, userId as UserId), ]); await this.stateEventRunnerService.handleEvent("logout", userId as UserId); diff --git a/apps/desktop/src/app/app.component.ts b/apps/desktop/src/app/app.component.ts index 51bde2ef1c4..cbee441fa39 100644 --- a/apps/desktop/src/app/app.component.ts +++ b/apps/desktop/src/app/app.component.ts @@ -23,6 +23,7 @@ import { SettingsService } from "@bitwarden/common/abstractions/settings.service import { VaultTimeoutSettingsService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout-settings.service"; import { VaultTimeoutService } from "@bitwarden/common/abstractions/vault-timeout/vault-timeout.service"; import { InternalPolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { ProviderService } from "@bitwarden/common/admin-console/abstractions/provider.service"; import { AuthService } from "@bitwarden/common/auth/abstractions/auth.service"; import { KeyConnectorService } from "@bitwarden/common/auth/abstractions/key-connector.service"; import { UserVerificationService } from "@bitwarden/common/auth/abstractions/user-verification/user-verification.service.abstraction"; @@ -151,6 +152,7 @@ export class AppComponent implements OnInit, OnDestroy { private dialogService: DialogService, private biometricStateService: BiometricStateService, private stateEventRunnerService: StateEventRunnerService, + private providerService: ProviderService, ) {} ngOnInit() { @@ -584,6 +586,7 @@ export class AppComponent implements OnInit, OnDestroy { await this.policyService.clear(userBeingLoggedOut); await this.keyConnectorService.clear(); await this.biometricStateService.logout(userBeingLoggedOut as UserId); + await this.providerService.save(null, userBeingLoggedOut as UserId); await this.stateEventRunnerService.handleEvent("logout", userBeingLoggedOut as UserId); diff --git a/libs/angular/src/services/jslib-services.module.ts b/libs/angular/src/services/jslib-services.module.ts index daf39717583..be3a65dcf24 100644 --- a/libs/angular/src/services/jslib-services.module.ts +++ b/libs/angular/src/services/jslib-services.module.ts @@ -745,7 +745,7 @@ import { ModalService } from "./modal.service"; { provide: ProviderServiceAbstraction, useClass: ProviderService, - deps: [StateServiceAbstraction], + deps: [StateProvider], }, { provide: TwoFactorServiceAbstraction, diff --git a/libs/common/src/admin-console/abstractions/provider.service.ts b/libs/common/src/admin-console/abstractions/provider.service.ts index d843154f3f0..eb5e347edac 100644 --- a/libs/common/src/admin-console/abstractions/provider.service.ts +++ b/libs/common/src/admin-console/abstractions/provider.service.ts @@ -1,8 +1,9 @@ +import { UserId } from "../../types/guid"; import { ProviderData } from "../models/data/provider.data"; import { Provider } from "../models/domain/provider"; export abstract class ProviderService { get: (id: string) => Promise<Provider>; getAll: () => Promise<Provider[]>; - save: (providers: { [id: string]: ProviderData }) => Promise<any>; + save: (providers: { [id: string]: ProviderData }, userId?: UserId) => Promise<any>; } diff --git a/libs/common/src/admin-console/services/provider.service.spec.ts b/libs/common/src/admin-console/services/provider.service.spec.ts index b76cf3476ee..4fc02ee8936 100644 --- a/libs/common/src/admin-console/services/provider.service.spec.ts +++ b/libs/common/src/admin-console/services/provider.service.spec.ts @@ -1,7 +1,56 @@ +import { FakeAccountService, FakeStateProvider, mockAccountServiceWith } from "../../../spec"; +import { FakeActiveUserState } from "../../../spec/fake-state"; +import { Utils } from "../../platform/misc/utils"; +import { UserId } from "../../types/guid"; import { ProviderUserStatusType, ProviderUserType } from "../enums"; import { ProviderData } from "../models/data/provider.data"; +import { Provider } from "../models/domain/provider"; -import { PROVIDERS } from "./provider.service"; +import { PROVIDERS, ProviderService } from "./provider.service"; + +/** + * It is easier to read arrays than records in code, but we store a record + * in state. This helper methods lets us build provider arrays in tests + * and easily map them to records before storing them in state. + */ +function arrayToRecord(input: ProviderData[]): Record<string, ProviderData> { + if (input == null) { + return undefined; + } + return Object.fromEntries(input?.map((i) => [i.id, i])); +} + +/** + * Builds a simple mock `ProviderData[]` array that can be used in tests + * to populate state. + * @param count The number of organizations to populate the list with. The + * function returns undefined if this is less than 1. The default value is 1. + * @param suffix A string to append to data fields on each provider. + * This defaults to the index of the organization in the list. + * @returns a `ProviderData[]` array that can be used to populate + * stateProvider. + */ +function buildMockProviders(count = 1, suffix?: string): ProviderData[] { + if (count < 1) { + return undefined; + } + + function buildMockProvider(id: string, name: string): ProviderData { + const data = new ProviderData({} as any); + data.id = id; + data.name = name; + + return data; + } + + const mockProviders = []; + for (let i = 0; i < count; i++) { + const s = suffix ? suffix + i.toString() : i.toString(); + mockProviders.push(buildMockProvider("provider" + s, "provider" + s)); + } + + return mockProviders; +} describe("PROVIDERS key definition", () => { const sut = PROVIDERS; @@ -21,3 +70,75 @@ describe("PROVIDERS key definition", () => { expect(result).toEqual(expectedResult); }); }); + +describe("ProviderService", () => { + let providerService: ProviderService; + + const fakeUserId = Utils.newGuid() as UserId; + let fakeAccountService: FakeAccountService; + let fakeStateProvider: FakeStateProvider; + let fakeActiveUserState: FakeActiveUserState<Record<string, ProviderData>>; + + beforeEach(async () => { + fakeAccountService = mockAccountServiceWith(fakeUserId); + fakeStateProvider = new FakeStateProvider(fakeAccountService); + fakeActiveUserState = fakeStateProvider.activeUser.getFake(PROVIDERS); + providerService = new ProviderService(fakeStateProvider); + }); + + describe("getAll()", () => { + it("Returns an array of all providers stored in state", async () => { + const mockData: ProviderData[] = buildMockProviders(5); + fakeActiveUserState.nextState(arrayToRecord(mockData)); + const providers = await providerService.getAll(); + expect(providers).toHaveLength(5); + expect(providers).toEqual(mockData.map((x) => new Provider(x))); + }); + + it("Returns an empty array if no providers are found in state", async () => { + const mockData: ProviderData[] = undefined; + fakeActiveUserState.nextState(arrayToRecord(mockData)); + const result = await providerService.getAll(); + expect(result).toEqual([]); + }); + }); + + describe("get()", () => { + it("Returns a single provider from state that matches the specified id", async () => { + const mockData = buildMockProviders(5); + fakeActiveUserState.nextState(arrayToRecord(mockData)); + const result = await providerService.get(mockData[3].id); + expect(result).toEqual(new Provider(mockData[3])); + }); + + it("Returns undefined if the specified provider id is not found", async () => { + const result = await providerService.get("this-provider-does-not-exist"); + expect(result).toBe(undefined); + }); + }); + + describe("save()", () => { + it("replaces the entire provider list in state for the active user", async () => { + const originalData = buildMockProviders(10); + fakeActiveUserState.nextState(arrayToRecord(originalData)); + + const newData = buildMockProviders(10, "newData"); + await providerService.save(arrayToRecord(newData)); + + const result = await providerService.getAll(); + + expect(result).toEqual(newData); + expect(result).not.toEqual(originalData); + }); + + // This is more or less a test for logouts + it("can replace state with null", async () => { + const originalData = buildMockProviders(2); + fakeActiveUserState.nextState(arrayToRecord(originalData)); + await providerService.save(null); + const result = await providerService.getAll(); + expect(result).toEqual([]); + expect(result).not.toEqual(originalData); + }); + }); +}); diff --git a/libs/common/src/admin-console/services/provider.service.ts b/libs/common/src/admin-console/services/provider.service.ts index 520424373bf..e68ea5bf9d3 100644 --- a/libs/common/src/admin-console/services/provider.service.ts +++ b/libs/common/src/admin-console/services/provider.service.ts @@ -1,5 +1,7 @@ -import { StateService } from "../../platform/abstractions/state.service"; -import { KeyDefinition, PROVIDERS_DISK } from "../../platform/state"; +import { Observable, map, firstValueFrom } from "rxjs"; + +import { KeyDefinition, PROVIDERS_DISK, StateProvider } from "../../platform/state"; +import { UserId } from "../../types/guid"; import { ProviderService as ProviderServiceAbstraction } from "../abstractions/provider.service"; import { ProviderData } from "../models/data/provider.data"; import { Provider } from "../models/domain/provider"; @@ -8,32 +10,34 @@ export const PROVIDERS = KeyDefinition.record<ProviderData>(PROVIDERS_DISK, "pro deserializer: (obj: ProviderData) => obj, }); +function mapToSingleProvider(providerId: string) { + return map<Provider[], Provider>((providers) => providers?.find((p) => p.id === providerId)); +} + export class ProviderService implements ProviderServiceAbstraction { - constructor(private stateService: StateService) {} + constructor(private stateProvider: StateProvider) {} + + private providers$(userId?: UserId): Observable<Provider[] | undefined> { + return this.stateProvider + .getUserState$(PROVIDERS, userId) + .pipe(this.mapProviderRecordToArray()); + } + + private mapProviderRecordToArray() { + return map<Record<string, ProviderData>, Provider[]>((providers) => + Object.values(providers ?? {})?.map((o) => new Provider(o)), + ); + } async get(id: string): Promise<Provider> { - const providers = await this.stateService.getProviders(); - // eslint-disable-next-line - if (providers == null || !providers.hasOwnProperty(id)) { - return null; - } - - return new Provider(providers[id]); + return await firstValueFrom(this.providers$().pipe(mapToSingleProvider(id))); } async getAll(): Promise<Provider[]> { - const providers = await this.stateService.getProviders(); - const response: Provider[] = []; - for (const id in providers) { - // eslint-disable-next-line - if (providers.hasOwnProperty(id)) { - response.push(new Provider(providers[id])); - } - } - return response; + return await firstValueFrom(this.providers$()); } - async save(providers: { [id: string]: ProviderData }) { - await this.stateService.setProviders(providers); + async save(providers: { [id: string]: ProviderData }, userId?: UserId) { + await this.stateProvider.setUserState(PROVIDERS, providers, userId); } } diff --git a/libs/common/src/platform/abstractions/state.service.ts b/libs/common/src/platform/abstractions/state.service.ts index 338e3b6df3d..18b2f79483b 100644 --- a/libs/common/src/platform/abstractions/state.service.ts +++ b/libs/common/src/platform/abstractions/state.service.ts @@ -2,7 +2,6 @@ import { Observable } from "rxjs"; import { OrganizationData } from "../../admin-console/models/data/organization.data"; import { PolicyData } from "../../admin-console/models/data/policy.data"; -import { ProviderData } from "../../admin-console/models/data/provider.data"; import { Policy } from "../../admin-console/models/domain/policy"; import { AdminAuthRequestStorable } from "../../auth/models/domain/admin-auth-req-storable"; import { ForceSetPasswordReason } from "../../auth/models/domain/force-set-password-reason"; @@ -371,8 +370,6 @@ export abstract class StateService<T extends Account = Account> { * Sets the user's Pin, encrypted by the user key */ setProtectedPin: (value: string, options?: StorageOptions) => Promise<void>; - getProviders: (options?: StorageOptions) => Promise<{ [id: string]: ProviderData }>; - setProviders: (value: { [id: string]: ProviderData }, options?: StorageOptions) => Promise<void>; getRefreshToken: (options?: StorageOptions) => Promise<string>; setRefreshToken: (value: string, options?: StorageOptions) => Promise<void>; getRememberedEmail: (options?: StorageOptions) => Promise<string>; diff --git a/libs/common/src/platform/models/domain/account.ts b/libs/common/src/platform/models/domain/account.ts index 7ab98eec7d8..2b2024270ff 100644 --- a/libs/common/src/platform/models/domain/account.ts +++ b/libs/common/src/platform/models/domain/account.ts @@ -2,7 +2,6 @@ import { Jsonify } from "type-fest"; import { OrganizationData } from "../../../admin-console/models/data/organization.data"; import { PolicyData } from "../../../admin-console/models/data/policy.data"; -import { ProviderData } from "../../../admin-console/models/data/provider.data"; import { Policy } from "../../../admin-console/models/domain/policy"; import { AdminAuthRequestStorable } from "../../../auth/models/domain/admin-auth-req-storable"; import { ForceSetPasswordReason } from "../../../auth/models/domain/force-set-password-reason"; @@ -96,7 +95,6 @@ export class AccountData { addEditCipherInfo?: AddEditCipherInfo; eventCollection?: EventData[]; organizations?: { [id: string]: OrganizationData }; - providers?: { [id: string]: ProviderData }; static fromJSON(obj: DeepJsonify<AccountData>): AccountData { if (obj == null) { diff --git a/libs/common/src/platform/services/state.service.ts b/libs/common/src/platform/services/state.service.ts index 22a3c884f15..67c71c610d9 100644 --- a/libs/common/src/platform/services/state.service.ts +++ b/libs/common/src/platform/services/state.service.ts @@ -3,7 +3,6 @@ import { Jsonify, JsonValue } from "type-fest"; import { OrganizationData } from "../../admin-console/models/data/organization.data"; import { PolicyData } from "../../admin-console/models/data/policy.data"; -import { ProviderData } from "../../admin-console/models/data/provider.data"; import { Policy } from "../../admin-console/models/domain/policy"; import { AccountService } from "../../auth/abstractions/account.service"; import { AuthenticationStatus } from "../../auth/enums/authentication-status"; @@ -1821,27 +1820,6 @@ export class StateService< ); } - @withPrototypeForObjectValues(ProviderData) - async getProviders(options?: StorageOptions): Promise<{ [id: string]: ProviderData }> { - return ( - await this.getAccount(this.reconcileOptions(options, await this.defaultOnDiskOptions())) - )?.data?.providers; - } - - async setProviders( - value: { [id: string]: ProviderData }, - options?: StorageOptions, - ): Promise<void> { - const account = await this.getAccount( - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - account.data.providers = value; - await this.saveAccount( - account, - this.reconcileOptions(options, await this.defaultOnDiskOptions()), - ); - } - async getRefreshToken(options?: StorageOptions): Promise<string> { options = await this.getTimeoutBasedStorageOptions(options); return (await this.getAccount(options))?.tokens?.refreshToken; diff --git a/libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts b/libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts new file mode 100644 index 00000000000..ae1b30caa22 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.spec.ts @@ -0,0 +1,145 @@ +import { any, MockProxy } from "jest-mock-extended"; + +import { MigrationHelper } from "../migration-helper"; +import { mockMigrationHelper } from "../migration-helper.spec"; + +import { ProviderMigrator } from "./28-move-provider-state-to-state-provider"; + +function exampleProvider1() { + return JSON.stringify({ + id: "id", + name: "name", + status: 0, + type: 0, + enabled: true, + useEvents: true, + }); +} + +function exampleJSON() { + return { + global: { + otherStuff: "otherStuff1", + }, + authenticatedAccounts: ["user-1", "user-2"], + "user-1": { + data: { + providers: { + "provider-id-1": exampleProvider1(), + "provider-id-2": { + // ... + }, + }, + otherStuff: "overStuff2", + }, + otherStuff: "otherStuff3", + }, + "user-2": { + data: { + otherStuff: "otherStuff4", + }, + otherStuff: "otherStuff5", + }, + }; +} + +function rollbackJSON() { + return { + "user_user-1_providers_providers": { + "provider-id-1": exampleProvider1(), + "provider-id-2": { + // ... + }, + }, + "user_user-2_providers_providers": null as any, + global: { + otherStuff: "otherStuff1", + }, + authenticatedAccounts: ["user-1", "user-2"], + "user-1": { + data: { + otherStuff: "overStuff2", + }, + otherStuff: "otherStuff3", + }, + "user-2": { + data: { + otherStuff: "otherStuff4", + }, + otherStuff: "otherStuff5", + }, + }; +} + +describe("ProviderMigrator", () => { + let helper: MockProxy<MigrationHelper>; + let sut: ProviderMigrator; + const keyDefinitionLike = { + key: "providers", + stateDefinition: { + name: "providers", + }, + }; + + describe("migrate", () => { + beforeEach(() => { + helper = mockMigrationHelper(exampleJSON(), 28); + sut = new ProviderMigrator(27, 28); + }); + + it("should remove providers from all accounts", async () => { + await sut.migrate(helper); + expect(helper.set).toHaveBeenCalledWith("user-1", { + data: { + otherStuff: "overStuff2", + }, + otherStuff: "otherStuff3", + }); + }); + + it("should set providers value for each account", async () => { + await sut.migrate(helper); + + expect(helper.setToUser).toHaveBeenCalledWith("user-1", keyDefinitionLike, { + "provider-id-1": exampleProvider1(), + "provider-id-2": { + // ... + }, + }); + }); + }); + + describe("rollback", () => { + beforeEach(() => { + helper = mockMigrationHelper(rollbackJSON(), 27); + sut = new ProviderMigrator(27, 28); + }); + + it.each(["user-1", "user-2"])("should null out new values", async (userId) => { + await sut.rollback(helper); + expect(helper.setToUser).toHaveBeenCalledWith(userId, keyDefinitionLike, null); + }); + + it("should add explicit value back to accounts", async () => { + await sut.rollback(helper); + + expect(helper.set).toHaveBeenCalledWith("user-1", { + data: { + providers: { + "provider-id-1": exampleProvider1(), + "provider-id-2": { + // ... + }, + }, + otherStuff: "overStuff2", + }, + otherStuff: "otherStuff3", + }); + }); + + it("should not try to restore values to missing accounts", async () => { + await sut.rollback(helper); + expect(helper.set).not.toHaveBeenCalledWith("user-3", any()); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.ts b/libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.ts new file mode 100644 index 00000000000..3bb5874ee88 --- /dev/null +++ b/libs/common/src/state-migrations/migrations/28-move-provider-state-to-state-provider.ts @@ -0,0 +1,71 @@ +import { Jsonify } from "type-fest"; + +import { KeyDefinitionLike, MigrationHelper } from "../migration-helper"; +import { Migrator } from "../migrator"; + +enum ProviderUserStatusType { + Invited = 0, + Accepted = 1, + Confirmed = 2, + Revoked = -1, +} + +enum ProviderUserType { + ProviderAdmin = 0, + ServiceUser = 1, +} + +type ProviderData = { + id: string; + name: string; + status: ProviderUserStatusType; + type: ProviderUserType; + enabled: boolean; + userId: string; + useEvents: boolean; +}; + +type ExpectedAccountType = { + data?: { + providers?: Record<string, Jsonify<ProviderData>>; + }; +}; + +const USER_PROVIDERS: KeyDefinitionLike = { + key: "providers", + stateDefinition: { + name: "providers", + }, +}; + +export class ProviderMigrator extends Migrator<27, 28> { + async migrate(helper: MigrationHelper): Promise<void> { + const accounts = await helper.getAccounts<ExpectedAccountType>(); + async function migrateAccount(userId: string, account: ExpectedAccountType): Promise<void> { + const value = account?.data?.providers; + if (value != null) { + await helper.setToUser(userId, USER_PROVIDERS, value); + delete account.data.providers; + await helper.set(userId, account); + } + } + + await Promise.all(accounts.map(({ userId, account }) => migrateAccount(userId, account))); + } + + async rollback(helper: MigrationHelper): Promise<void> { + const accounts = await helper.getAccounts<ExpectedAccountType>(); + async function rollbackAccount(userId: string, account: ExpectedAccountType): Promise<void> { + const value = await helper.getFromUser(userId, USER_PROVIDERS); + if (account) { + account.data = Object.assign(account.data ?? {}, { + providers: value, + }); + await helper.set(userId, account); + } + await helper.setToUser(userId, USER_PROVIDERS, null); + } + + await Promise.all(accounts.map(({ userId, account }) => rollbackAccount(userId, account))); + } +} From 16c5fe65caaebf5eb4594ce88d0391b0244dc10a Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez <cesar.a.gonzalezcs@gmail.com> Date: Tue, 5 Mar 2024 13:39:58 -0600 Subject: [PATCH 13/14] [PM-5876] Adjust the `LpFilelessImporter.supressDownload` method to inject through the `executeScript` API instead within manifest v3 (#7787) * [PM-5876] Adjust LP Fileless Importer to Suppress Download with DOM Append in Manifest v3 * [PM-5876] Incorporating jest tests for affected logic * [PM-5876] Fixing jest test that leverages rxjs * [PM-5876] Updating documentation within BrowserApi.executeScriptInTab * [PM-5876] Implementing jest tests for the new LP suppress download content scripts * [PM-5876] Adding a change to webpack to ensure we do not package the mv2 side script for `lp-suppress-import-download.mv2.ts` if building the extension for mv3 * [PM-5876] Implementing changes based on feedback during code review * [PM-5876] Implementing changes based on feedback during code review * [PM-5876] Implementing changes based on feedback during code review * [PM-5876] Implementing changes based on feedback during code review * [PM-5876] Implementing a configuration to feed the script injection of the Fileless Importer CSV download supression script --- apps/browser/src/manifest.json | 1 + apps/browser/src/manifest.v3.json | 6 ++ .../src/platform/browser/browser-api.spec.ts | 54 +++++++++++++ .../src/platform/browser/browser-api.ts | 16 +++- .../fileless-importer.background.ts | 6 ++ .../fileless-importer.background.spec.ts | 57 ++++++++++--- .../fileless-importer.background.ts | 25 ++++++ .../fileless-importer-injected-scripts.ts | 22 +++++ .../content/lp-fileless-importer.spec.ts | 14 ---- .../src/tools/content/lp-fileless-importer.ts | 41 ---------- ...-import-download-script-append.mv2.spec.ts | 21 +++++ ...press-import-download-script-append.mv2.ts | 9 +++ .../lp-suppress-import-download.spec.ts | 81 +++++++++++++++++++ .../content/lp-suppress-import-download.ts | 50 ++++++++++++ apps/browser/webpack.config.js | 3 + 15 files changed, 338 insertions(+), 68 deletions(-) create mode 100644 apps/browser/src/tools/config/fileless-importer-injected-scripts.ts create mode 100644 apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.spec.ts create mode 100644 apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.ts create mode 100644 apps/browser/src/tools/content/lp-suppress-import-download.spec.ts create mode 100644 apps/browser/src/tools/content/lp-suppress-import-download.ts diff --git a/apps/browser/src/manifest.json b/apps/browser/src/manifest.json index ff36eae02ed..f56395cd999 100644 --- a/apps/browser/src/manifest.json +++ b/apps/browser/src/manifest.json @@ -108,6 +108,7 @@ }, "web_accessible_resources": [ "content/fido2/page-script.js", + "content/lp-suppress-import-download.js", "notification/bar.html", "images/icon38.png", "images/icon38_locked.png", diff --git a/apps/browser/src/manifest.v3.json b/apps/browser/src/manifest.v3.json index 912945af9b8..7bbf95234b5 100644 --- a/apps/browser/src/manifest.v3.json +++ b/apps/browser/src/manifest.v3.json @@ -31,6 +31,12 @@ "matches": ["http://*/*", "https://*/*", "file:///*"], "run_at": "document_start" }, + { + "all_frames": false, + "js": ["content/lp-fileless-importer.js"], + "matches": ["https://lastpass.com/export.php"], + "run_at": "document_start" + }, { "all_frames": true, "css": ["content/autofill.css"], diff --git a/apps/browser/src/platform/browser/browser-api.spec.ts b/apps/browser/src/platform/browser/browser-api.spec.ts index 5bf2507194e..3761f421085 100644 --- a/apps/browser/src/platform/browser/browser-api.spec.ts +++ b/apps/browser/src/platform/browser/browser-api.spec.ts @@ -320,6 +320,60 @@ describe("BrowserApi", () => { }, files: [injectDetails.file], injectImmediately: true, + world: "ISOLATED", + }); + expect(result).toEqual(executeScriptResult); + }); + + it("injects the script into a specified frameId when the extension is built for manifest v3", async () => { + const tabId = 1; + const frameId = 2; + const injectDetails = mock<chrome.tabs.InjectDetails>({ + file: "file.js", + allFrames: true, + runAt: "document_start", + frameId, + }); + jest.spyOn(BrowserApi, "manifestVersion", "get").mockReturnValue(3); + (chrome.scripting.executeScript as jest.Mock).mockResolvedValue(executeScriptResult); + + await BrowserApi.executeScriptInTab(tabId, injectDetails); + + expect(chrome.scripting.executeScript).toHaveBeenCalledWith({ + target: { + tabId: tabId, + allFrames: injectDetails.allFrames, + frameIds: [frameId], + }, + files: [injectDetails.file], + injectImmediately: true, + world: "ISOLATED", + }); + }); + + it("injects the script into the MAIN world context when injecting a script for manifest v3", async () => { + const tabId = 1; + const injectDetails = mock<chrome.tabs.InjectDetails>({ + file: null, + allFrames: true, + runAt: "document_start", + frameId: null, + }); + const scriptingApiDetails = { world: "MAIN" as chrome.scripting.ExecutionWorld }; + jest.spyOn(BrowserApi, "manifestVersion", "get").mockReturnValue(3); + (chrome.scripting.executeScript as jest.Mock).mockResolvedValue(executeScriptResult); + + const result = await BrowserApi.executeScriptInTab(tabId, injectDetails, scriptingApiDetails); + + expect(chrome.scripting.executeScript).toHaveBeenCalledWith({ + target: { + tabId: tabId, + allFrames: injectDetails.allFrames, + frameIds: null, + }, + files: null, + injectImmediately: true, + world: "MAIN", }); expect(result).toEqual(executeScriptResult); }); diff --git a/apps/browser/src/platform/browser/browser-api.ts b/apps/browser/src/platform/browser/browser-api.ts index 3321296e9e2..b09f3e02664 100644 --- a/apps/browser/src/platform/browser/browser-api.ts +++ b/apps/browser/src/platform/browser/browser-api.ts @@ -475,12 +475,19 @@ export class BrowserApi { /** * Extension API helper method used to execute a script in a tab. + * * @see https://developer.chrome.com/docs/extensions/reference/tabs/#method-executeScript - * @param {number} tabId - * @param {chrome.tabs.InjectDetails} details - * @returns {Promise<unknown>} + * @param tabId - The id of the tab to execute the script in. + * @param details {@link "InjectDetails" https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/extensionTypes/InjectDetails} + * @param scriptingApiDetails {@link "ExecutionWorld" https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/scripting/ExecutionWorld} */ - static executeScriptInTab(tabId: number, details: chrome.tabs.InjectDetails) { + static executeScriptInTab( + tabId: number, + details: chrome.tabs.InjectDetails, + scriptingApiDetails?: { + world: chrome.scripting.ExecutionWorld; + }, + ): Promise<unknown> { if (BrowserApi.manifestVersion === 3) { return chrome.scripting.executeScript({ target: { @@ -490,6 +497,7 @@ export class BrowserApi { }, files: details.file ? [details.file] : null, injectImmediately: details.runAt === "document_start", + world: scriptingApiDetails?.world || "ISOLATED", }); } diff --git a/apps/browser/src/tools/background/abstractions/fileless-importer.background.ts b/apps/browser/src/tools/background/abstractions/fileless-importer.background.ts index 2ade5bf7672..e4b84137183 100644 --- a/apps/browser/src/tools/background/abstractions/fileless-importer.background.ts +++ b/apps/browser/src/tools/background/abstractions/fileless-importer.background.ts @@ -1,5 +1,10 @@ import { FilelessImportTypeKeys } from "../../enums/fileless-import.enums"; +type SuppressDownloadScriptInjectionConfig = { + file: string; + scriptingApiDetails?: { world: chrome.scripting.ExecutionWorld }; +}; + type FilelessImportPortMessage = { command?: string; importType?: FilelessImportTypeKeys; @@ -27,6 +32,7 @@ interface FilelessImporterBackground { } export { + SuppressDownloadScriptInjectionConfig, FilelessImportPortMessage, ImportNotificationMessageHandlers, LpImporterMessageHandlers, diff --git a/apps/browser/src/tools/background/fileless-importer.background.spec.ts b/apps/browser/src/tools/background/fileless-importer.background.spec.ts index 9fc87d65a43..d3436099ef1 100644 --- a/apps/browser/src/tools/background/fileless-importer.background.spec.ts +++ b/apps/browser/src/tools/background/fileless-importer.background.spec.ts @@ -1,4 +1,5 @@ import { mock } from "jest-mock-extended"; +import { firstValueFrom } from "rxjs"; import { PolicyService } from "@bitwarden/common/admin-console/services/policy/policy.service"; import { AuthenticationStatus } from "@bitwarden/common/auth/enums/authentication-status"; @@ -14,10 +15,20 @@ import { sendPortMessage, triggerRuntimeOnConnectEvent, } from "../../autofill/spec/testing-utils"; +import { BrowserApi } from "../../platform/browser/browser-api"; import { FilelessImportPort, FilelessImportType } from "../enums/fileless-import.enums"; import FilelessImporterBackground from "./fileless-importer.background"; +jest.mock("rxjs", () => { + const rxjs = jest.requireActual("rxjs"); + const { firstValueFrom } = rxjs; + return { + ...rxjs, + firstValueFrom: jest.fn(firstValueFrom), + }; +}); + describe("FilelessImporterBackground ", () => { let filelessImporterBackground: FilelessImporterBackground; const configService = mock<ConfigService>(); @@ -51,14 +62,17 @@ describe("FilelessImporterBackground ", () => { describe("handle ports onConnect", () => { let lpImporterPort: chrome.runtime.Port; + let manifestVersionSpy: jest.SpyInstance; + let executeScriptInTabSpy: jest.SpyInstance; beforeEach(() => { lpImporterPort = createPortSpyMock(FilelessImportPort.LpImporter); + manifestVersionSpy = jest.spyOn(BrowserApi, "manifestVersion", "get"); + executeScriptInTabSpy = jest.spyOn(BrowserApi, "executeScriptInTab").mockResolvedValue(null); jest.spyOn(authService, "getAuthStatus").mockResolvedValue(AuthenticationStatus.Unlocked); jest.spyOn(configService, "getFeatureFlag").mockResolvedValue(true); - jest - .spyOn(filelessImporterBackground as any, "removeIndividualVault") - .mockResolvedValue(false); + jest.spyOn(filelessImporterBackground as any, "removeIndividualVault"); + (firstValueFrom as jest.Mock).mockResolvedValue(false); }); it("ignores the port connection if the port name is not present in the set of filelessImportNames", async () => { @@ -83,9 +97,7 @@ describe("FilelessImporterBackground ", () => { }); it("posts a message to the port indicating that the fileless import feature is disabled if the user's policy removes individual vaults", async () => { - jest - .spyOn(filelessImporterBackground as any, "removeIndividualVault") - .mockResolvedValue(true); + (firstValueFrom as jest.Mock).mockResolvedValue(true); triggerRuntimeOnConnectEvent(lpImporterPort); await flushPromises(); @@ -117,6 +129,35 @@ describe("FilelessImporterBackground ", () => { filelessImportEnabled: true, }); }); + + it("triggers an injection of the `lp-suppress-import-download.js` script in manifest v3", async () => { + manifestVersionSpy.mockReturnValue(3); + + triggerRuntimeOnConnectEvent(lpImporterPort); + await flushPromises(); + + expect(executeScriptInTabSpy).toHaveBeenCalledWith( + lpImporterPort.sender.tab.id, + { file: "content/lp-suppress-import-download.js", runAt: "document_start" }, + { world: "MAIN" }, + ); + }); + + it("triggers an injection of the `lp-suppress-import-download-script-append-mv2.js` script in manifest v2", async () => { + manifestVersionSpy.mockReturnValue(2); + + triggerRuntimeOnConnectEvent(lpImporterPort); + await flushPromises(); + + expect(executeScriptInTabSpy).toHaveBeenCalledWith( + lpImporterPort.sender.tab.id, + { + file: "content/lp-suppress-import-download-script-append-mv2.js", + runAt: "document_start", + }, + undefined, + ); + }); }); describe("port messages", () => { @@ -126,9 +167,7 @@ describe("FilelessImporterBackground ", () => { beforeEach(async () => { jest.spyOn(authService, "getAuthStatus").mockResolvedValue(AuthenticationStatus.Unlocked); jest.spyOn(configService, "getFeatureFlag").mockResolvedValue(true); - jest - .spyOn(filelessImporterBackground as any, "removeIndividualVault") - .mockResolvedValue(false); + (firstValueFrom as jest.Mock).mockResolvedValue(false); triggerRuntimeOnConnectEvent(createPortSpyMock(FilelessImportPort.NotificationBar)); triggerRuntimeOnConnectEvent(createPortSpyMock(FilelessImportPort.LpImporter)); await flushPromises(); diff --git a/apps/browser/src/tools/background/fileless-importer.background.ts b/apps/browser/src/tools/background/fileless-importer.background.ts index f7f32c67d59..3ddc7bd1b76 100644 --- a/apps/browser/src/tools/background/fileless-importer.background.ts +++ b/apps/browser/src/tools/background/fileless-importer.background.ts @@ -11,6 +11,7 @@ import { ImportServiceAbstraction } from "@bitwarden/importer/core"; import NotificationBackground from "../../autofill/background/notification.background"; import { BrowserApi } from "../../platform/browser/browser-api"; +import { FilelessImporterInjectedScriptsConfig } from "../config/fileless-importer-injected-scripts"; import { FilelessImportPort, FilelessImportType, @@ -22,6 +23,7 @@ import { LpImporterMessageHandlers, FilelessImporterBackground as FilelessImporterBackgroundInterface, FilelessImportPortMessage, + SuppressDownloadScriptInjectionConfig, } from "./abstractions/fileless-importer.background"; class FilelessImporterBackground implements FilelessImporterBackgroundInterface { @@ -108,6 +110,23 @@ class FilelessImporterBackground implements FilelessImporterBackgroundInterface await this.notificationBackground.requestFilelessImport(tab, importType); } + /** + * Injects the script used to suppress the download of the LP importer export file. + * + * @param sender - The sender of the message. + * @param injectionConfig - The configuration for the injection. + */ + private async injectScriptConfig( + sender: chrome.runtime.MessageSender, + injectionConfig: SuppressDownloadScriptInjectionConfig, + ) { + await BrowserApi.executeScriptInTab( + sender.tab.id, + { file: injectionConfig.file, runAt: "document_start" }, + injectionConfig.scriptingApiDetails, + ); + } + /** * Triggers the download of the CSV file from the LP importer. This is triggered * when the user opts to not save the export to Bitwarden within the notification bar. @@ -200,6 +219,12 @@ class FilelessImporterBackground implements FilelessImporterBackgroundInterface switch (port.name) { case FilelessImportPort.LpImporter: this.lpImporterPort = port; + await this.injectScriptConfig( + port.sender, + BrowserApi.manifestVersion === 3 + ? FilelessImporterInjectedScriptsConfig.LpSuppressImportDownload.mv3 + : FilelessImporterInjectedScriptsConfig.LpSuppressImportDownload.mv2, + ); break; case FilelessImportPort.NotificationBar: this.importNotificationsPort = port; diff --git a/apps/browser/src/tools/config/fileless-importer-injected-scripts.ts b/apps/browser/src/tools/config/fileless-importer-injected-scripts.ts new file mode 100644 index 00000000000..dbc05fe18c0 --- /dev/null +++ b/apps/browser/src/tools/config/fileless-importer-injected-scripts.ts @@ -0,0 +1,22 @@ +import { SuppressDownloadScriptInjectionConfig } from "../background/abstractions/fileless-importer.background"; + +type FilelessImporterInjectedScriptsConfigurations = { + LpSuppressImportDownload: { + mv2: SuppressDownloadScriptInjectionConfig; + mv3: SuppressDownloadScriptInjectionConfig; + }; +}; + +const FilelessImporterInjectedScriptsConfig: FilelessImporterInjectedScriptsConfigurations = { + LpSuppressImportDownload: { + mv2: { + file: "content/lp-suppress-import-download-script-append-mv2.js", + }, + mv3: { + file: "content/lp-suppress-import-download.js", + scriptingApiDetails: { world: "MAIN" }, + }, + }, +} as const; + +export { FilelessImporterInjectedScriptsConfig }; diff --git a/apps/browser/src/tools/content/lp-fileless-importer.spec.ts b/apps/browser/src/tools/content/lp-fileless-importer.spec.ts index 9646eaa893b..432754ab91c 100644 --- a/apps/browser/src/tools/content/lp-fileless-importer.spec.ts +++ b/apps/browser/src/tools/content/lp-fileless-importer.spec.ts @@ -43,20 +43,6 @@ describe("LpFilelessImporter", () => { expect(portSpy.disconnect).toHaveBeenCalled(); }); - it("injects a script element that suppresses the download of the LastPass export", () => { - const script = document.createElement("script"); - jest.spyOn(document, "createElement").mockReturnValue(script); - jest.spyOn(document.documentElement, "appendChild"); - - lpFilelessImporter.handleFeatureFlagVerification({ filelessImportEnabled: true }); - - expect(document.createElement).toHaveBeenCalledWith("script"); - expect(document.documentElement.appendChild).toHaveBeenCalled(); - expect(script.textContent).toContain( - "const defaultAppendChild = Element.prototype.appendChild;", - ); - }); - it("sets up an event listener for DOMContentLoaded that triggers the importer when the document ready state is `loading`", () => { Object.defineProperty(document, "readyState", { value: "loading", diff --git a/apps/browser/src/tools/content/lp-fileless-importer.ts b/apps/browser/src/tools/content/lp-fileless-importer.ts index 107159e5a5e..6f091ecf5a5 100644 --- a/apps/browser/src/tools/content/lp-fileless-importer.ts +++ b/apps/browser/src/tools/content/lp-fileless-importer.ts @@ -36,7 +36,6 @@ class LpFilelessImporter implements LpFilelessImporterInterface { return; } - this.suppressDownload(); if (document.readyState === "loading") { document.addEventListener("DOMContentLoaded", this.loadImporter); return; @@ -52,46 +51,6 @@ class LpFilelessImporter implements LpFilelessImporterInterface { this.postWindowMessage({ command: "triggerCsvDownload" }); } - /** - * Suppresses the download of the CSV file by overriding the `download` attribute of the - * anchor element that is created by the LP importer. This is done by injecting a script - * into the page that overrides the `appendChild` method of the `Element` prototype. - */ - private suppressDownload() { - const script = document.createElement("script"); - script.textContent = ` - let csvDownload = ''; - let csvHref = ''; - const defaultAppendChild = Element.prototype.appendChild; - Element.prototype.appendChild = function (newChild) { - if (newChild.nodeName.toLowerCase() === 'a' && newChild.download) { - csvDownload = newChild.download; - csvHref = newChild.href; - newChild.setAttribute('href', 'javascript:void(0)'); - newChild.setAttribute('download', ''); - Element.prototype.appendChild = defaultAppendChild; - } - - return defaultAppendChild.call(this, newChild); - }; - - window.addEventListener('message', (event) => { - const command = event.data?.command; - if (event.source !== window || command !== 'triggerCsvDownload') { - return; - } - - const anchor = document.createElement('a'); - anchor.setAttribute('href', csvHref); - anchor.setAttribute('download', csvDownload); - document.body.appendChild(anchor); - anchor.click(); - document.body.removeChild(anchor); - }); - `; - document.documentElement.appendChild(script); - } - /** * Initializes the importing mechanism used to import the CSV file into Bitwarden. * This is done by observing the DOM for the addition of the LP importer element. diff --git a/apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.spec.ts b/apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.spec.ts new file mode 100644 index 00000000000..95b49ea00ec --- /dev/null +++ b/apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.spec.ts @@ -0,0 +1,21 @@ +describe("LP Suppress Import Download for Manifest v2", () => { + it("appends the `lp-suppress-import-download.js` script to the document element", () => { + let createdScriptElement: HTMLScriptElement; + jest.spyOn(window.document, "createElement"); + jest.spyOn(window.document.documentElement, "appendChild").mockImplementation((node) => { + createdScriptElement = node as HTMLScriptElement; + return node; + }); + + require("./lp-suppress-import-download-script-append.mv2"); + + expect(window.document.createElement).toHaveBeenCalledWith("script"); + expect(chrome.runtime.getURL).toHaveBeenCalledWith("content/lp-suppress-import-download.js"); + expect(window.document.documentElement.appendChild).toHaveBeenCalledWith( + expect.any(HTMLScriptElement), + ); + expect(createdScriptElement.src).toBe( + "chrome-extension://id/content/lp-suppress-import-download.js", + ); + }); +}); diff --git a/apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.ts b/apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.ts new file mode 100644 index 00000000000..cd641590ad1 --- /dev/null +++ b/apps/browser/src/tools/content/lp-suppress-import-download-script-append.mv2.ts @@ -0,0 +1,9 @@ +/** + * This script handles injection of the LP suppress import download script into the document. + * This is required for manifest v2, but will be removed when we migrate fully to manifest v3. + */ +(function (globalContext) { + const script = globalContext.document.createElement("script"); + script.src = chrome.runtime.getURL("content/lp-suppress-import-download.js"); + globalContext.document.documentElement.appendChild(script); +})(window); diff --git a/apps/browser/src/tools/content/lp-suppress-import-download.spec.ts b/apps/browser/src/tools/content/lp-suppress-import-download.spec.ts new file mode 100644 index 00000000000..bfff3787506 --- /dev/null +++ b/apps/browser/src/tools/content/lp-suppress-import-download.spec.ts @@ -0,0 +1,81 @@ +import { flushPromises, postWindowMessage } from "../../autofill/spec/testing-utils"; + +describe("LP Suppress Import Download", () => { + const downloadAttribute = "file.csv"; + const hrefAttribute = "https://example.com/file.csv"; + const overridenHrefAttribute = "javascript:void(0)"; + let anchor: HTMLAnchorElement; + + beforeEach(() => { + jest.spyOn(Element.prototype, "appendChild"); + jest.spyOn(window, "addEventListener"); + + require("./lp-suppress-import-download"); + + anchor = document.createElement("a"); + anchor.download = downloadAttribute; + anchor.href = hrefAttribute; + anchor.click = jest.fn(); + }); + + afterEach(() => { + jest.resetModules(); + jest.clearAllMocks(); + }); + + it("disables the automatic download anchor", () => { + document.body.appendChild(anchor); + + expect(anchor.href).toBe(overridenHrefAttribute); + expect(anchor.download).toBe(""); + }); + + it("triggers the CSVDownload when receiving a `triggerCsvDownload` window message", async () => { + window.document.createElement = jest.fn(() => anchor); + jest.spyOn(window, "removeEventListener"); + + document.body.appendChild(anchor); + + // Precondition - Ensure the anchor in the document has overridden href and download attributes + expect(anchor.href).toBe(overridenHrefAttribute); + expect(anchor.download).toBe(""); + + postWindowMessage({ command: "triggerCsvDownload" }); + await flushPromises(); + + expect(anchor.click).toHaveBeenCalled(); + expect(anchor.href).toEqual(hrefAttribute); + expect(anchor.download).toEqual(downloadAttribute); + expect(window.removeEventListener).toHaveBeenCalledWith("message", expect.any(Function)); + }); + + it("skips subsequent calls to trigger a CSVDownload", async () => { + window.document.createElement = jest.fn(() => anchor); + + document.body.appendChild(anchor); + + postWindowMessage({ command: "triggerCsvDownload" }); + await flushPromises(); + + postWindowMessage({ command: "triggerCsvDownload" }); + await flushPromises(); + + expect(anchor.click).toHaveBeenCalledTimes(1); + }); + + it("skips triggering the CSV download for window messages that do not have the correct command", () => { + document.body.appendChild(anchor); + + postWindowMessage({ command: "notTriggerCsvDownload" }); + + expect(anchor.click).not.toHaveBeenCalled(); + }); + + it("skips triggering the CSV download for window messages that do not have a data value", () => { + document.body.appendChild(anchor); + + postWindowMessage(null); + + expect(anchor.click).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/browser/src/tools/content/lp-suppress-import-download.ts b/apps/browser/src/tools/content/lp-suppress-import-download.ts new file mode 100644 index 00000000000..486d391279d --- /dev/null +++ b/apps/browser/src/tools/content/lp-suppress-import-download.ts @@ -0,0 +1,50 @@ +/** + * Handles intercepting the injection of the CSV download link, and ensures the + * download of the script is suppressed until the user opts to download the file. + * The download is triggered by a window message sent from the LpFilelessImporter + * content script. + */ +(function (globalContext) { + let csvDownload = ""; + let csvHref = ""; + let isCsvDownloadTriggered = false; + const defaultAppendChild = Element.prototype.appendChild; + Element.prototype.appendChild = function (newChild: Node) { + if (isAnchorElement(newChild) && newChild.download) { + csvDownload = newChild.download; + csvHref = newChild.href; + newChild.setAttribute("href", "javascript:void(0)"); + newChild.setAttribute("download", ""); + Element.prototype.appendChild = defaultAppendChild; + } + + return defaultAppendChild.call(this, newChild); + }; + + function isAnchorElement(node: Node): node is HTMLAnchorElement { + return node.nodeName.toLowerCase() === "a"; + } + + const handleWindowMessage = (event: MessageEvent) => { + const command = event.data?.command; + if ( + event.source !== globalContext || + command !== "triggerCsvDownload" || + isCsvDownloadTriggered + ) { + return; + } + + isCsvDownloadTriggered = true; + globalContext.removeEventListener("message", handleWindowMessage); + + const anchor = globalContext.document.createElement("a"); + anchor.setAttribute("href", csvHref); + anchor.setAttribute("download", csvDownload); + globalContext.document.body.appendChild(anchor); + anchor.click(); + globalContext.document.body.removeChild(anchor); + }; + + globalContext.addEventListener("message", handleWindowMessage); +})(window); diff --git a/apps/browser/webpack.config.js b/apps/browser/webpack.config.js index 8a074c259a3..19d65dbf05c 100644 --- a/apps/browser/webpack.config.js +++ b/apps/browser/webpack.config.js @@ -179,6 +179,7 @@ const mainConfig = { "overlay/list": "./src/autofill/overlay/pages/list/bootstrap-autofill-overlay-list.ts", "encrypt-worker": "../../libs/common/src/platform/services/cryptography/encrypt.worker.ts", "content/lp-fileless-importer": "./src/tools/content/lp-fileless-importer.ts", + "content/lp-suppress-import-download": "./src/tools/content/lp-suppress-import-download.ts", }, optimization: { minimize: ENV !== "development", @@ -276,6 +277,8 @@ if (manifestVersion == 2) { // Manifest V2 background pages can be run through the regular build pipeline. // Since it's a standard webpage. mainConfig.entry.background = "./src/platform/background.ts"; + mainConfig.entry["content/lp-suppress-import-download-script-append-mv2"] = + "./src/tools/content/lp-suppress-import-download-script-append.mv2.ts"; configs.push(mainConfig); } else { From 8e3a7239080fcf77adc49c3bae7199c782163ae0 Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez <cesar.a.gonzalezcs@gmail.com> Date: Tue, 5 Mar 2024 13:42:16 -0600 Subject: [PATCH 14/14] [PM-5879] Remove `backgroundWindow` reference used for determing system theme preference in Safari (#7859) * [PM-5879] Removing `backgroundWindow` reference used for determing system theme preference in Safari * [PM-5879] Removing `backgroundWindow` reference used for determing system theme preference in Safari * [PM-5879] Reworking factory logic within ThemingService factory --- apps/browser/src/background/main.background.ts | 3 --- .../src/popup/services/services.module.ts | 16 +++++++++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/apps/browser/src/background/main.background.ts b/apps/browser/src/background/main.background.ts index f4505ee0d95..94e466b9434 100644 --- a/apps/browser/src/background/main.background.ts +++ b/apps/browser/src/background/main.background.ts @@ -305,9 +305,6 @@ export default class MainBackground { stateEventRunnerService: StateEventRunnerService; ssoLoginService: SsoLoginServiceAbstraction; - // Passed to the popup for Safari to workaround issues with theming, downloading, etc. - backgroundWindow = window; - onUpdatedRan: boolean; onReplacedRan: boolean; loginToAutoFill: CipherView = null; diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index 2d9d4c68da8..d4c8ec57fda 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -509,13 +509,15 @@ function getBgService<T>(service: keyof MainBackground) { stateService: StateServiceAbstraction, platformUtilsService: PlatformUtilsService, ) => { - return new ThemingService( - stateService, - // Safari doesn't properly handle the (prefers-color-scheme) media query in the popup window, it always returns light. - // In Safari we have to use the background page instead, which comes with limitations like not dynamically changing the extension theme when the system theme is changed. - platformUtilsService.isSafari() ? getBgService<Window>("backgroundWindow")() : window, - document, - ); + // Safari doesn't properly handle the (prefers-color-scheme) media query in the popup window, it always returns light. + // In Safari, we have to use the background page instead, which comes with limitations like not dynamically changing the extension theme when the system theme is changed. + let windowContext = window; + const backgroundWindow = BrowserApi.getBackgroundPage(); + if (platformUtilsService.isSafari() && backgroundWindow) { + windowContext = backgroundWindow; + } + + return new ThemingService(stateService, windowContext, document); }, deps: [StateServiceAbstraction, PlatformUtilsService], },