From 581e64b8f7c850196a52a7a20a60e2a5c52f8d18 Mon Sep 17 00:00:00 2001 From: rr-bw <102181210+rr-bw@users.noreply.github.com> Date: Mon, 18 Aug 2025 13:09:41 -0700 Subject: [PATCH] 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. --- .../src/auth/services/account.service.ts | 8 +++-- .../src/auth/services/token.service.spec.ts | 8 ++++- .../common/src/auth/services/token.service.ts | 33 ++++++++++++++----- .../services/master-password.service.ts | 8 +++-- 4 files changed, 43 insertions(+), 14 deletions(-) diff --git a/libs/common/src/auth/services/account.service.ts b/libs/common/src/auth/services/account.service.ts index 50ba2455d78..1be11b03461 100644 --- a/libs/common/src/auth/services/account.service.ts +++ b/libs/common/src/auth/services/account.service.ts @@ -232,9 +232,11 @@ export class AccountServiceImplementation implements InternalAccountService { return; } - await this.singleUserStateProvider.get(userId, ACCOUNT_VERIFY_NEW_DEVICE_LOGIN).update(() => { - return setVerifyNewDeviceLogin; - }); + await this.singleUserStateProvider + .get(userId, ACCOUNT_VERIFY_NEW_DEVICE_LOGIN) + .update(() => setVerifyNewDeviceLogin, { + shouldUpdate: (previousValue) => previousValue !== setVerifyNewDeviceLogin, + }); } async removeAccountActivity(userId: UserId): Promise { diff --git a/libs/common/src/auth/services/token.service.spec.ts b/libs/common/src/auth/services/token.service.spec.ts index e67e522368f..7ed375da377 100644 --- a/libs/common/src/auth/services/token.service.spec.ts +++ b/libs/common/src/auth/services/token.service.spec.ts @@ -1476,8 +1476,14 @@ describe("TokenService", () => { 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 + + // 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); // Act diff --git a/libs/common/src/auth/services/token.service.ts b/libs/common/src/auth/services/token.service.ts index 4076cd68d3c..80e61d4636f 100644 --- a/libs/common/src/auth/services/token.service.ts +++ b/libs/common/src/auth/services/token.service.ts @@ -356,7 +356,10 @@ export class TokenService implements TokenServiceAbstraction { // Save the encrypted access token to disk await this.singleUserStateProvider .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 // 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 decryptedAccessToken = await this.singleUserStateProvider .get(userId, ACCESS_TOKEN_DISK) - .update((_) => accessToken); + .update((_) => accessToken, { + shouldUpdate: (previousValue) => previousValue !== accessToken, + }); } return decryptedAccessToken; @@ -384,7 +389,9 @@ export class TokenService implements TokenServiceAbstraction { // Access token stored on disk unencrypted as platform does not support secure storage return await this.singleUserStateProvider .get(userId, ACCESS_TOKEN_DISK) - .update((_) => accessToken); + .update((_) => accessToken, { + shouldUpdate: (previousValue) => previousValue !== accessToken, + }); case TokenStorageLocation.Memory: // Access token stored in memory due to vault timeout settings return await this.singleUserStateProvider @@ -439,7 +446,9 @@ export class TokenService implements TokenServiceAbstraction { } // 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); } @@ -586,7 +595,9 @@ export class TokenService implements TokenServiceAbstraction { // TODO: PM-6408 // 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. - 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); } catch (error) { // 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 decryptedRefreshToken = await this.singleUserStateProvider .get(userId, REFRESH_TOKEN_DISK) - .update((_) => refreshToken); + .update((_) => refreshToken, { + shouldUpdate: (previousValue) => previousValue !== refreshToken, + }); } return decryptedRefreshToken; @@ -607,7 +620,9 @@ export class TokenService implements TokenServiceAbstraction { case TokenStorageLocation.Disk: return await this.singleUserStateProvider .get(userId, REFRESH_TOKEN_DISK) - .update((_) => refreshToken); + .update((_) => refreshToken, { + shouldUpdate: (previousValue) => previousValue !== refreshToken, + }); case TokenStorageLocation.Memory: return await this.singleUserStateProvider @@ -687,7 +702,9 @@ export class TokenService implements TokenServiceAbstraction { // 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_DISK).update((_) => null); + await this.singleUserStateProvider.get(userId, REFRESH_TOKEN_DISK).update((_) => null, { + shouldUpdate: (previousValue) => previousValue !== null, + }); } async setClientId( diff --git a/libs/common/src/key-management/master-password/services/master-password.service.ts b/libs/common/src/key-management/master-password/services/master-password.service.ts index 41cfb268f44..6ca73a2ae14 100644 --- a/libs/common/src/key-management/master-password/services/master-password.service.ts +++ b/libs/common/src/key-management/master-password/services/master-password.service.ts @@ -149,14 +149,18 @@ export class MasterPasswordService implements InternalMasterPasswordServiceAbstr if (userId == null) { 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 { if (userId == null) { 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 {