increase state consistency of generator code

libs/common/src/tools/rx.ts

@@ -21,6 +21,8 @@ import {
   pairwise,
   MonoTypeOperatorFunction,
   Cons,
+  scan,
+  filter,
 } from "rxjs";
 
 import { ObservableTuple } from "./rx.rxjs";
@@ -245,3 +247,52 @@ export function pin<T>(options?: {
     }),
   );
 }
+
+/** maps a value to a source and keeps a LRC cache of the results
+ *  @param mapResult - maps the stream to a result; this function must return
+ *    a value. It must not return null or undefined.
+ *  @param options.size - the number of entries in the cache
+ *  @param options.key - maps the source to a cache key
+ *  @remarks - LRC is least recently created
+ */
+export function memoizedMap<Source, Result extends NonNullable<any>>(
+  mapResult: (source: Source) => Result,
+  options?: { size?: number; key?: (source: Source) => unknown },
+): OperatorFunction<Source, Result> {
+  return pipe(
+    // scan's accumulator contains the cache
+    scan(
+      ([cache], source) => {
+        const key: unknown = options?.key?.(source) ?? source;
+
+        // cache hit?
+        let result = cache?.get(key);
+        if (result) {
+          return [cache, result] as const;
+        }
+
+        // cache miss
+        result = mapResult(source);
+        cache?.set(key, result);
+
+        // trim cache
+        const overage = cache.size - (options?.size ?? 1);
+        if (overage > 0) {
+          Array.from(cache?.keys() ?? [])
+            .slice(0, overage)
+            .forEach((k) => cache?.delete(k));
+        }
+
+        return [cache, result] as const;
+      },
+      // FIXME: upgrade to a least-recently-used cache
+      [new Map(), null] as [Map<unknown, Result>, Source | null],
+    ),
+
+    // encapsulate cache
+    map(([, result]) => result),
+
+    // preserve `NonNullable` constraint on `Result`
+    filter((result): result is Result => !!result),
+  );
+}

libs/tools/generator/core/src/services/default-credential-generator.service.spec.ts

@@ -1,11 +1,14 @@
-import { BehaviorSubject, firstValueFrom, of } from "rxjs";
+import { BehaviorSubject, Subject, firstValueFrom, of } from "rxjs";
 
 import { Account } from "@bitwarden/common/auth/abstractions/account.service";
+import { ConsoleLogService } from "@bitwarden/common/platform/services/console-log.service";
 import { Site, VendorId } from "@bitwarden/common/tools/extension";
 import { Bitwarden } from "@bitwarden/common/tools/extension/vendor/bitwarden";
 import { Vendor } from "@bitwarden/common/tools/extension/vendor/data";
+import { SemanticLogger, ifEnabledSemanticLoggerProvider } from "@bitwarden/common/tools/log";
 import { UserId } from "@bitwarden/common/types/guid";
 
