diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt index 3675f59fad..cf5c64b55b 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt @@ -59,6 +59,7 @@ import com.x8bit.bitwarden.data.auth.datasource.disk.model.ForcePasswordResetRea import com.x8bit.bitwarden.data.auth.datasource.disk.model.OnboardingStatus import com.x8bit.bitwarden.data.auth.datasource.network.model.DeviceDataModel import com.x8bit.bitwarden.data.auth.datasource.sdk.AuthSdkSource +import com.x8bit.bitwarden.data.auth.datasource.sdk.util.toKdf import com.x8bit.bitwarden.data.auth.datasource.sdk.util.toKdfRequestModel import com.x8bit.bitwarden.data.auth.manager.AuthRequestManager import com.x8bit.bitwarden.data.auth.manager.AuthStateManager @@ -309,14 +310,22 @@ internal class AuthRepositoryImpl( override suspend fun deleteAccountWithMasterPassword( masterPassword: String, ): DeleteAccountResult { - val profile = authDiskSource.userState?.activeAccount?.profile - ?: return DeleteAccountResult.Error(message = null, error = NoActiveUserException()) + val masterPasswordUnlock = authDiskSource + .userState + ?.activeAccount + ?.profile + ?.userDecryptionOptions + ?.masterPasswordUnlock + ?: return DeleteAccountResult.Error( + message = null, + error = MissingPropertyException("Master Password Unlock"), + ) userStateManager.hasPendingAccountDeletion = true return authSdkSource .hashPassword( - salt = profile.email, + salt = masterPasswordUnlock.salt, password = masterPassword, - kdf = profile.toSdkParams(), + kdf = masterPasswordUnlock.kdf.toKdf(), purpose = HashPurpose.SERVER_AUTHORIZATION, ) .flatMap { hashedPassword -> @@ -978,12 +987,14 @@ internal class AuthRepositoryImpl( ): ResetPasswordResult { val profile = authDiskSource.userState?.activeAccount?.profile ?: return ResetPasswordResult.Error(error = NoActiveUserException()) + val masterPasswordUnlock = profile.userDecryptionOptions?.masterPasswordUnlock + ?: return ResetPasswordResult.Error(MissingPropertyException("Master Password Unlock")) val currentPasswordHash = currentPassword?.let { password -> authSdkSource .hashPassword( - salt = profile.email, + salt = masterPasswordUnlock.salt, password = password, - kdf = profile.toSdkParams(), + kdf = masterPasswordUnlock.kdf.toKdf(), purpose = HashPurpose.SERVER_AUTHORIZATION, ) .fold( @@ -1002,8 +1013,8 @@ internal class AuthRepositoryImpl( body = ResetPasswordRequestJson( currentPasswordHash = currentPasswordHash, passwordHint = passwordHint, - kdf = profile.toKdfRequestModel(), - salt = profile.email, + kdf = masterPasswordUnlock.kdf, + salt = masterPasswordUnlock.salt, masterPasswordAuthenticationHash = response.passwordHash, masterKeyWrappedUserKey = response.newKey, ), @@ -1701,23 +1712,6 @@ internal class AuthRepositoryImpl( } } - password?.let { - // Save the master password hash. - authSdkSource - .hashPassword( - salt = email, - password = it, - kdf = profile.toSdkParams(), - purpose = HashPurpose.LOCAL_AUTHORIZATION, - ) - .onSuccess { passwordHash -> - authDiskSource.storeMasterPasswordHash( - userId = userId, - passwordHash = passwordHash, - ) - } - } - settingsRepository.hasUserLoggedInOrCreatedAccount = true authDiskSource.userState = userStateJson password?.let { diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt index b588a87dd8..c78e1b934c 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerImpl.kt @@ -231,8 +231,6 @@ internal class VaultLockManagerImpl( .also { processMasterPassword( initUserCryptoMethod = initUserCryptoMethod, - email = email, - kdf = kdf, userId = userId, ) if (it is VaultUnlockResult.Success) { @@ -265,30 +263,28 @@ internal class VaultLockManagerImpl( */ private suspend fun processMasterPassword( initUserCryptoMethod: InitUserCryptoMethod, - email: String, - kdf: Kdf, userId: String, ) { - initUserCryptoMethod.password?.let { password -> + (initUserCryptoMethod as? InitUserCryptoMethod.MasterPasswordUnlock)?.let { // Save the master password hash. authSdkSource .hashPassword( - salt = email, - password = password, - kdf = kdf, + salt = it.masterPasswordUnlock.salt, + password = it.password, + kdf = it.masterPasswordUnlock.kdf, purpose = HashPurpose.LOCAL_AUTHORIZATION, ) - .onSuccess { + .onSuccess { passwordHash -> authDiskSource.storeMasterPasswordHash( userId = userId, - passwordHash = it, + passwordHash = passwordHash, ) } // If there is currently no forcePasswordResetReason, then we want to check to see if // the password is strong enough based on known policies. if (userState?.accounts[userId]?.profile?.forcePasswordResetReason == null && - !passwordPolicyManager.validatePasswordAgainstPolicies(password, false) + !passwordPolicyManager.validatePasswordAgainstPolicies(it.password, false) ) { authDiskSource.userState = userState?.updateForcePasswordReset( userId = userId, diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt index cbccf5bf5f..6a9b8c811d 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt @@ -407,13 +407,36 @@ class AuthRepositoryTest { } @Test - fun `delete account fails if not logged in`() = runTest { + fun `delete account fails if there is no master password unlock data`() = runTest { val masterPassword = "hello world" + fakeAuthDiskSource.userState = SINGLE_USER_STATE_1.copy( + accounts = mapOf( + USER_ID_1 to ACCOUNT_1.copy( + profile = ACCOUNT_1.profile.copy(userDecryptionOptions = null), + ), + ), + ) + val result = repository.deleteAccountWithMasterPassword(masterPassword = masterPassword) + assertEquals( - DeleteAccountResult.Error(message = null, error = NoActiveUserException()), + DeleteAccountResult.Error( + message = null, + error = MissingPropertyException("Master Password Unlock"), + ), result, ) + verify(exactly = 0) { + userStateManager.hasPendingAccountDeletion = true + } + coVerify(exactly = 0) { + authSdkSource.hashPassword( + salt = any(), + password = any(), + kdf = any(), + purpose = any(), + ) + } } @Test @@ -503,36 +526,73 @@ class AuthRepositoryTest { } } + @Suppress("MaxLineLength") @Test - fun `deleteAccountWithMasterPassword succeeds`() = runTest { - val masterPassword = "hello world" - val hashedMasterPassword = "hashed password" - fakeAuthDiskSource.userState = SINGLE_USER_STATE_1 - val kdf = SINGLE_USER_STATE_1.activeAccount.profile.toSdkParams() - coEvery { - authSdkSource.hashPassword(EMAIL, masterPassword, kdf, HashPurpose.SERVER_AUTHORIZATION) - } returns hashedMasterPassword.asSuccess() - coEvery { - accountsService.deleteAccount( - masterPasswordHash = hashedMasterPassword, - oneTimePassword = null, + fun `deleteAccountWithMasterPassword succeeds and uses the master password unlock salt and kdf`() = + runTest { + val masterPassword = "hello world" + val hashedMasterPassword = "hashed password" + // Deliberately differs from the profile email and KDF to verify the unlock data is used + // instead. + val masterPasswordUnlock = MasterPasswordUnlockDataJson( + kdf = KdfJson( + kdfType = KdfTypeJson.PBKDF2_SHA256, + iterations = 500_000, + memory = null, + parallelism = null, + ), + masterKeyWrappedUserKey = ENCRYPTED_USER_KEY, + salt = SALT, ) - } returns DeleteAccountResponseJson.Success.asSuccess() - - val result = repository.deleteAccountWithMasterPassword(masterPassword = masterPassword) - - assertEquals(DeleteAccountResult.Success, result) - verify(exactly = 1) { - userStateManager.hasPendingAccountDeletion = true - } - coVerify { - authSdkSource.hashPassword(EMAIL, masterPassword, kdf, HashPurpose.SERVER_AUTHORIZATION) - accountsService.deleteAccount( - masterPasswordHash = hashedMasterPassword, - oneTimePassword = null, + fakeAuthDiskSource.userState = SINGLE_USER_STATE_1.copy( + accounts = mapOf( + USER_ID_1 to ACCOUNT_1.copy( + profile = ACCOUNT_1.profile.copy( + userDecryptionOptions = UserDecryptionOptionsJson( + hasMasterPassword = true, + trustedDeviceUserDecryptionOptions = null, + keyConnectorUserDecryptionOptions = null, + masterPasswordUnlock = masterPasswordUnlock, + ), + ), + ), + ), ) + val kdf = Kdf.Pbkdf2(iterations = 500000u) + coEvery { + authSdkSource.hashPassword( + salt = SALT, + password = masterPassword, + kdf = kdf, + purpose = HashPurpose.SERVER_AUTHORIZATION, + ) + } returns hashedMasterPassword.asSuccess() + coEvery { + accountsService.deleteAccount( + masterPasswordHash = hashedMasterPassword, + oneTimePassword = null, + ) + } returns DeleteAccountResponseJson.Success.asSuccess() + + val result = repository.deleteAccountWithMasterPassword(masterPassword = masterPassword) + + assertEquals(DeleteAccountResult.Success, result) + verify(exactly = 1) { + userStateManager.hasPendingAccountDeletion = true + } + coVerify(exactly = 1) { + authSdkSource.hashPassword( + salt = SALT, + password = masterPassword, + kdf = kdf, + purpose = HashPurpose.SERVER_AUTHORIZATION, + ) + accountsService.deleteAccount( + masterPasswordHash = hashedMasterPassword, + oneTimePassword = null, + ) + } } - } @Test fun `deleteAccountWithOneTimePassword succeeds`() = runTest { @@ -1848,10 +1908,6 @@ class AuthRepositoryTest { userId = USER_ID_1, accountCryptographicState = ACCOUNT_CRYPTOGRAPHIC_STATE_V2, ) - fakeAuthDiskSource.assertMasterPasswordHash( - userId = USER_ID_1, - passwordHash = PASSWORD_HASH, - ) coVerify { identityService.getToken( email = EMAIL, @@ -2167,10 +2223,6 @@ class AuthRepositoryTest { userId = USER_ID_1, accountCryptographicState = accountCryptographicState, ) - fakeAuthDiskSource.assertMasterPasswordHash( - userId = USER_ID_1, - passwordHash = PASSWORD_HASH, - ) coVerify { identityService.getToken( email = EMAIL, @@ -2342,10 +2394,6 @@ class AuthRepositoryTest { ) assertEquals(LoginResult.Success, result) coVerify { identityService.preLogin(email = EMAIL) } - fakeAuthDiskSource.assertMasterPasswordHash( - userId = USER_ID_1, - passwordHash = PASSWORD_HASH, - ) coVerify { identityService.getToken( email = EMAIL, @@ -7097,10 +7145,6 @@ class AuthRepositoryTest { userId = USER_ID_1, accountCryptographicState = ACCOUNT_CRYPTOGRAPHIC_STATE_V2, ) - fakeAuthDiskSource.assertMasterPasswordHash( - userId = USER_ID_1, - passwordHash = PASSWORD_HASH, - ) // This should only be set after they complete a registration and not based on login. assertNull(fakeAuthDiskSource.getOnboardingStatus(USER_ID_1)) verify(exactly = 1) { @@ -7155,10 +7199,6 @@ class AuthRepositoryTest { userId = USER_ID_1, accountCryptographicState = ACCOUNT_CRYPTOGRAPHIC_STATE_V2, ) - fakeAuthDiskSource.assertMasterPasswordHash( - userId = USER_ID_1, - passwordHash = PASSWORD_HASH, - ) verify(exactly = 1) { userStateManager.hasPendingAccountAddition = false settingsRepository.setDefaultsIfNecessary(userId = USER_ID_1) diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerTest.kt index 87cdc371af..9534c32094 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/VaultLockManagerTest.kt @@ -82,9 +82,9 @@ class VaultLockManagerTest { private val authSdkSource: AuthSdkSource = mockk { coEvery { hashPassword( - salt = MOCK_PROFILE.email, + salt = MOCK_MASTER_PASSWORD_UNLOCK_DATA.salt, password = "mockValue", - kdf = MOCK_PROFILE.toSdkParams(), + kdf = MOCK_MASTER_PASSWORD_UNLOCK_DATA.kdf, purpose = HashPurpose.LOCAL_AUTHORIZATION, ) } returns "hashedPassword".asSuccess() @@ -1848,6 +1848,12 @@ class VaultLockManagerTest { ) trustedDeviceManager.trustThisDeviceIfNecessary(userId = USER_ID) kdfManager.updateKdfToMinimumsIfNeeded(password = masterPassword) + authSdkSource.hashPassword( + salt = MOCK_MASTER_PASSWORD_UNLOCK_DATA.salt, + password = masterPassword, + kdf = MOCK_MASTER_PASSWORD_UNLOCK_DATA.kdf, + purpose = HashPurpose.LOCAL_AUTHORIZATION, + ) } } @@ -2088,12 +2094,21 @@ class VaultLockManagerTest { assertEquals(VaultUnlockResult.Success, result) fakeAuthDiskSource.assertUserState(userState = MOCK_USER_STATE) + fakeAuthDiskSource.assertMasterPasswordHash(userId = USER_ID, passwordHash = null) verify(exactly = 0) { passwordPolicyManager.validatePasswordAgainstPolicies( password = any(), isCreation = any(), ) } + coVerify(exactly = 0) { + authSdkSource.hashPassword( + salt = any(), + password = any(), + kdf = any(), + purpose = any(), + ) + } } @Suppress("MaxLineLength")