Merge e05e8395bf into c21a58f2fb

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

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

apps/cli/src/bw.ts

@@ -338,6 +338,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

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

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

@@ -6,13 +6,16 @@ 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";
+import { SymmetricCryptoKey } from "../../platform/models/domain/symmetric-crypto-key";
+import { CsprngArray } from "../../types/csprng";
 import { UserId } from "../../types/guid";
 
 import { ACCOUNT_ACTIVE_ACCOUNT_ID } from "./account.service";
-import { DecodedAccessToken, TokenService } from "./token.service";
+import { AccessTokenKey, DecodedAccessToken, TokenService } from "./token.service";
 import {
   ACCESS_TOKEN_DISK,
   ACCESS_TOKEN_MEMORY,
@@ -26,6 +29,8 @@ import {
   SECURITY_STAMP_MEMORY,
 } from "./token.state";
 
+// TODO: add specific tests for new secure storage scenarios.
+
 describe("TokenService", () => {
   let tokenService: TokenService;
   let singleUserStateProvider: FakeSingleUserStateProvider;
@@ -35,6 +40,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 +98,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);
@@ -213,6 +220,14 @@ describe("TokenService", () => {
       });
 
       describe("Disk storage tests (secure storage supported on platform)", () => {
+        const accessTokenKey = new SymmetricCryptoKey(
+          new Uint8Array(64) as CsprngArray,
+        ) as AccessTokenKey;
+
+        const accessTokenKeyB64 = {
+          keyB64:
+            "lI7lSoejJ1HsrTkRs2Ipm0x+YcZMKpgm7WQGCNjAWmFAyGOKossXwBJvvtbxcYDZ0G0XNY8Gp7DBXZV2tWAO5w==",
+        };
         beforeEach(() => {
           const supportsSecureStorage = true;
           tokenService = createTokenService(supportsSecureStorage);
@@ -226,7 +241,7 @@ describe("TokenService", () => {
             .getFake(userIdFromAccessToken, ACCESS_TOKEN_MEMORY)
             .stateSubject.next([userIdFromAccessToken, accessTokenJwt]);
 
-          keyGenerationService.createKey.mockResolvedValue("accessTokenKey" as any);
+          keyGenerationService.createKey.mockResolvedValue(accessTokenKey);
 
           const mockEncryptedAccessToken = "encryptedAccessToken";
 
@@ -234,6 +249,11 @@ describe("TokenService", () => {
             encryptedString: mockEncryptedAccessToken,
           } as any);
 
+          // First call resolves to null to simulate no key in secure storage
+          // then resolves to the key to simulate the key being set in secure storage
+          // and retrieved successfully to ensure it was set.
+          secureStorageService.get.mockResolvedValueOnce(null).mockResolvedValue(accessTokenKeyB64);
+
           // Act
           await tokenService.setAccessToken(
             accessTokenJwt,
@@ -245,7 +265,7 @@ describe("TokenService", () => {
           // assert that the AccessTokenKey was set in secure storage
           expect(secureStorageService.save).toHaveBeenCalledWith(
             accessTokenKeySecureStorageKey,
-            "accessTokenKey",
+            accessTokenKey,
             secureStorageOptions,
           );
 
@@ -2280,6 +2300,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";
@@ -110,10 +111,12 @@ export type DecodedAccessToken = {
  * 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 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,14 @@ 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);
+
+    if (!accessTokenKey) {
+      throw new Error(this.accessTokenKeyFailedToSaveToSecureStorageError);
+    }
+
     return newAccessTokenKey;
   }
 
@@ -287,17 +299,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 +409,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;
       }
 

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

@@ -11,6 +11,7 @@ export abstract class EncryptService {
     plainValue: Uint8Array,
     key?: SymmetricCryptoKey,
   ): Promise<EncArrayBuffer>;
+  abstract stringIsEncString(value: string): boolean;
   abstract decryptToUtf8(encString: EncString, key: SymmetricCryptoKey): Promise<string>;
   abstract decryptToBytes(encThing: Encrypted, key: SymmetricCryptoKey): Promise<Uint8Array>;
   abstract rsaEncrypt(data: Uint8Array, publicKey: Uint8Array): Promise<EncString>;

libs/common/src/platform/models/domain/enc-string.ts

@@ -102,7 +102,7 @@ export class EncString implements Encrypted {
     }
   }
 
-  private static parseEncryptedString(encryptedString: string): {
+  static parseEncryptedString(encryptedString: string): {
     encType: EncryptionType;
     encPieces: string[];
   } {

libs/common/src/platform/services/cryptography/encrypt.service.implementation.ts

@@ -63,6 +63,10 @@ export class EncryptServiceImplementation implements EncryptService {
     return new EncArrayBuffer(encBytes);
   }
 
+  stringIsEncString(value: string): boolean {
+    return EncString.parseEncryptedString(value) !== undefined;
+  }
+
   async decryptToUtf8(encString: EncString, key: SymmetricCryptoKey): Promise<string> {
     if (key == null) {
       throw new Error("No key provided for decryption.");