[PM-3726] prevent legacy user login

[jlf0dev]

Type of change

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

Objective

Prevent legacy users from logging in and direct them to the web vault for migration.

Code changes

Legacy users are detected early in the AuthService.LoginHelper by checking for the existence of a local hashed password (meaning MP was used to login) and the absence of a IdentityToken.Key. We can then return early before the authentication process and prevent any state from being set.

• LockPageViewModel.cs: Prevents unlocks for legacy users. Most of these changes are formatting. I split out the SubmitAsync method into UnlockWithPinAsync and UnlockWithMasterPasswordAsync. I also am catching any LegacyUserException errors now in the SubmitAsync method in order to process them.
• BaseLockPasswordViewController.cs: Duplicate changes as LockPageViewModel
• LoginPageViewModel.cs: Prevent logins for legacy users. Note: Instead of using the LegacyUserException like on the LockPage, I have added AuthResult.RequiresEncryptionKeyMigration. The clients repo couldn't use an exception in the same way so I decided to keep the models similar.
• AuthService.cs: If a user is logging in with their MP, we can determine if they are a legacy user and return early.
• CryptoService.cs: Add ValidateUserKeyAsync and IsLegacyUserAsync for legacy user detection. Throw exceptions from migration methods if a legacy user is detected.
• VaultTimeoutService.cs: If a legacy user has an auto key set, we need to catch the exception and log them out before they are migrated.

Screenshots

Before you submit

• Please check for formatting errors (dotnet format --verify-no-changes) (required)
• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[bitwarden-bot]

Checkmarx One – Scan Summary & Details – 9d53fcac-8c0a-4f2f-b63b-17e1a1cc85b7

No New Or Fixed Issues Found

[fedemkr]

Great work!
I see this repeated several times:

if (await _cryptoService.IsLegacyUserAsync(masterKey))
{
    throw new LegacyUserException();
}

If every time checking legacy should throw that exception, couldn't that be just done inside the method like:

public async Task CheckIfUserIsLegacyAsync(MasterKey masterKey = null, string userId = null)
{
    if (await ValidateUserKeyAsync(new UserKey((masterKey ?? await GetMasterKeyAsync(userId)).Key))) // Also here see other comment to check the possibility of null getting the master key
    {
        throw new LegacyUserException();
    }
}

So we avoid checking and throwing it everywhere. Thoughts?

[fedemkr]

⛏️ Can the check be moved right after masterKey initialization, because there's no need to await for getting the master key hash if it's a legacy user. Unless it depends on some internal computation on that latter task to be able to say whether it's legacy or not.
🤔 Could the check be done directly in MakeMasterKeyAsync()? Or is it not done that way to prevent potentially breaking other places?

[jlf0dev]

We won't have all the pieces yet to determine if they are legacy when simply logging in. I also didn't know if we wanted to run that code every time so left it out for now.

[fedemkr]

⚠️ GetMasterKeyAsync(...) could potentially return null. Is it ok if that happens to throw a null reference exception here (when accessing .Key)?

[jlf0dev]

Good catch, added a check

[jlf0dev]

So we avoid checking and throwing it everywhere. Thoughts?

I actually had to add that check to the login process, where we don't want it to throw. Would definitely clean up the code but I didn't want to use exceptions for the login process.