Auth/PM-7072 - Token Service - Access Token Secure Storage Refactor (#8412)

* PM-5263 - TokenSvc - WIP on access token secure storage refactor

* PM-5263 - Add key generation svc to token svc.

* PM-5263 - TokenSvc - more progress on encrypt access token work.

* PM-5263 - TokenSvc TODO cleanup

* PM-5263 - TokenSvc - rename

* PM-5263 - TokenSvc - decryptAccess token must return null as that is a valid case.

* PM-5263 - Add EncryptSvc dep to TokenSvc

* PM-5263 - Add secure storage to token service

* PM-5263 - TokenSvc - (1) Finish implementing accessTokenKey stored in secure storage + encrypted access token stored on disk  (2) Remove no longer necessary migration flag as the presence of the accessTokenKey now serves the same purpose.

Co-authored-by: Jake Fink <jfink@bitwarden.com>

* PM-5263 - TokenSvc - (1) Tweak return structure of decryptAccessToken to be more debuggable (2) Add TODO to add more error handling.

* PM-5263 - TODO: update tests

* PM-5263 - add temp logs

* PM-5263 - TokenSvc - remove logs now that I don't need them.

* fix tests for access token

* PM-5263 - TokenSvc test cleanup - small tweaks / cleanup

* PM-5263 - TokenService - per PR feedback from Justin - add error message to error message if possible.

Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

---------

Co-authored-by: Jake Fink <jfink@bitwarden.com>
Co-authored-by: Justin Baur <19896123+justindbaur@users.noreply.github.com>

apps/browser/src/auth/background/service-factories/token-service.factory.ts

@@ -1,6 +1,10 @@
 import { TokenService as AbstractTokenService } from "@bitwarden/common/auth/abstractions/token.service";
 import { TokenService } from "@bitwarden/common/auth/services/token.service";
 
+import {
+  EncryptServiceInitOptions,
+  encryptServiceFactory,
+} from "../../../platform/background/service-factories/encrypt-service.factory";
 import {
   FactoryOptions,
   CachedServices,
@@ -10,6 +14,14 @@ import {
   GlobalStateProviderInitOptions,
   globalStateProviderFactory,
 } from "../../../platform/background/service-factories/global-state-provider.factory";
+import {
+  KeyGenerationServiceInitOptions,
+  keyGenerationServiceFactory,
+} from "../../../platform/background/service-factories/key-generation-service.factory";
+import {
+  LogServiceInitOptions,
+  logServiceFactory,
+} from "../../../platform/background/service-factories/log-service.factory";
 import {
   PlatformUtilsServiceInitOptions,
   platformUtilsServiceFactory,
@@ -29,7 +41,10 @@ export type TokenServiceInitOptions = TokenServiceFactoryOptions &
   SingleUserStateProviderInitOptions &
   GlobalStateProviderInitOptions &
   PlatformUtilsServiceInitOptions &
-  SecureStorageServiceInitOptions;
+  SecureStorageServiceInitOptions &
+  KeyGenerationServiceInitOptions &
+  EncryptServiceInitOptions &
+  LogServiceInitOptions;
 
 export function tokenServiceFactory(
   cache: { tokenService?: AbstractTokenService } & CachedServices,
@@ -45,6 +60,9 @@ export function tokenServiceFactory(
         await globalStateProviderFactory(cache, opts),
         (await platformUtilsServiceFactory(cache, opts)).supportsSecureStorage(),
         await secureStorageServiceFactory(cache, opts),
+        await keyGenerationServiceFactory(cache, opts),
+        await encryptServiceFactory(cache, opts),
+        await logServiceFactory(cache, opts),
       ),
   );
 }

apps/browser/src/background/main.background.ts

@@ -443,6 +443,9 @@ export default class MainBackground {
       this.globalStateProvider,
       this.platformUtilsService.supportsSecureStorage(),
       this.secureStorageService,
+      this.keyGenerationService,
+      this.encryptService,
+      this.logService,
     );
 
     const migrationRunner = new MigrationRunner(

apps/cli/src/bw.ts

@@ -318,11 +318,16 @@ export class Main {
       this.accountService,
     );
 
+    this.keyGenerationService = new KeyGenerationService(this.cryptoFunctionService);
+
     this.tokenService = new TokenService(
       this.singleUserStateProvider,
       this.globalStateProvider,
       this.platformUtilsService.supportsSecureStorage(),
       this.secureStorageService,
+      this.keyGenerationService,
+      this.encryptService,
+      this.logService,
     );
 
     const migrationRunner = new MigrationRunner(
@@ -343,8 +348,6 @@ export class Main {
       migrationRunner,
     );
 
-    this.keyGenerationService = new KeyGenerationService(this.cryptoFunctionService);
-
     this.cryptoService = new CryptoService(
       this.keyGenerationService,
       this.cryptoFunctionService,

apps/desktop/src/main.ts

@@ -6,11 +6,15 @@ import { firstValueFrom } from "rxjs";
 import { TokenService as TokenServiceAbstraction } from "@bitwarden/common/auth/abstractions/token.service";
 import { AccountServiceImplementation } from "@bitwarden/common/auth/services/account.service";
 import { TokenService } from "@bitwarden/common/auth/services/token.service";
+import { EncryptService } from "@bitwarden/common/platform/abstractions/encrypt.service";
+import { KeyGenerationService as KeyGenerationServiceAbstraction } from "@bitwarden/common/platform/abstractions/key-generation.service";
 import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
 import { DefaultBiometricStateService } from "@bitwarden/common/platform/biometrics/biometric-state.service";
 import { StateFactory } from "@bitwarden/common/platform/factories/state-factory";
 import { GlobalState } from "@bitwarden/common/platform/models/domain/global-state";
+import { EncryptServiceImplementation } from "@bitwarden/common/platform/services/cryptography/encrypt.service.implementation";
 import { DefaultEnvironmentService } from "@bitwarden/common/platform/services/default-environment.service";
+import { KeyGenerationService } from "@bitwarden/common/platform/services/key-generation.service";
 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";
@@ -45,6 +49,7 @@ import { ELECTRON_SUPPORTS_SECURE_STORAGE } from "./platform/services/electron-p
 import { ElectronStateService } from "./platform/services/electron-state.service";
 import { ElectronStorageService } from "./platform/services/electron-storage.service";
 import { I18nMainService } from "./platform/services/i18n.main.service";
+import { IllegalSecureStorageService } from "./platform/services/illegal-secure-storage.service";
 import { ElectronMainMessagingService } from "./services/electron-main-messaging.service";
 import { isMacAppStore } from "./utils";
 
@@ -62,6 +67,8 @@ export class Main {
   desktopSettingsService: DesktopSettingsService;
   migrationRunner: MigrationRunner;
   tokenService: TokenServiceAbstraction;
+  keyGenerationService: KeyGenerationServiceAbstraction;
+  encryptService: EncryptService;
 
   windowMain: WindowMain;
   messagingMain: MessagingMain;
@@ -153,11 +160,28 @@ export class Main {
 
     this.environmentService = new DefaultEnvironmentService(stateProvider, accountService);
 
+    this.mainCryptoFunctionService = new MainCryptoFunctionService();
+    this.mainCryptoFunctionService.init();
+
+    this.keyGenerationService = new KeyGenerationService(this.mainCryptoFunctionService);
+
+    this.encryptService = new EncryptServiceImplementation(
+      this.mainCryptoFunctionService,
+      this.logService,
+      true, // log mac failures
+    );
+
+    // Note: secure storage service is not available and should not be called in the main background process.
+    const illegalSecureStorageService = new IllegalSecureStorageService();
+
     this.tokenService = new TokenService(
       singleUserStateProvider,
       globalStateProvider,
       ELECTRON_SUPPORTS_SECURE_STORAGE,
-      this.storageService,
+      illegalSecureStorageService,
+      this.keyGenerationService,
+      this.encryptService,
+      this.logService,
     );
 
     this.migrationRunner = new MigrationRunner(
@@ -239,9 +263,6 @@ export class Main {
 
     this.clipboardMain = new ClipboardMain();
     this.clipboardMain.init();
-
-    this.mainCryptoFunctionService = new MainCryptoFunctionService();
-    this.mainCryptoFunctionService.init();
   }
 
   bootstrap() {

apps/desktop/src/platform/services/illegal-secure-storage.service.ts

@@ -0,0 +1,28 @@
+import { AbstractStorageService } from "@bitwarden/common/platform/abstractions/storage.service";
+import { StorageOptions } from "@bitwarden/common/platform/models/domain/storage-options";
+
+export class IllegalSecureStorageService implements AbstractStorageService {
+  constructor() {}
+
+  get valuesRequireDeserialization(): boolean {
+    throw new Error("Method not implemented.");
+  }
+  has(key: string, options?: StorageOptions): Promise<boolean> {
+    throw new Error("Method not implemented.");
+  }
+  save<T>(key: string, obj: T, options?: StorageOptions): Promise<void> {
+    throw new Error("Method not implemented.");
+  }
+  async get<T>(key: string): Promise<T> {
+    throw new Error("Method not implemented.");
+  }
+  async set<T>(key: string, obj: T): Promise<void> {
+    throw new Error("Method not implemented.");
+  }
+  async remove(key: string): Promise<void> {
+    throw new Error("Method not implemented.");
+  }
+  async clear(): Promise<void> {
+    throw new Error("Method not implemented.");
+  }
+}

libs/angular/src/services/jslib-services.module.ts

@@ -503,7 +503,10 @@ const typesafeProviders: Array<SafeProvider> = [
       SingleUserStateProvider,
       GlobalStateProvider,
       SUPPORTS_SECURE_STORAGE,
-      AbstractStorageService,
+      SECURE_STORAGE,
+      KeyGenerationServiceAbstraction,
+      EncryptService,
+      LogService,
     ],
   }),
   safeProvider({

libs/common/src/auth/services/token.service.spec.ts

@@ -1,7 +1,10 @@
-import { mock } from "jest-mock-extended";
+import { MockProxy, mock } from "jest-mock-extended";
 
 import { FakeSingleUserStateProvider, FakeGlobalStateProvider } from "../../../spec";
 import { VaultTimeoutAction } from "../../enums/vault-timeout-action.enum";
+import { EncryptService } from "../../platform/abstractions/encrypt.service";
+import { KeyGenerationService } from "../../platform/abstractions/key-generation.service";
+import { LogService } from "../../platform/abstractions/log.service";
 import { AbstractStorageService } from "../../platform/abstractions/storage.service";
 import { StorageLocation } from "../../platform/enums";
 import { StorageOptions } from "../../platform/models/domain/storage-options";
@@ -12,7 +15,6 @@ import { DecodedAccessToken, TokenService } from "./token.service";
 import {
   ACCESS_TOKEN_DISK,
   ACCESS_TOKEN_MEMORY,
-  ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE,
   API_KEY_CLIENT_ID_DISK,
   API_KEY_CLIENT_ID_MEMORY,
   API_KEY_CLIENT_SECRET_DISK,
@@ -28,7 +30,10 @@ describe("TokenService", () => {
   let singleUserStateProvider: FakeSingleUserStateProvider;
   let globalStateProvider: FakeGlobalStateProvider;
 
-  const secureStorageService = mock<AbstractStorageService>();
+  let secureStorageService: MockProxy<AbstractStorageService>;
+  let keyGenerationService: MockProxy<KeyGenerationService>;
+  let encryptService: MockProxy<EncryptService>;
+  let logService: MockProxy<LogService>;
 
   const memoryVaultTimeoutAction = VaultTimeoutAction.LogOut;
   const memoryVaultTimeout = 30;
@@ -74,12 +79,19 @@ describe("TokenService", () => {
     userId: userIdFromAccessToken,
   };
 
+  const accessTokenKeyB64 = { keyB64: "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8" };
+
   beforeEach(() => {
     jest.clearAllMocks();
 
     singleUserStateProvider = new FakeSingleUserStateProvider();
     globalStateProvider = new FakeGlobalStateProvider();
 
+    secureStorageService = mock<AbstractStorageService>();
+    keyGenerationService = mock<KeyGenerationService>();
+    encryptService = mock<EncryptService>();
+    logService = mock<LogService>();
+
     const supportsSecureStorage = false; // default to false; tests will override as needed
     tokenService = createTokenService(supportsSecureStorage);
   });
@@ -89,8 +101,8 @@ describe("TokenService", () => {
   });
 
   describe("Access Token methods", () => {
-    const accessTokenPartialSecureStorageKey = `_accessToken`;
+    const accessTokenKeyPartialSecureStorageKey = `_accessTokenKey`;
-    const accessTokenSecureStorageKey = `${userIdFromAccessToken}${accessTokenPartialSecureStorageKey}`;
+    const accessTokenKeySecureStorageKey = `${userIdFromAccessToken}${accessTokenKeyPartialSecureStorageKey}`;
 
     describe("setAccessToken", () => {
       it("should throw an error if the access token is null", async () => {
@@ -150,18 +162,22 @@ describe("TokenService", () => {
           tokenService = createTokenService(supportsSecureStorage);
         });
 
-        it("should set the access token in secure storage, null out data on disk or in memory, and set a flag to indicate the token has been migrated", async () => {
+        it("should set an access token key in secure storage, the encrypted access token in disk, and clear out the token in memory", async () => {
           // Arrange:
 
-          // For testing purposes, let's assume that the access token is already in disk and memory
+          // For testing purposes, let's assume that the access token is already in memory
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
-
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
             .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
 
+          keyGenerationService.createKey.mockResolvedValue("accessTokenKey" as any);
+
+          const mockEncryptedAccessToken = "encryptedAccessToken";
+
+          encryptService.encrypt.mockResolvedValue({
+            encryptedString: mockEncryptedAccessToken,
+          } as any);
+
           // Act
           await tokenService.setAccessToken(
             accessTokenJwt,
@@ -170,27 +186,22 @@ describe("TokenService", () => {
           );
           // Assert
 
-          // assert that the access token was set in secure storage
+          // assert that the AccessTokenKey was set in secure storage
           expect(secureStorageService.save).toHaveBeenCalledWith(
-            accessTokenSecureStorageKey,
+            accessTokenKeySecureStorageKey,
-            accessTokenJwt,
+            "accessTokenKey",
             secureStorageOptions,
           );
 
-          // assert data was migrated out of disk and memory + flag was set
+          // assert that the access token was encrypted and set in disk
           expect(
             singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK).nextMock,
-          ).toHaveBeenCalledWith(null);
+          ).toHaveBeenCalledWith(mockEncryptedAccessToken);
+
+          // assert data was migrated out of memory
           expect(
             singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY).nextMock,
           ).toHaveBeenCalledWith(null);
-
-          expect(
-            singleUserStateProvider.getFake(
-              userIdFromAccessToken,
-              ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE,
-            ).nextMock,
-          ).toHaveBeenCalledWith(true);
         });
       });
     });
@@ -216,7 +227,13 @@ describe("TokenService", () => {
       });
 
       describe("Memory storage tests", () => {
-        it("should get the access token from memory with no user id specified (uses global active user)", async () => {
+        test.each([
+          [
+            "should get the access token from memory for the provided user id",
+            userIdFromAccessToken,
+          ],
+          ["should get the access token from memory with no user id provided", undefined],
+        ])("%s", async (_, userId) => {
           // Arrange
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
@@ -228,37 +245,28 @@ describe("TokenService", () => {
             .stateSubject.next([userIdFromAccessToken, undefined]);
 
           // Need to have global active id set to the user id
+          if (!userId) {
             globalStateProvider
               .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
               .stateSubject.next(userIdFromAccessToken);
+          }
 
           // Act
-          const result = await tokenService.getAccessToken();
+          const result = await tokenService.getAccessToken(userId);
 
           // Assert
           expect(result).toEqual(accessTokenJwt);
         });
-
-        it("should get the access token from memory for the specified user id", async () => {
-          // Arrange
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
-            .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
-
-          // set disk to undefined
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, undefined]);
-
-          // Act
-          const result = await tokenService.getAccessToken(userIdFromAccessToken);
-          // Assert
-          expect(result).toEqual(accessTokenJwt);
-        });
       });
 
       describe("Disk storage tests (secure storage not supported on platform)", () => {
-        it("should get the access token from disk with no user id specified", async () => {
+        test.each([
+          [
+            "should get the access token from disk for the specified user id",
+            userIdFromAccessToken,
+          ],
+          ["should get the access token from disk with no user id specified", undefined],
+        ])("%s", async (_, userId) => {
           // Arrange
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
@@ -269,28 +277,14 @@ describe("TokenService", () => {
             .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
 
           // Need to have global active id set to the user id
+          if (!userId) {
             globalStateProvider
               .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
               .stateSubject.next(userIdFromAccessToken);
+          }
 
           // Act
-          const result = await tokenService.getAccessToken();
+          const result = await tokenService.getAccessToken(userId);
-          // Assert
-          expect(result).toEqual(accessTokenJwt);
-        });
-
-        it("should get the access token from disk for the specified user id", async () => {
-          // Arrange
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
-            .stateSubject.next([userIdFromAccessToken, undefined]);
-
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
-
-          // Act
-          const result = await tokenService.getAccessToken(userIdFromAccessToken);
           // Assert
           expect(result).toEqual(accessTokenJwt);
         });
@@ -302,7 +296,16 @@ describe("TokenService", () => {
           tokenService = createTokenService(supportsSecureStorage);
         });
 
-        it("should get the access token from secure storage when no user id is specified and the migration flag is set to true", async () => {
+        test.each([
+          [
+            "should get the encrypted access token from disk, decrypt it, and return it when user id is provided",
+            userIdFromAccessToken,
+          ],
+          [
+            "should get the encrypted access token from disk, decrypt it, and return it when no user id is provided",
+            undefined,
+          ],
+        ])("%s", async (_, userId) => {
           // Arrange
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
@@ -310,76 +313,35 @@ describe("TokenService", () => {
 
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, undefined]);
+            .stateSubject.next([userIdFromAccessToken, "encryptedAccessToken"]);
 
-          secureStorageService.get.mockResolvedValue(accessTokenJwt);
+          secureStorageService.get.mockResolvedValue(accessTokenKeyB64);
+          encryptService.decryptToUtf8.mockResolvedValue("decryptedAccessToken");
 
           // Need to have global active id set to the user id
+          if (!userId) {
             globalStateProvider
               .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
               .stateSubject.next(userIdFromAccessToken);
-
+          }
-          // set access token migration flag to true
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, true]);
 
           // Act
-          const result = await tokenService.getAccessToken();
+          const result = await tokenService.getAccessToken(userId);
-          // Assert
-          expect(result).toEqual(accessTokenJwt);
-        });
-
-        it("should get the access token from secure storage when user id is specified and the migration flag set to true", async () => {
-          // Arrange
-
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
-            .stateSubject.next([userIdFromAccessToken, undefined]);
-
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, undefined]);
-
-          secureStorageService.get.mockResolvedValue(accessTokenJwt);
-
-          // set access token migration flag to true
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, true]);
-
-          // Act
-          const result = await tokenService.getAccessToken(userIdFromAccessToken);
-          // Assert
-          expect(result).toEqual(accessTokenJwt);
-        });
-
-        it("should fallback and get the access token from disk when user id is specified and the migration flag is set to false even if the platform supports secure storage", async () => {
-          // Arrange
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
-            .stateSubject.next([userIdFromAccessToken, undefined]);
-
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
-
-          // set access token migration flag to false
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, false]);
-
-          // Act
-          const result = await tokenService.getAccessToken(userIdFromAccessToken);
 
           // Assert
-          expect(result).toEqual(accessTokenJwt);
+          expect(result).toEqual("decryptedAccessToken");
-
-          // assert that secure storage was not called
-          expect(secureStorageService.get).not.toHaveBeenCalled();
         });
 
-        it("should fallback and get the access token from disk when no user id is specified and the migration flag is set to false even if the platform supports secure storage", async () => {
+        test.each([
+          [
+            "should fallback and get the unencrypted access token from disk when there isn't an access token key in secure storage and a user id is provided",
+            userIdFromAccessToken,
+          ],
+          [
+            "should fallback and get the unencrypted access token from disk when there isn't an access token key in secure storage and no user id is provided",
+            undefined,
+          ],
+        ])("%s", async (_, userId) => {
           // Arrange
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
@@ -390,23 +352,19 @@ describe("TokenService", () => {
             .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
 
           // Need to have global active id set to the user id
+          if (!userId) {
             globalStateProvider
               .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
               .stateSubject.next(userIdFromAccessToken);
+          }
 
-          // set access token migration flag to false
+          // No access token key set
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, false]);
 
           // Act
-          const result = await tokenService.getAccessToken();
+          const result = await tokenService.getAccessToken(userId);
 
           // Assert
           expect(result).toEqual(accessTokenJwt);
-
-          // assert that secure storage was not called
-          expect(secureStorageService.get).not.toHaveBeenCalled();
         });
       });
     });
@@ -426,7 +384,16 @@ describe("TokenService", () => {
           tokenService = createTokenService(supportsSecureStorage);
         });
 
-        it("should clear the access token from all storage locations for the specified user id", async () => {
+        test.each([
+          [
+            "should clear the access token from all storage locations for the provided user id",
+            userIdFromAccessToken,
+          ],
+          [
+            "should clear the access token from all storage locations for the global active user",
+            undefined,
+          ],
+        ])("%s", async (_, userId) => {
           // Arrange
           singleUserStateProvider
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
@@ -436,6 +403,13 @@ describe("TokenService", () => {
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
             .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
 
+          // Need to have global active id set to the user id
+          if (!userId) {
+            globalStateProvider
+              .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
+              .stateSubject.next(userIdFromAccessToken);
+          }
+
           // Act
           await tokenService.clearAccessToken(userIdFromAccessToken);
 
@@ -448,39 +422,7 @@ describe("TokenService", () => {
           ).toHaveBeenCalledWith(null);
 
           expect(secureStorageService.remove).toHaveBeenCalledWith(
-            accessTokenSecureStorageKey,
+            accessTokenKeySecureStorageKey,
-            secureStorageOptions,
-          );
-        });
-
-        it("should clear the access token from all storage locations for the global active user", async () => {
-          // Arrange
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
-            .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
-
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK)
-            .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
-
-          // Need to have global active id set to the user id
-          globalStateProvider
-            .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
-            .stateSubject.next(userIdFromAccessToken);
-
-          // Act
-          await tokenService.clearAccessToken();
-
-          // Assert
-          expect(
-            singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY).nextMock,
-          ).toHaveBeenCalledWith(null);
-          expect(
-            singleUserStateProvider.getFake(userIdFromAccessToken, ACCESS_TOKEN_DISK).nextMock,
-          ).toHaveBeenCalledWith(null);
-
-          expect(secureStorageService.remove).toHaveBeenCalledWith(
-            accessTokenSecureStorageKey,
             secureStorageOptions,
           );
         });
@@ -2232,6 +2174,9 @@ describe("TokenService", () => {
       globalStateProvider,
       supportsSecureStorage,
       secureStorageService,
+      keyGenerationService,
+      encryptService,
+      logService,
     );
   }
 });

libs/common/src/auth/services/token.service.ts

@@ -1,11 +1,17 @@
 import { firstValueFrom } from "rxjs";
+import { Opaque } from "type-fest";
 
 import { decodeJwtTokenToJson } from "@bitwarden/auth/common";
 
 import { VaultTimeoutAction } from "../../enums/vault-timeout-action.enum";
+import { EncryptService } from "../../platform/abstractions/encrypt.service";
+import { KeyGenerationService } from "../../platform/abstractions/key-generation.service";
+import { LogService } from "../../platform/abstractions/log.service";
 import { AbstractStorageService } from "../../platform/abstractions/storage.service";
 import { StorageLocation } from "../../platform/enums";
+import { EncString, EncryptedString } from "../../platform/models/domain/enc-string";
 import { StorageOptions } from "../../platform/models/domain/storage-options";
+import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key";
 import {
   GlobalState,
   GlobalStateProvider,
@@ -19,7 +25,6 @@ import { ACCOUNT_ACTIVE_ACCOUNT_ID } from "./account.service";
 import {
   ACCESS_TOKEN_DISK,
   ACCESS_TOKEN_MEMORY,
-  ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE,
   API_KEY_CLIENT_ID_DISK,
   API_KEY_CLIENT_ID_MEMORY,
   API_KEY_CLIENT_SECRET_DISK,
@@ -101,8 +106,14 @@ export type DecodedAccessToken = {
   jti?: string;
 };
 
+/**
+ * A symmetric key for encrypting the access token before the token is stored on disk.
+ * This key should be stored in secure storage.
+ * */
+type AccessTokenKey = Opaque<SymmetricCryptoKey, "AccessTokenKey">;
+
 export class TokenService implements TokenServiceAbstraction {
-  private readonly accessTokenSecureStorageKey: string = "_accessToken";
+  private readonly accessTokenKeySecureStorageKey: string = "_accessTokenKey";
 
   private readonly refreshTokenSecureStorageKey: string = "_refreshToken";
 
@@ -117,10 +128,17 @@ export class TokenService implements TokenServiceAbstraction {
     private globalStateProvider: GlobalStateProvider,
     private readonly platformSupportsSecureStorage: boolean,
     private secureStorageService: AbstractStorageService,
+    private keyGenerationService: KeyGenerationService,
+    private encryptService: EncryptService,
+    private logService: LogService,
   ) {
     this.initializeState();
   }
 
+  // pivoting to an approach where we create a symmetric key we store in secure storage
+  // which is used to protect the data before persisting to disk.
+  // We will also use the same symmetric key to decrypt the data when reading from disk.
+
   private initializeState(): void {
     this.emailTwoFactorTokenRecordGlobalState = this.globalStateProvider.get(
       EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL,
@@ -155,6 +173,84 @@ export class TokenService implements TokenServiceAbstraction {
     }
   }
 
+  private async getAccessTokenKey(userId: UserId): Promise<AccessTokenKey | null> {
+    const accessTokenKeyB64 = await this.secureStorageService.get<
+      ReturnType<SymmetricCryptoKey["toJSON"]>
+    >(`${userId}${this.accessTokenKeySecureStorageKey}`, this.getSecureStorageOptions(userId));
+
+    if (!accessTokenKeyB64) {
+      return null;
+    }
+
+    const accessTokenKey = SymmetricCryptoKey.fromJSON(accessTokenKeyB64) as AccessTokenKey;
+    return accessTokenKey;
+  }
+
+  private async createAndSaveAccessTokenKey(userId: UserId): Promise<AccessTokenKey> {
+    const newAccessTokenKey = (await this.keyGenerationService.createKey(512)) as AccessTokenKey;
+
+    await this.secureStorageService.save<AccessTokenKey>(
+      `${userId}${this.accessTokenKeySecureStorageKey}`,
+      newAccessTokenKey,
+      this.getSecureStorageOptions(userId),
+    );
+
+    return newAccessTokenKey;
+  }
+
+  private async clearAccessTokenKey(userId: UserId): Promise<void> {
+    await this.secureStorageService.remove(
+      `${userId}${this.accessTokenKeySecureStorageKey}`,
+      this.getSecureStorageOptions(userId),
+    );
+  }
+
+  private async getOrCreateAccessTokenKey(userId: UserId): Promise<AccessTokenKey> {
+    if (!this.platformSupportsSecureStorage) {
+      throw new Error("Platform does not support secure storage. Cannot obtain access token key.");
+    }
+
+    if (!userId) {
+      throw new Error("User id not found. Cannot obtain access token key.");
+    }
+
+    // First see if we have an accessTokenKey in secure storage and return it if we do
+    let accessTokenKey: AccessTokenKey = await this.getAccessTokenKey(userId);
+
+    if (!accessTokenKey) {
+      // Otherwise, create a new one and save it to secure storage, then return it
+      accessTokenKey = await this.createAndSaveAccessTokenKey(userId);
+    }
+
+    return accessTokenKey;
+  }
+
+  private async encryptAccessToken(accessToken: string, userId: UserId): Promise<EncString> {
+    const accessTokenKey = await this.getOrCreateAccessTokenKey(userId);
+
+    return await this.encryptService.encrypt(accessToken, accessTokenKey);
+  }
+
+  private async decryptAccessToken(
+    encryptedAccessToken: EncString,
+    userId: UserId,
+  ): Promise<string | null> {
+    const accessTokenKey = await this.getAccessTokenKey(userId);
+
+    if (!accessTokenKey) {
+      // If we don't have an accessTokenKey, then that means we don't have an access token as it hasn't been set yet
+      // and we have to return null here to properly indicate the the user isn't logged in.
+      return null;
+    }
+
+    const decryptedAccessToken = await this.encryptService.decryptToUtf8(
+      encryptedAccessToken,
+      accessTokenKey,
+    );
+
+    return decryptedAccessToken;
+  }
+
   /**
    * Internal helper for set access token which always requires user id.
    * This is useful because setTokens always will have a user id from the access token whereas
@@ -173,26 +269,33 @@ export class TokenService implements TokenServiceAbstraction {
     );
 
     switch (storageLocation) {
-      case TokenStorageLocation.SecureStorage:
+      case TokenStorageLocation.SecureStorage: {
-        await this.saveStringToSecureStorage(userId, this.accessTokenSecureStorageKey, accessToken);
+        // Secure storage implementations have variable length limitations (Windows), so we cannot
+        // store the access token directly. Instead, we encrypt with accessTokenKey and store that
+        // in secure storage.
+
+        const encryptedAccessToken: EncString = await this.encryptAccessToken(accessToken, userId);
+
+        // Save the encrypted access token to disk
+        await this.singleUserStateProvider
+          .get(userId, ACCESS_TOKEN_DISK)
+          .update((_) => encryptedAccessToken.encryptedString);
 
         // TODO: PM-6408 - https://bitwarden.atlassian.net/browse/PM-6408
-        // 2024-02-20: Remove access token from memory and disk so that we migrate to secure storage over time.
+        // 2024-02-20: Remove access token from memory so that we migrate to encrypt the access token over time.
-        // Remove these 2 calls to remove the access token from memory and disk after 3 releases.
+        // Remove this call to remove the access token from memory after 3 releases.
-
-        await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_DISK).update((_) => null);
         await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MEMORY).update((_) => null);
 
-        // Set flag to indicate that the access token has been migrated to secure storage (don't remove this)
-        await this.setAccessTokenMigratedToSecureStorage(userId);
-
         return;
+      }
       case TokenStorageLocation.Disk:
+        // Access token stored on disk unencrypted as platform does not support secure storage
         await this.singleUserStateProvider
           .get(userId, ACCESS_TOKEN_DISK)
           .update((_) => accessToken);
         return;
       case TokenStorageLocation.Memory:
+        // Access token stored in memory due to vault timeout settings
         await this.singleUserStateProvider
           .get(userId, ACCESS_TOKEN_MEMORY)
           .update((_) => accessToken);
@@ -226,15 +329,14 @@ export class TokenService implements TokenServiceAbstraction {
       throw new Error("User id not found. Cannot clear access token.");
     }
 
-    // TODO: re-eval this once we get shared key definitions for vault timeout and vault timeout action data.
+    // TODO: re-eval this implementation once we get shared key definitions for vault timeout and vault timeout action data.
     // we can't determine storage location w/out vaultTimeoutAction and vaultTimeout
-    // but we can simply clear all locations to avoid the need to require those parameters
+    // but we can simply clear all locations to avoid the need to require those parameters.
 
     if (this.platformSupportsSecureStorage) {
-      await this.secureStorageService.remove(
+      // Always clear the access token key when clearing the access token
-        `${userId}${this.accessTokenSecureStorageKey}`,
+      // The next set of the access token will create a new access token key
-        this.getSecureStorageOptions(userId),
+      await this.clearAccessTokenKey(userId);
-      );
     }
 
     // Platform doesn't support secure storage, so use state provider implementation
@@ -249,36 +351,48 @@ export class TokenService implements TokenServiceAbstraction {
       return undefined;
     }
 
-    const accessTokenMigratedToSecureStorage =
-      await this.getAccessTokenMigratedToSecureStorage(userId);
-    if (this.platformSupportsSecureStorage && accessTokenMigratedToSecureStorage) {
-      return await this.getStringFromSecureStorage(userId, this.accessTokenSecureStorageKey);
-    }
-
     // Try to get the access token from memory
     const accessTokenMemory = await this.getStateValueByUserIdAndKeyDef(
       userId,
       ACCESS_TOKEN_MEMORY,
     );
-
     if (accessTokenMemory != null) {
       return accessTokenMemory;
     }
 
     // If memory is null, read from disk
-    return await this.getStateValueByUserIdAndKeyDef(userId, ACCESS_TOKEN_DISK);
+    const accessTokenDisk = await this.getStateValueByUserIdAndKeyDef(userId, ACCESS_TOKEN_DISK);
+    if (!accessTokenDisk) {
+      return null;
     }
 
-  private async getAccessTokenMigratedToSecureStorage(userId: UserId): Promise<boolean> {
+    if (this.platformSupportsSecureStorage) {
-    return await firstValueFrom(
+      const accessTokenKey = await this.getAccessTokenKey(userId);
-      this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE).state$,
+
+      if (!accessTokenKey) {
+        // We know this is an unencrypted access token because we don't have an access token key
+        return accessTokenDisk;
+      }
+
+      try {
+        const encryptedAccessTokenEncString = new EncString(accessTokenDisk as EncryptedString);
+
+        const decryptedAccessToken = await this.decryptAccessToken(
+          encryptedAccessTokenEncString,
+          userId,
         );
+        return decryptedAccessToken;
+      } catch (error) {
+        // If an error occurs during decryption, return null for logout.
+        // We don't try to recover here since we'd like to know
+        // if access token and key are getting out of sync.
+        this.logService.error(
+          `Failed to decrypt access token: ${error?.message ?? "Unknown error."}`,
+        );
+        return null;
       }
-
+    }
-  private async setAccessTokenMigratedToSecureStorage(userId: UserId): Promise<void> {
+    return accessTokenDisk;
-    await this.singleUserStateProvider
-      .get(userId, ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-      .update((_) => true);
   }
 
   // Private because we only ever set the refresh token when also setting the access token
@@ -417,7 +531,7 @@ export class TokenService implements TokenServiceAbstraction {
     const storageLocation = await this.determineStorageLocation(
       vaultTimeoutAction,
       vaultTimeout,
-      false,
+      false, // don't use secure storage for client id
     );
 
     if (storageLocation === TokenStorageLocation.Disk) {
@@ -484,7 +598,7 @@ export class TokenService implements TokenServiceAbstraction {
     const storageLocation = await this.determineStorageLocation(
       vaultTimeoutAction,
       vaultTimeout,
-      false,
+      false, // don't use secure storage for client secret
     );
 
     if (storageLocation === TokenStorageLocation.Disk) {
@@ -567,6 +681,7 @@ export class TokenService implements TokenServiceAbstraction {
     });
   }
 
+  // TODO: stop accepting optional userIds
   async clearTokens(userId?: UserId): Promise<void> {
     userId ??= await firstValueFrom(this.activeUserIdGlobalState.state$);
 

libs/common/src/auth/services/token.state.spec.ts

@@ -3,7 +3,6 @@ import { KeyDefinition } from "../../platform/state";
 import {
   ACCESS_TOKEN_DISK,
   ACCESS_TOKEN_MEMORY,
-  ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE,
   API_KEY_CLIENT_ID_DISK,
   API_KEY_CLIENT_ID_MEMORY,
   API_KEY_CLIENT_SECRET_DISK,
@@ -17,7 +16,6 @@ import {
 describe.each([
   [ACCESS_TOKEN_DISK, "accessTokenDisk"],
   [ACCESS_TOKEN_MEMORY, "accessTokenMemory"],
-  [ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE, true],
   [REFRESH_TOKEN_DISK, "refreshTokenDisk"],
   [REFRESH_TOKEN_MEMORY, "refreshTokenMemory"],
   [REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE, true],

libs/common/src/auth/services/token.state.ts

@@ -8,14 +8,6 @@ export const ACCESS_TOKEN_MEMORY = new KeyDefinition<string>(TOKEN_MEMORY, "acce
   deserializer: (accessToken) => accessToken,
 });
 
-export const ACCESS_TOKEN_MIGRATED_TO_SECURE_STORAGE = new KeyDefinition<boolean>(
-  TOKEN_DISK,
-  "accessTokenMigratedToSecureStorage",
-  {
-    deserializer: (accessTokenMigratedToSecureStorage) => accessTokenMigratedToSecureStorage,
-  },
-);
-
 export const REFRESH_TOKEN_DISK = new KeyDefinition<string>(TOKEN_DISK, "refreshToken", {
   deserializer: (refreshToken) => refreshToken,
 });