[PS-74] Fix user authentication state checks

[eliykat]

Type of change

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

Objective

Fix #1557

Fix LockGuardService allowing access to the lock page when the user is actually logged out:

Steps to reproduce: navigate to <vault URL>/#/lock when logged out.

Code changes

Currently, vaultTimeoutService.isLocked() returns true if the user is logged out OR if the vault is locked, and it's the caller's responsibility to check both those states in the correct order. That is, you have to do await stateService.getIsAuthenticated() && await vaultTimeoutService.isLocked() to know if the vault is actually locked.

#600 caused this regression by removing the getIsAuthenticated check, assuming - reasonably - that it was redundant.

Rather than restore the original code in LockGuardService, we should make vaultTimeoutService.isLocked() only return true if the vault is actually locked (and not just logged out).

I'll need to do a sweep of each client when updating jslib, to make sure this won't break existing logic anywhere else.

Testing requirements

Confirm bug is fixed.

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)

[eliykat]

Another option is to have a VaultState enum that has values for logged out, locked, or unlocked - making it clear that these are 3 mutually exclusive states and providing a single interface to get the state, whatever it is. I'm not sure that's worth doing, but I'm floating the idea here it in case the reviewer thinks it is worth doing.

[cscharf]

Another option is to have a VaultState enum that has values for logged out, locked, or unlocked - making it clear that these are 3 mutually exclusive states and providing a single interface to get the state, whatever it is. I'm not sure that's worth doing, but I'm floating the idea here it in case the reviewer thinks it is worth doing.

I think this may be worth doing.

[mpbw2]

Another option is to have a VaultState enum that has values for logged out, locked, or unlocked - making it clear that these are 3 mutually exclusive states and providing a single interface to get the state, whatever it is.

Just FYI: https://github.com/bitwarden/jslib/blob/master/common/src/enums/authenticationStatus.ts

[eliykat]

Thanks both! I'll do this.

[eliykat]

Further additional changes after the feedback above:

• add new method, authService.authStatus(userId?: string). This will return the AuthenticationStatus enum reflecting the state of the account.
• remove vaultTimeoutService.isLocked, which is now deprecated
• unfortunately we can't remove stateService.getIsAuthenticated because it's required internally for stateService's own use. We can't move it to authService because that'll create a circular dependency. And we can't move authStatus into stateService, because that will also create a circular dependency with cryptoService. 😞 So I've left this in place as a lower-level method.
• change AuthenticationStatus to be a regular (number) enum, not a string enum. No reason to do string comparisons every time we want to check it, also this lets us use < and > which I think is neat.
• remove the AuthenticationStatus.active value. Now that we're using this enum in other contexts, this is confusing, because an account can be active/inactive and also unlocked. It turns out we weren't really using this value anyway, so it's safe to clean up.
• update all services etc. to use the new interface.

Related desktop PR: bitwarden/desktop#1464
That's the main client affected by this and should be reviewed in conjunction with this one.

Overall this still isn't perfect, it makes the checks more verbose in some places and less verbose in others, but on balance I still think it's an improvement and should hopefully avoid this regression popping up again.

[djsmith85]

@eliykat The code changes look fine, besides the things I commented on and some general questions/concerns

The heavy use of authStatus, does change the amount of work to be done in some parts.

For example authStatus() not only replaces stateService.getIsAuthenticated() but also vaultTimeoutService.isLocked(). Which means when a user is logged in, we always check for the lock state of the vault, even if the calling code only wants to know if a user is authed.

The same goes for vaultTimeoutService.isLocked(), which now always checks if the user is authed, which I guess is generally a good thing, just means as that method gets called often, it also does more than before.

In the first case it might be better to offer a authService.sAuthed() instead of authService.getAuthStatus() which also checks additional state.

[djsmith85]

As this a method and not a property/field of a class, I'd prefer having a name indicating what it's doing. getAuthState/retrieveAuthState

Similiar to the 'authing'-methods above. As they are returning booleans, they should maybe be renamed to isAuthingWithPassword

[eliykat]

Renamed to getAuthState.

I think the 'authing' methods are clear enough, I don't really want to mess with them in this PR.

[eliykat]

You raise some good points about performance.

It's okay that we check whether the user is authenticated when getting lock status - that's required to get an accurate answer. But I agree that the reverse (checking lock status when asking if the user is authenticated) is unnecessary.

How about we just leave all the stateService.getIsAuthenticated calls in place, and only use the new authService.authStatus:

• as a replacement for vaultTimeoutService.isLocked()
• where we want to know about both, e.g. as a replacement for isAuthed && isUnlocked, or routing guards where we want to take different actions based on status

Using the new method felt a bit cumbersome in some places, that might be a better balance.

[djsmith85]

You raise some good points about performance.

It's okay that we check whether the user is authenticated when getting lock status - that's required to get an accurate answer. But I agree that the reverse (checking lock status when asking if the user is authenticated) is unnecessary.

How about we just leave all the stateService.getIsAuthenticated calls in place, and only use the new authService.authStatus:

• as a replacement for vaultTimeoutService.isLocked()
• where we want to know about both, e.g. as a replacement for isAuthed && isUnlocked, or routing guards where we want to take different actions based on status

Using the new method felt a bit cumbersome in some places, that might be a better balance.

That sounds like a good approach to go forward with.

[eliykat]

These changes have been made.

[djsmith85]

@eliykat Sorry for the delay on this. This is looking good now.

[eliykat]

Had to resolve conflicts @djsmith85