1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-12 22:33:35 +00:00

refactor(auth-state-updates): [Auth/PM-18544] Add shouldUpdate() checks to frequently updated state (#15994)

Adds `shouldUpdate()` checks to frequently updated disk state in the Auth domain, specifically:
- `account_verifyNewDeviceLogin`
- `token_accessToken`
- `token_refreshToken`
- `masterPassword_masterKeyHash`
This ensures that the state only updates if the new value is different from the previous value, thus avoiding unnecessary updates.
This commit is contained in:
rr-bw
2025-08-18 13:09:41 -07:00
committed by GitHub
parent 827c4c0301
commit 581e64b8f7
4 changed files with 43 additions and 14 deletions

View File

@@ -232,9 +232,11 @@ export class AccountServiceImplementation implements InternalAccountService {
return; return;
} }
await this.singleUserStateProvider.get(userId, ACCOUNT_VERIFY_NEW_DEVICE_LOGIN).update(() => { await this.singleUserStateProvider
return setVerifyNewDeviceLogin; .get(userId, ACCOUNT_VERIFY_NEW_DEVICE_LOGIN)
}); .update(() => setVerifyNewDeviceLogin, {
shouldUpdate: (previousValue) => previousValue !== setVerifyNewDeviceLogin,
});
} }
async removeAccountActivity(userId: UserId): Promise<void> { async removeAccountActivity(userId: UserId): Promise<void> {

View File

@@ -1476,8 +1476,14 @@ describe("TokenService", () => {
expect(logoutCallback).not.toHaveBeenCalled(); expect(logoutCallback).not.toHaveBeenCalled();
}); });
it("does not error and fallback to disk storage when passed a null value for the refresh token", async () => { it("does not error and does not fallback to disk storage when passed a null value for the refresh token", async () => {
// Arrange // Arrange
// We must have an initial value in disk so that we can assert that it gets cleaned up
singleUserStateProvider
.getFake(userIdFromAccessToken, REFRESH_TOKEN_DISK)
.nextState(refreshToken);
secureStorageService.get.mockResolvedValue(null); secureStorageService.get.mockResolvedValue(null);
// Act // Act

View File

@@ -356,7 +356,10 @@ export class TokenService implements TokenServiceAbstraction {
// Save the encrypted access token to disk // Save the encrypted access token to disk
await this.singleUserStateProvider await this.singleUserStateProvider
.get(userId, ACCESS_TOKEN_DISK) .get(userId, ACCESS_TOKEN_DISK)
.update((_) => encryptedAccessToken.encryptedString); .update((_) => encryptedAccessToken.encryptedString, {
shouldUpdate: (previousValue) =>
previousValue !== encryptedAccessToken.encryptedString,
});
// If we've successfully stored the encrypted access token to disk, we can return the decrypted access token // If we've successfully stored the encrypted access token to disk, we can return the decrypted access token
// so that the caller can use it immediately. // so that the caller can use it immediately.
@@ -375,7 +378,9 @@ export class TokenService implements TokenServiceAbstraction {
// Fall back to disk storage for unecrypted access token // Fall back to disk storage for unecrypted access token
decryptedAccessToken = await this.singleUserStateProvider decryptedAccessToken = await this.singleUserStateProvider
.get(userId, ACCESS_TOKEN_DISK) .get(userId, ACCESS_TOKEN_DISK)
.update((_) => accessToken); .update((_) => accessToken, {
shouldUpdate: (previousValue) => previousValue !== accessToken,
});
} }
return decryptedAccessToken; return decryptedAccessToken;
@@ -384,7 +389,9 @@ export class TokenService implements TokenServiceAbstraction {
// Access token stored on disk unencrypted as platform does not support secure storage // Access token stored on disk unencrypted as platform does not support secure storage
return await this.singleUserStateProvider return await this.singleUserStateProvider
.get(userId, ACCESS_TOKEN_DISK) .get(userId, ACCESS_TOKEN_DISK)
.update((_) => accessToken); .update((_) => accessToken, {
shouldUpdate: (previousValue) => previousValue !== accessToken,
});
case TokenStorageLocation.Memory: case TokenStorageLocation.Memory:
// Access token stored in memory due to vault timeout settings // Access token stored in memory due to vault timeout settings
return await this.singleUserStateProvider return await this.singleUserStateProvider
@@ -439,7 +446,9 @@ export class TokenService implements TokenServiceAbstraction {
} }
// Platform doesn't support secure storage, so use state provider implementation // Platform doesn't support secure storage, so use state provider implementation
await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_DISK).update((_) => null); await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_DISK).update((_) => null, {
shouldUpdate: (previousValue) => previousValue !== null,
});
await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MEMORY).update((_) => null); await this.singleUserStateProvider.get(userId, ACCESS_TOKEN_MEMORY).update((_) => null);
} }
@@ -586,7 +595,9 @@ export class TokenService implements TokenServiceAbstraction {
// TODO: PM-6408 // TODO: PM-6408
// 2024-02-20: Remove refresh token from memory and disk so that we migrate to secure storage over time. // 2024-02-20: Remove refresh token from memory and disk so that we migrate to secure storage over time.
// Remove these 2 calls to remove the refresh token from memory and disk after 3 months. // Remove these 2 calls to remove the refresh token from memory and disk after 3 months.
await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null); await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null, {
shouldUpdate: (previousValue) => previousValue !== null,
});
await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MEMORY).update((_) => null); await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MEMORY).update((_) => null);
} catch (error) { } catch (error) {
// This case could be hit for both Linux users who don't have secure storage configured // This case could be hit for both Linux users who don't have secure storage configured
@@ -599,7 +610,9 @@ export class TokenService implements TokenServiceAbstraction {
// Fall back to disk storage for refresh token // Fall back to disk storage for refresh token
decryptedRefreshToken = await this.singleUserStateProvider decryptedRefreshToken = await this.singleUserStateProvider
.get(userId, REFRESH_TOKEN_DISK) .get(userId, REFRESH_TOKEN_DISK)
.update((_) => refreshToken); .update((_) => refreshToken, {
shouldUpdate: (previousValue) => previousValue !== refreshToken,
});
} }
return decryptedRefreshToken; return decryptedRefreshToken;
@@ -607,7 +620,9 @@ export class TokenService implements TokenServiceAbstraction {
case TokenStorageLocation.Disk: case TokenStorageLocation.Disk:
return await this.singleUserStateProvider return await this.singleUserStateProvider
.get(userId, REFRESH_TOKEN_DISK) .get(userId, REFRESH_TOKEN_DISK)
.update((_) => refreshToken); .update((_) => refreshToken, {
shouldUpdate: (previousValue) => previousValue !== refreshToken,
});
case TokenStorageLocation.Memory: case TokenStorageLocation.Memory:
return await this.singleUserStateProvider return await this.singleUserStateProvider
@@ -687,7 +702,9 @@ export class TokenService implements TokenServiceAbstraction {
// Platform doesn't support secure storage, so use state provider implementation // Platform doesn't support secure storage, so use state provider implementation
await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MEMORY).update((_) => null); await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_MEMORY).update((_) => null);
await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null); await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null, {
shouldUpdate: (previousValue) => previousValue !== null,
});
} }
async setClientId( async setClientId(

View File

@@ -149,14 +149,18 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr
if (userId == null) { if (userId == null) {
throw new Error("User ID is required."); throw new Error("User ID is required.");
} }
await this.stateProvider.getUser(userId, MASTER_KEY_HASH).update((_) => masterKeyHash); await this.stateProvider.getUser(userId, MASTER_KEY_HASH).update((_) => masterKeyHash, {
shouldUpdate: (previousValue) => previousValue !== masterKeyHash,
});
} }
async clearMasterKeyHash(userId: UserId): Promise<void> { async clearMasterKeyHash(userId: UserId): Promise<void> {
if (userId == null) { if (userId == null) {
throw new Error("User ID is required."); throw new Error("User ID is required.");
} }
await this.stateProvider.getUser(userId, MASTER_KEY_HASH).update((_) => null); await this.stateProvider.getUser(userId, MASTER_KEY_HASH).update((_) => null, {
shouldUpdate: (previousValue) => previousValue !== null,
});
} }
async setMasterKeyEncryptedUserKey(encryptedKey: EncString, userId: UserId): Promise<void> { async setMasterKeyEncryptedUserKey(encryptedKey: EncString, userId: UserId): Promise<void> {