+import { awaitAsync } from "../../../../../common/spec";
 import {
   Algorithm,
   CredentialAlgorithm,
@@ -13,8 +16,10 @@ import {
   ForwarderExtensionId,
   GeneratorMetadata,
   Profile,
+  Type,
 } from "../metadata";
 import { CredentialGeneratorProviders } from "../providers";
+import { GenerateRequest, GeneratedCredential } from "../types";
 
 import { DefaultCredentialGeneratorService } from "./default-credential-generator.service";
 
@@ -36,19 +41,15 @@ describe("DefaultCredentialGeneratorService", () => {
   let service: DefaultCredentialGeneratorService;
   let providers: MockTwoLevelPartial<CredentialGeneratorProviders>;
   let system: any;
-  let mockLogger: any;
+  let log: SemanticLogger;
   let mockExtension: { settings: jest.Mock };
   let account: Account;
   let createService: (overrides?: any) => DefaultCredentialGeneratorService;
 
   beforeEach(() => {
-    mockLogger = {
-      info: jest.fn(),
-      debug: jest.fn(),
-      panic: jest.fn().mockImplementationOnce((c, m) => {
-        throw new Error(m ?? c);
-      }),
-    };
+    log = ifEnabledSemanticLoggerProvider(false, new ConsoleLogService(true), {
+      from: "DefaultCredentialGeneratorService tests",
+    });
 
     mockExtension = { settings: jest.fn() };
 
@@ -61,7 +62,7 @@ describe("DefaultCredentialGeneratorService", () => {
     };
 
     system = {
-      log: jest.fn().mockReturnValue(mockLogger),
+      log: jest.fn().mockReturnValue(log),
       extension: mockExtension,
     };
 
@@ -96,33 +97,46 @@ describe("DefaultCredentialGeneratorService", () => {
 
   describe("generate$", () => {
     it("should generate credentials when provided a specific algorithm", async () => {
-      const mockEngine = { generate: jest.fn().mockReturnValue(of("generatedPassword")) };
+      const mockEngine = {
+        generate: jest
+          .fn()
+          .mockReturnValue(
+            of(
+              new GeneratedCredential("generatedPassword", Type.password, Date.now(), "unit test"),
+            ),
+          ),
+      };
       const mockMetadata = {
-        id: "testAlgorithm",
+        id: Algorithm.password,
         engine: { create: jest.fn().mockReturnValue(mockEngine) },
       } as unknown as GeneratorMetadata<any>;
       const mockSettings = new BehaviorSubject({ length: 12 });
-
       providers.metadata!.metadata = jest.fn().mockReturnValue(mockMetadata);
       service = createService({
         settings: () => mockSettings as any,
       });
+      const on$ = new Subject<GenerateRequest>();
+      const account$ = new BehaviorSubject(account);
+      const result$ = new BehaviorSubject<GeneratedCredential | null>(null);
 
-      const dependencies = {
-        on$: of({ algorithm: "testAlgorithm" as CredentialAlgorithm }),
-        account$: of(account),
-      };
+      service.generate$({ on$, account$ }).subscribe(result$);
+      on$.next({ algorithm: Algorithm.password });
+      await awaitAsync();
 
-      const result = await firstValueFrom(service.generate$(dependencies));
-
-      expect(result).toBe("generatedPassword");
-      expect(providers.metadata!.metadata).toHaveBeenCalledWith("testAlgorithm");
+      expect(result$.value?.credential).toEqual("generatedPassword");
+      expect(providers.metadata!.metadata).toHaveBeenCalledWith(Algorithm.password);
       expect(mockMetadata.engine.create).toHaveBeenCalled();
       expect(mockEngine.generate).toHaveBeenCalled();
     });
 
     it("should determine preferred algorithm from credential type and generate credentials", async () => {
-      const mockEngine = { generate: jest.fn().mockReturnValue(of("generatedPassword")) };
+      const mockEngine = {
+        generate: jest
+          .fn()
+          .mockReturnValue(
+            of(new GeneratedCredential("generatedPassword", "password", Date.now(), "unit test")),
+          ),
+      };
       const mockMetadata = {
         id: "testAlgorithm",
         engine: { create: jest.fn().mockReturnValue(mockEngine) },
@@ -137,14 +151,16 @@ describe("DefaultCredentialGeneratorService", () => {
         settings: () => mockSettings as any,
       });
 
-      const dependencies = {
-        on$: of({ type: "password" as CredentialType }),
-        account$: of(account),
-      };
+      const on$ = new Subject<GenerateRequest>();
+      const account$ = new BehaviorSubject(account);
+      const result$ = new BehaviorSubject<GeneratedCredential | null>(null);
 
-      const result = await firstValueFrom(service.generate$(dependencies));
+      service.generate$({ on$, account$ }).subscribe(result$);
+      on$.next({ type: Type.password });
+      await awaitAsync();
 
-      expect(result).toBe("generatedPassword");
+      expect(result$.value?.credential).toBe("generatedPassword");
+      expect(result$.value?.category).toBe(Type.password);
       expect(providers.metadata!.metadata).toHaveBeenCalledWith("testAlgorithm");
     });
   });
@@ -219,10 +235,6 @@ describe("DefaultCredentialGeneratorService", () => {
       expect(() => service.algorithm("invalidAlgo" as CredentialAlgorithm)).toThrow(
         "invalid credential algorithm",
       );
-      expect(mockLogger.panic).toHaveBeenCalledWith(
-        { algorithm: "invalidAlgo" },
-        "invalid credential algorithm",
-      );
     });
   });
 
@@ -248,10 +260,6 @@ describe("DefaultCredentialGeneratorService", () => {
       providers.metadata!.metadata = jest.fn().mockReturnValue(null);
 
       expect(() => service.forwarder(invalidVendorId)).toThrow("invalid vendor");
-      expect(mockLogger.panic).toHaveBeenCalledWith(
-        { algorithm: invalidVendorId },
-        "invalid vendor",
-      );
     });
   });
 
@@ -314,10 +322,6 @@ describe("DefaultCredentialGeneratorService", () => {
       expect(() => service.settings(mockMetadata, { account$: of(account) })).toThrow(
         "failed to load settings; profile metadata not found",
       );
-      expect(mockLogger.panic).toHaveBeenCalledWith(
-        { algorithm: "test", profile: "account" },
-        "failed to load settings; profile metadata not found",
-      );
     });
   });
 
@@ -347,10 +351,6 @@ describe("DefaultCredentialGeneratorService", () => {
       expect(() => service.policy$(mockMetadata, { account$: of(account) })).toThrow(
         "failed to load policy; profile metadata not found",
       );
-      expect(mockLogger.panic).toHaveBeenCalledWith(
-        { algorithm: "test", profile: "account" },
-        "failed to load policy; profile metadata not found",
-      );
     });
   });
 });

