Adjust `PerformCreate[ExternalEmail]User`:

- those use cases can no longer be called multiple times: calling them twice will likely result in "Account already exists" error
- the error recovery (when receiving "Account already exists" error) is in `SignupViewModel` (`catchWhen`)
This commit is contained in:
Mateusz Armatys 2021-11-15 18:55:42 +01:00
parent e904e6d786
commit f3f2efa010
8 changed files with 165 additions and 131 deletions

View File

@ -19,7 +19,6 @@
package me.proton.core.auth.domain.usecase.signup
import me.proton.core.auth.domain.repository.AuthRepository
import me.proton.core.auth.domain.usecase.PerformLogin
import me.proton.core.crypto.common.keystore.EncryptedString
import me.proton.core.crypto.common.keystore.KeyStoreCrypto
import me.proton.core.crypto.common.keystore.decrypt
@ -34,8 +33,7 @@ class PerformCreateExternalEmailUser @Inject constructor(
private val authRepository: AuthRepository,
private val userRepository: UserRepository,
private val srpCrypto: SrpCrypto,
private val keyStoreCrypto: KeyStoreCrypto,
private val performLogin: PerformLogin
private val keyStoreCrypto: KeyStoreCrypto
) {
suspend operator fun invoke(
@ -45,10 +43,6 @@ class PerformCreateExternalEmailUser @Inject constructor(
): UserId {
require(email.isNotBlank()) { "Email must not be empty." }
if (!userRepository.isUsernameAvailable(email)) {
return performLogin.invoke(email, password).userId
}
val modulus = authRepository.randomModulus()
password.decrypt(keyStoreCrypto).toByteArray().use { decryptedPassword ->

View File

@ -19,7 +19,6 @@
package me.proton.core.auth.domain.usecase.signup
import me.proton.core.auth.domain.repository.AuthRepository
import me.proton.core.auth.domain.usecase.PerformLogin
import me.proton.core.crypto.common.keystore.EncryptedString
import me.proton.core.crypto.common.keystore.KeyStoreCrypto
import me.proton.core.crypto.common.keystore.decrypt
@ -34,8 +33,7 @@ class PerformCreateUser @Inject constructor(
private val authRepository: AuthRepository,
private val userRepository: UserRepository,
private val srpCrypto: SrpCrypto,
private val keyStoreCrypto: KeyStoreCrypto,
private val performLogin: PerformLogin
private val keyStoreCrypto: KeyStoreCrypto
) {
suspend operator fun invoke(
@ -52,10 +50,6 @@ class PerformCreateUser @Inject constructor(
recoveryEmail != null && recoveryPhone == null
) { "Recovery Email and Phone could not be set together" }
if (!userRepository.isUsernameAvailable(username)) {
return performLogin.invoke(username, password).userId
}
val modulus = authRepository.randomModulus()
password.decrypt(keyStoreCrypto).toByteArray().use { decryptedPassword ->

View File

@ -27,12 +27,10 @@ import io.mockk.verify
import kotlinx.coroutines.test.runBlockingTest
import me.proton.core.auth.domain.entity.Modulus
import me.proton.core.auth.domain.repository.AuthRepository
import me.proton.core.auth.domain.usecase.PerformLogin
import me.proton.core.crypto.common.keystore.EncryptedString
import me.proton.core.crypto.common.keystore.KeyStoreCrypto
import me.proton.core.crypto.common.srp.Auth
import me.proton.core.crypto.common.srp.SrpCrypto
import me.proton.core.domain.entity.UserId
import me.proton.core.network.domain.ApiException
import me.proton.core.user.domain.entity.CreateUserType
import me.proton.core.user.domain.repository.UserRepository
@ -49,7 +47,6 @@ class PerformCreateExternalEmailUserTest {
private val userRepository = mockk<UserRepository>(relaxed = true)
private val srpCrypto = mockk<SrpCrypto>(relaxed = true)
private val keyStoreCrypto = mockk<KeyStoreCrypto>(relaxed = true)
private val performLogin = mockk<PerformLogin>()
// endregion
@ -76,8 +73,7 @@ class PerformCreateExternalEmailUserTest {
authRepository,
userRepository,
srpCrypto,
keyStoreCrypto,
performLogin
keyStoreCrypto
)
every {
srpCrypto.calculatePasswordVerifier(testEmail, any(), any(), any())
@ -94,8 +90,6 @@ class PerformCreateExternalEmailUserTest {
@Test
fun `create external user success`() = runBlockingTest {
coEvery { userRepository.isUsernameAvailable(eq(testEmail)) } returns true
useCase.invoke(
testEmail,
keyStoreCrypto.encrypt(testPassword),
@ -145,26 +139,11 @@ class PerformCreateExternalEmailUserTest {
}
@Test
fun `user already exists but can log in`() = runBlockingTest {
val testUserId = UserId("user-id")
coEvery { userRepository.isUsernameAvailable(testEmail) } returns false
coEvery { performLogin.invoke(testEmail, testEncryptedPassword) } returns mockk {
every { userId } returns testUserId
}
val createdUserId = useCase.invoke(
testEmail,
keyStoreCrypto.encrypt(testPassword),
referrer = null
)
assertEquals(testUserId, createdUserId)
}
@Test
fun `user already exists and cannot log in`() = runBlockingTest {
fun `user already exists`() = runBlockingTest {
val apiException = mockk<ApiException>()
coEvery { userRepository.isUsernameAvailable(testEmail) } returns false
coEvery { performLogin.invoke(testEmail, testEncryptedPassword) } throws apiException
coEvery {
userRepository.createExternalEmailUser(any(), any(), any(), any(), any())
} throws apiException
val result = assertFailsWith(ApiException::class) {
useCase.invoke(

View File

@ -27,16 +27,13 @@ import io.mockk.verify
import kotlinx.coroutines.test.runBlockingTest
import me.proton.core.auth.domain.entity.Modulus
import me.proton.core.auth.domain.repository.AuthRepository
import me.proton.core.auth.domain.usecase.PerformLogin
import me.proton.core.crypto.common.keystore.EncryptedString
import me.proton.core.crypto.common.keystore.KeyStoreCrypto
import me.proton.core.crypto.common.srp.Auth
import me.proton.core.crypto.common.srp.SrpCrypto
import me.proton.core.domain.entity.UserId
import me.proton.core.network.domain.ApiException
import me.proton.core.user.domain.entity.CreateUserType
import me.proton.core.user.domain.repository.UserRepository
import org.junit.Assert
import org.junit.Before
import org.junit.Test
import kotlin.test.assertEquals
@ -50,7 +47,6 @@ class PerformCreateUserTest {
private val userRepository = mockk<UserRepository>(relaxed = true)
private val srpCrypto = mockk<SrpCrypto>(relaxed = true)
private val keyStoreCrypto = mockk<KeyStoreCrypto>(relaxed = true)
private val performLogin = mockk<PerformLogin>()
// endregion
@ -75,7 +71,7 @@ class PerformCreateUserTest {
@Before
fun beforeEveryTest() {
// GIVEN
useCase = PerformCreateUser(authRepository, userRepository, srpCrypto, keyStoreCrypto, performLogin)
useCase = PerformCreateUser(authRepository, userRepository, srpCrypto, keyStoreCrypto)
every {
srpCrypto.calculatePasswordVerifier(testUsername, any(), any(), any())
} returns testAuth
@ -90,8 +86,6 @@ class PerformCreateUserTest {
@Test
fun `create user no recovery success`() = runBlockingTest {
coEvery { userRepository.isUsernameAvailable(testUsername) } returns true
useCase.invoke(
testUsername,
keyStoreCrypto.encrypt(testPassword),
@ -132,8 +126,6 @@ class PerformCreateUserTest {
@Test
fun `create user email recovery success`() = runBlockingTest {
coEvery { userRepository.isUsernameAvailable(testUsername) } returns true
useCase.invoke(
testUsername,
keyStoreCrypto.encrypt(testPassword),
@ -175,8 +167,6 @@ class PerformCreateUserTest {
@Test
fun `create user phone recovery success`() = runBlockingTest {
coEvery { userRepository.isUsernameAvailable(testUsername) } returns true
useCase.invoke(
testUsername,
keyStoreCrypto.encrypt(testPassword),
@ -235,30 +225,13 @@ class PerformCreateUserTest {
)
}
@Test
fun `user already exists but can log in`() = runBlockingTest {
val testUserId = UserId("user-id")
coEvery { userRepository.isUsernameAvailable(testEmail) } returns false
coEvery { performLogin.invoke(testEmail, testEncryptedPassword) } returns mockk {
every { userId } returns testUserId
}
val createdUserId = useCase.invoke(
testEmail,
keyStoreCrypto.encrypt(testPassword),
recoveryEmail = null,
recoveryPhone = null,
referrer = null,
type = CreateUserType.Normal
)
Assert.assertEquals(testUserId, createdUserId)
}
@Test
fun `user already exists and cannot log in`() = runBlockingTest {
val apiException = mockk<ApiException>()
coEvery { userRepository.isUsernameAvailable(testEmail) } returns false
coEvery { performLogin.invoke(testEmail, testEncryptedPassword) } throws apiException
coEvery {
userRepository.createUser(any(), any(), any(), any(), any(), any(), any())
} throws apiException
val result = assertFailsWith(ApiException::class) {
useCase.invoke(

View File

@ -33,10 +33,10 @@ import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
import me.proton.core.account.domain.entity.AccountType
import me.proton.core.auth.domain.usecase.PerformLogin
import me.proton.core.auth.domain.usecase.signup.PerformCreateExternalEmailUser
import me.proton.core.auth.domain.usecase.signup.PerformCreateUser
import me.proton.core.auth.domain.usecase.userAlreadyExists
import me.proton.core.auth.presentation.LogTag
import me.proton.core.auth.presentation.entity.signup.RecoveryMethod
import me.proton.core.auth.presentation.entity.signup.RecoveryMethodType
import me.proton.core.auth.presentation.entity.signup.SubscriptionDetails
@ -55,9 +55,8 @@ import me.proton.core.plan.presentation.PlansOrchestrator
import me.proton.core.presentation.savedstate.flowState
import me.proton.core.presentation.savedstate.state
import me.proton.core.user.domain.entity.createUserType
import me.proton.core.util.kotlin.CoreLogger
import me.proton.core.util.kotlin.catchWhen
import me.proton.core.util.kotlin.exhaustive
import me.proton.core.util.kotlin.retryOnceWhen
import javax.inject.Inject
@HiltViewModel
@ -69,6 +68,7 @@ internal class SignupViewModel @Inject constructor(
private val paymentsOrchestrator: PaymentsOrchestrator,
private val clientIdProvider: ClientIdProvider,
private val humanVerificationManager: HumanVerificationManager,
private val performLogin: PerformLogin,
humanVerificationOrchestrator: HumanVerificationOrchestrator,
savedStateHandle: SavedStateHandle
) : AuthViewModel(humanVerificationManager, humanVerificationOrchestrator) {
@ -209,53 +209,65 @@ internal class SignupViewModel @Inject constructor(
// endregion
// region private functions
private suspend fun createUser() = flow {
val username = requireNotNull(username) { "Username is not set." }
val encryptedPassword = requireNotNull(_password) { "Password is not set (initialized)." }
emit(State.Processing)
private suspend fun createUser() {
val username = username
val encryptedPassword = _password
val verification = _recoveryMethod?.let {
val email = if (it.type == RecoveryMethodType.EMAIL) {
it.destination
} else null
val phone = if (it.type == RecoveryMethodType.SMS) {
it.destination
} else null
Pair(email, phone)
} ?: run {
Pair(null, null)
}
flow {
requireNotNull(username) { "Username is not set." }
requireNotNull(encryptedPassword) { "Password is not set (initialized)." }
emit(State.Processing)
val result = performCreateUser(
username = username, password = encryptedPassword, recoveryEmail = verification.first,
recoveryPhone = verification.second, referrer = null, type = currentAccountType.createUserType()
)
emit(State.Success(result.id, username, encryptedPassword))
}.retryOnceWhen(Throwable::userAlreadyExists) {
CoreLogger.e(LogTag.FLOW_ERROR_RETRY, it, "Retrying to create a user")
}.catch { error ->
emit(State.Error.Message(error.message))
}.onEach {
_userCreationState.tryEmit(it)
}.launchIn(viewModelScope)
val verification = _recoveryMethod?.let {
val email = if (it.type == RecoveryMethodType.EMAIL) {
it.destination
} else null
val phone = if (it.type == RecoveryMethodType.SMS) {
it.destination
} else null
Pair(email, phone)
} ?: run {
Pair(null, null)
}
private suspend fun createExternalUser() = flow {
val externalEmail = requireNotNull(externalEmail) { "External email is not set." }
val encryptedPassword = requireNotNull(_password) { "Password is not set (initialized)." }
emit(State.Processing)
val userId = performCreateExternalEmailUser(
email = externalEmail,
password = encryptedPassword,
referrer = null
)
emit(State.Success(userId.id, externalEmail, encryptedPassword))
}.retryOnceWhen(Throwable::userAlreadyExists) {
CoreLogger.e(LogTag.FLOW_ERROR_RETRY, it, "Retrying to create an external user")
}.catch { error ->
emit(State.Error.Message(error.message))
}.onEach {
_userCreationState.tryEmit(it)
}.launchIn(viewModelScope)
val result = performCreateUser(
username = username, password = encryptedPassword, recoveryEmail = verification.first,
recoveryPhone = verification.second, referrer = null, type = currentAccountType.createUserType()
)
emit(State.Success(result.id, username, encryptedPassword))
}.catchWhen(Throwable::userAlreadyExists) {
val userId = performLogin.invoke(username!!, encryptedPassword!!).userId
emit(State.Success(userId.id, username, encryptedPassword))
}.catch { error ->
emit(State.Error.Message(error.message))
}.onEach {
_userCreationState.tryEmit(it)
}.launchIn(viewModelScope)
}
private suspend fun createExternalUser() {
val externalEmail = externalEmail
val encryptedPassword = _password
flow {
requireNotNull(externalEmail) { "External email is not set." }
requireNotNull(encryptedPassword) { "Password is not set (initialized)." }
emit(State.Processing)
val userId = performCreateExternalEmailUser(
email = externalEmail,
password = encryptedPassword,
referrer = null
)
emit(State.Success(userId.id, externalEmail, encryptedPassword))
}.catchWhen(Throwable::userAlreadyExists) {
val userId = performLogin.invoke(externalEmail!!, encryptedPassword!!).userId
emit(State.Success(userId.id, externalEmail, encryptedPassword))
}.catch { error ->
emit(State.Error.Message(error.message))
}.onEach {
_userCreationState.tryEmit(it)
}.launchIn(viewModelScope)
}
private fun onUserCreationStateRestored(state: State) {
if (state == State.Processing) {

View File

@ -24,6 +24,7 @@ import io.mockk.coVerify
import io.mockk.every
import io.mockk.mockk
import me.proton.core.account.domain.entity.AccountType
import me.proton.core.auth.domain.usecase.PerformLogin
import me.proton.core.auth.domain.usecase.signup.PerformCreateExternalEmailUser
import me.proton.core.auth.domain.usecase.signup.PerformCreateUser
import me.proton.core.auth.presentation.entity.signup.RecoveryMethod
@ -58,6 +59,7 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
private val plansOrchestrator = mockk<PlansOrchestrator>(relaxed = true)
private val paymentsOrchestrator = mockk<PaymentsOrchestrator>(relaxed = true)
private val clientIdProvider = mockk<ClientIdProvider>(relaxed = true)
private val performLogin = mockk<PerformLogin>()
// endregion
@ -108,6 +110,7 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
paymentsOrchestrator,
clientIdProvider,
humanVerificationManager,
performLogin,
humanVerificationOrchestrator,
mockk(relaxed = true)
)
@ -488,7 +491,7 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
}
@Test
fun `retries creating internal user`() = coroutinesTest {
fun `tries login if internal username taken`() = coroutinesTest {
coEvery {
performCreateUser.invoke(
username = testUsername,
@ -500,6 +503,10 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
)
} throws usernameTakenError
coEvery { performLogin.invoke(testUsername, any()) } returns mockk {
every { userId } returns testUser.userId
}
// GIVEN
viewModel.username = testUsername
viewModel.setPassword(testPassword)
@ -510,12 +517,11 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
// THEN
assertTrue(awaitItem() is SignupViewModel.State.Idle)
assertTrue(awaitItem() is SignupViewModel.State.Processing)
assertTrue(awaitItem() is SignupViewModel.State.Processing) // retried
val errorItem = awaitItem()
assertTrue(errorItem is SignupViewModel.State.Error.Message)
assertEquals("Username taken", errorItem.message)
val successItem = awaitItem()
assertTrue(successItem is SignupViewModel.State.Success)
assertEquals(testUser.userId.id, successItem.userId)
coVerify(exactly = 2) {
coVerify(exactly = 1) {
performCreateUser(
username = testUsername,
password = "encrypted-$testPassword",
@ -525,11 +531,18 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
type = CreateUserType.Normal
)
}
coVerify(exactly = 1) {
performLogin(
username = testUsername,
password = "encrypted-$testPassword"
)
}
}
}
@Test
fun `retries creating External user`() = coroutinesTest {
fun `tries login if External username taken`() = coroutinesTest {
coEvery {
performCreateExternalUser.invoke(
email = testEmail,
@ -538,6 +551,10 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
)
} throws usernameTakenError
coEvery { performLogin.invoke(testEmail, any()) } returns mockk {
every { userId } returns testUser.userId
}
// GIVEN
viewModel.currentAccountType = AccountType.External
viewModel.externalEmail = testEmail
@ -548,18 +565,24 @@ class SignupViewModelTest : ArchTest, CoroutinesTest {
// THEN
assertTrue(awaitItem() is SignupViewModel.State.Idle)
assertTrue(awaitItem() is SignupViewModel.State.Processing)
assertTrue(awaitItem() is SignupViewModel.State.Processing) // retried
val errorItem = awaitItem()
assertTrue(errorItem is SignupViewModel.State.Error.Message)
assertEquals("Username taken", errorItem.message)
val successItem = awaitItem()
assertTrue(successItem is SignupViewModel.State.Success)
assertEquals(testUser.userId.id, successItem.userId)
coVerify(exactly = 2) {
coVerify(exactly = 1) {
performCreateExternalUser(
email = testEmail,
password = "encrypted-$testPassword",
referrer = null
)
}
coVerify(exactly = 1) {
performLogin(
username = testEmail,
password = "encrypted-$testPassword"
)
}
}
}
}

View File

@ -19,8 +19,23 @@
package me.proton.core.util.kotlin
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.FlowCollector
import kotlinx.coroutines.flow.catch
import kotlinx.coroutines.flow.retryWhen
fun <T> Flow<T>.catchWhen(
predicate: suspend (Throwable) -> Boolean,
action: suspend FlowCollector<T>.() -> Unit
): Flow<T> {
return catch { error ->
if (predicate(error)) {
action()
} else {
throw error
}
}
}
/**
* Retries the collection of the flow, in case an exception occurred in upstream flow, and [predicate] returns `true`.
* If the flow will be retried, [onRetryAction] will be called before, with a [Throwable] that caused the retry.

View File

@ -19,12 +19,56 @@
package me.proton.core.util.kotlin
import app.cash.turbine.test
import kotlinx.coroutines.flow.catch
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.test.runBlockingTest
import org.junit.Assert.assertEquals
import org.junit.Test
class FlowUtilsTest {
@Test
fun `catches known error`() = runBlockingTest {
flow {
emit(1)
throw TestError
}.catchWhen({ it is TestError }) {
emit(2)
}.test {
assertEquals(1, awaitItem())
assertEquals(2, awaitItem())
awaitComplete()
}
}
@Test
fun `does not catch unknown error`() = runBlockingTest {
flow {
emit(1)
error("random error")
}.catchWhen({ it is TestError }) {
emit(2)
}.test {
assertEquals(1, awaitItem())
assertEquals("random error", awaitError().message)
}
}
@Test
fun `re-catches unknown error`() = runBlockingTest {
flow {
emit(1)
error("random error")
}.catchWhen({ it is TestError }) {
emit(2)
}.catch {
emit(3)
}.test {
assertEquals(1, awaitItem())
assertEquals(3, awaitItem())
awaitComplete()
}
}
@Test
fun `retries after detecting known error`() = runBlockingTest {
var retryCount = 0