mirror of
https://github.com/signalapp/Signal-Android.git
synced 2026-09-19 16:24:41 +01:00
Add global configs for two factor limits.
This commit is contained in:
+2
@@ -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.
|
||||
|
||||
+6
-5
@@ -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
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
+14
-7
@@ -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
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
+19
-1
@@ -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<String>() == 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
|
||||
|
||||
+29
@@ -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()),
|
||||
|
||||
+10
-1
@@ -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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -86,6 +86,8 @@
|
||||
<string name="AccountSettingsFragment__cant_add_authenticator_app">Can\'t add authenticator app</string>
|
||||
<!-- Body of the dialog shown when the account already has as many authenticator apps as it\'s allowed -->
|
||||
<string name="AccountSettingsFragment__you_cant_add_more_than_d">You can\'t add more than %1$d authenticator apps. Try removing one first.</string>
|
||||
<!-- Body of the dialog shown when the account has room for another authenticator app but already has as many two-factor methods of all kinds as it\'s allowed -->
|
||||
<string name="AccountSettingsFragment__you_cant_add_more_than_d_two_factor_methods">You can\'t add more than %1$d two-factor authentication methods. Try removing one first.</string>
|
||||
<!-- Shown in place of the list when we couldn\'t work out which second factors are on the account -->
|
||||
<string name="AccountSettingsFragment__couldnt_load_your_two_factor_methods">Couldn\'t load your two-factor authentication methods. Check your connection and try again.</string>
|
||||
<!-- Toast shown after an authenticator app has been removed -->
|
||||
|
||||
+14
-2
@@ -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<TwoFactorMethod> = 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. */
|
||||
|
||||
Reference in New Issue
Block a user