Upstream/bitwarden-browser
1
0
Fork 0
mirror of https://github.com/bitwarden/browser.git synced 2024-12-22 16:29:09 +01:00
Code Issues Projects Releases Wiki Activity

Auth/PM-5263 - TokenService State provider migration bug fix to avoid persisting tokens in local storage (#8413)

Browse Source 
* 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 <jfink@bitwarden.com>
This commit is contained in:
Jared Snider 2024-03-25 16:26:27 -04:00 committed by GitHub
parent 0957b54d03
commit 899172722a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 109 additions and 55 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

144
libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.spec.ts
View File

@ -124,65 +124,107 @@ describe("TokenServiceStateProviderMigrator", () => {
sut = new TokenServiceStateProviderMigrator(37, 38); sut = new TokenServiceStateProviderMigrator(37, 38);
}); });
it("should remove state service data from all accounts that have it", async () => { describe("Session storage", () => {
await sut.migrate(helper); it("should remove state service data from all accounts that have it", async () => {
await sut.migrate(helper);
expect(helper.set).toHaveBeenCalledWith("user1", { expect(helper.set).toHaveBeenCalledWith("user1", {
tokens: { tokens: {
otherStuff: "overStuff2", otherStuff: "overStuff2",
}, },
profile: { profile: {
email: "user1Email", email: "user1Email",
otherStuff: "overStuff3", otherStuff: "overStuff3",
}, },
keys: { keys: {
otherStuff: "overStuff4", otherStuff: "overStuff4",
}, },
otherStuff: "otherStuff5", 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); it("should migrate data to state providers for defined accounts that have the data", async () => {
expect(helper.set).not.toHaveBeenCalledWith("user2", any()); await sut.migrate(helper);
expect(helper.set).not.toHaveBeenCalledWith("user3", any());
// 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 () => { expect(helper.set).toHaveBeenCalledWith("user1", {
await sut.migrate(helper); tokens: {
otherStuff: "overStuff2",
},
profile: {
email: "user1Email",
otherStuff: "overStuff3",
},
keys: {
otherStuff: "overStuff4",
},
otherStuff: "otherStuff5",
});
// Two factor Token Migration expect(helper.set).toHaveBeenCalledTimes(2);
expect(helper.setToGlobal).toHaveBeenLastCalledWith( expect(helper.set).not.toHaveBeenCalledWith("user2", any());
EMAIL_TWO_FACTOR_TOKEN_RECORD_DISK_LOCAL, expect(helper.set).not.toHaveBeenCalledWith("user3", any());
{ });
user1Email: "twoFactorToken",
user2Email: "twoFactorToken",
},
);
expect(helper.setToGlobal).toHaveBeenCalledTimes(1);
expect(helper.setToUser).toHaveBeenCalledWith("user1", ACCESS_TOKEN_DISK, "accessToken"); it("should not migrate any data to local storage", async () => {
expect(helper.setToUser).toHaveBeenCalledWith("user1", REFRESH_TOKEN_DISK, "refreshToken"); await sut.migrate(helper);
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.toHaveBeenCalled();
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());
}); });
}); });

20
libs/common/src/state-migrations/migrations/38-migrate-token-svc-to-state-provider.ts
View File

@ -84,7 +84,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> {
if (existingAccessToken != null) { if (existingAccessToken != null) {
// Only migrate data that exists // 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; delete account.tokens.accessToken;
updatedAccount = true; updatedAccount = true;
} }
@ -93,7 +96,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> {
const existingRefreshToken = account?.tokens?.refreshToken; const existingRefreshToken = account?.tokens?.refreshToken;
if (existingRefreshToken != null) { 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; delete account.tokens.refreshToken;
updatedAccount = true; updatedAccount = true;
} }
@ -102,7 +108,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> {
const existingApiKeyClientId = account?.profile?.apiKeyClientId; const existingApiKeyClientId = account?.profile?.apiKeyClientId;
if (existingApiKeyClientId != null) { 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; delete account.profile.apiKeyClientId;
updatedAccount = true; updatedAccount = true;
} }
@ -110,7 +119,10 @@ export class TokenServiceStateProviderMigrator extends Migrator<37, 38> {
// Migrate API key client secret // Migrate API key client secret
const existingApiKeyClientSecret = account?.keys?.apiKeyClientSecret; const existingApiKeyClientSecret = account?.keys?.apiKeyClientSecret;
if (existingApiKeyClientSecret != null) { 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; delete account.keys.apiKeyClientSecret;
updatedAccount = true; updatedAccount = true;
} }