PM-13690: Add dialog before switching account during passwordless login (#4114)

This commit is contained in:
David Perez 2024-10-21 14:35:19 -05:00 committed by GitHub
parent d1f13e49a4
commit 27beb25bf7
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 341 additions and 40 deletions

View file

@ -63,7 +63,7 @@ class MainViewModel @Inject constructor(
private val fido2CredentialManager: Fido2CredentialManager,
private val intentManager: IntentManager,
settingsRepository: SettingsRepository,
vaultRepository: VaultRepository,
private val vaultRepository: VaultRepository,
private val authRepository: AuthRepository,
private val environmentRepository: EnvironmentRepository,
private val savedStateHandle: SavedStateHandle,
@ -244,8 +244,15 @@ class MainViewModel @Inject constructor(
val fido2GetCredentialsRequest = intent.getFido2GetCredentialsRequestOrNull()
when {
passwordlessRequestData != null -> {
if (authRepository.activeUserId != passwordlessRequestData.userId) {
authRepository.switchAccount(passwordlessRequestData.userId)
authRepository.activeUserId?.let {
if (it != passwordlessRequestData.userId &&
!vaultRepository.isVaultUnlocked(it)
) {
// We only switch the account here if the current user's vault is not
// unlocked, otherwise prompt the user to allow us to change the account
// in the LoginApprovalScreen
authRepository.switchAccount(passwordlessRequestData.userId)
}
}
specialCircumstanceManager.specialCircumstance =
SpecialCircumstance.PasswordlessRequest(

View file

@ -31,7 +31,6 @@ import androidx.hilt.navigation.compose.hiltViewModel
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import com.x8bit.bitwarden.R
import com.x8bit.bitwarden.ui.platform.base.util.EventsEffect
import com.x8bit.bitwarden.ui.platform.base.util.asText
import com.x8bit.bitwarden.ui.platform.components.appbar.BitwardenTopAppBar
import com.x8bit.bitwarden.ui.platform.components.button.BitwardenFilledButton
import com.x8bit.bitwarden.ui.platform.components.button.BitwardenOutlinedButton
@ -39,6 +38,7 @@ import com.x8bit.bitwarden.ui.platform.components.content.BitwardenErrorContent
import com.x8bit.bitwarden.ui.platform.components.content.BitwardenLoadingContent
import com.x8bit.bitwarden.ui.platform.components.dialog.BasicDialogState
import com.x8bit.bitwarden.ui.platform.components.dialog.BitwardenBasicDialog
import com.x8bit.bitwarden.ui.platform.components.dialog.BitwardenTwoButtonDialog
import com.x8bit.bitwarden.ui.platform.components.scaffold.BitwardenScaffold
import com.x8bit.bitwarden.ui.platform.components.util.rememberVectorPainter
import com.x8bit.bitwarden.ui.platform.composition.LocalExitManager
@ -70,18 +70,17 @@ fun LoginApprovalScreen(
}
}
BitwardenBasicDialog(
visibilityState = if (state.shouldShowErrorDialog) {
BasicDialogState.Shown(
title = R.string.an_error_has_occurred.asText(),
message = R.string.generic_error_message.asText(),
)
} else {
BasicDialogState.Hidden
},
onDismissRequest = remember(viewModel) {
LoginApprovalDialogs(
state = state.dialogState,
onDismissError = remember(viewModel) {
{ viewModel.trySendAction(LoginApprovalAction.ErrorDialogDismiss) }
},
onConfirmChangeAccount = remember(viewModel) {
{ viewModel.trySendAction(LoginApprovalAction.ApproveAccountChangeClick) }
},
onDismissChangeAccount = remember(viewModel) {
{ viewModel.trySendAction(LoginApprovalAction.CancelAccountChangeClick) }
},
)
BackHandler(
@ -282,3 +281,33 @@ private fun LoginApprovalInfoColumn(
Spacer(modifier = Modifier.height(8.dp))
}
@Composable
private fun LoginApprovalDialogs(
state: LoginApprovalState.DialogState?,
onDismissError: () -> Unit,
onConfirmChangeAccount: () -> Unit,
onDismissChangeAccount: () -> Unit,
) {
when (state) {
is LoginApprovalState.DialogState.ChangeAccount -> BitwardenTwoButtonDialog(
title = stringResource(id = R.string.log_in_requested),
message = state.message(),
confirmButtonText = stringResource(id = R.string.ok),
dismissButtonText = stringResource(id = R.string.cancel),
onConfirmClick = onConfirmChangeAccount,
onDismissClick = onDismissChangeAccount,
onDismissRequest = onDismissChangeAccount,
)
is LoginApprovalState.DialogState.Error -> BitwardenBasicDialog(
visibilityState = BasicDialogState.Shown(
title = state.title,
message = state.message,
),
onDismissRequest = onDismissError,
)
null -> Unit
}
}

View file

@ -1,3 +1,5 @@
@file:Suppress("TooManyFunctions")
package com.x8bit.bitwarden.ui.platform.feature.settings.accountsecurity.loginapproval
import android.os.Parcelable
@ -47,20 +49,41 @@ class LoginApprovalViewModel @Inject constructor(
masterPasswordHash = null,
publicKey = "",
requestId = "",
shouldShowErrorDialog = false,
viewState = LoginApprovalState.ViewState.Loading,
dialogState = null,
)
},
) {
init {
state
.specialCircumstance
?.let { circumstance ->
authRepository
.getAuthRequestByIdFlow(circumstance.passwordlessRequestData.loginRequestId)
.map { LoginApprovalAction.Internal.AuthRequestResultReceive(it) }
.onEach(::sendAction)
.launchIn(viewModelScope)
?.passwordlessRequestData
?.let { passwordlessRequestData ->
if (authRepository.activeUserId != passwordlessRequestData.userId) {
mutableStateFlow.update {
it.copy(
dialogState = LoginApprovalState.DialogState.ChangeAccount(
message = R.string
.login_attempt_from_x_do_you_want_to_switch_to_this_account
.asText(
authRepository
.userStateFlow
.value
?.accounts
?.find { it.userId == passwordlessRequestData.userId }
?.email
.orEmpty(),
),
),
)
}
} else {
authRepository
.getAuthRequestByIdFlow(passwordlessRequestData.loginRequestId)
.map { LoginApprovalAction.Internal.AuthRequestResultReceive(it) }
.onEach(::sendAction)
.launchIn(viewModelScope)
}
}
?: run {
authRepository
@ -77,6 +100,8 @@ class LoginApprovalViewModel @Inject constructor(
LoginApprovalAction.CloseClick -> handleCloseClicked()
LoginApprovalAction.DeclineRequestClick -> handleDeclineRequestClicked()
LoginApprovalAction.ErrorDialogDismiss -> handleErrorDialogDismissed()
LoginApprovalAction.ApproveAccountChangeClick -> handleApproveAccountChangeClick()
LoginApprovalAction.CancelAccountChangeClick -> handleCancelAccountChangeClick()
is LoginApprovalAction.Internal.ApproveRequestResultReceive -> {
handleApproveRequestResultReceived(action)
@ -128,10 +153,27 @@ class LoginApprovalViewModel @Inject constructor(
private fun handleErrorDialogDismissed() {
mutableStateFlow.update {
it.copy(shouldShowErrorDialog = false)
it.copy(dialogState = null)
}
}
private fun handleApproveAccountChangeClick() {
state.specialCircumstance?.passwordlessRequestData?.let { data ->
authRepository.switchAccount(userId = data.userId)
mutableStateFlow.update { it.copy(dialogState = null) }
authRepository
.getAuthRequestByIdFlow(data.loginRequestId)
.map { LoginApprovalAction.Internal.AuthRequestResultReceive(it) }
.onEach(::sendAction)
.launchIn(viewModelScope)
}
}
private fun handleCancelAccountChangeClick() {
mutableStateFlow.update { it.copy(dialogState = null) }
sendClosingEvent()
}
private fun handleApproveRequestResultReceived(
action: LoginApprovalAction.Internal.ApproveRequestResultReceive,
) {
@ -143,7 +185,12 @@ class LoginApprovalViewModel @Inject constructor(
is AuthRequestResult.Error -> {
mutableStateFlow.update {
it.copy(shouldShowErrorDialog = true)
it.copy(
dialogState = LoginApprovalState.DialogState.Error(
title = R.string.an_error_has_occurred.asText(),
message = R.string.generic_error_message.asText(),
),
)
}
}
}
@ -200,7 +247,12 @@ class LoginApprovalViewModel @Inject constructor(
is AuthRequestResult.Error -> {
mutableStateFlow.update {
it.copy(shouldShowErrorDialog = true)
it.copy(
dialogState = LoginApprovalState.DialogState.Error(
title = R.string.an_error_has_occurred.asText(),
message = R.string.generic_error_message.asText(),
),
)
}
}
}
@ -223,7 +275,7 @@ class LoginApprovalViewModel @Inject constructor(
@Parcelize
data class LoginApprovalState(
val viewState: ViewState,
val shouldShowErrorDialog: Boolean,
val dialogState: DialogState?,
// Internal
val specialCircumstance: SpecialCircumstance.PasswordlessRequest?,
val fingerprint: String,
@ -263,6 +315,27 @@ data class LoginApprovalState(
@Parcelize
data object Loading : ViewState()
}
/**
* Represents the various dialogs that can be displayed.
*/
@Parcelize
sealed class DialogState : Parcelable {
/**
* Requests permission to change active user.
*/
data class Error(
val title: Text?,
val message: Text,
) : DialogState()
/**
* Requests permission to change active user.
*/
data class ChangeAccount(
val message: Text,
) : DialogState()
}
}
/**
@ -311,6 +384,16 @@ sealed class LoginApprovalAction {
*/
data object ErrorDialogDismiss : LoginApprovalAction()
/**
* User approved changing the account.
*/
data object ApproveAccountChangeClick : LoginApprovalAction()
/**
* User dismissed the change account dialog.
*/
data object CancelAccountChangeClick : LoginApprovalAction()
/**
* Models action the view model could send itself.
*/

View file

@ -1106,10 +1106,12 @@ class MainViewModelTest : BaseViewModelTest() {
@Suppress("MaxLineLength")
@Test
fun `on ReceiveNewIntent with a passwordless auth request data userId that doesn't match activeUserId should switchAccount`() {
fun `on ReceiveNewIntent with a passwordless auth request data userId that doesn't match activeUserId and the vault is not locked should switchAccount`() {
val userId = "userId"
val viewModel = createViewModel()
val mockIntent = mockk<Intent>()
val passwordlessRequestData = mockk<PasswordlessRequestData>()
every { vaultRepository.isVaultUnlocked(ACTIVE_USER_ID) } returns false
every {
mockIntent.getPasswordlessRequestDataIntentOrNull()
} returns passwordlessRequestData
@ -1121,7 +1123,7 @@ class MainViewModelTest : BaseViewModelTest() {
every { mockIntent.isMyVaultShortcut } returns false
every { mockIntent.isPasswordGeneratorShortcut } returns false
every { mockIntent.isAccountSecurityShortcut } returns false
every { passwordlessRequestData.userId } returns "userId"
every { passwordlessRequestData.userId } returns userId
viewModel.trySendAction(
MainAction.ReceiveNewIntent(
@ -1129,7 +1131,7 @@ class MainViewModelTest : BaseViewModelTest() {
),
)
verify { authRepository.switchAccount("userId") }
verify { authRepository.switchAccount(userId) }
}
private fun createViewModel(
@ -1162,8 +1164,9 @@ private val DEFAULT_FIRST_TIME_STATE = FirstTimeState(
)
private const val SPECIAL_CIRCUMSTANCE_KEY: String = "special-circumstance"
private const val ACTIVE_USER_ID: String = "activeUserId"
private val DEFAULT_ACCOUNT = UserState.Account(
userId = "activeUserId",
userId = ACTIVE_USER_ID,
name = "Active User",
email = "active@bitwarden.com",
environment = Environment.Us,

View file

@ -2,13 +2,16 @@ package com.x8bit.bitwarden.ui.platform.feature.settings.accountsecurity.loginap
import androidx.compose.ui.test.assert
import androidx.compose.ui.test.assertIsDisplayed
import androidx.compose.ui.test.filterToOne
import androidx.compose.ui.test.hasAnyAncestor
import androidx.compose.ui.test.isDialog
import androidx.compose.ui.test.onAllNodesWithText
import androidx.compose.ui.test.onNodeWithText
import androidx.compose.ui.test.performClick
import androidx.compose.ui.test.performScrollTo
import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow
import com.x8bit.bitwarden.ui.platform.base.BaseComposeTest
import com.x8bit.bitwarden.ui.platform.base.util.asText
import com.x8bit.bitwarden.ui.platform.manager.exit.ExitManager
import io.mockk.every
import io.mockk.just
@ -93,14 +96,23 @@ class LoginApprovalScreenTest : BaseComposeTest() {
@Test
fun `on error dialog dismiss click should send ErrorDialogDismiss`() = runTest {
val title = "An error has occurred."
val message = "We were unable to process your request."
mutableStateFlow.tryEmit(
DEFAULT_STATE.copy(
shouldShowErrorDialog = true,
dialogState = LoginApprovalState.DialogState.Error(
title = title.asText(),
message = message.asText(),
),
),
)
composeTestRule
.onNodeWithText("An error has occurred.")
.onNodeWithText(text = title)
.assert(hasAnyAncestor(isDialog()))
.assertIsDisplayed()
composeTestRule
.onNodeWithText(text = message)
.assert(hasAnyAncestor(isDialog()))
.assertIsDisplayed()
@ -112,6 +124,64 @@ class LoginApprovalScreenTest : BaseComposeTest() {
viewModel.trySendAction(LoginApprovalAction.ErrorDialogDismiss)
}
}
@Test
fun `on change account dialog confirm click should send ApproveAccountChangeClick`() = runTest {
val message = "We were unable to process your request."
mutableStateFlow.tryEmit(
DEFAULT_STATE.copy(
dialogState = LoginApprovalState.DialogState.ChangeAccount(
message = message.asText(),
),
),
)
composeTestRule
.onAllNodesWithText(text = "Login requested")
.filterToOne(hasAnyAncestor(isDialog()))
.assertIsDisplayed()
composeTestRule
.onNodeWithText(text = message)
.assert(hasAnyAncestor(isDialog()))
.assertIsDisplayed()
composeTestRule
.onNodeWithText(text = "Ok")
.performClick()
verify {
viewModel.trySendAction(LoginApprovalAction.ApproveAccountChangeClick)
}
}
@Test
fun `on change account dialog dismiss click should send CancelAccountChangeClick`() = runTest {
val message = "We were unable to process your request."
mutableStateFlow.tryEmit(
DEFAULT_STATE.copy(
dialogState = LoginApprovalState.DialogState.ChangeAccount(
message = message.asText(),
),
),
)
composeTestRule
.onAllNodesWithText(text = "Login requested")
.filterToOne(hasAnyAncestor(isDialog()))
.assertIsDisplayed()
composeTestRule
.onNodeWithText(text = message)
.assert(hasAnyAncestor(isDialog()))
.assertIsDisplayed()
composeTestRule
.onNodeWithText(text = "Cancel")
.performClick()
verify {
viewModel.trySendAction(LoginApprovalAction.CancelAccountChangeClick)
}
}
}
private const val FINGERPRINT = "fingerprint"
@ -121,7 +191,7 @@ private val DEFAULT_STATE: LoginApprovalState = LoginApprovalState(
masterPasswordHash = null,
publicKey = "publicKey",
requestId = "",
shouldShowErrorDialog = false,
dialogState = null,
viewState = LoginApprovalState.ViewState.Content(
deviceType = "Android",
domainUrl = "bitwarden.com",

View file

@ -8,9 +8,10 @@ import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest
import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestResult
import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestUpdatesResult
import com.x8bit.bitwarden.data.auth.repository.AuthRepository
import com.x8bit.bitwarden.data.platform.manager.model.FirstTimeState
import com.x8bit.bitwarden.data.auth.repository.model.SwitchAccountResult
import com.x8bit.bitwarden.data.auth.repository.model.UserState
import com.x8bit.bitwarden.data.platform.manager.SpecialCircumstanceManager
import com.x8bit.bitwarden.data.platform.manager.model.FirstTimeState
import com.x8bit.bitwarden.data.platform.manager.model.PasswordlessRequestData
import com.x8bit.bitwarden.data.platform.manager.model.SpecialCircumstance
import com.x8bit.bitwarden.data.platform.repository.model.Environment
@ -21,6 +22,7 @@ import io.mockk.coEvery
import io.mockk.coVerify
import io.mockk.every
import io.mockk.mockk
import io.mockk.verify
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.test.runTest
import org.junit.jupiter.api.Assertions.assertEquals
@ -42,6 +44,7 @@ class LoginApprovalViewModelTest : BaseViewModelTest() {
private val mutableUserStateFlow = MutableStateFlow<UserState?>(DEFAULT_USER_STATE)
private val mutableAuthRequestSharedFlow = bufferedMutableSharedFlow<AuthRequestUpdatesResult>()
private val mockAuthRepository = mockk<AuthRepository> {
every { activeUserId } returns USER_ID
coEvery {
getAuthRequestByFingerprintFlow(FINGERPRINT)
} returns mutableAuthRequestSharedFlow
@ -74,6 +77,85 @@ class LoginApprovalViewModelTest : BaseViewModelTest() {
}
}
@Suppress("MaxLineLength")
@Test
fun `init should call show change account dialog when special circumstance is present but user IDs do not match`() {
every {
mockSpecialCircumstanceManager.specialCircumstance
} returns SpecialCircumstance.PasswordlessRequest(
passwordlessRequestData = PasswordlessRequestData(
loginRequestId = REQUEST_ID,
userId = USER_ID_2,
),
shouldFinishWhenComplete = false,
)
val viewModel = createViewModel(state = null)
verify(exactly = 1) {
mockAuthRepository.userStateFlow
}
assertEquals(
viewModel.stateFlow.value,
LoginApprovalState(
fingerprint = "",
specialCircumstance = SpecialCircumstance.PasswordlessRequest(
passwordlessRequestData = PasswordlessRequestData(
loginRequestId = REQUEST_ID,
userId = USER_ID_2,
),
shouldFinishWhenComplete = false,
),
masterPasswordHash = null,
publicKey = "",
requestId = "",
viewState = LoginApprovalState.ViewState.Loading,
dialogState = LoginApprovalState.DialogState.ChangeAccount(
message = R.string.login_attempt_from_x_do_you_want_to_switch_to_this_account
.asText(EMAIL_2),
),
),
)
}
@Suppress("MaxLineLength")
@Test
fun `on ApproveAccountChangeClick dialog state should be cleared, user should be switched, and getAuthRequestByIdFlow should be called`() {
every {
mockAuthRepository.switchAccount(userId = USER_ID_2)
} returns SwitchAccountResult.AccountSwitched
val specialCircumstance = SpecialCircumstance.PasswordlessRequest(
passwordlessRequestData = PasswordlessRequestData(
loginRequestId = REQUEST_ID,
userId = USER_ID_2,
),
shouldFinishWhenComplete = false,
)
val viewModel = createViewModel(
state = DEFAULT_STATE.copy(
specialCircumstance = specialCircumstance,
dialogState = LoginApprovalState.DialogState.ChangeAccount(
message = R.string.login_attempt_from_x_do_you_want_to_switch_to_this_account
.asText(EMAIL_2),
),
),
)
viewModel.trySendAction(LoginApprovalAction.ApproveAccountChangeClick)
verify(exactly = 1) {
mockAuthRepository.switchAccount(userId = USER_ID_2)
}
coVerify(exactly = 1) {
mockAuthRepository.getAuthRequestByIdFlow(requestId = REQUEST_ID)
}
assertEquals(
viewModel.stateFlow.value,
DEFAULT_STATE.copy(
specialCircumstance = specialCircumstance,
dialogState = null,
),
)
}
@Test
fun `getAuthRequest update should update state`() {
val expected = DEFAULT_STATE.copy(
@ -311,20 +393,26 @@ class LoginApprovalViewModelTest : BaseViewModelTest() {
} returns AuthRequestResult.Error
viewModel.trySendAction(LoginApprovalAction.DeclineRequestClick)
assertEquals(viewModel.stateFlow.value, DEFAULT_STATE.copy(shouldShowErrorDialog = true))
assertEquals(
viewModel.stateFlow.value,
DEFAULT_STATE.copy(
dialogState = LoginApprovalState.DialogState.Error(
title = R.string.an_error_has_occurred.asText(),
message = R.string.generic_error_message.asText(),
),
),
)
viewModel.trySendAction(LoginApprovalAction.ErrorDialogDismiss)
assertEquals(viewModel.stateFlow.value, DEFAULT_STATE.copy(shouldShowErrorDialog = false))
assertEquals(viewModel.stateFlow.value, DEFAULT_STATE.copy(dialogState = null))
}
private fun createViewModel(
authRepository: AuthRepository = mockAuthRepository,
specialCircumstanceManager: SpecialCircumstanceManager = mockSpecialCircumstanceManager,
state: LoginApprovalState? = DEFAULT_STATE,
): LoginApprovalViewModel = LoginApprovalViewModel(
clock = fixedClock,
authRepository = authRepository,
specialCircumstanceManager = specialCircumstanceManager,
authRepository = mockAuthRepository,
specialCircumstanceManager = mockSpecialCircumstanceManager,
savedStateHandle = SavedStateHandle()
.also { it["fingerprint"] = FINGERPRINT }
.apply { set("state", state) },
@ -332,6 +420,7 @@ class LoginApprovalViewModelTest : BaseViewModelTest() {
}
private const val EMAIL = "test@bitwarden.com"
private const val EMAIL_2 = "test2@bitwarden.com"
private const val FINGERPRINT = "fingerprint"
private const val PASSWORD_HASH = "verySecureHash"
private const val PUBLIC_KEY = "publicKey"
@ -342,7 +431,7 @@ private val DEFAULT_STATE: LoginApprovalState = LoginApprovalState(
masterPasswordHash = PASSWORD_HASH,
publicKey = PUBLIC_KEY,
requestId = REQUEST_ID,
shouldShowErrorDialog = false,
dialogState = null,
viewState = LoginApprovalState.ViewState.Content(
deviceType = "Android",
domainUrl = "www.bitwarden.com",
@ -353,6 +442,7 @@ private val DEFAULT_STATE: LoginApprovalState = LoginApprovalState(
),
)
private const val USER_ID = "userID"
private const val USER_ID_2 = "userId_2"
private val DEFAULT_USER_STATE = UserState(
activeUserId = USER_ID,
accounts = listOf(
@ -375,6 +465,25 @@ private val DEFAULT_USER_STATE = UserState(
onboardingStatus = OnboardingStatus.COMPLETE,
firstTimeState = FirstTimeState(showImportLoginsCard = true),
),
UserState.Account(
userId = USER_ID_2,
name = "Second User",
email = EMAIL_2,
environment = Environment.Us,
avatarColorHex = "#aa00aa",
isBiometricsEnabled = false,
isPremium = true,
isLoggedIn = true,
isVaultUnlocked = true,
needsPasswordReset = false,
organizations = emptyList(),
needsMasterPassword = false,
trustedDevice = null,
hasMasterPassword = true,
isUsingKeyConnector = false,
onboardingStatus = OnboardingStatus.COMPLETE,
firstTimeState = FirstTimeState(showImportLoginsCard = true),
),
),
)
private val AUTH_REQUEST = AuthRequest(