libs/tools/generator/core/src/services/default-credential-generator.service.ts

@@ -1,15 +1,18 @@
 import {
-  EMPTY,
-  combineLatest,
+  ReplaySubject,
   concatMap,
-  distinctUntilChanged,
   filter,
+  first,
   map,
   of,
+  share,
   shareReplay,
+  switchAll,
   switchMap,
   takeUntil,
   tap,
+  timer,
+  zip,
 } from "rxjs";
 
 import { Account } from "@bitwarden/common/auth/abstractions/account.service";
@@ -17,7 +20,7 @@ import { BoundDependency, OnDependency } from "@bitwarden/common/tools/dependenc
 import { VendorId } from "@bitwarden/common/tools/extension";
 import { SemanticLogger } from "@bitwarden/common/tools/log";
 import { SystemServiceProvider } from "@bitwarden/common/tools/providers";
-import { anyComplete, withLatestReady } from "@bitwarden/common/tools/rx";
+import { anyComplete, memoizedMap } from "@bitwarden/common/tools/rx";
 import { UserStateSubject } from "@bitwarden/common/tools/state/user-state-subject";
 
 import { CredentialGeneratorService } from "../abstractions";
@@ -34,6 +37,9 @@ import { CredentialGeneratorProviders } from "../providers";
 import { GenerateRequest } from "../types";
 import { isAlgorithmRequest, isTypeRequest } from "../types/metadata-request";
 
