CodeCow/clients
1
0
Fork 0
mirror of https://github.com/bitwarden/clients.git synced 2026-07-07 21:10:55 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

[PM-32761] Lock causes log out on TDE account with PIN (#19594)

Browse Source 
* switch back to default (Lock) session timeout action when PIN is enabled in master password less accounts.

* always reset to null (default) when only one action is available.

* unnecessary additional keys refresh on token storage migration
This commit is contained in:
Maciej Zieniuk 2026-04-02 18:48:53 +02:00 committed by GitHub
parent dd2c8765d6
commit 27efcdb9a0
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 232 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

172
libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.spec.ts
View File

@ -307,6 +307,176 @@ describe("VaultTimeoutSettingsService", () => {
},
);
});
describe("state persistence side effects", () => {
beforeEach(() => {
sessionTimeoutTypeService.getOrPromoteToAvailable.mockImplementation(
async (timeout) => timeout,
);
policyService.policiesByType$.mockReturnValue(of([]));
});
it("resets incompatible stored action to null when only one action is available (e.g. lock stored, only LogOut available)", async () => {
// TDE user removes PIN — only LogOut available but "lock" was stored
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(false));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
await stateProvider.setUserState(VAULT_TIMEOUT_ACTION, VaultTimeoutAction.Lock, mockUserId);
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
expect(
stateProvider.singleUser.getFake(mockUserId, VAULT_TIMEOUT_ACTION).nextMock,
).toHaveBeenCalledWith(null);
});
it("migrates tokens to memory when stored Lock is reset because only LogOut is available", async () => {
const mockAccessToken = "mockAccessToken";
const mockRefreshToken = "mockRefreshToken";
const mockClientId = "mockClientId";
const mockClientSecret = "mockClientSecret";
// TDE user removes PIN — only LogOut available, but "lock" was stored
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(false));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
tokenService.getAccessToken.mockResolvedValue(mockAccessToken);
tokenService.getRefreshToken.mockResolvedValue(mockRefreshToken);
tokenService.getClientId.mockResolvedValue(mockClientId);
tokenService.getClientSecret.mockResolvedValue(mockClientSecret);
await stateProvider.setUserState(VAULT_TIMEOUT_ACTION, VaultTimeoutAction.Lock, mockUserId);
await stateProvider.setUserState(VAULT_TIMEOUT, 15, mockUserId);
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
expect(tokenService.setTokens).toHaveBeenCalledWith(
mockAccessToken,
VaultTimeoutAction.LogOut,
15,
mockRefreshToken,
[mockClientId, mockClientSecret],
);
});
it("resets stored LogOut to null when only LogOut is available (master-password-less)", async () => {
// TDE user removed PIN — stored LogOut must be cleared so that re-enabling PIN later
// defaults back to Lock instead of staying on LogOut.
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(false));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
await stateProvider.setUserState(
VAULT_TIMEOUT_ACTION,
VaultTimeoutAction.LogOut,
mockUserId,
);
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
// State must be reset to null; no token migration since the effective action didn't change
expect(
stateProvider.singleUser.getFake(mockUserId, VAULT_TIMEOUT_ACTION).nextMock,
).toHaveBeenCalledWith(null);
expect(tokenService.setTokens).not.toHaveBeenCalled();
});
it("does not write when state is already null and only one action is available", async () => {
// TDE user with no unlock methods, state already null — no write needed
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(false));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
await stateProvider.setUserState(VAULT_TIMEOUT_ACTION, null, mockUserId);
const stateFake = stateProvider.singleUser.getFake(mockUserId, VAULT_TIMEOUT_ACTION);
stateFake.nextMock.mockClear();
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
expect(stateFake.nextMock).not.toHaveBeenCalled();
});
it("stores policy-forced LogOut when multiple actions are available", async () => {
// User with PIN — both Lock and LogOut available; policy forces LogOut
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(true));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
policyService.policiesByType$.mockReturnValue(
of([
{ data: { action: VaultTimeoutAction.LogOut, type: "never" } },
] as unknown as Policy[]),
);
await stateProvider.setUserState(VAULT_TIMEOUT_ACTION, null, mockUserId);
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
expect(
stateProvider.singleUser.getFake(mockUserId, VAULT_TIMEOUT_ACTION).nextMock,
).toHaveBeenCalledWith(VaultTimeoutAction.LogOut);
});
it("migrates tokens when effective action changes from null (forced LogOut reset) to Lock", async () => {
const mockAccessToken = "mockAccessToken";
const mockRefreshToken = "mockRefreshToken";
const mockClientId = "mockClientId";
const mockClientSecret = "mockClientSecret";
// User adds PIN — Lock becomes available; stored action is null (reset by migrator or new code)
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(true));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
tokenService.getAccessToken.mockResolvedValue(mockAccessToken);
tokenService.getRefreshToken.mockResolvedValue(mockRefreshToken);
tokenService.getClientId.mockResolvedValue(mockClientId);
tokenService.getClientSecret.mockResolvedValue(mockClientSecret);
await stateProvider.setUserState(VAULT_TIMEOUT_ACTION, null, mockUserId);
await stateProvider.setUserState(VAULT_TIMEOUT, 15, mockUserId);
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
expect(tokenService.setTokens).toHaveBeenCalledWith(
mockAccessToken,
VaultTimeoutAction.Lock,
15,
mockRefreshToken,
[mockClientId, mockClientSecret],
);
});
it("does not call setTokens when accessToken is null during null to Lock transition", async () => {
// User adds PIN but accessToken is null (e.g. not yet authenticated)
userDecryptionOptionsSubject.next(new UserDecryptionOptions({ hasMasterPassword: false }));
pinStateService.pinSet$.mockReturnValue(of(true));
biometricStateService.biometricUnlockEnabled$.mockReturnValue(of(false));
tokenService.getAccessToken.mockResolvedValue(null);
await stateProvider.setUserState(VAULT_TIMEOUT_ACTION, null, mockUserId);
await firstValueFrom(
vaultTimeoutSettingsService.getVaultTimeoutActionByUserId$(mockUserId),
);
expect(tokenService.setTokens).not.toHaveBeenCalled();
});
});
});
describe("getVaultTimeoutByUserId$", () => {
@ -803,6 +973,7 @@ describe("VaultTimeoutSettingsService", () => {
// Arrange
const action = VaultTimeoutAction.LogOut;
const timeout = 30;
tokenService.getAccessToken.mockResolvedValue(mockAccessToken);
// Act
await vaultTimeoutSettingsService.setVaultTimeoutOptions(mockUserId, timeout, action);
@ -815,6 +986,7 @@ describe("VaultTimeoutSettingsService", () => {
// Arrange
const action = VaultTimeoutAction.LogOut;
const timeout = VaultTimeoutStringType.Never;
tokenService.getAccessToken.mockResolvedValue(mockAccessToken);
// Act
await vaultTimeoutSettingsService.setVaultTimeoutOptions(mockUserId, timeout, action);

83
libs/common/src/key-management/vault-timeout/services/vault-timeout-settings.service.ts
View File

@ -3,16 +3,16 @@
import {
catchError,
combineLatest,
concatMap,
distinctUntilChanged,
EMPTY,
firstValueFrom,
from,
map,
of,
Observable,
of,
shareReplay,
switchMap,
concatMap,
} from "rxjs";
// This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop.
@ -76,27 +76,11 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
throw new Error("Vault Timeout Action cannot be null.");
}
// We swap these tokens from being on disk for lock actions, and in memory for logout actions
// Get them here to set them to their new location after changing the timeout action and clearing if needed
const accessToken = await this.tokenService.getAccessToken(userId);
const refreshToken = await this.tokenService.getRefreshToken(userId);
const clientId = await this.tokenService.getClientId(userId);
const clientSecret = await this.tokenService.getClientSecret(userId);
await this.setVaultTimeout(userId, timeout);
if (timeout != VaultTimeoutStringType.Never && action === VaultTimeoutAction.LogOut) {
// if we have a vault timeout and the action is log out, reset tokens
// as the tokens were stored on disk and now should be stored in memory
await this.tokenService.clearTokens(userId);
}
await this.setVaultTimeoutAction(userId, action);
await this.tokenService.setTokens(accessToken, action, timeout, refreshToken, [
clientId,
clientSecret,
]);
await this.migrateTokenStorage(userId, action, timeout);
await this.keyService.refreshAdditionalKeys(userId);
}
@ -259,6 +243,35 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
return currentVaultTimeout;
}
/**
* Re-stores tokens in the correct location (memory vs disk) for the given action and timeout.
*/
private async migrateTokenStorage(
userId: UserId,
action: VaultTimeoutAction,
timeout: VaultTimeout,
): Promise<void> {
// Read tokens before any clearing so they can be re-stored in the new location
const accessToken = await this.tokenService.getAccessToken(userId);
const refreshToken = await this.tokenService.getRefreshToken(userId);
const clientId = await this.tokenService.getClientId(userId);
const clientSecret = await this.tokenService.getClientSecret(userId);
if (timeout != VaultTimeoutStringType.Never && action === VaultTimeoutAction.LogOut) {
// Switching to LogOut: clear tokens from disk before re-storing in memory
await this.tokenService.clearTokens(userId);
}
if (!accessToken) {
return;
}
await this.tokenService.setTokens(accessToken, action, timeout, refreshToken, [
clientId,
clientSecret,
]);
}
private async setVaultTimeoutAction(userId: UserId, action: VaultTimeoutAction): Promise<void> {
if (!userId) {
throw new Error("User id required. Cannot set vault timeout action.");
@ -280,12 +293,14 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
this.stateProvider.getUserState$(VAULT_TIMEOUT_ACTION, userId),
this.getMaxSessionTimeoutPolicyDataByUserId$(userId),
this.availableVaultTimeoutActions$(userId),
this.getVaultTimeoutByUserId$(userId),
]).pipe(
concatMap(
async ([
currentVaultTimeoutAction,
maxSessionTimeoutPolicyData,
availableVaultTimeoutActions,
vaultTimeout,
]) => {
const vaultTimeoutAction = this.determineVaultTimeoutAction(
availableVaultTimeoutActions,
@ -293,11 +308,33 @@ export class VaultTimeoutSettingsService implements VaultTimeoutSettingsServiceA
maxSessionTimeoutPolicyData,
);
// As a side effect, set the new value determined by determineVaultTimeout into state if it's different from the current
// We want to avoid having a null timeout action always so we set it to the default if it is null
// and if the user becomes subject to a policy that requires a specific action, we set it to that
if (vaultTimeoutAction !== currentVaultTimeoutAction) {
// As a side effect, persist the determined action back to state when needed.
const oneActionAvailable = availableVaultTimeoutActions.length === 1;
if (oneActionAvailable) {
const availableAction = availableVaultTimeoutActions[0];
// Always reset to null when only one action is available — even if the stored action
// matches. Ensures the default (Lock) is used when an unlock method (e.g. PIN) is
// re-enabled later.
if (currentVaultTimeoutAction !== null) {
await this.stateProvider.setUserState(VAULT_TIMEOUT_ACTION, null, userId);
// Only migrate tokens if the actual action changes (e.g. Lock → LogOut).
// No migration needed when stored action already matches the only available one.
if (currentVaultTimeoutAction !== availableAction) {
await this.migrateTokenStorage(userId, availableAction, vaultTimeout);
}
}
} else if (vaultTimeoutAction !== currentVaultTimeoutAction) {
await this.stateProvider.setUserState(VAULT_TIMEOUT_ACTION, vaultTimeoutAction, userId);
// Migrate tokens when effective action changes from LogOut (memory) to Lock (disk).
// currentVaultTimeoutAction is null when forced LogOut was reset to null by the
// side effect (no unlock methods) or the state migrator.
if (
currentVaultTimeoutAction === null &&
vaultTimeoutAction === VaultTimeoutAction.Lock
) {
await this.migrateTokenStorage(userId, vaultTimeoutAction, vaultTimeout);
}
}
return vaultTimeoutAction;