From 899172722af5826e0a27efb18782a0b13ee7d95a Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Mon, 25 Mar 2024 16:26:27 -0400 Subject: [PATCH] Auth/PM-5263 - TokenService State provider migration bug fix to avoid persisting tokens in local storage (#8413) * PM-5263 - Update token svc state provider migration to avoid persisting secrets that shouldn't exist in local storage to state provider local storage using new migration helper type. * PM-5263 - TokenSvc migration - tests TODO * write tests for migration * fix tests --------- Co-authored-by: Jake Fink --- ...igrate-token-svc-to-state-provider.spec.ts | 144 +++++++++++------- .../38-migrate-token-svc-to-state-provider.ts | 20 ++- 2 files changed, 109 insertions(+), 55 deletions(-) diff --git a/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.spec.ts b/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.spec.ts index a5243c261a..7dae6eeeb6 100644 --- a/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.spec.ts +++ b/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.spec.ts @@ -124,65 +124,107 @@ describe("TokenServiceStateProviderMigrator", () => { sut = new TokenServiceStateProviderMigrator(37, 38); }); - it("should remove state service data from all accounts that have it", async () => { - await sut.migrate(helper); + describe("Session storage", () => { + it("should remove state service data from all accounts that have it", async () => { + await sut.migrate(helper); - expect(helper.set).toHaveBeenCalledWith("user1", { - tokens: { - otherStuff: "overStuff2", - }, - profile: { - email: "user1Email", - otherStuff: "overStuff3", - }, - keys: { - otherStuff: "overStuff4", - }, - otherStuff: "otherStuff5", + expect(helper.set).toHaveBeenCalledWith("user1", { + tokens: { + otherStuff: "overStuff2", + }, + profile: { + email: "user1Email", + otherStuff: "overStuff3", + }, + keys: { + otherStuff: "overStuff4", + }, + otherStuff: "otherStuff5", + }); + + expect(helper.set).toHaveBeenCalledTimes(2); + expect(helper.set).not.toHaveBeenCalledWith("user2", any()); + expect(helper.set).not.toHaveBeenCalledWith("user3", any()); }); - expect(helper.set).toHaveBeenCalledTimes(2); - expect(helper.set).not.toHaveBeenCalledWith("user2", any()); - expect(helper.set).not.toHaveBeenCalledWith("user3", any()); + it("should migrate data to state providers for defined accounts that have the data", async () => { + await sut.migrate(helper); + + // Two factor Token Migration + expect(helper.setToGlobal).toHaveBeenLastCalledWith( + EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, + { + user1Email: "twoFactorToken", + user2Email: "twoFactorToken", + }, + ); + expect(helper.setToGlobal).toHaveBeenCalledTimes(1); + + expect(helper.setToUser).toHaveBeenCalledWith("user1", ACCESS_TOKEN_DISK, "accessToken"); + expect(helper.setToUser).toHaveBeenCalledWith("user1", REFRESH_TOKEN_DISK, "refreshToken"); + expect(helper.setToUser).toHaveBeenCalledWith( + "user1", + API_KEY_CLIENT_ID_DISK, + "apiKeyClientId", + ); + expect(helper.setToUser).toHaveBeenCalledWith( + "user1", + API_KEY_CLIENT_SECRET_DISK, + "apiKeyClientSecret", + ); + + expect(helper.setToUser).not.toHaveBeenCalledWith("user2", ACCESS_TOKEN_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith("user2", REFRESH_TOKEN_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith("user2", API_KEY_CLIENT_ID_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith( + "user2", + API_KEY_CLIENT_SECRET_DISK, + any(), + ); + + // Expect that we didn't migrate anything to user 3 + + expect(helper.setToUser).not.toHaveBeenCalledWith("user3", ACCESS_TOKEN_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith("user3", REFRESH_TOKEN_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith("user3", API_KEY_CLIENT_ID_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalledWith( + "user3", + API_KEY_CLIENT_SECRET_DISK, + any(), + ); + }); }); + describe("Local storage", () => { + beforeEach(() => { + helper = mockMigrationHelper(preMigrationJson(), 37, "web-disk-local"); + }); + it("should remove state service data from all accounts that have it", async () => { + await sut.migrate(helper); - it("should migrate data to state providers for defined accounts that have the data", async () => { - await sut.migrate(helper); + expect(helper.set).toHaveBeenCalledWith("user1", { + tokens: { + otherStuff: "overStuff2", + }, + profile: { + email: "user1Email", + otherStuff: "overStuff3", + }, + keys: { + otherStuff: "overStuff4", + }, + otherStuff: "otherStuff5", + }); - // Two factor Token Migration - expect(helper.setToGlobal).toHaveBeenLastCalledWith( - EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, - { - user1Email: "twoFactorToken", - user2Email: "twoFactorToken", - }, - ); - expect(helper.setToGlobal).toHaveBeenCalledTimes(1); + expect(helper.set).toHaveBeenCalledTimes(2); + expect(helper.set).not.toHaveBeenCalledWith("user2", any()); + expect(helper.set).not.toHaveBeenCalledWith("user3", any()); + }); - expect(helper.setToUser).toHaveBeenCalledWith("user1", ACCESS_TOKEN_DISK, "accessToken"); - expect(helper.setToUser).toHaveBeenCalledWith("user1", REFRESH_TOKEN_DISK, "refreshToken"); - expect(helper.setToUser).toHaveBeenCalledWith( - "user1", - API_KEY_CLIENT_ID_DISK, - "apiKeyClientId", - ); - expect(helper.setToUser).toHaveBeenCalledWith( - "user1", - API_KEY_CLIENT_SECRET_DISK, - "apiKeyClientSecret", - ); + it("should not migrate any data to local storage", async () => { + await sut.migrate(helper); - expect(helper.setToUser).not.toHaveBeenCalledWith("user2", ACCESS_TOKEN_DISK, any()); - expect(helper.setToUser).not.toHaveBeenCalledWith("user2", REFRESH_TOKEN_DISK, any()); - expect(helper.setToUser).not.toHaveBeenCalledWith("user2", API_KEY_CLIENT_ID_DISK, any()); - expect(helper.setToUser).not.toHaveBeenCalledWith("user2", API_KEY_CLIENT_SECRET_DISK, any()); - - // Expect that we didn't migrate anything to user 3 - - expect(helper.setToUser).not.toHaveBeenCalledWith("user3", ACCESS_TOKEN_DISK, any()); - expect(helper.setToUser).not.toHaveBeenCalledWith("user3", REFRESH_TOKEN_DISK, any()); - expect(helper.setToUser).not.toHaveBeenCalledWith("user3", API_KEY_CLIENT_ID_DISK, any()); - expect(helper.setToUser).not.toHaveBeenCalledWith("user3", API_KEY_CLIENT_SECRET_DISK, any()); + expect(helper.setToUser).not.toHaveBeenCalled(); + }); }); }); diff --git a/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.ts b/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.ts index 17753d2187..640e63cdc5 100644 --- a/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.ts +++ b/libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.ts @@ -84,7 +84,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> { if (existingAccessToken != null) { // Only migrate data that exists - await helper.setToUser(userId, ACCESS_TOKEN_DISK, existingAccessToken); + if (helper.type !== "web-disk-local") { + // only migrate access token to session storage - never local. + await helper.setToUser(userId, ACCESS_TOKEN_DISK, existingAccessToken); + } delete account.tokens.accessToken; updatedAccount = true; } @@ -93,7 +96,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> { const existingRefreshToken = account?.tokens?.refreshToken; if (existingRefreshToken != null) { - await helper.setToUser(userId, REFRESH_TOKEN_DISK, existingRefreshToken); + if (helper.type !== "web-disk-local") { + // only migrate refresh token to session storage - never local. + await helper.setToUser(userId, REFRESH_TOKEN_DISK, existingRefreshToken); + } delete account.tokens.refreshToken; updatedAccount = true; } @@ -102,7 +108,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> { const existingApiKeyClientId = account?.profile?.apiKeyClientId; if (existingApiKeyClientId != null) { - await helper.setToUser(userId, API_KEY_CLIENT_ID_DISK, existingApiKeyClientId); + if (helper.type !== "web-disk-local") { + // only migrate client id to session storage - never local. + await helper.setToUser(userId, API_KEY_CLIENT_ID_DISK, existingApiKeyClientId); + } delete account.profile.apiKeyClientId; updatedAccount = true; } @@ -110,7 +119,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> { // Migrate API key client secret const existingApiKeyClientSecret = account?.keys?.apiKeyClientSecret; if (existingApiKeyClientSecret != null) { - await helper.setToUser(userId, API_KEY_CLIENT_SECRET_DISK, existingApiKeyClientSecret); + if (helper.type !== "web-disk-local") { + // only migrate client secret to session storage - never local. + await helper.setToUser(userId, API_KEY_CLIENT_SECRET_DISK, existingApiKeyClientSecret); + } delete account.keys.apiKeyClientSecret; updatedAccount = true; }