+const ALGORITHM_CACHE_SIZE = 10;
+const THREE_MINUTES = 3 * 60 * 1000;
+
 export class DefaultCredentialGeneratorService implements CredentialGeneratorService {
   /** Instantiate the `DefaultCredentialGeneratorService`.
    *  @param provide application services required by the credential generator.
@@ -49,50 +55,73 @@ export class DefaultCredentialGeneratorService implements CredentialGeneratorSer
   private readonly log: SemanticLogger;
 
   generate$(dependencies: OnDependency<GenerateRequest> & BoundDependency<"account", Account>) {
-    // `on$` is partitioned into several streams so that the generator
-    // engine and settings refresh only when their respective inputs change
-    const on$ = dependencies.on$.pipe(shareReplay({ refCount: true, bufferSize: 1 }));
+    const request$ = dependencies.on$.pipe(shareReplay({ refCount: true, bufferSize: 1 }));
     const account$ = dependencies.account$.pipe(shareReplay({ refCount: true, bufferSize: 1 }));
 
     // load algorithm metadata
-    const metadata$ = on$.pipe(
-      switchMap((requested) => {
-        if (isAlgorithmRequest(requested)) {
-          return of(requested.algorithm);
-        } else if (isTypeRequest(requested)) {
-          return this.provide.metadata.preference$(requested.type, { account$ });
+    const metadata$ = request$.pipe(
+      switchMap((request) => {
+        if (isAlgorithmRequest(request)) {
+          return of(request.algorithm);
+        } else if (isTypeRequest(request)) {
+          return this.provide.metadata.preference$(request.type, { account$ }).pipe(first());
         } else {
-          this.log.warn(requested, "algorithm or category required");
-          return EMPTY;
+          this.log.panic(request, "algorithm or category required");
         }
       }),
       filter((algorithm): algorithm is CredentialAlgorithm => !!algorithm),
-      distinctUntilChanged(),
-      map((algorithm) => this.provide.metadata.metadata(algorithm)),
+      memoizedMap((algorithm) => this.provide.metadata.metadata(algorithm), {
+        size: ALGORITHM_CACHE_SIZE,
+      }),
       shareReplay({ refCount: true, bufferSize: 1 }),
     );
 
     // load the active profile's settings
-    const profile$ = on$.pipe(
-      map((request) => request.profile ?? Profile.account),
-      distinctUntilChanged(),
-    );
-    const settings$ = combineLatest([metadata$, profile$]).pipe(
-      tap(([metadata, profile]) =>
-        this.log.debug({ algorithm: metadata.id, profile }, "settings loaded"),
+    const settings$ = zip(request$, metadata$).pipe(
+      memoizedMap(
+        ([request, metadata]) => {
+          const profile = request.profile ?? Profile.account;
+          const algorithm = metadata.id;
+
+          // settings stays hot and buffers the most recent value in the cache
+          // for the next `request`
+          const settings$ = this.settings(metadata, { account$ }, profile).pipe(
+            tap(() => this.log.debug({ algorithm, profile }, "settings update received")),
+            share({
+              connector: () => new ReplaySubject<object>(1, THREE_MINUTES),
+              resetOnRefCountZero: () => timer(THREE_MINUTES),
+            }),
+            tap({
+              subscribe: () => this.log.debug({ algorithm, profile }, "settings hot"),
+              complete: () => this.log.debug({ algorithm, profile }, "settings cold"),
+            }),
+            first(),
+          );
+
+          this.log.debug({ algorithm, profile }, "settings cached");
+          return settings$;
+        },
+        { key: ([, metadata]) => metadata.id },
       ),
-      switchMap(([metadata, profile]) => this.settings(metadata, { account$ }, profile)),
+      switchAll(),
     );
 
     // load the algorithm's engine
     const engine$ = metadata$.pipe(
-      tap((metadata) => this.log.debug({ algorithm: metadata.id }, "engine selected")),
-      map((meta) => meta.engine.create(this.provide.generator)),
+      memoizedMap(
+        (metadata) => {
+          const engine = metadata.engine.create(this.provide.generator);
+
+          this.log.debug({ algorithm: metadata.id }, "engine cached");
+          return engine;
+        },
+        { size: ALGORITHM_CACHE_SIZE },
+      ),
     );
 
     // generation proper
-    const generate$ = on$.pipe(
-      withLatestReady(settings$, engine$),
+    const generate$ = zip([request$, settings$, engine$]).pipe(
+      tap(([request]) => this.log.debug(request, "generating credential")),
       concatMap(([request, settings, engine]) => engine.generate(request, settings)),
       takeUntil(anyComplete([settings$])),
     );