[PM-20466] Invalid master password returns generic error. (#5100)

This commit is contained in:
André Bispo
2025-05-01 14:37:59 +00:00
committed by GitHub
parent 7436122953
commit 0353f0c153
13 changed files with 405 additions and 41 deletions
@@ -4,6 +4,7 @@ import com.bitwarden.core.KeyConnectorResponse
import com.bitwarden.crypto.Kdf
import com.bitwarden.network.model.KdfTypeJson
import com.bitwarden.network.model.KeyConnectorMasterKeyResponseJson
import com.x8bit.bitwarden.data.auth.manager.model.MigrateExistingUserToKeyConnectorResult
/**
* Manager used to interface with a key connector.
@@ -28,7 +29,7 @@ interface KeyConnectorManager {
email: String,
masterPassword: String,
kdf: Kdf,
): Result<Unit>
): Result<MigrateExistingUserToKeyConnectorResult>
/**
* Migrates a new user to use the key connector.
@@ -8,7 +8,9 @@ import com.bitwarden.network.model.KeyConnectorKeyRequestJson
import com.bitwarden.network.model.KeyConnectorMasterKeyResponseJson
import com.bitwarden.network.service.AccountsService
import com.x8bit.bitwarden.data.auth.datasource.sdk.AuthSdkSource
import com.x8bit.bitwarden.data.auth.manager.model.MigrateExistingUserToKeyConnectorResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.DeriveKeyConnectorResult
/**
* The default implementation of the [KeyConnectorManager].
@@ -34,7 +36,7 @@ class KeyConnectorManagerImpl(
email: String,
masterPassword: String,
kdf: Kdf,
): Result<Unit> =
): Result<MigrateExistingUserToKeyConnectorResult> =
vaultSdkSource
.deriveKeyConnector(
userId = userId,
@@ -43,10 +45,36 @@ class KeyConnectorManagerImpl(
password = masterPassword,
kdf = kdf,
)
.flatMap { masterKey ->
accountsService.storeMasterKeyToKeyConnector(url = url, masterKey = masterKey)
.map { result: DeriveKeyConnectorResult ->
when (result) {
is DeriveKeyConnectorResult.Error -> {
MigrateExistingUserToKeyConnectorResult.Error(result.error)
}
is DeriveKeyConnectorResult.Success -> {
accountsService
.storeMasterKeyToKeyConnector(
url = url,
masterKey = result.derivedKey,
)
.flatMap {
accountsService.convertToKeyConnector()
}
.fold(
onSuccess = {
MigrateExistingUserToKeyConnectorResult.Success
},
onFailure = {
MigrateExistingUserToKeyConnectorResult.Error(it)
},
)
}
is DeriveKeyConnectorResult.WrongPasswordError -> {
MigrateExistingUserToKeyConnectorResult.WrongPasswordError
}
}
}
.flatMap { accountsService.convertToKeyConnector() }
override suspend fun migrateNewUserToKeyConnector(
url: String,
@@ -0,0 +1,23 @@
package com.x8bit.bitwarden.data.auth.manager.model
/**
* Models result of migrating existing user to key connector.
* */
sealed class MigrateExistingUserToKeyConnectorResult {
/**
* Operation succeeded.
*/
data object Success : MigrateExistingUserToKeyConnectorResult()
/**
* There was an error.
*/
data class Error(
val error: Throwable,
) : MigrateExistingUserToKeyConnectorResult()
/**
* Incorrect password provided.
*/
data object WrongPasswordError : MigrateExistingUserToKeyConnectorResult()
}
@@ -54,6 +54,7 @@ import com.x8bit.bitwarden.data.auth.manager.AuthRequestManager
import com.x8bit.bitwarden.data.auth.manager.KeyConnectorManager
import com.x8bit.bitwarden.data.auth.manager.TrustedDeviceManager
import com.x8bit.bitwarden.data.auth.manager.UserLogoutManager
import com.x8bit.bitwarden.data.auth.manager.model.MigrateExistingUserToKeyConnectorResult
import com.x8bit.bitwarden.data.auth.repository.model.AuthState
import com.x8bit.bitwarden.data.auth.repository.model.BreachCountResult
import com.x8bit.bitwarden.data.auth.repository.model.DeleteAccountResult
@@ -356,7 +357,7 @@ class AuthRepositoryImpl(
get() = activeUserId?.let { authDiskSource.getIsTdeLoginComplete(userId = it) }
override var shouldTrustDevice: Boolean
get() = activeUserId?.let { authDiskSource.getShouldTrustDevice(userId = it) } ?: false
get() = activeUserId?.let { authDiskSource.getShouldTrustDevice(userId = it) } == true
set(value) {
activeUserId?.let {
authDiskSource.storeShouldTrustDevice(userId = it, shouldTrustDevice = value)
@@ -985,17 +986,29 @@ class AuthRepositoryImpl(
masterPassword = masterPassword,
kdf = profile.toSdkParams(),
)
.onSuccess {
authDiskSource.userState = authDiskSource
.userState
?.toRemovedPasswordUserStateJson(userId = userId)
vaultRepository.sync()
settingsRepository.setDefaultsIfNecessary(userId = userId)
.map { migrateResult: MigrateExistingUserToKeyConnectorResult ->
when (migrateResult) {
is MigrateExistingUserToKeyConnectorResult.Error -> {
RemovePasswordResult.Error(error = migrateResult.error)
}
MigrateExistingUserToKeyConnectorResult.Success -> {
authDiskSource.userState = authDiskSource
.userState
?.toRemovedPasswordUserStateJson(userId = userId)
vaultRepository.sync()
settingsRepository.setDefaultsIfNecessary(userId = userId)
RemovePasswordResult.Success
}
MigrateExistingUserToKeyConnectorResult.WrongPasswordError -> {
RemovePasswordResult.WrongPasswordError
}
}
}
.getOrElse {
RemovePasswordResult.Error(error = it)
}
.fold(
onFailure = { RemovePasswordResult.Error(error = it) },
onSuccess = { RemovePasswordResult.Success },
)
}
override suspend fun resetPassword(
@@ -15,4 +15,9 @@ sealed class RemovePasswordResult {
data class Error(
val error: Throwable,
) : RemovePasswordResult()
/**
* There was wrong password error removing the password.
*/
data object WrongPasswordError : RemovePasswordResult()
}
@@ -28,6 +28,7 @@ import com.bitwarden.vault.PasswordHistory
import com.bitwarden.vault.PasswordHistoryView
import com.bitwarden.vault.TotpResponse
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.AuthenticateFido2CredentialRequest
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.DeriveKeyConnectorResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.InitializeCryptoResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.RegisterFido2CredentialRequest
import java.io.File
@@ -66,7 +67,7 @@ interface VaultSdkSource {
email: String,
password: String,
kdf: Kdf,
): Result<String>
): Result<DeriveKeyConnectorResult>
/**
* Derives a "pin key" from the given [pin] for the given [userId]. This can be used to later
@@ -33,6 +33,7 @@ import com.bitwarden.vault.TotpResponse
import com.x8bit.bitwarden.data.platform.datasource.sdk.BaseSdkSource
import com.x8bit.bitwarden.data.platform.manager.SdkClientManager
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.AuthenticateFido2CredentialRequest
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.DeriveKeyConnectorResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.Fido2CredentialAuthenticationUserInterfaceImpl
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.Fido2CredentialRegistrationUserInterfaceImpl
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.Fido2CredentialSearchUserInterfaceImpl
@@ -75,18 +76,28 @@ class VaultSdkSourceImpl(
email: String,
password: String,
kdf: Kdf,
): Result<String> =
): Result<DeriveKeyConnectorResult> =
runCatchingWithLogs {
getClient(userId = userId)
.crypto()
.deriveKeyConnector(
request = DeriveKeyConnectorRequest(
userKeyEncrypted = userKeyEncrypted,
password = password,
kdf = kdf,
email = email,
),
)
try {
val key = getClient(userId = userId)
.crypto()
.deriveKeyConnector(
request = DeriveKeyConnectorRequest(
userKeyEncrypted = userKeyEncrypted,
password = password,
kdf = kdf,
email = email,
),
)
DeriveKeyConnectorResult.Success(key)
} catch (exception: BitwardenException) {
when {
exception.message == "Wrong password" -> {
DeriveKeyConnectorResult.WrongPasswordError
}
else -> DeriveKeyConnectorResult.Error(exception)
}
}
}
override suspend fun derivePinKey(
@@ -0,0 +1,26 @@
package com.x8bit.bitwarden.data.vault.datasource.sdk.model
/**
* Models result of derive key connector for the Bitwarden SDK.
*/
sealed class DeriveKeyConnectorResult {
/**
* Successful derive key operation.
*/
data class Success(
val derivedKey: String,
) : DeriveKeyConnectorResult()
/**
* Generic error.
*/
data class Error(
val error: Throwable,
) : DeriveKeyConnectorResult()
/**
* Incorrect password provided.
*/
data object WrongPasswordError : DeriveKeyConnectorResult()
}
@@ -127,6 +127,17 @@ class RemovePasswordViewModel @Inject constructor(
}
}
is RemovePasswordResult.WrongPasswordError -> {
mutableStateFlow.update {
it.copy(
dialogState = RemovePasswordState.DialogState.Error(
title = R.string.an_error_has_occurred.asText(),
message = R.string.invalid_master_password.asText(),
),
)
}
}
RemovePasswordResult.Success -> {
mutableStateFlow.update { it.copy(dialogState = null) }
// We do nothing here because state-based navigation will handle it.
@@ -10,7 +10,9 @@ import com.bitwarden.network.model.KeyConnectorKeyRequestJson
import com.bitwarden.network.model.KeyConnectorMasterKeyResponseJson
import com.bitwarden.network.service.AccountsService
import com.x8bit.bitwarden.data.auth.datasource.sdk.AuthSdkSource
import com.x8bit.bitwarden.data.auth.manager.model.MigrateExistingUserToKeyConnectorResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.DeriveKeyConnectorResult
import io.mockk.coEvery
import io.mockk.every
import io.mockk.mockk
@@ -90,7 +92,7 @@ class KeyConnectorManagerTest {
@Test
fun `migrateExistingUserToKeyConnector with storeMasterKeyToKeyConnector failure should return failure`() =
runTest {
val expectedResult = Throwable("Fail").asFailure()
val expectedResult = Throwable("Fail")
coEvery {
vaultSdkSource.deriveKeyConnector(
userId = USER_ID,
@@ -99,10 +101,10 @@ class KeyConnectorManagerTest {
password = MASTER_PASSWORD,
kdf = KDF,
)
} returns MASTER_KEY.asSuccess()
} returns DeriveKeyConnectorResult.Success(MASTER_KEY).asSuccess()
coEvery {
accountsService.storeMasterKeyToKeyConnector(url = URL, masterKey = MASTER_KEY)
} returns expectedResult
} returns expectedResult.asFailure()
val result = keyConnectorManager.migrateExistingUserToKeyConnector(
userId = USER_ID,
@@ -113,14 +115,46 @@ class KeyConnectorManagerTest {
kdf = KDF,
)
assertEquals(expectedResult, result)
assertEquals(
MigrateExistingUserToKeyConnectorResult.Error(error = expectedResult),
result.getOrNull(),
)
}
@Suppress("MaxLineLength")
@Test
fun `migrateExistingUserToKeyConnector with wrong password error should return WrongPasswordError`() =
runTest {
coEvery {
vaultSdkSource.deriveKeyConnector(
userId = USER_ID,
userKeyEncrypted = ENCRYPTED_USER_KEY,
email = EMAIL,
password = MASTER_PASSWORD,
kdf = KDF,
)
} returns DeriveKeyConnectorResult.WrongPasswordError.asSuccess()
val result = keyConnectorManager.migrateExistingUserToKeyConnector(
userId = USER_ID,
url = URL,
userKeyEncrypted = ENCRYPTED_USER_KEY,
email = EMAIL,
masterPassword = MASTER_PASSWORD,
kdf = KDF,
)
assertEquals(
MigrateExistingUserToKeyConnectorResult.WrongPasswordError,
result.getOrNull(),
)
}
@Suppress("MaxLineLength")
@Test
fun `migrateExistingUserToKeyConnector with convertToKeyConnector failure should return failure`() =
runTest {
val expectedResult = Throwable("Fail").asFailure()
val expectedResult = Throwable("Fail")
coEvery {
vaultSdkSource.deriveKeyConnector(
userId = USER_ID,
@@ -129,11 +163,11 @@ class KeyConnectorManagerTest {
password = MASTER_PASSWORD,
kdf = KDF,
)
} returns MASTER_KEY.asSuccess()
} returns DeriveKeyConnectorResult.Success(MASTER_KEY).asSuccess()
coEvery {
accountsService.storeMasterKeyToKeyConnector(url = URL, masterKey = MASTER_KEY)
} returns Unit.asSuccess()
coEvery { accountsService.convertToKeyConnector() } returns expectedResult
coEvery { accountsService.convertToKeyConnector() } returns expectedResult.asFailure()
val result = keyConnectorManager.migrateExistingUserToKeyConnector(
userId = USER_ID,
@@ -144,7 +178,10 @@ class KeyConnectorManagerTest {
kdf = KDF,
)
assertEquals(expectedResult, result)
assertEquals(
MigrateExistingUserToKeyConnectorResult.Error(error = expectedResult),
result.getOrNull(),
)
}
@Test
@@ -157,7 +194,7 @@ class KeyConnectorManagerTest {
password = MASTER_PASSWORD,
kdf = KDF,
)
} returns MASTER_KEY.asSuccess()
} returns DeriveKeyConnectorResult.Success(MASTER_KEY).asSuccess()
coEvery {
accountsService.storeMasterKeyToKeyConnector(url = URL, masterKey = MASTER_KEY)
} returns Unit.asSuccess()
@@ -172,7 +209,7 @@ class KeyConnectorManagerTest {
kdf = KDF,
)
assertEquals(Unit.asSuccess(), result)
assertEquals(MigrateExistingUserToKeyConnectorResult.Success, result.getOrNull())
}
@Test
@@ -77,6 +77,7 @@ import com.x8bit.bitwarden.data.auth.manager.KeyConnectorManager
import com.x8bit.bitwarden.data.auth.manager.TrustedDeviceManager
import com.x8bit.bitwarden.data.auth.manager.UserLogoutManager
import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest
import com.x8bit.bitwarden.data.auth.manager.model.MigrateExistingUserToKeyConnectorResult
import com.x8bit.bitwarden.data.auth.repository.model.AuthState
import com.x8bit.bitwarden.data.auth.repository.model.BreachCountResult
import com.x8bit.bitwarden.data.auth.repository.model.DeleteAccountResult
@@ -4692,7 +4693,7 @@ class AuthRepositoryTest {
}
@Test
fun `removePassword with migrateExistingUserToKeyConnector error should return error`() =
fun `removePassword with migrateExistingUserToKeyConnector exception should return error`() =
runTest {
fakeAuthDiskSource.userState = SINGLE_USER_STATE_1
fakeAuthDiskSource.storeUserKey(userId = USER_ID_1, userKey = ENCRYPTED_USER_KEY)
@@ -4727,6 +4728,86 @@ class AuthRepositoryTest {
assertEquals(RemovePasswordResult.Error(error = error), result)
}
@Test
fun `removePassword with migrateExistingUserToKeyConnector error should return error`() =
runTest {
fakeAuthDiskSource.userState = SINGLE_USER_STATE_1
fakeAuthDiskSource.storeUserKey(userId = USER_ID_1, userKey = ENCRYPTED_USER_KEY)
val url = "www.example.com"
val error = Throwable("Fail!")
val expectedResult = MigrateExistingUserToKeyConnectorResult.Error(error)
val organizations = listOf(
mockk<SyncResponseJson.Profile.Organization> {
every { id } returns "orgId"
every { name } returns "orgName"
every { permissions } returns mockk {
every { shouldManageResetPassword } returns false
}
every { shouldUseKeyConnector } returns true
every { type } returns OrganizationType.USER
every { keyConnectorUrl } returns url
},
)
fakeAuthDiskSource.storeOrganizations(userId = USER_ID_1, organizations = organizations)
coEvery {
keyConnectorManager.migrateExistingUserToKeyConnector(
userId = USER_ID_1,
url = url,
userKeyEncrypted = ENCRYPTED_USER_KEY,
email = PROFILE_1.email,
masterPassword = PASSWORD,
kdf = PROFILE_1.toSdkParams(),
)
} returns expectedResult.asSuccess()
val result = repository.removePassword(masterPassword = PASSWORD)
assertEquals(
RemovePasswordResult.Error(error = error),
result,
)
}
@Test
@Suppress("MaxLineLength")
fun `removePassword with migrateExistingUserToKeyConnector wrong password error should return WrongPasswordError error`() =
runTest {
fakeAuthDiskSource.userState = SINGLE_USER_STATE_1
fakeAuthDiskSource.storeUserKey(userId = USER_ID_1, userKey = ENCRYPTED_USER_KEY)
val url = "www.example.com"
val expectedResult = MigrateExistingUserToKeyConnectorResult.WrongPasswordError
val organizations = listOf(
mockk<SyncResponseJson.Profile.Organization> {
every { id } returns "orgId"
every { name } returns "orgName"
every { permissions } returns mockk {
every { shouldManageResetPassword } returns false
}
every { shouldUseKeyConnector } returns true
every { type } returns OrganizationType.USER
every { keyConnectorUrl } returns url
},
)
fakeAuthDiskSource.storeOrganizations(userId = USER_ID_1, organizations = organizations)
coEvery {
keyConnectorManager.migrateExistingUserToKeyConnector(
userId = USER_ID_1,
url = url,
userKeyEncrypted = ENCRYPTED_USER_KEY,
email = PROFILE_1.email,
masterPassword = PASSWORD,
kdf = PROFILE_1.toSdkParams(),
)
} returns expectedResult.asSuccess()
val result = repository.removePassword(masterPassword = PASSWORD)
assertEquals(
RemovePasswordResult.WrongPasswordError,
result,
)
}
@Suppress("MaxLineLength")
@Test
fun `removePassword with migrateExistingUserToKeyConnector success should sync and return success`() =
@@ -4756,7 +4837,7 @@ class AuthRepositoryTest {
masterPassword = PASSWORD,
kdf = PROFILE_1.toSdkParams(),
)
} returns Unit.asSuccess()
} returns MigrateExistingUserToKeyConnectorResult.Success.asSuccess()
every {
SINGLE_USER_STATE_1.toRemovedPasswordUserStateJson(userId = USER_ID_1)
} returns SINGLE_USER_STATE_1
@@ -48,6 +48,7 @@ import com.bitwarden.vault.PasswordHistoryView
import com.bitwarden.vault.TotpResponse
import com.x8bit.bitwarden.data.platform.manager.SdkClientManager
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.AuthenticateFido2CredentialRequest
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.DeriveKeyConnectorResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.Fido2CredentialSearchUserInterfaceImpl
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.InitializeCryptoResult
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.RegisterFido2CredentialRequest
@@ -175,7 +176,100 @@ class VaultSdkSourceTest {
password = password,
kdf = kdf,
)
assertEquals(expectedResult.asSuccess(), result)
assertEquals(
DeriveKeyConnectorResult.Success(derivedKey = expectedResult),
result.getOrNull(),
)
coVerify(exactly = 1) {
sdkClientManager.getOrCreateClient(userId = userId)
clientCrypto.deriveKeyConnector(
request = DeriveKeyConnectorRequest(
userKeyEncrypted = userKeyEncrypted,
email = email,
password = password,
kdf = kdf,
),
)
}
}
@Test
fun `deriveKeyConnector should call SDK and return a Result with wrong password`() =
runBlocking {
val userId = "userId"
val userKeyEncrypted = "userKeyEncrypted"
val email = "email"
val password = "password"
val error = mockk<BitwardenException> {
every { message } returns "Wrong password"
}
val kdf = mockk<Kdf>()
coEvery {
clientCrypto.deriveKeyConnector(
request = DeriveKeyConnectorRequest(
userKeyEncrypted = userKeyEncrypted,
email = email,
password = password,
kdf = kdf,
),
)
} throws error
val result = vaultSdkSource.deriveKeyConnector(
userId = userId,
userKeyEncrypted = userKeyEncrypted,
email = email,
password = password,
kdf = kdf,
)
assertEquals(
DeriveKeyConnectorResult.WrongPasswordError,
result.getOrNull(),
)
coVerify(exactly = 1) {
sdkClientManager.getOrCreateClient(userId = userId)
clientCrypto.deriveKeyConnector(
request = DeriveKeyConnectorRequest(
userKeyEncrypted = userKeyEncrypted,
email = email,
password = password,
kdf = kdf,
),
)
}
}
@Test
fun `deriveKeyConnector should call SDK and return a Result with error`() =
runBlocking {
val userId = "userId"
val userKeyEncrypted = "userKeyEncrypted"
val email = "email"
val password = "password"
val error = mockk<BitwardenException> {
every { message } returns "Other error"
}
val kdf = mockk<Kdf>()
coEvery {
clientCrypto.deriveKeyConnector(
request = DeriveKeyConnectorRequest(
userKeyEncrypted = userKeyEncrypted,
email = email,
password = password,
kdf = kdf,
),
)
} throws error
val result = vaultSdkSource.deriveKeyConnector(
userId = userId,
userKeyEncrypted = userKeyEncrypted,
email = email,
password = password,
kdf = kdf,
)
assertEquals(
DeriveKeyConnectorResult.Error(error = error),
result.getOrNull(),
)
coVerify(exactly = 1) {
sdkClientManager.getOrCreateClient(userId = userId)
clientCrypto.deriveKeyConnector(
@@ -82,6 +82,39 @@ class RemovePasswordViewModelTest : BaseViewModelTest() {
}
}
@Test
@Suppress("MaxLineLength")
fun `ContinueClick with input and remove password wrong password error with should show error dialog with message`() = runTest {
val password = "123"
val initialState = DEFAULT_STATE.copy(input = password)
val viewModel = createViewModel(state = initialState)
coEvery {
authRepository.removePassword(masterPassword = password)
} returns RemovePasswordResult.WrongPasswordError
viewModel.stateFlow.test {
assertEquals(initialState, awaitItem())
viewModel.trySendAction(RemovePasswordAction.ContinueClick)
assertEquals(
initialState.copy(
dialogState = RemovePasswordState.DialogState.Loading(
title = R.string.deleting.asText(),
),
),
awaitItem(),
)
assertEquals(
initialState.copy(
dialogState = RemovePasswordState.DialogState.Error(
title = R.string.an_error_has_occurred.asText(),
message = R.string.invalid_master_password.asText(),
),
),
awaitItem(),
)
}
}
@Test
fun `ContinueClick with input and remove password success should dismiss dialog`() = runTest {
val password = "123"