[PS-1203] Call hasKey() to determine if vault is locked

[kbialek]

Type of change

- [X] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Fix for https://github.com/bitwarden/clients/issues/2729

I clearly admit that I do not have good understanding of this codebase. This proposal is a result of my experiments.

Code changes

• auth.service.ts: Use hasKey() instead of hasKeyInMemory() to verify if vault is unlocked

Screenshots

Before you submit

- [ ] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

[CLAassistant]

All committers have signed the CLA.

[Hinton]

Hi @kbialek,

Unfortunately while this might solve the linked error it will introduce other issues, primarily around account switching. getAuthStatus has an optional parameter userId which is passed along to hasKeyInMemory. While hasKey just checks if the currently active account is locked.

[kbialek]

Hi @Hinton

Okay, I get the point. In my latest commit I've enhanced getKey() method to also accept optional userId. WDYT?

[shawnfunke]

Hey,

is there any update for this? I've looked into this issue as well, the provided implementation here also makes the workaround below obsolete, this code snippet is not required anymore and is correct due to the check with the getEverBeenUnlocked function.

https://github.com/bitwarden/clients/blob/25caeaa26f21c4d6aca74436fd82dd6eebc9d2b5/libs/common/src/services/auth.service.ts#L168-L177

Even tho the check is unrequired, without the additional check it would also match the exact behaviour already used throughout the CLI, for example when running the bw list items command.

https://github.com/bitwarden/clients/blob/25caeaa26f21c4d6aca74436fd82dd6eebc9d2b5/apps/cli/src/program.ts#L513-L522

If this implementation is used throughout the application the workaround in getKey where it also sets the symmetric key can be phased out.

https://github.com/bitwarden/clients/blob/25caeaa26f21c4d6aca74436fd82dd6eebc9d2b5/libs/common/src/services/crypto.service.ts#L96-L112

/cc @Hinton, @djsmith85

[sgallagher]

Can we get an update on this? This broke one of my CI automation workflows.

[Hinton]

Hi,

I took a quick look at the proposed change, and it seems to completely break the desktop application.

I don't believe hasKey is the right method to determine the CLI lock status. That method checks for existence of both automatic and biometric keys stored in secure storage, which the CLI does not support.

A better way to approach this is to do some pre-precessing in some common code path (maybe exitIfLocked) to load in the BW_SESSION variable and attempt to decrypt the account key.

[kbialek]

@Hinton Thanks for having a look. Could you shortly explain what do you mean by that?

it seems to completely break the desktop application.

I built the desktop client locally from the latest master with the fix applied and it is working fine.

A better way to approach this is to do some pre-precessing in some common code path (maybe exitIfLocked) to load in the BW_SESSION variable and attempt to decrypt the account key.

Imo, loading BW_SESSION is not the right approach, as the session key can also be provided as a CLI argument

[milkpirate]

Could all the reviewers and contributors have another look at the PR? This issue https://github.com/bitwarden/clients/issues/2729 is really annoying.

[djsmith85]

@kbialek Thank you for your contribution

Closing this, as the issue seems to be resolved with 2024.2 as per your comment

Kind regards, Daniel