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; + } +}