PM-7392 - TokenSvc - add checks when setting and retrieving the access token to improve handling around the access token encryption.

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

@@ -22,6 +22,10 @@ import {
   LogServiceInitOptions,
   logServiceFactory,
 } from "../../../platform/background/service-factories/log-service.factory";
+import {
+  MessagingServiceInitOptions,
+  messagingServiceFactory,
+} from "../../../platform/background/service-factories/messaging-service.factory";
 import {
   PlatformUtilsServiceInitOptions,
   platformUtilsServiceFactory,
@@ -44,7 +48,8 @@ export type TokenServiceInitOptions = TokenServiceFactoryOptions &
   SecureStorageServiceInitOptions &
   KeyGenerationServiceInitOptions &
   EncryptServiceInitOptions &
-  LogServiceInitOptions;
+  LogServiceInitOptions &
+  MessagingServiceInitOptions;
 
 export function tokenServiceFactory(
   cache: { tokenService?: AbstractTokenService } & CachedServices,
@@ -63,6 +68,7 @@ export function tokenServiceFactory(
         await keyGenerationServiceFactory(cache, opts),
         await encryptServiceFactory(cache, opts),
         await logServiceFactory(cache, opts),
+        await messagingServiceFactory(cache, opts),
       ),
   );
 }

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

@@ -517,6 +517,7 @@ export default class MainBackground {
       this.keyGenerationService,
       this.encryptService,
       this.logService,
+      this.messagingService,
     );
 
     const migrationRunner = new MigrationRunner(

apps/cli/src/bw.ts

@@ -335,6 +335,7 @@ export class Main {
       this.keyGenerationService,
       this.encryptService,
       this.logService,
+      this.messagingService,
     );
 
     const migrationRunner = new MigrationRunner(

apps/desktop/src/main.ts

@@ -176,6 +176,16 @@ export class Main {
     // Note: secure storage service is not available and should not be called in the main background process.
     const illegalSecureStorageService = new IllegalSecureStorageService();
 
+    const messageSubject = new Subject<Message<object>>();
+    this.messagingService = MessageSender.combine(
+      new SubjectMessageSender(messageSubject), // For local messages
+      new ElectronMainMessagingService(this.windowMain),
+    );
+
+    messageSubject.asObservable().subscribe((message) => {
+      this.messagingMain.onMessage(message);
+    });
+
     this.tokenService = new TokenService(
       singleUserStateProvider,
       globalStateProvider,
@@ -184,6 +194,7 @@ export class Main {
       this.keyGenerationService,
       this.encryptService,
       this.logService,
+      this.messagingService,
     );
 
     this.migrationRunner = new MigrationRunner(
@@ -224,16 +235,6 @@ export class Main {
     this.updaterMain = new UpdaterMain(this.i18nService, this.windowMain);
     this.trayMain = new TrayMain(this.windowMain, this.i18nService, this.desktopSettingsService);
 
-    const messageSubject = new Subject<Message<object>>();
-    this.messagingService = MessageSender.combine(
-      new SubjectMessageSender(messageSubject), // For local messages
-      new ElectronMainMessagingService(this.windowMain),
-    );
-
-    messageSubject.asObservable().subscribe((message) => {
-      this.messagingMain.onMessage(message);
-    });
-
     this.powerMonitorMain = new PowerMonitorMain(this.messagingService);
     this.menuMain = new MenuMain(
       this.i18nService,

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

@@ -523,6 +523,7 @@ const safeProviders: SafeProvider[] = [
       KeyGenerationServiceAbstraction,
       EncryptService,
       LogService,
+      MessagingServiceAbstraction,
     ],
   }),
   safeProvider({

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

@@ -6,6 +6,7 @@ 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 { MessagingService } from "../../platform/abstractions/messaging.service";
 import { AbstractStorageService } from "../../platform/abstractions/storage.service";
 import { StorageLocation } from "../../platform/enums";
 import { StorageOptions } from "../../platform/models/domain/storage-options";
@@ -35,6 +36,7 @@ describe("TokenService", () => {
   let keyGenerationService: MockProxy<KeyGenerationService>;
   let encryptService: MockProxy<EncryptService>;
   let logService: MockProxy<LogService>;
+  let messagingService: MockProxy<MessagingService>;
 
   const memoryVaultTimeoutAction = VaultTimeoutAction.LogOut;
   const memoryVaultTimeout = 30;
@@ -92,6 +94,7 @@ describe("TokenService", () => {
     keyGenerationService = mock<KeyGenerationService>();
     encryptService = mock<EncryptService>();
     logService = mock<LogService>();
+    messagingService = mock<MessagingService>();
 
     const supportsSecureStorage = false; // default to false; tests will override as needed
     tokenService = createTokenService(supportsSecureStorage);
@@ -2280,6 +2283,7 @@ describe("TokenService", () => {
       keyGenerationService,
       encryptService,
       logService,
+      messagingService,
     );
   }
 });

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

@@ -7,6 +7,7 @@ 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 { MessagingService } from "../../platform/abstractions/messaging.service";
 import { AbstractStorageService } from "../../platform/abstractions/storage.service";
 import { StorageLocation } from "../../platform/enums";
 import { EncString, EncryptedString } from "../../platform/models/domain/enc-string";
@@ -114,6 +115,8 @@ type AccessTokenKey = Opaque<SymmetricCryptoKey, "AccessTokenKey">;
 
 export class TokenService implements TokenServiceAbstraction {
   private readonly accessTokenKeySecureStorageKey: string = "_accessTokenKey";
+  private readonly accessTokenKeyFailedToSaveToSecureStorageError =
+    "Access token key not saved to secure storage.";
 
   private readonly refreshTokenSecureStorageKey: string = "_refreshToken";
 
@@ -131,6 +134,7 @@ export class TokenService implements TokenServiceAbstraction {
     private keyGenerationService: KeyGenerationService,
     private encryptService: EncryptService,
     private logService: LogService,
+    private messagingService: MessagingService,
   ) {
     this.initializeState();
   }
@@ -208,6 +212,17 @@ export class TokenService implements TokenServiceAbstraction {
       this.getSecureStorageOptions(userId),
     );
 
+    // We are having intermittent issues with access token keys not saving into secure storage on windows 10/11.
+    // So, let's add a check to ensure we can read the value after writing it.
+
+    const accessTokenKey = await this.getAccessTokenKey(userId); // replace a
+
+    // accessTokenKey = null; // TODO: remove this after testing
+
+    if (!accessTokenKey) {
+      throw new Error(this.accessTokenKeyFailedToSaveToSecureStorageError);
+    }
+
     return newAccessTokenKey;
   }
 
@@ -287,17 +302,31 @@ export class TokenService implements TokenServiceAbstraction {
         // store the access token directly. Instead, we encrypt with accessTokenKey and store that
         // in secure storage.
 
-        const encryptedAccessToken: EncString = await this.encryptAccessToken(accessToken, userId);
+        try {
+          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);
 
-        // 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 so that we migrate to encrypt the access token over time.
-        // Remove this call to remove the access token from memory after 3 releases.
-        await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MEMORY).update((_) => null);
+          // TODO: PM-6408 - https://bitwarden.atlassian.net/browse/PM-6408
+          // 2024-02-20: Remove access token from memory so that we migrate to encrypt the access token over time.
+          // Remove this call to remove the access token from memory after 3 releases.
+          await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MEMORY).update((_) => null);
+        } catch (error) {
+          // check if error is Access token key not saved to secure storage.
+          if (error.message === this.accessTokenKeyFailedToSaveToSecureStorageError) {
+            // Fall back to disk storage for unecrypted access token
+            await this.singleUserStateProvider
+              .get(userId, ACCESS_TOKEN_DISK)
+              .update((_) => accessToken);
+          }
+          // re-throw the error if it's not the expected error
+          throw error;
+        }
 
         return;
       }
@@ -383,7 +412,15 @@ export class TokenService implements TokenServiceAbstraction {
       const accessTokenKey = await this.getAccessTokenKey(userId);
 
       if (!accessTokenKey) {
-        // We know this is an unencrypted access token because we don't have an access token key
+        if (this.encryptService.stringIsEncString(accessTokenDisk)) {
+          // We have to log the user out if we can't decrypt the access token
+          this.logService.error(
+            "Access token key not found to decrypt encrypted access token. Logging user out.",
+          );
+          this.messagingService.send("logout", { userId });
+        }
+
+        // We know this is an unencrypted access token
         return accessTokenDisk;
       }