From 818f22576f451a593b8476ea44a6e751a9d23dad Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Tue, 8 Sep 2026 10:44:24 -0400 Subject: [PATCH] Add screenlock guard to removing two-factor method. --- .../app/account/AccountSettingsFragment.kt | 19 +++++++- .../app/account/AccountSettingsViewModel.kt | 10 ++++- .../account/AccountSettingsViewModelTest.kt | 45 +++++++++++++++---- .../account/AccountSettingsAction.kt | 6 +++ .../account/AccountSettingsEvent.kt | 8 +++- .../src/main/res/values/strings.xml | 6 +++ 6 files changed, 81 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsFragment.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsFragment.kt index 3747956125..da3699c2dd 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsFragment.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsFragment.kt @@ -14,6 +14,7 @@ import androidx.activity.result.contract.ActivityResultContracts.StartActivityFo import androidx.annotation.StringRes import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue +import androidx.compose.ui.res.stringResource import androidx.fragment.app.viewModels import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.navigation.fragment.findNavController @@ -26,6 +27,8 @@ import org.signal.core.ui.compose.ComposeFragment import org.signal.core.util.ServiceUtil import org.signal.core.util.logging.Log import org.thoughtcrime.securesms.R +import org.thoughtcrime.securesms.components.compose.BiometricsAuthentication +import org.thoughtcrime.securesms.components.compose.rememberBiometricsAuthentication import org.thoughtcrime.securesms.components.settings.app.account.authenticator.TotpNavArgs import org.thoughtcrime.securesms.dependencies.AppDependencies import org.thoughtcrime.securesms.lock.v2.CreateSvrPinActivity @@ -67,7 +70,13 @@ class AccountSettingsFragment : ComposeFragment() { override fun FragmentContent() { val state by viewModel.state.collectAsStateWithLifecycle() - CollectActions(viewModel.actions) { action -> handleAction(action) } + val removalBiometrics = rememberBiometricsAuthentication( + promptTitle = stringResource(AppSettingsR.string.AccountSettingsFragment__unlock_to_remove_two_factor_method), + educationSheetMessage = stringResource(AppSettingsR.string.AccountSettingsFragment__to_remove_this_method_confirm_its_you), + onAuthenticationFailed = { viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticationFailed) } + ) + + CollectActions(viewModel.actions) { action -> handleAction(action, removalBiometrics) } AccountSettingsScreen( state = state, @@ -75,7 +84,7 @@ class AccountSettingsFragment : ComposeFragment() { ) } - private fun handleAction(action: AccountSettingsAction) { + private fun handleAction(action: AccountSettingsAction, removalBiometrics: BiometricsAuthentication) { when (action) { AccountSettingsAction.NavigateBack -> requireActivity().onBackPressedDispatcher.onBackPressed() AccountSettingsAction.LaunchCreatePinFlow -> pinFlowLauncher.launch(CreateSvrPinActivity.getIntentForPinCreate(requireContext())) @@ -89,6 +98,12 @@ class AccountSettingsFragment : ComposeFragment() { Bundle().apply { TotpNavArgs.putRenamedApp(this, action.app) } ) } + is AccountSettingsAction.AuthenticateToRemoveMethod -> { + removalBiometrics.withBiometricsAuthentication { + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticated(action.method)) + } + } + AccountSettingsAction.ShowRemovalAuthenticationFailed -> toast(AppSettingsR.string.AccountSettingsFragment__authentication_required) AccountSettingsAction.ShowTotpAppRemoved -> toast(AppSettingsR.string.AccountSettingsFragment__authenticator_app_removed) AccountSettingsAction.ShowTotpAppRemovalFailed -> toast(AppSettingsR.string.AccountSettingsFragment__couldnt_remove_authenticator_app) // TODO Open the two-factor authentication support article once one exists. diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModel.kt index eab4a73807..810b995b16 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModel.kt @@ -95,7 +95,13 @@ class AccountSettingsViewModel( applyRenameMethodClicked(event.method) } is AccountSettingsEvent.RemoveMethodClicked -> { - applyRemoveMethodClicked(event.method) + _actions.send(AccountSettingsAction.AuthenticateToRemoveMethod(event.method)) + } + is AccountSettingsEvent.MethodRemovalAuthenticated -> { + applyMethodRemovalAuthenticated(event.method) + } + AccountSettingsEvent.MethodRemovalAuthenticationFailed -> { + _actions.send(AccountSettingsAction.ShowRemovalAuthenticationFailed) } AccountSettingsEvent.RemoveTotpAppConfirmed -> { applyRemoveTotpAppConfirmed() @@ -194,7 +200,7 @@ class AccountSettingsViewModel( } } - private fun applyRemoveMethodClicked(method: TwoFactorMethod) { + private fun applyMethodRemovalAuthenticated(method: TwoFactorMethod) { when (method.kind) { TwoFactorMethod.Kind.AUTHENTICATOR_APP -> { _state.update { it.copy(dialog = Dialog.ConfirmRemoveTotpApp(method.id)) } diff --git a/app/src/test/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModelTest.kt b/app/src/test/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModelTest.kt index 244cd2cf31..1dc8751d25 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModelTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsViewModelTest.kt @@ -416,8 +416,9 @@ class AccountSettingsViewModelTest { assertThat(actions).isEmpty() } + /** Removing a second factor is guarded by the screen lock, so nothing happens until the user gets past it. */ @Test - fun `RemoveMethodClicked asks the user to confirm first`() = runTest(testDispatcher) { + fun `RemoveMethodClicked asks for the screen lock first`() = runTest(testDispatcher) { every { repository.isPhoneNumberless() } returns true coEvery { repository.getTwoFactorMethods() } returns methods(TOTP_APP) @@ -426,18 +427,46 @@ class AccountSettingsViewModelTest { viewModel.onEvent(AccountSettingsEvent.RemoveMethodClicked(TOTP_APP)) - assertThat(viewModel.state.value.dialog).isEqualTo(Dialog.ConfirmRemoveTotpApp(TOTP_APP.id)) - assertThat(actions).isEmpty() + assertThat(actions.last()).isEqualTo(AccountSettingsAction.AuthenticateToRemoveMethod(TOTP_APP)) + assertThat(viewModel.state.value.dialog).isEqualTo(Dialog.None) } @Test - fun `RemoveMethodClicked for an unsupported passkey does nothing`() = runTest(testDispatcher) { + fun `MethodRemovalAuthenticated asks the user to confirm before removing`() = runTest(testDispatcher) { + every { repository.isPhoneNumberless() } returns true + coEvery { repository.getTwoFactorMethods() } returns methods(TOTP_APP) + + val viewModel = createViewModel() + + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticated(TOTP_APP)) + + assertThat(viewModel.state.value.dialog).isEqualTo(Dialog.ConfirmRemoveTotpApp(TOTP_APP.id)) + } + + @Test + fun `MethodRemovalAuthenticationFailed says so and removes nothing`() = runTest(testDispatcher) { + every { repository.isPhoneNumberless() } returns true + coEvery { repository.getTwoFactorMethods() } returns methods(TOTP_APP) + + val viewModel = createViewModel() + val actions = collectActions(viewModel.actions) + + viewModel.onEvent(AccountSettingsEvent.RemoveMethodClicked(TOTP_APP)) + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticationFailed) + + assertThat(actions.last()).isEqualTo(AccountSettingsAction.ShowRemovalAuthenticationFailed) + assertThat(viewModel.state.value.dialog).isEqualTo(Dialog.None) + coVerify(exactly = 0) { repository.removeTotpApp(any()) } + } + + @Test + fun `MethodRemovalAuthenticated for an unsupported passkey does nothing`() = runTest(testDispatcher) { every { repository.isPhoneNumberless() } returns true coEvery { repository.getTwoFactorMethods() } returns methods(PASSKEY) val viewModel = createViewModel() - viewModel.onEvent(AccountSettingsEvent.RemoveMethodClicked(PASSKEY)) + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticated(PASSKEY)) assertThat(viewModel.state.value.dialog).isEqualTo(Dialog.None) } @@ -450,7 +479,7 @@ class AccountSettingsViewModelTest { val viewModel = createViewModel() val actions = collectActions(viewModel.actions) - viewModel.onEvent(AccountSettingsEvent.RemoveMethodClicked(TOTP_APP)) + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticated(TOTP_APP)) viewModel.onEvent(AccountSettingsEvent.RemoveTotpAppConfirmed) coVerify { repository.removeTotpApp(TOTP_APP.id) } @@ -480,7 +509,7 @@ class AccountSettingsViewModelTest { val viewModel = createViewModel() - viewModel.onEvent(AccountSettingsEvent.RemoveMethodClicked(TOTP_APP)) + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticated(TOTP_APP)) coEvery { repository.getTwoFactorMethods() } returns methods() viewModel.onEvent(AccountSettingsEvent.RemoveTotpAppConfirmed) @@ -497,7 +526,7 @@ class AccountSettingsViewModelTest { val viewModel = createViewModel() val actions = collectActions(viewModel.actions) - viewModel.onEvent(AccountSettingsEvent.RemoveMethodClicked(TOTP_APP)) + viewModel.onEvent(AccountSettingsEvent.MethodRemovalAuthenticated(TOTP_APP)) viewModel.onEvent(AccountSettingsEvent.RemoveTotpAppConfirmed) assertThat(actions.last()).isEqualTo(AccountSettingsAction.ShowTotpAppRemovalFailed) diff --git a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsAction.kt b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsAction.kt index b0cb9e7e5f..071870966b 100644 --- a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsAction.kt +++ b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsAction.kt @@ -36,6 +36,12 @@ sealed interface AccountSettingsAction { /** Open the screen that renames [app]. */ data class NavigateToRenameTotpApp(val app: TotpApp) : AccountSettingsAction + /** Ask the user to get past their screen lock before we remove [method] from the account. */ + data class AuthenticateToRemoveMethod(val method: TwoFactorMethod) : AccountSettingsAction + + /** Tell the user we couldn't confirm it was them, so nothing was removed. */ + data object ShowRemovalAuthenticationFailed : AccountSettingsAction + /** Tell the user their authenticator app was removed. */ data object ShowTotpAppRemoved : AccountSettingsAction diff --git a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsEvent.kt b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsEvent.kt index 78e8c54961..9c3ef68841 100644 --- a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsEvent.kt +++ b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsEvent.kt @@ -54,9 +54,15 @@ sealed interface AccountSettingsEvent { /** The user tapped the rename option in [method]'s overflow menu. */ data class RenameMethodClicked(val method: TwoFactorMethod) : AccountSettingsEvent - /** The user tapped the remove option in [method]'s overflow menu, which asks them to confirm first. */ + /** The user tapped the remove option in [method]'s overflow menu, which asks for the screen lock first. */ data class RemoveMethodClicked(val method: TwoFactorMethod) : AccountSettingsEvent + /** The user got past their screen lock, so we can go on asking them to confirm removing [method]. */ + data class MethodRemovalAuthenticated(val method: TwoFactorMethod) : AccountSettingsEvent + + /** The screen lock turned the user away, so [RemoveMethodClicked] goes no further. */ + data object MethodRemovalAuthenticationFailed : AccountSettingsEvent + /** The user confirmed removing the authenticator app named by the open dialog, which removes it. */ data object RemoveTotpAppConfirmed : AccountSettingsEvent diff --git a/feature/app-settings/src/main/res/values/strings.xml b/feature/app-settings/src/main/res/values/strings.xml index 83b0184d19..6314e00dab 100644 --- a/feature/app-settings/src/main/res/values/strings.xml +++ b/feature/app-settings/src/main/res/values/strings.xml @@ -72,6 +72,12 @@ Remove authenticator app? You won\'t be able to use this app for one-time verification codes when you sign in. + + Unlock to remove two-factor method + + To remove this method, confirm it\'s you + + Authentication required Can\'t add authenticator app