diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsRepository.kt index 5975725ae3..6f3da90d51 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/AccountSettingsRepository.kt @@ -52,6 +52,8 @@ class AccountSettingsRepository { fun getMaxTotpApps(): Int = totpRepository.getMaxApps() + fun getMaxMfaKeys(): Int = totpRepository.getMaxMfaKeys() + /** * Every second factor on the account, authenticator apps first, or a failure if we couldn't find out. Passkeys are * mocked for now, so only the authenticator apps can actually fail to load. 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 868ab6e1df..2c846fa09d 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 @@ -184,10 +184,11 @@ class AccountSettingsViewModel( } private suspend fun applyAddTotpAppClicked() { - if (_state.value.signalLogin?.atMaxTotpApps == true) { - _state.update { it.copy(dialog = Dialog.MaxTotpAppsReached) } - } else { - _actions.send(AccountSettingsAction.NavigateToTotpSetup) + val signalLogin = _state.value.signalLogin + when { + signalLogin?.atMaxTotpApps == true -> _state.update { it.copy(dialog = Dialog.MaxTotpAppsReached) } + signalLogin?.atMaxMfaKeys == true -> _state.update { it.copy(dialog = Dialog.MaxMfaKeysReached) } + else -> _actions.send(AccountSettingsAction.NavigateToTotpSetup) } } @@ -242,7 +243,7 @@ class AccountSettingsViewModel( clientDeprecated = repository.isClientDeprecated(), isPhoneNumberless = isPhoneNumberless, // Held onto across refreshes so a resume doesn't drop the list back to its loading state. - signalLogin = if (isPhoneNumberless) it.signalLogin ?: SignalLogin(maxTotpApps = repository.getMaxTotpApps()) else null + signalLogin = if (isPhoneNumberless) it.signalLogin ?: SignalLogin(maxTotpApps = repository.getMaxTotpApps(), maxMfaKeys = repository.getMaxMfaKeys()) else null ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/authenticator/TotpRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/authenticator/TotpRepository.kt index addddb8bc8..fded0b29fe 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/authenticator/TotpRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/account/authenticator/TotpRepository.kt @@ -19,6 +19,7 @@ import org.signal.libsignal.net.TotpParameters import org.signal.network.api.AccountApiV2 import org.thoughtcrime.securesms.keyvalue.SignalStore import org.thoughtcrime.securesms.net.SignalNetwork +import org.thoughtcrime.securesms.util.RemoteConfig import java.net.URLEncoder import java.time.Instant @@ -38,12 +39,6 @@ class TotpRepository( companion object { private val TAG = Log.tag(TotpRepository::class) - /** - * How many authenticator apps an account may have, which the service enforces. libsignal reports hitting the - * limit but doesn't expose the number, so the screens that want to show it get it from here. - */ - const val MAX_APPS = 2 - private const val ISSUER = "Signal" /** The algorithm names the Key Uri Format defines, keyed by what [TotpParameters.algorithm] calls them. */ @@ -60,8 +55,20 @@ class TotpRepository( const val MAX_NAME_LENGTH_GRAPHEMES = 30 } + /** + * How many authenticator apps the account is allowed at once. libsignal reports hitting the limit but doesn't expose + * the number, so the screens that want to show it get it from here. + */ fun getMaxApps(): Int { - return MAX_APPS + return RemoteConfig.maxTotpApps + } + + /** + * How many two-factor methods of every kind the account is allowed at once. Authenticator apps share this limit with + * passkeys, so it can be reached even when there's room left under [getMaxApps]. + */ + fun getMaxMfaKeys(): Int { + return RemoteConfig.maxMfaKeys } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/RemoteConfig.kt b/app/src/main/java/org/thoughtcrime/securesms/util/RemoteConfig.kt index 6480c22a35..8e98de78d9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/RemoteConfig.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/util/RemoteConfig.kt @@ -1471,5 +1471,22 @@ object RemoteConfig { defaultValue = 3.days.inWholeSeconds, hotSwappable = true ) + + /** The maximum number of authenticator apps a user can have on their account. */ + val maxTotpApps: Int by remoteInt( + key = "global.maxTotpApps", + defaultValue = 2, + hotSwappable = true + ) + + /** + * The maximum number of two-factor methods of every kind, authenticator apps and passkeys alike, a user can have on + * their account. Every method counts against this, so it's the limit on the total rather than on any one kind. + */ + val maxMfaKeys: Int by remoteInt( + key = "global.maxMfaKeys", + defaultValue = 10, + hotSwappable = true + ) // endregion } 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 05c119d490..309b727f3e 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 @@ -74,6 +74,7 @@ class AccountSettingsViewModelTest { every { repository.getPinKeyboardType() } returns PinKeyboardType.NUMERIC every { repository.isPhoneNumberless() } returns false every { repository.getMaxTotpApps() } returns 2 + every { repository.getMaxMfaKeys() } returns 10 coEvery { repository.getTwoFactorMethods() } returns AccountSettingsRepository.TwoFactorMethodsResult.Success(emptyList()) coEvery { repository.removeTotpApp(any()) } returns true every { repository.verifyLocalPin(any()) } answers { firstArg() == CORRECT_PIN } @@ -322,6 +323,7 @@ class AccountSettingsViewModelTest { assertThat(viewModel.state.value.signalLogin!!.twoFactorMethods).containsExactly(TOTP_APP, PASSKEY) assertThat(viewModel.state.value.signalLogin?.loadState).isEqualTo(LoadState.LOADED) assertThat(viewModel.state.value.signalLogin?.maxTotpApps).isEqualTo(2) + assertThat(viewModel.state.value.signalLogin?.maxMfaKeys).isEqualTo(10) } /** An empty list says nothing on its own, so the screen leans on the load state to know we haven't heard back yet. */ @@ -373,7 +375,7 @@ class AccountSettingsViewModelTest { assertThat(actions.last()).isEqualTo(AccountSettingsAction.NavigateToTotpSetup) } - /** Passkeys share the list but not the limit, so they can't be what stops another app from being added. */ + /** The app limit is the more specific of the two, so it's what an account with room to spare overall is told about. */ @Test fun `AddTotpAppClicked explains the limit when there's no room for another app`() = runTest(testDispatcher) { every { repository.isPhoneNumberless() } returns true @@ -388,6 +390,22 @@ class AccountSettingsViewModelTest { assertThat(actions).isEmpty() } + /** Every second factor counts against one overall limit, so a passkey can be what leaves no room for another app. */ + @Test + fun `AddTotpAppClicked explains the overall limit when there's no room for another second factor`() = runTest(testDispatcher) { + every { repository.isPhoneNumberless() } returns true + every { repository.getMaxMfaKeys() } returns 2 + coEvery { repository.getTwoFactorMethods() } returns methods(TOTP_APP, PASSKEY) + + val viewModel = createViewModel() + val actions = collectActions(viewModel.actions) + + viewModel.onEvent(AccountSettingsEvent.AddTotpAppClicked) + + assertThat(viewModel.state.value.dialog).isEqualTo(Dialog.MaxMfaKeysReached) + assertThat(actions).isEmpty() + } + @Test fun `RenameMethodClicked opens the naming screen for that app`() = runTest(testDispatcher) { every { repository.isPhoneNumberless() } returns true diff --git a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsScreen.kt b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsScreen.kt index e726c3eb75..aab562da2f 100644 --- a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsScreen.kt +++ b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsScreen.kt @@ -101,6 +101,7 @@ object AccountSettingsTestTags { const val DIALOG_CONFIRM_REGISTRATION_LOCK = "dialog-confirm-registration-lock" const val DIALOG_CONFIRM_REMOVE_TOTP_APP = "dialog-confirm-remove-totp-app" const val DIALOG_MAX_TOTP_APPS_REACHED = "dialog-max-totp-apps-reached" + const val DIALOG_MAX_MFA_KEYS_REACHED = "dialog-max-mfa-keys-reached" const val PIN_INPUT = "pin-input" const val PIN_KEYBOARD_TOGGLE = "pin-keyboard-toggle" } @@ -369,6 +370,7 @@ fun AccountSettingsScreen( } is Dialog.ConfirmRemoveTotpApp -> ConfirmRemoveTotpAppDialog(appId = dialog.appId, onEvent = onEvent) Dialog.MaxTotpAppsReached -> MaxTotpAppsReachedDialog(maxApps = state.signalLogin?.maxTotpApps ?: 0, onEvent = onEvent) + Dialog.MaxMfaKeysReached -> MaxMfaKeysReachedDialog(maxMfaKeys = state.signalLogin?.maxMfaKeys ?: 0, onEvent = onEvent) } } @@ -645,6 +647,25 @@ private fun MaxTotpAppsReachedDialog( ) } +/** Shown when there's still room for another authenticator app, but not for another second factor of any kind. */ +@Composable +private fun MaxMfaKeysReachedDialog( + maxMfaKeys: Int, + onEvent: (AccountSettingsEvent) -> Unit +) { + Dialogs.SimpleAlertDialog( + title = stringResource(R.string.AccountSettingsFragment__cant_add_authenticator_app), + body = stringResource(R.string.AccountSettingsFragment__you_cant_add_more_than_d_two_factor_methods, maxMfaKeys), + confirm = stringResource(android.R.string.ok), + onConfirm = {}, + onDismiss = { onEvent(AccountSettingsEvent.DialogDismissed) }, + dismiss = stringResource(R.string.AccountSettingsFragment__learn_more), + onDeny = { onEvent(AccountSettingsEvent.LearnMoreClicked("https://support.signal.org/hc/articles/11228705649690")) }, + onDismissRequest = { onEvent(AccountSettingsEvent.DialogDismissed) }, + modifier = Modifier.testTag(AccountSettingsTestTags.DIALOG_MAX_MFA_KEYS_REACHED) + ) +} + @Composable private fun RegistrationLockConfirmationDialog( dialog: Dialog.ConfirmRegistrationLock, @@ -893,6 +914,14 @@ private fun MaxTotpAppsReachedDialogPreview() { } } +@DayNightPreviews +@Composable +private fun MaxMfaKeysReachedDialogPreview() { + Previews.Preview { + MaxMfaKeysReachedDialog(maxMfaKeys = 10, onEvent = {}) + } +} + private val PREVIEW_TWO_FACTOR_METHODS = listOf( TwoFactorMethod(id = 1, kind = TwoFactorMethod.Kind.AUTHENTICATOR_APP, name = "Bitwarden Authenticator", createdAt = System.currentTimeMillis()), TwoFactorMethod(id = 2, kind = TwoFactorMethod.Kind.AUTHENTICATOR_APP, name = "Twilio Authy", createdAt = System.currentTimeMillis()), diff --git a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsState.kt b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsState.kt index 2a471c89af..17cf68e26a 100644 --- a/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsState.kt +++ b/feature/app-settings/src/main/java/org/signal/appsettings/account/AccountSettingsState.kt @@ -31,11 +31,17 @@ data class AccountSettingsState( /** How the last look at the account went, which decides what the two-factor list shows in place of rows. */ val loadState: LoadState = LoadState.LOADING, /** How many authenticator apps the account is allowed to have at once. */ - val maxTotpApps: Int = 0 + val maxTotpApps: Int = 0, + /** How many second factors of every kind the account is allowed at once, authenticator apps included. */ + val maxMfaKeys: Int = 0 ) { val atMaxTotpApps: Boolean get() = twoFactorMethods.count { it.kind == TwoFactorMethod.Kind.AUTHENTICATOR_APP } >= maxTotpApps + + /** Whether the account is out of room for second factors of any kind, which stops another app being added too. */ + val atMaxMfaKeys: Boolean + get() = twoFactorMethods.size >= maxMfaKeys } /** How the last attempt to read the account's second factors went, since an empty list can't say on its own. */ @@ -80,5 +86,8 @@ data class AccountSettingsState( /** Explains that the account already has as many authenticator apps as it's allowed. */ data object MaxTotpAppsReached : Dialog + + /** Explains that the account already has as many second factors of all kinds as it's allowed. */ + data object MaxMfaKeysReached : Dialog } } diff --git a/feature/app-settings/src/main/res/values/strings.xml b/feature/app-settings/src/main/res/values/strings.xml index 4e9369f1ab..f972424d8a 100644 --- a/feature/app-settings/src/main/res/values/strings.xml +++ b/feature/app-settings/src/main/res/values/strings.xml @@ -86,6 +86,8 @@ Can\'t add authenticator app You can\'t add more than %1$d authenticator apps. Try removing one first. + + You can\'t add more than %1$d two-factor authentication methods. Try removing one first. Couldn\'t load your two-factor authentication methods. Check your connection and try again. diff --git a/feature/app-settings/src/test/java/org/signal/appsettings/account/AccountSettingsScreenTest.kt b/feature/app-settings/src/test/java/org/signal/appsettings/account/AccountSettingsScreenTest.kt index 169a160d48..39ae673c1c 100644 --- a/feature/app-settings/src/test/java/org/signal/appsettings/account/AccountSettingsScreenTest.kt +++ b/feature/app-settings/src/test/java/org/signal/appsettings/account/AccountSettingsScreenTest.kt @@ -429,6 +429,17 @@ class AccountSettingsScreenTest { assertThat(events).contains(AccountSettingsEvent.DialogDismissed) } + @Test + fun givenTheMaxMfaKeysDialog_whenIClickLearnMore_thenIExpectLearnMoreAndDismissEvents() { + setContent(createState(signalLogin = signalLogin(), dialog = Dialog.MaxMfaKeysReached)) + + composeTestRule.onNodeWithTag(AccountSettingsTestTags.DIALOG_MAX_MFA_KEYS_REACHED).assertIsDisplayed() + composeTestRule.onNodeWithTag(Dialogs.TEST_TAG_ALERT_DIALOG_DISMISS_BUTTON).performClick() + + assertThat(events).contains(AccountSettingsEvent.LearnMoreClicked("https://support.signal.org/hc/articles/11228705649690")) + assertThat(events).contains(AccountSettingsEvent.DialogDismissed) + } + @Test fun whenIClickTheSignalLoginLearnMore_thenIExpectLearnMoreForTheSignalLoginArticle() { setContent(createState(signalLogin = signalLogin())) @@ -496,9 +507,10 @@ class AccountSettingsScreenTest { private fun signalLogin( twoFactorMethods: List = emptyList(), loadState: LoadState = LoadState.LOADED, - maxTotpApps: Int = 2 + maxTotpApps: Int = 2, + maxMfaKeys: Int = 10 ): AccountSettingsState.SignalLogin { - return AccountSettingsState.SignalLogin(twoFactorMethods = twoFactorMethods, loadState = loadState, maxTotpApps = maxTotpApps) + return AccountSettingsState.SignalLogin(twoFactorMethods = twoFactorMethods, loadState = loadState, maxTotpApps = maxTotpApps, maxMfaKeys = maxMfaKeys) } /** Links inside an [androidx.compose.ui.text.AnnotatedString] have no bounds to tap, so their click action is invoked directly. */