[PM-16231] Improved SDK referencing (#12475)

* feat: implement Rc

* feat: use Rc in sdk service

* docs: add an example to `take()`

* fix: clarify function doc

* Add custom eslint rule package with enforced `using` rule (#13009)

* feat: add custom eslint rule

* feat: check for `UsingRequired` instead of hardcoding `Rc`

* chore: move package to libs

* wip: add tests. Tests work when run from same folder but not from root

* fix: add dependencies to renovate

* fix: add empty ts file to avoid typechecking throwing errors

* fix: tests not running from root

* chore: remove unecessary config

* fix: linting

* docs: add readme

* chore: add platform ownership

* chore: clean up comment

* Add support for flat config to "Improved sdk referencing" (#13054)

* WIP flat config for eslint

* Add rxjs

* Configure vscode to use flat config

* Fix some new linting errors

* Remove directory overrides of .eslintrc

* Remove explicit dependencies on typescript-eslint/ and @angular-eslint/

* Add missing rules

* Add rxjs recommended rules

* Add storybook and enabled rxjs-angular rule

* Add buildNoRestrictedImports helper

* Ignore platform import restrictions

* Remove unused ignores

* feat: migrate rules over to .mjs and flat config

* feat: implement support for .mjs tests

* chore: remove old package approach

* chore: update package-lock

* fix: add empty TS file to stop errors

* chore: clean up comments

---------

Co-authored-by: Hinton <hinton@users.noreply.github.com>

* fix: update CODEOWNERS to match folder name

* fix: renovate.json after merge

* fix: package.json, pin versions, sort order

* fix: update package-lock.json

---------

Co-authored-by: Hinton <hinton@users.noreply.github.com>

.github/CODEOWNERS

@@ -97,6 +97,8 @@ apps/web/src/translation-constants.ts @bitwarden/team-platform-dev
 .github/workflows/scan.yml @bitwarden/team-platform-dev
 .github/workflows/test.yml @bitwarden/team-platform-dev
 .github/workflows/version-auto-bump.yml @bitwarden/team-platform-dev
+# ESLint custom rules
+libs/eslint @bitwarden/team-platform-dev
 
 ## Autofill team files ##
 apps/browser/src/autofill @bitwarden/team-autofill-dev

.github/renovate.json

@@ -211,6 +211,8 @@
         "@storybook/angular",
         "@storybook/manager-api",
         "@storybook/theming",
+        "@typescript-eslint/utils",
+        "@typescript-eslint/rule-tester",
         "@types/react",
         "autoprefixer",
         "bootstrap",

eslint.config.mjs

@@ -11,6 +11,8 @@ import rxjs from "eslint-plugin-rxjs";
 import angularRxjs from "eslint-plugin-rxjs-angular";
 import storybook from "eslint-plugin-storybook";
 
+import platformPlugins from "./libs/eslint/platform/index.mjs";
+
 export default tseslint.config(
   ...storybook.configs["flat/recommended"],
   {
@@ -28,6 +30,7 @@ export default tseslint.config(
     plugins: {
       rxjs: rxjs,
       "rxjs-angular": angularRxjs,
+      "@bitwarden/platform": platformPlugins,
     },
     languageOptions: {
       parserOptions: {
@@ -66,7 +69,7 @@ export default tseslint.config(
       "@angular-eslint/no-outputs-metadata-property": 0,
       "@angular-eslint/use-lifecycle-interface": "error",
       "@angular-eslint/use-pipe-transform-interface": 0,
-
+      "@bitwarden/platform/required-using": "error",
       "@typescript-eslint/explicit-member-accessibility": ["error", { accessibility: "no-public" }],
       "@typescript-eslint/no-explicit-any": "off", // TODO: This should be re-enabled
       "@typescript-eslint/no-floating-promises": "error",

jest.config.js

@@ -30,6 +30,7 @@ module.exports = {
     "<rootDir>/libs/billing/jest.config.js",
     "<rootDir>/libs/common/jest.config.js",
     "<rootDir>/libs/components/jest.config.js",
+    "<rootDir>/libs/eslint/jest.config.js",
     "<rootDir>/libs/tools/export/vault-export/vault-export-core/jest.config.js",
     "<rootDir>/libs/tools/generator/core/jest.config.js",
     "<rootDir>/libs/tools/generator/components/jest.config.js",

libs/common/src/platform/abstractions/sdk/sdk.service.ts

@@ -3,6 +3,7 @@ import { Observable } from "rxjs";
 import { BitwardenClient } from "@bitwarden/sdk-internal";
 
 import { UserId } from "../../../types/guid";
+import { Rc } from "../../misc/reference-counting/rc";
 
 export abstract class SdkService {
   /**
@@ -27,5 +28,5 @@ export abstract class SdkService {
    *
    * @param userId
    */
-  abstract userClient$(userId: UserId): Observable<BitwardenClient | undefined>;
+  abstract userClient$(userId: UserId): Observable<Rc<BitwardenClient> | undefined>;
 }

libs/common/src/platform/misc/reference-counting/rc.spec.ts

@@ -0,0 +1,93 @@
+// Temporary workaround for Symbol.dispose
+// remove when https://github.com/jestjs/jest/issues/14874 is resolved and *released*
+const disposeSymbol: unique symbol = Symbol("Symbol.dispose");
+const asyncDisposeSymbol: unique symbol = Symbol("Symbol.asyncDispose");
+(Symbol as any).asyncDispose ??= asyncDisposeSymbol as unknown as SymbolConstructor["asyncDispose"];
+(Symbol as any).dispose ??= disposeSymbol as unknown as SymbolConstructor["dispose"];
+
+// Import needs to be after the workaround
+import { Rc } from "./rc";
+
+export class FreeableTestValue {
+  isFreed = false;
+
+  free() {
+    this.isFreed = true;
+  }
+}
+
+describe("Rc", () => {
+  let value: FreeableTestValue;
+  let rc: Rc<FreeableTestValue>;
+
+  beforeEach(() => {
+    value = new FreeableTestValue();
+    rc = new Rc(value);
+  });
+
+  it("should increase refCount when taken", () => {
+    rc.take();
+
+    expect(rc["refCount"]).toBe(1);
+  });
+
+  it("should return value on take", () => {
+    // eslint-disable-next-line @bitwarden/platform/required-using
+    const reference = rc.take();
+
+    expect(reference.value).toBe(value);
+  });
+
+  it("should decrease refCount when disposing reference", () => {
+    // eslint-disable-next-line @bitwarden/platform/required-using
+    const reference = rc.take();
+
+    reference[Symbol.dispose]();
+
+    expect(rc["refCount"]).toBe(0);
+  });
+
+  it("should automatically decrease refCount when reference goes out of scope", () => {
+    {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      using reference = rc.take();
+    }
+
+    expect(rc["refCount"]).toBe(0);
+  });
+
+  it("should not free value when refCount reaches 0 if not marked for disposal", () => {
+    // eslint-disable-next-line @bitwarden/platform/required-using
+    const reference = rc.take();
+
+    reference[Symbol.dispose]();
+
+    expect(value.isFreed).toBe(false);
+  });
+
+  it("should free value when refCount reaches 0 and rc is marked for disposal", () => {
+    // eslint-disable-next-line @bitwarden/platform/required-using
+    const reference = rc.take();
+    rc.markForDisposal();
+
+    reference[Symbol.dispose]();
+
+    expect(value.isFreed).toBe(true);
+  });
+
+  it("should free value when marked for disposal if refCount is 0", () => {
+    // eslint-disable-next-line @bitwarden/platform/required-using
+    const reference = rc.take();
+    reference[Symbol.dispose]();
+
+    rc.markForDisposal();
+
+    expect(value.isFreed).toBe(true);
+  });
+
+  it("should throw error when trying to take a disposed reference", () => {
+    rc.markForDisposal();
+
+    expect(() => rc.take()).toThrow();
+  });
+});

libs/common/src/platform/misc/reference-counting/rc.ts

@@ -0,0 +1,76 @@
+import { UsingRequired } from "../using-required";
+
+export type Freeable = { free: () => void };
+
+/**
+ * Reference counted disposable value.
+ * This class is used to manage the lifetime of a value that needs to be
+ * freed of at a specific time but might still be in-use when that happens.
+ */
+export class Rc<T extends Freeable> {
+  private markedForDisposal = false;
+  private refCount = 0;
+  private value: T;
+
+  constructor(value: T) {
+    this.value = value;
+  }
+
+  /**
+   * Use this function when you want to use the underlying object.
+   * This will guarantee that you have a reference to the object
+   * and that it won't be freed until your reference goes out of scope.
+   *
+   * This function must be used with the `using` keyword.
+   *
+   * @example
+   * ```typescript
+   * function someFunction(rc: Rc<SomeValue>) {
+   *   using reference = rc.take();
+   *   reference.value.doSomething();
+   *   // reference is automatically disposed here
+   * }
+   * ```
+   *
+   * @returns The value.
+   */
+  take(): Ref<T> {
+    if (this.markedForDisposal) {
+      throw new Error("Cannot take a reference to a value marked for disposal");
+    }
+
+    this.refCount++;
+    return new Ref(() => this.release(), this.value);
+  }
+
+  /**
+   * Mark this Rc for disposal. When the refCount reaches 0, the value
+   * will be freed.
+   */
+  markForDisposal() {
+    this.markedForDisposal = true;
+    this.freeIfPossible();
+  }
+
+  private release() {
+    this.refCount--;
+    this.freeIfPossible();
+  }
+
+  private freeIfPossible() {
+    if (this.refCount === 0 && this.markedForDisposal) {
+      this.value.free();
+    }
+  }
+}
+
+export class Ref<T extends Freeable> implements UsingRequired {
+  constructor(
+    private readonly release: () => void,
+    readonly value: T,
+  ) {}
+
+  [Symbol.dispose]() {
+    this.release();
+  }
+}

libs/common/src/platform/misc/using-required.ts

@@ -0,0 +1,11 @@
+export type Disposable = { [Symbol.dispose]: () => void };
+
+/**
+ * Types implementing this type must be used together with the `using` keyword
+ *
+ * @example using ref = rc.take();
+ */
+// We want to use `interface` here because it creates a separate type.
+// Type aliasing would not expose `UsingRequired` to the linter.
+// eslint-disable-next-line @typescript-eslint/no-empty-object-type
+export interface UsingRequired extends Disposable {}

libs/common/src/platform/services/sdk/default-sdk.service.spec.ts

@@ -10,6 +10,7 @@ import { UserKey } from "../../../types/key";
 import { Environment, EnvironmentService } from "../../abstractions/environment.service";
 import { PlatformUtilsService } from "../../abstractions/platform-utils.service";
 import { SdkClientFactory } from "../../abstractions/sdk/sdk-client-factory";
+import { Rc } from "../../misc/reference-counting/rc";
 import { EncryptedString } from "../../models/domain/enc-string";
 import { SymmetricCryptoKey } from "../../models/domain/symmetric-crypto-key";
 
@@ -75,15 +76,14 @@ describe("DefaultSdkService", () => {
       });
 
       it("creates an SDK client when called the first time", async () => {
-        const result = await firstValueFrom(service.userClient$(userId));
+        await firstValueFrom(service.userClient$(userId));
 
-        expect(result).toBe(mockClient);
         expect(sdkClientFactory.createSdkClient).toHaveBeenCalled();
       });
 
       it("does not create an SDK client when called the second time with same userId", async () => {
-        const subject_1 = new BehaviorSubject(undefined);
-        const subject_2 = new BehaviorSubject(undefined);
+        const subject_1 = new BehaviorSubject<Rc<BitwardenClient> | undefined>(undefined);
+        const subject_2 = new BehaviorSubject<Rc<BitwardenClient> | undefined>(undefined);
 
         // Use subjects to ensure the subscription is kept alive
         service.userClient$(userId).subscribe(subject_1);
@@ -92,14 +92,14 @@ describe("DefaultSdkService", () => {
         // Wait for the next tick to ensure all async operations are done
         await new Promise(process.nextTick);
 
-        expect(subject_1.value).toBe(mockClient);
-        expect(subject_2.value).toBe(mockClient);
+        expect(subject_1.value.take().value).toBe(mockClient);
+        expect(subject_2.value.take().value).toBe(mockClient);
         expect(sdkClientFactory.createSdkClient).toHaveBeenCalledTimes(1);
       });
 
       it("destroys the SDK client when all subscriptions are closed", async () => {
-        const subject_1 = new BehaviorSubject(undefined);
-        const subject_2 = new BehaviorSubject(undefined);
+        const subject_1 = new BehaviorSubject<Rc<BitwardenClient> | undefined>(undefined);
+        const subject_2 = new BehaviorSubject<Rc<BitwardenClient> | undefined>(undefined);
         const subscription_1 = service.userClient$(userId).subscribe(subject_1);
         const subscription_2 = service.userClient$(userId).subscribe(subject_2);
         await new Promise(process.nextTick);
@@ -107,6 +107,7 @@ describe("DefaultSdkService", () => {
         subscription_1.unsubscribe();
         subscription_2.unsubscribe();
 
+        await new Promise(process.nextTick);
         expect(mockClient.free).toHaveBeenCalledTimes(1);
       });
 
@@ -114,7 +115,7 @@ describe("DefaultSdkService", () => {
         const userKey$ = new BehaviorSubject(new SymmetricCryptoKey(new Uint8Array(64)) as UserKey);
         keyService.userKey$.calledWith(userId).mockReturnValue(userKey$);
 
-        const subject = new BehaviorSubject(undefined);
+        const subject = new BehaviorSubject<Rc<BitwardenClient> | undefined>(undefined);
         service.userClient$(userId).subscribe(subject);
         await new Promise(process.nextTick);
 

libs/common/src/platform/services/sdk/default-sdk.service.ts

@@ -30,10 +30,11 @@ import { PlatformUtilsService } from "../../abstractions/platform-utils.service"
 import { SdkClientFactory } from "../../abstractions/sdk/sdk-client-factory";
 import { SdkService } from "../../abstractions/sdk/sdk.service";
 import { compareValues } from "../../misc/compare-values";
+import { Rc } from "../../misc/reference-counting/rc";
 import { EncryptedString } from "../../models/domain/enc-string";
 
 export class DefaultSdkService implements SdkService {
-  private sdkClientCache = new Map<UserId, Observable<BitwardenClient>>();
+  private sdkClientCache = new Map<UserId, Observable<Rc<BitwardenClient>>>();
 
   client$ = this.environmentService.environment$.pipe(
     concatMap(async (env) => {
@@ -58,7 +59,7 @@ export class DefaultSdkService implements SdkService {
     private userAgent: string = null,
   ) {}
 
-  userClient$(userId: UserId): Observable<BitwardenClient | undefined> {
+  userClient$(userId: UserId): Observable<Rc<BitwardenClient> | undefined> {
     // TODO: Figure out what happens when the user logs out
     if (this.sdkClientCache.has(userId)) {
       return this.sdkClientCache.get(userId);
@@ -88,32 +89,31 @@ export class DefaultSdkService implements SdkService {
       // switchMap is required to allow the clean-up logic to be executed when `combineLatest` emits a new value.
       switchMap(([env, account, kdfParams, privateKey, userKey, orgKeys]) => {
         // Create our own observable to be able to implement clean-up logic
-        return new Observable<BitwardenClient>((subscriber) => {
-          let client: BitwardenClient;
-
+        return new Observable<Rc<BitwardenClient>>((subscriber) => {
           const createAndInitializeClient = async () => {
             if (privateKey == null || userKey == null) {
               return undefined;
             }
 
             const settings = this.toSettings(env);
-            client = await this.sdkClientFactory.createSdkClient(settings, LogLevel.Info);
+            const client = await this.sdkClientFactory.createSdkClient(settings, LogLevel.Info);
 
             await this.initializeClient(client, account, kdfParams, privateKey, userKey, orgKeys);
 
             return client;
           };
 
+          let client: Rc<BitwardenClient>;
           createAndInitializeClient()
             .then((c) => {
-              client = c;
-              subscriber.next(c);
+              client = c === undefined ? undefined : new Rc(c);
+              subscriber.next(client);
             })
             .catch((e) => {
               subscriber.error(e);
             });
 
-          return () => client?.free();
+          return () => client?.markForDisposal();
         });
       }),
       tap({

libs/eslint/empty.ts

@@ -0,0 +1,2 @@
+// This file is used to avoid TS errors. This package only uses `tsconfig.json` for dynamically generated test files but
+// TS doesn't know that in the CI.

libs/eslint/jest.config.js

@@ -0,0 +1,10 @@
+const sharedConfig = require("../../libs/shared/jest.config.angular");
+
+/** @type {import('jest').Config} */
+module.exports = {
+  ...sharedConfig,
+  testMatch: ["**/+(*.)+(spec).+(mjs)"],
+  displayName: "libs/eslint tests",
+  preset: "jest-preset-angular",
+  setupFilesAfterEnv: ["<rootDir>/test.setup.mjs"],
+};

libs/eslint/platform/index.mjs

@@ -0,0 +1,3 @@
+import requiredUsing from "./required-using.mjs";
+
+export default { rules: { "required-using": requiredUsing } };

libs/eslint/platform/required-using.mjs

@@ -0,0 +1,83 @@
+import { ESLintUtils } from "@typescript-eslint/utils";
+
+export const errorMessage = "'using' keyword is required but not used";
+
+export default {
+  meta: {
+    type: "problem",
+    docs: {
+      description: "Ensure objects implementing UsingRequired are used with the using keyword",
+      category: "Best Practices",
+      recommended: false,
+    },
+    schema: [],
+  },
+  create(context) {
+    const parserServices = ESLintUtils.getParserServices(context);
+    const checker = parserServices.program.getTypeChecker();
+
+    // Function to check if a type implements the `UsingRequired` interface
+    function implementsUsingRequired(type) {
+      const symbol = type.getSymbol();
+      if (!symbol) {
+        return false;
+      }
+
+      const declarations = symbol.getDeclarations() || [];
+      for (const declaration of declarations) {
+        const heritageClauses = declaration.heritageClauses || [];
+        for (const clause of heritageClauses) {
+          if (
+            clause.types.some(
+              (typeExpression) =>
+                checker.typeToString(checker.getTypeAtLocation(typeExpression.expression)) ===
+                "UsingRequired",
+            )
+          ) {
+            return true;
+          }
+        }
+      }
+
+      return false;
+    }
+
+    // Function to check if a function call returns a `UsingRequired`
+    function returnsUsingRequired(node) {
+      if (node.type === "CallExpression") {
+        const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
+        const returnType = checker.getTypeAtLocation(tsNode);
+
+        return implementsUsingRequired(returnType);
+      }
+
+      return false;
+    }
+
+    return {
+      VariableDeclarator(node) {
+        // Skip if `using` is already present
+        if (node.parent.type === "VariableDeclaration" && node.parent.kind === "using") {
+          return;
+        }
+
+        // Check if the initializer returns a `UsingRequired`
+        if (node.init && returnsUsingRequired(node.init)) {
+          context.report({
+            node,
+            message: errorMessage,
+          });
+        }
+      },
+      AssignmentExpression(node) {
+        // Check if the right-hand side returns a `UsingRequired`
+        if (returnsUsingRequired(node.right)) {
+          context.report({
+            node,
+            message: errorMessage,
+          });
+        }
+      },
+    };
+  },
+};

libs/eslint/platform/required-using.spec.mjs

@@ -0,0 +1,98 @@
+import { RuleTester } from "@typescript-eslint/rule-tester";
+
+import rule, { errorMessage } from "./required-using.mjs";
+
+const ruleTester = new RuleTester({
+  languageOptions: {
+    parserOptions: {
+      project: [__dirname + "/../tsconfig.spec.json"],
+      projectService: {
+        allowDefaultProject: ["*.ts*"],
+      },
+      tsconfigRootDir: __dirname + "/..",
+    },
+  },
+});
+
+const setup = `
+  interface UsingRequired {}
+  class Ref implements UsingRequired {}
+
+  const rc = {
+    take(): Ref {
+      return new Ref();
+    },
+  };
+`;
+
+ruleTester.run("required-using", rule.default, {
+  valid: [
+    {
+      name: "Direct declaration with `using`",
+      code: `
+        ${setup}
+        using client = rc.take();
+      `,
+    },
+    {
+      name: "Function reference with `using`",
+      code: `
+        ${setup}
+        const t = rc.take;
+        using client = t();
+      `,
+    },
+  ],
+  invalid: [
+    {
+      name: "Direct declaration without `using`",
+      code: `
+        ${setup}
+        const client = rc.take();
+      `,
+      errors: [
+        {
+          message: errorMessage,
+        },
+      ],
+    },
+    {
+      name: "Assignment without `using`",
+      code: `
+        ${setup}
+        let client;
+        client = rc.take();
+      `,
+      errors: [
+        {
+          message: errorMessage,
+        },
+      ],
+    },
+    {
+      name: "Function reference without `using`",
+      code: `
+        ${setup}
+        const t = rc.take;
+        const client = t();
+      `,
+      errors: [
+        {
+          message: errorMessage,
+        },
+      ],
+    },
+    {
+      name: "Destructuring without `using`",
+      code: `
+        ${setup}
+        const { value } = rc.take();
+      `,
+      errors: [
+        {
+          message: errorMessage,
+        },
+      ],
+    },
+  ],
+});

libs/eslint/test.setup.mjs

@@ -0,0 +1,8 @@
+/* eslint-disable no-undef */
+
+import { clearImmediate, setImmediate } from "node:timers";
+
+Object.defineProperties(globalThis, {
+  clearImmediate: { value: clearImmediate },
+  setImmediate: { value: setImmediate },
+});

libs/eslint/tsconfig.json

@@ -0,0 +1,5 @@
+{
+  "extends": "../shared/tsconfig",
+  "compilerOptions": {},
+  "exclude": ["node_modules", "dist"]
+}

libs/eslint/tsconfig.spec.json

@@ -0,0 +1,3 @@
+{
+  "extends": "./tsconfig.json"
+}

package.json

@@ -78,6 +78,8 @@
     "@types/proper-lockfile": "4.1.4",
     "@types/retry": "0.12.5",
     "@types/zxcvbn": "4.4.5",
+    "@typescript-eslint/rule-tester": "8.22.0",
+    "@typescript-eslint/utils": "8.22.0",
     "@webcomponents/custom-elements": "1.6.0",
     "@yao-pkg/pkg": "5.16.1",
     "angular-eslint": "18.4.3",