Merge branch 'main' into autofill/pm-5189-fix-issues-present-with-inline-menu-rendering-in-iframes

apps/browser/src/_locales/en/messages.json

@@ -172,6 +172,12 @@
   "changeMasterPassword": {
     "message": "Change master password"
   },
+  "continueToWebApp": {
+    "message": "Continue to web app?"
+  },
+  "changeMasterPasswordOnWebConfirmation": {
+    "message": "You can change your master password on the Bitwarden web app."
+  },
   "fingerprintPhrase": {
     "message": "Fingerprint phrase",
     "description": "A 'fingerprint phrase' is a unique word phrase (similar to a passphrase) that a user can use to authenticate their public key with another user, for the purposes of sharing."
@@ -557,12 +563,6 @@
   "addedFolder": {
     "message": "Folder added"
   },
-  "changeMasterPass": {
-    "message": "Change master password"
-  },
-  "changeMasterPasswordConfirmation": {
-    "message": "You can change your master password on the bitwarden.com web vault. Do you want to visit the website now?"
-  },
   "twoStepLoginConfirmation": {
     "message": "Two-step login makes your account more secure by requiring you to verify your login with another device such as a security key, authenticator app, SMS, phone call, or email. Two-step login can be set up on the bitwarden.com web vault. Do you want to visit the website now?"
   },

apps/browser/src/popup/settings/settings.component.html

@@ -153,7 +153,7 @@
         *ngIf="showChangeMasterPass"
       >
         <div class="row-main">{{ "changeMasterPassword" | i18n }}</div>
-        <i class="bwi bwi-angle-right bwi-lg row-sub-icon" aria-hidden="true"></i>
+        <i class="bwi bwi-external-link bwi-lg row-sub-icon" aria-hidden="true"></i>
       </button>
       <button
         type="button"

apps/browser/src/popup/settings/settings.component.ts

@@ -441,9 +441,10 @@ export class SettingsComponent implements OnInit {
 
   async changePassword() {
     const confirmed = await this.dialogService.openSimpleDialog({
-      title: { key: "changeMasterPassword" },
+      title: { key: "continueToWebApp" },
-      content: { key: "changeMasterPasswordConfirmation" },
+      content: { key: "changeMasterPasswordOnWebConfirmation" },
       type: "info",
+      acceptButtonText: { key: "continue" },
     });
     if (confirmed) {
       const env = await firstValueFrom(this.environmentService.environment$);

apps/desktop/src/locales/en/messages.json

@@ -800,8 +800,11 @@
   "changeMasterPass": {
     "message": "Change master password"
   },
-  "changeMasterPasswordConfirmation": {
+  "continueToWebApp": {
-    "message": "You can change your master password on the bitwarden.com web vault. Do you want to visit the website now?"
+    "message": "Continue to web app?"
+  },
+  "changeMasterPasswordOnWebConfirmation": {
+    "message": "You can change your master password on the Bitwarden web app."
   },
   "fingerprintPhrase": {
     "message": "Fingerprint phrase",

apps/desktop/src/main/menu/menu.account.ts

@@ -65,10 +65,10 @@ export class AccountMenu implements IMenubarMenu {
       id: "changeMasterPass",
       click: async () => {
         const result = await dialog.showMessageBox(this._window, {
-          title: this.localize("changeMasterPass"),
+          title: this.localize("continueToWebApp"),
-          message: this.localize("changeMasterPass"),
+          message: this.localize("continueToWebApp"),
-          detail: this.localize("changeMasterPasswordConfirmation"),
+          detail: this.localize("changeMasterPasswordOnWebConfirmation"),
-          buttons: [this.localize("yes"), this.localize("no")],
+          buttons: [this.localize("continue"), this.localize("cancel")],
           cancelId: 1,
           defaultId: 0,
           noLink: true,

libs/auth/src/common/login-strategies/login.strategy.ts

@@ -166,8 +166,8 @@ export abstract class LoginStrategy {
 
     const userId = accountInformation.sub;
 
-    const vaultTimeoutAction = await this.stateService.getVaultTimeoutAction();
+    const vaultTimeoutAction = await this.stateService.getVaultTimeoutAction({ userId });
-    const vaultTimeout = await this.stateService.getVaultTimeout();
+    const vaultTimeout = await this.stateService.getVaultTimeout({ userId });
 
     // set access token and refresh token before account initialization so authN status can be accurate
     // User id will be derived from the access token.

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

@@ -23,7 +23,6 @@ import {
   EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL,
   REFRESH_TOKEN_DISK,
   REFRESH_TOKEN_MEMORY,
-  REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
 } from "./token.state";
 
 describe("TokenService", () => {
@@ -1120,20 +1119,13 @@ describe("TokenService", () => {
             secureStorageOptions,
           );
 
-          // assert data was migrated out of disk and memory + flag was set
+          // assert data was migrated out of disk and memory
           expect(
             singleUserStateProvider.getFake(userIdFromAccessToken, REFRESH_TOKEN_DISK).nextMock,
           ).toHaveBeenCalledWith(null);
           expect(
             singleUserStateProvider.getFake(userIdFromAccessToken, REFRESH_TOKEN_MEMORY).nextMock,
           ).toHaveBeenCalledWith(null);
-
-          expect(
-            singleUserStateProvider.getFake(
-              userIdFromAccessToken,
-              REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
-            ).nextMock,
-          ).toHaveBeenCalledWith(true);
         });
       });
     });
@@ -1260,11 +1252,6 @@ describe("TokenService", () => {
             .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
             .stateSubject.next(userIdFromAccessToken);
 
-          // set access token migration flag to true
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, true]);
-
           // Act
           const result = await tokenService.getRefreshToken();
           // Assert
@@ -1284,11 +1271,6 @@ describe("TokenService", () => {
 
           secureStorageService.get.mockResolvedValue(refreshToken);
 
-          // set access token migration flag to true
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, true]);
-
           // Act
           const result = await tokenService.getRefreshToken(userIdFromAccessToken);
           // Assert
@@ -1305,11 +1287,6 @@ describe("TokenService", () => {
             .getFake(userIdFromAccessToken, REFRESH_TOKEN_DISK)
             .stateSubject.next([userIdFromAccessToken, refreshToken]);
 
-          // set refresh token migration flag to false
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, false]);
-
           // Act
           const result = await tokenService.getRefreshToken(userIdFromAccessToken);
 
@@ -1335,11 +1312,6 @@ describe("TokenService", () => {
             .getFake(ACCOUNT_ACTIVE_ACCOUNT_ID)
             .stateSubject.next(userIdFromAccessToken);
 
-          // set access token migration flag to false
-          singleUserStateProvider
-            .getFake(userIdFromAccessToken, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-            .stateSubject.next([userIdFromAccessToken, false]);
-
           // Act
           const result = await tokenService.getRefreshToken();
 

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

@@ -32,7 +32,6 @@ import {
   EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL,
   REFRESH_TOKEN_DISK,
   REFRESH_TOKEN_MEMORY,
-  REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
 } from "./token.state";
 
 export enum TokenStorageLocation {
@@ -441,9 +440,6 @@ export class TokenService implements TokenServiceAbstraction {
         await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null);
         await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MEMORY).update((_) => null);
 
-        // Set flag to indicate that the refresh token has been migrated to secure storage (don't remove this)
-        await this.setRefreshTokenMigratedToSecureStorage(userId);
-
         return;
 
       case TokenStorageLocation.Disk:
@@ -467,12 +463,6 @@ export class TokenService implements TokenServiceAbstraction {
       return undefined;
     }
 
-    const refreshTokenMigratedToSecureStorage =
-      await this.getRefreshTokenMigratedToSecureStorage(userId);
-    if (this.platformSupportsSecureStorage && refreshTokenMigratedToSecureStorage) {
-      return await this.getStringFromSecureStorage(userId, this.refreshTokenSecureStorageKey);
-    }
-
     // pre-secure storage migration:
     // Always read memory first b/c faster
     const refreshTokenMemory = await this.getStateValueByUserIdAndKeyDef(
@@ -484,13 +474,24 @@ export class TokenService implements TokenServiceAbstraction {
       return refreshTokenMemory;
     }
 
-    // if memory is null, read from disk
+    // if memory is null, read from disk and then secure storage
     const refreshTokenDisk = await this.getStateValueByUserIdAndKeyDef(userId, REFRESH_TOKEN_DISK);
 
     if (refreshTokenDisk != null) {
       return refreshTokenDisk;
     }
 
+    if (this.platformSupportsSecureStorage) {
+      const refreshTokenSecureStorage = await this.getStringFromSecureStorage(
+        userId,
+        this.refreshTokenSecureStorageKey,
+      );
+
+      if (refreshTokenSecureStorage != null) {
+        return refreshTokenSecureStorage;
+      }
+    }
+
     return null;
   }
 
@@ -516,18 +517,6 @@ export class TokenService implements TokenServiceAbstraction {
     await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null);
   }
 
-  private async getRefreshTokenMigratedToSecureStorage(userId: UserId): Promise<boolean> {
-    return await firstValueFrom(
-      this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE).state$,
-    );
-  }
-
-  private async setRefreshTokenMigratedToSecureStorage(userId: UserId): Promise<void> {
-    await this.singleUserStateProvider
-      .get(userId, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE)
-      .update((_) => true);
-  }
-
   async setClientId(
     clientId: string,
     vaultTimeoutAction: VaultTimeoutAction,

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

@@ -10,7 +10,6 @@ import {
   EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL,
   REFRESH_TOKEN_DISK,
   REFRESH_TOKEN_MEMORY,
-  REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
 } from "./token.state";
 
 describe.each([
@@ -18,7 +17,6 @@ describe.each([
   [ACCESS_TOKEN_MEMORY, "accessTokenMemory"],
   [REFRESH_TOKEN_DISK, "refreshTokenDisk"],
   [REFRESH_TOKEN_MEMORY, "refreshTokenMemory"],
-  [REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE, true],
   [EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, { user: "token" }],
   [API_KEY_CLIENT_ID_DISK, "apiKeyClientIdDisk"],
   [API_KEY_CLIENT_ID_MEMORY, "apiKeyClientIdMemory"],

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

@@ -30,15 +30,6 @@ export const REFRESH_TOKEN_MEMORY = new UserKeyDefinition<string>(TOKEN_MEMORY,
   clearOn: [], // Manually handled
 });
 
-export const REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE = new UserKeyDefinition<boolean>(
-  TOKEN_DISK,
-  "refreshTokenMigratedToSecureStorage",
-  {
-    deserializer: (refreshTokenMigratedToSecureStorage) => refreshTokenMigratedToSecureStorage,
-    clearOn: [], // Don't clear on lock/logout so that we always check the correct place (secure storage) for the refresh token if it's been migrated
-  },
-);
-
 export const EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL = KeyDefinition.record<string, string>(
   TOKEN_DISK_LOCAL,
   "emailTwoFactorTokenRecord",

libs/common/src/state-migrations/migrate.ts

@@ -54,6 +54,7 @@ import { SendMigrator } from "./migrations/54-move-encrypted-sends";
 import { MoveMasterKeyStateToProviderMigrator } from "./migrations/55-move-master-key-state-to-provider";
 import { AuthRequestMigrator } from "./migrations/56-move-auth-requests";
 import { CipherServiceMigrator } from "./migrations/57-move-cipher-service-to-state-provider";
+import { RemoveRefreshTokenMigratedFlagMigrator } from "./migrations/58-remove-refresh-token-migrated-state-provider-flag";
 import { RemoveLegacyEtmKeyMigrator } from "./migrations/6-remove-legacy-etm-key";
 import { MoveBiometricAutoPromptToAccount } from "./migrations/7-move-biometric-auto-prompt-to-account";
 import { MoveStateVersionMigrator } from "./migrations/8-move-state-version";
@@ -61,7 +62,7 @@ import { MoveBrowserSettingsToGlobal } from "./migrations/9-move-browser-setting
 import { MinVersionMigrator } from "./migrations/min-version";
 
 export const MIN_VERSION = 3;
-export const CURRENT_VERSION = 57;
+export const CURRENT_VERSION = 58;
 export type MinVersion = typeof MIN_VERSION;
 
 export function createMigrationBuilder() {
@@ -120,7 +121,8 @@ export function createMigrationBuilder() {
     .with(SendMigrator, 53, 54)
     .with(MoveMasterKeyStateToProviderMigrator, 54, 55)
     .with(AuthRequestMigrator, 55, 56)
-    .with(CipherServiceMigrator, 56, CURRENT_VERSION);
+    .with(CipherServiceMigrator, 56, 57)
+    .with(RemoveRefreshTokenMigratedFlagMigrator, 57, CURRENT_VERSION);
 }
 
 export async function currentVersion(

libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.spec.ts

@@ -0,0 +1,72 @@
+import { MockProxy, any } from "jest-mock-extended";
+
+import { MigrationHelper } from "../migration-helper";
+import { mockMigrationHelper } from "../migration-helper.spec";
+import { IRREVERSIBLE } from "../migrator";
+
+import {
+  REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
+  RemoveRefreshTokenMigratedFlagMigrator,
+} from "./58-remove-refresh-token-migrated-state-provider-flag";
+
+// Represents data in state service pre-migration
+function preMigrationJson() {
+  return {
+    global: {
+      otherStuff: "otherStuff1",
+    },
+    authenticatedAccounts: ["user1", "user2", "user3"],
+
+    user_user1_token_refreshTokenMigratedToSecureStorage: true,
+    user_user2_token_refreshTokenMigratedToSecureStorage: false,
+  };
+}
+
+function rollbackJSON() {
+  return {
+    global: {
+      otherStuff: "otherStuff1",
+    },
+    authenticatedAccounts: ["user1", "user2", "user3"],
+  };
+}
+
+describe("RemoveRefreshTokenMigratedFlagMigrator", () => {
+  let helper: MockProxy<MigrationHelper>;
+  let sut: RemoveRefreshTokenMigratedFlagMigrator;
+
+  describe("migrate", () => {
+    beforeEach(() => {
+      helper = mockMigrationHelper(preMigrationJson(), 57);
+      sut = new RemoveRefreshTokenMigratedFlagMigrator(57, 58);
+    });
+
+    it("should remove refreshTokenMigratedToSecureStorage from state provider for all accounts that have it", async () => {
+      await sut.migrate(helper);
+
+      expect(helper.removeFromUser).toHaveBeenCalledWith(
+        "user1",
+        REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
+      );
+      expect(helper.removeFromUser).toHaveBeenCalledWith(
+        "user2",
+        REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
+      );
+
+      expect(helper.removeFromUser).toHaveBeenCalledTimes(2);
+
+      expect(helper.removeFromUser).not.toHaveBeenCalledWith("user3", any());
+    });
+  });
+
+  describe("rollback", () => {
+    beforeEach(() => {
+      helper = mockMigrationHelper(rollbackJSON(), 58);
+      sut = new RemoveRefreshTokenMigratedFlagMigrator(57, 58);
+    });
+
+    it("should not add data back and throw IRREVERSIBLE error on call", async () => {
+      await expect(sut.rollback(helper)).rejects.toThrow(IRREVERSIBLE);
+    });
+  });
+});

libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.ts

@@ -0,0 +1,34 @@
+import { KeyDefinitionLike, MigrationHelper } from "../migration-helper";
+import { IRREVERSIBLE, Migrator } from "../migrator";
+
+type ExpectedAccountType = NonNullable<unknown>;
+
+export const REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE: KeyDefinitionLike = {
+  key: "refreshTokenMigratedToSecureStorage", // matches KeyDefinition.key in DeviceTrustCryptoService
+  stateDefinition: {
+    name: "token", // matches StateDefinition.name in StateDefinitions
+  },
+};
+
+export class RemoveRefreshTokenMigratedFlagMigrator extends Migrator<57, 58> {
+  async migrate(helper: MigrationHelper): Promise<void> {
+    const accounts = await helper.getAccounts<ExpectedAccountType>();
+    async function migrateAccount(userId: string, account: ExpectedAccountType): Promise<void> {
+      const refreshTokenMigratedFlag = await helper.getFromUser(
+        userId,
+        REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE,
+      );
+
+      if (refreshTokenMigratedFlag != null) {
+        // Only delete the flag if it exists
+        await helper.removeFromUser(userId, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE);
+      }
+    }
+
+    await Promise.all([...accounts.map(({ userId, account }) => migrateAccount(userId, account))]);
+  }
+
+  async rollback(helper: MigrationHelper): Promise<void> {
+    throw IRREVERSIBLE;
+  }
+}