From 51a6b34cc29dd3316c89c7fa706d263cbc127c9c Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Tue, 16 Apr 2024 14:05:47 -0400 Subject: [PATCH] Auth/PM-7467- Fix Refresh token issues (#8757) * PM-7467 - Login Strategy bug - VaultTimeoutSettings will be undefined before the account is activated unless you pass in user ids to retrieve the data. This resulted in refresh tokens always being set into secure storage regardless of a user's vault timeout settings (logout should translate to memory) * PM-7467 - TokenSvc - Fix bug in getRefreshToken which would retrieve the user's refresh token from secure storage even if the user had changed their vault timeout setting to log out which moved the refresh token into memory. Includes a migration to remove the no longer required REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE state provider flag. * PM-7467 - Per PR feedback, use IRREVERSIBLE for rollback. Co-authored-by: Jake Fink * PM-7467 - fix tests * PM-7467 - Fix migrator based on PR feedback. * PM-7467 - Bump migration version --------- Co-authored-by: Jake Fink --- .../common/login-strategies/login.strategy.ts | 4 +- .../src/auth/services/token.service.spec.ts | 30 +------- .../common/src/auth/services/token.service.ts | 35 ++++----- .../src/auth/services/token.state.spec.ts | 2 - libs/common/src/auth/services/token.state.ts | 9 --- libs/common/src/state-migrations/migrate.ts | 6 +- ...token-migrated-state-provider-flag.spec.ts | 72 +++++++++++++++++++ ...resh-token-migrated-state-provider-flag.ts | 34 +++++++++ 8 files changed, 125 insertions(+), 67 deletions(-) create mode 100644 libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.spec.ts create mode 100644 libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.ts diff --git a/libs/auth/src/common/login-strategies/login.strategy.ts b/libs/auth/src/common/login-strategies/login.strategy.ts index 94f96d40d0..a6dc193183 100644 --- a/libs/auth/src/common/login-strategies/login.strategy.ts +++ b/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 vaultTimeout = await this.stateService.getVaultTimeout(); + const vaultTimeoutAction = await this.stateService.getVaultTimeoutAction({ userId }); + 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. diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index c409263209..d32c4d8e1c 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/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(); diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index db39997663..c24a2c186b 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/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 { - return await firstValueFrom( - this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE).state$, - ); - } - - private async setRefreshTokenMigratedToSecureStorage(userId: UserId): Promise { - await this.singleUserStateProvider - .get(userId, REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE) - .update((_) => true); - } - async setClientId( clientId: string, vaultTimeoutAction: VaultTimeoutAction, diff --git a/libs/common/src/auth/services/token.state.spec.ts b/libs/common/src/auth/services/token.state.spec.ts index 55f97b7e00..dc00fec383 100644 --- a/libs/common/src/auth/services/token.state.spec.ts +++ b/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"], diff --git a/libs/common/src/auth/services/token.state.ts b/libs/common/src/auth/services/token.state.ts index a8c6878fbb..458d6846c1 100644 --- a/libs/common/src/auth/services/token.state.ts +++ b/libs/common/src/auth/services/token.state.ts @@ -30,15 +30,6 @@ export const REFRESH_TOKEN_MEMORY = new UserKeyDefinition(TOKEN_MEMORY, clearOn: [], // Manually handled }); -export const REFRESH_TOKEN_MIGRATED_TO_SECURE_STORAGE = new UserKeyDefinition( - 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( TOKEN_DISK_LOCAL, "emailTwoFactorTokenRecord", diff --git a/libs/common/src/state-migrations/migrate.ts b/libs/common/src/state-migrations/migrate.ts index 000f85519e..f9a8734731 100644 --- a/libs/common/src/state-migrations/migrate.ts +++ b/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( diff --git a/libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.spec.ts b/libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.spec.ts new file mode 100644 index 0000000000..8d8040e4a0 --- /dev/null +++ b/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; + 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); + }); + }); +}); diff --git a/libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.ts b/libs/common/src/state-migrations/migrations/58-remove-refresh-token-migrated-state-provider-flag.ts new file mode 100644 index 0000000000..9c6d3776fe --- /dev/null +++ b/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; + +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 { + const accounts = await helper.getAccounts(); + async function migrateAccount(userId: string, account: ExpectedAccountType): Promise { + 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 { + throw IRREVERSIBLE; + } +}