diff --git a/feature/registration/src/main/java/org/signal/registration/screens/aepentry/AepInput.kt b/feature/registration/src/main/java/org/signal/registration/screens/aepentry/AepInput.kt index d5b31aacb7..1292aa7fe1 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/aepentry/AepInput.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/aepentry/AepInput.kt @@ -10,10 +10,9 @@ import org.signal.core.util.censor /** * Recovery key text as the user has typed it so far, alongside the normalized form and whatever is currently wrong with - * it. Every screen that collects a recovery key shares this so they all agree on when a key is too long, malformed, or - * finished. + * it. Every screen that collects a recovery key shares this so they all agree on when a key is malformed or finished. * - * @param enteredText The typed text, preserved verbatim (illegal characters stripped) so #/= stay visible as they are typed. + * @param enteredText The typed text, preserved verbatim (illegal characters stripped, cut off at a complete key) so #/= stay visible as they are typed. * @param normalized Storage-normalized lowercase form of [enteredText], used for validation and submit. */ data class AepInput( @@ -27,29 +26,24 @@ data class AepInput( companion object { /** - * Normalizes [input] and works out what, if anything, is wrong with it. An error the user has already been shown - * sticks around until it is actually resolved, so [previousError] gets a say in the outcome. + * Normalizes [input], cutting off anything past a complete key, and works out what, if anything, is wrong with what + * is left. An error the user has already been shown sticks around until it is actually resolved, so [previousError] + * gets a say in the outcome. */ fun from(input: String, previousError: AepValidationError? = null): AepInput { - val enteredText = AccountEntropyPool.removeIllegalCharacters(input).take(AccountEntropyPool.LENGTH + 16) + val enteredText = AccountEntropyPool.removeIllegalCharacters(input).take(AccountEntropyPool.LENGTH) val normalized = AccountEntropyPool.formatForStorage(enteredText).lowercase() val isValid = AccountEntropyPool.isFullyValid(normalized) - val isShort = normalized.length < AccountEntropyPool.LENGTH - val isExact = normalized.length == AccountEntropyPool.LENGTH + val isComplete = normalized.length == AccountEntropyPool.LENGTH val carriedError = when (previousError) { - is AepValidationError.TooLong -> if (isShort || isExact) null else previousError.copy(count = normalized.length) AepValidationError.Invalid -> if (isValid) null else previousError AepValidationError.Incorrect -> null null -> null } - val error = carriedError ?: when { - !isShort && !isExact -> AepValidationError.TooLong(normalized.length, AccountEntropyPool.LENGTH) - !isValid && isExact -> AepValidationError.Invalid - else -> null - } + val error = carriedError ?: AepValidationError.Invalid.takeIf { isComplete && !isValid } return AepInput(enteredText = enteredText, normalized = normalized, isValid = isValid, error = error) } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepScreen.kt b/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepScreen.kt index a3d77bff2c..20882c40b3 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepScreen.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepScreen.kt @@ -311,8 +311,7 @@ private fun RecoveryKeyTextField(state: EnterAepState, onEvent: (EnterAepEvents) } ), supportingText = { - when (val error = state.recoveryKey.error) { - is AepValidationError.TooLong -> Text(stringResource(R.string.EnterAepScreen__too_long, error.count, error.max)) + when (state.recoveryKey.error) { is AepValidationError.Invalid -> Text(stringResource(R.string.EnterAepScreen__invalid_recovery_key)) is AepValidationError.Incorrect -> Text(stringResource(R.string.EnterAepScreen__incorrect_recovery_key)) null -> {} diff --git a/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepState.kt b/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepState.kt index 863d0af1d0..27322d05e0 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepState.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/aepentry/EnterAepState.kt @@ -19,7 +19,6 @@ data class EnterAepState( } sealed interface AepValidationError { - data class TooLong(val count: Int, val max: Int) : AepValidationError data object Invalid : AepValidationError data object Incorrect : AepValidationError } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryScreen.kt b/feature/registration/src/main/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryScreen.kt index e458643b46..bed306630c 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryScreen.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryScreen.kt @@ -91,6 +91,7 @@ import org.signal.registration.screens.RegistrationScaffold import org.signal.registration.screens.TwoPaneRegistrationScaffold import org.signal.registration.screens.attachDebugLogHelper import org.signal.registration.screens.shared.AccountIdErrorText +import org.signal.registration.screens.shared.AccountIdFormat import org.signal.registration.screens.shared.AccountIdVisualTransformation import org.signal.registration.screens.shared.accountIdTextStyle import org.signal.registration.test.TestTags @@ -590,8 +591,11 @@ private fun PhoneNumberInputFields( TextField( value = phoneNumberTextFieldValue, onValueChange = { newValue -> - onEvent(PhoneNumberEntryScreenEvents.NationalNumberChanged(oldValue = phoneNumberTextFieldValue.text, newValue = newValue.text)) - phoneNumberTextFieldValue = newValue + // An account ID that is already complete leaves the state untouched, so there is no re-sync to lean on: the + // field has to turn away the extra characters itself. + val accepted = if (isAccountId) newValue.copy(text = AccountIdFormat.normalizeAndTruncate(newValue.text)) else newValue + onEvent(PhoneNumberEntryScreenEvents.NationalNumberChanged(oldValue = phoneNumberTextFieldValue.text, newValue = accepted.text)) + phoneNumberTextFieldValue = accepted }, modifier = Modifier .weight(1f) diff --git a/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdError.kt b/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdError.kt index e2449c40ad..9cc770af53 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdError.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdError.kt @@ -7,9 +7,6 @@ package org.signal.registration.screens.shared /** Why the entered account ID can't be submitted. Shown beneath the text field rather than in a dialog. */ sealed interface AccountIdError { - /** More than [AccountIdFormat.ACCOUNT_ID_LENGTH] characters were entered. */ - data class TooLong(val count: Int) : AccountIdError - /** The entered text contains characters that can't appear in an account ID. */ data object Invalid : AccountIdError } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdField.kt b/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdField.kt index b080108b29..af8d499a9b 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdField.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdField.kt @@ -32,7 +32,6 @@ internal fun accountIdTextStyle(): TextStyle { @Composable internal fun AccountIdErrorText(error: AccountIdError) { when (error) { - is AccountIdError.TooLong -> Text(stringResource(R.string.AccountIdField__too_long, error.count, AccountIdFormat.ACCOUNT_ID_LENGTH)) is AccountIdError.Invalid -> Text(stringResource(R.string.AccountIdField__invalid_account_id)) } } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdFormat.kt b/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdFormat.kt index b2ed4ea390..544e45dcf3 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdFormat.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/shared/AccountIdFormat.kt @@ -28,9 +28,13 @@ internal object AccountIdFormat { /** Strips the formatting a user may have typed or pasted, leaving the raw form the account ID is stored in. */ fun normalize(input: String): String = input.replace(FORMATTING_CHARACTERS, "").lowercase() + /** [normalize]s [input] and cuts it down to [ACCOUNT_ID_LENGTH], since nothing longer than a complete ID can be entered. */ + fun normalizeAndTruncate(input: String): String = normalize(input).take(ACCOUNT_ID_LENGTH) + /** - * Reads [input] as a raw account ID, or null if it doesn't read as one. Only text that couldn't plausibly be a phone - * number qualifies: it has to be entirely hex, and either contain a letter or be longer than any E164 number. + * Reads [input] as a raw account ID, truncated to [ACCOUNT_ID_LENGTH], or null if it doesn't read as one. Only text + * that couldn't plausibly be a phone number qualifies: it has to be entirely hex, and either contain a letter or be + * longer than any E164 number. */ fun asAccountIdOrNull(input: String): String? { val raw = normalize(input) @@ -40,7 +44,7 @@ internal object AccountIdFormat { } return if (raw.any { !it.isDigit() } || raw.length > MAX_PHONE_NUMBER_DIGITS) { - raw + raw.take(ACCOUNT_ID_LENGTH) } else { null } @@ -51,10 +55,10 @@ internal object AccountIdFormat { /** Why [accountId] can't be submitted, or null if there's nothing wrong with it. A too-short ID is not an error, since the user may be mid-entry. */ fun validate(accountId: String): AccountIdError? { - return when { - accountId.length > ACCOUNT_ID_LENGTH -> AccountIdError.TooLong(accountId.length) - !containsOnlyAccountIdCharacters(accountId) -> AccountIdError.Invalid - else -> null + return if (containsOnlyAccountIdCharacters(accountId)) { + null + } else { + AccountIdError.Invalid } } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreen.kt b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreen.kt index 129ba664f5..dfa0721aca 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreen.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreen.kt @@ -76,7 +76,6 @@ import org.signal.registration.screens.OnePaneRegistrationScaffold import org.signal.registration.screens.RegistrationScaffold import org.signal.registration.screens.TwoPaneRegistrationScaffold import org.signal.registration.screens.aepentry.AepInput -import org.signal.registration.screens.aepentry.AepValidationError import org.signal.registration.screens.aepentry.AepVisualTransformation import org.signal.registration.screens.attachDebugLogHelper import org.signal.registration.screens.shared.AccountIdErrorText @@ -397,12 +396,10 @@ private fun RecoveryKeyTextField( } }, supportingText = { - val error = state.recoveryKey.error when { state.areCredentialsIncorrect && state.mode == SignalLoginCredentialEntryState.Mode.ConfirmSaved -> Text(stringResource(R.string.SignalLoginCredentialEntryScreen__that_doesnt_match_the_signal_login_you_were_shown)) state.areCredentialsIncorrect -> Text(stringResource(R.string.SignalLoginCredentialEntryScreen__incorrect_account_id_or_recovery_key)) - error is AepValidationError.TooLong -> Text(stringResource(R.string.EnterAepScreen__too_long, error.count, error.max)) - error != null -> Text(stringResource(R.string.EnterAepScreen__invalid_recovery_key)) + state.recoveryKey.error != null -> Text(stringResource(R.string.EnterAepScreen__invalid_recovery_key)) } }, isError = isError, diff --git a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenEventHandler.kt b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenEventHandler.kt index 353c685d06..b164ec531b 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenEventHandler.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenEventHandler.kt @@ -17,7 +17,7 @@ object SignalLoginCredentialEntryScreenEventHandler { fun applyEvent(state: SignalLoginCredentialEntryState, event: SignalLoginCredentialEntryScreenEvents): SignalLoginCredentialEntryState { return when (event) { is SignalLoginCredentialEntryScreenEvents.AccountIdChanged -> { - val accountId = AccountIdFormat.normalize(event.value) + val accountId = AccountIdFormat.normalizeAndTruncate(event.value) state.copy(accountId = accountId, accountIdError = AccountIdFormat.validate(accountId), isAccountIdPrefilled = false, areCredentialsIncorrect = false) } diff --git a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModel.kt b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModel.kt index 9a678f9417..7f08e0a730 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModel.kt @@ -124,7 +124,7 @@ class SignalLoginCredentialEntryViewModel( parentEventEmitter: (RegistrationFlowEvent) -> Unit, stateEmitter: (SignalLoginCredentialEntryState) -> Unit ) { - val accountId = AccountIdFormat.normalize(event.accountId).ifEmpty { state.accountId } + val accountId = AccountIdFormat.normalizeAndTruncate(event.accountId).ifEmpty { state.accountId } val filledState = state.copy( accountId = accountId, accountIdError = AccountIdFormat.validate(accountId), diff --git a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginManualSaveConfirmationViewModel.kt b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginManualSaveConfirmationViewModel.kt index 15b8d85556..ad30d48857 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginManualSaveConfirmationViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginManualSaveConfirmationViewModel.kt @@ -100,7 +100,7 @@ class SignalLoginManualSaveConfirmationViewModel( parentEventEmitter: (RegistrationFlowEvent) -> Unit, stateEmitter: (SignalLoginCredentialEntryState) -> Unit ) { - val accountId = AccountIdFormat.normalize(event.accountId).ifEmpty { state.accountId } + val accountId = AccountIdFormat.normalizeAndTruncate(event.accountId).ifEmpty { state.accountId } val filledState = state.copy( accountId = accountId, accountIdError = AccountIdFormat.validate(accountId), diff --git a/feature/registration/src/main/res/values/strings.xml b/feature/registration/src/main/res/values/strings.xml index eb8002824d..ec39d88516 100644 --- a/feature/registration/src/main/res/values/strings.xml +++ b/feature/registration/src/main/res/values/strings.xml @@ -224,8 +224,6 @@ Recovery key No recovery key? - - Too long (%1$d/%2$d) Invalid recovery key @@ -725,8 +723,6 @@ Show login info again - - Too long. %1$d/%2$d characters. Invalid account ID diff --git a/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryViewModelTest.kt index 0d580edf12..d3f9b7ce80 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberEntryViewModelTest.kt @@ -2250,7 +2250,7 @@ class PhoneNumberEntryViewModelTest { } @Test - fun `NationalNumberChanged with an over-long account ID reports the error rather than silently refusing to submit`() = runTest { + fun `NationalNumberChanged with an over-long account ID keeps only a complete ID`() = runTest { val initialState = PhoneNumberEntryState(isPhoneNumberlessRegistrationAvailable = true) viewModel.applyEvent( @@ -2260,8 +2260,9 @@ class PhoneNumberEntryViewModelTest { stateEmitter ) - assertThat(emittedStates.last().accountIdError).isEqualTo(AccountIdError.TooLong(34)) - assertThat(emittedStates.last().isNextEnabled).isFalse() + assertThat(emittedStates.last().enteredAccountId).isEqualTo("a6b284822e3283d07f2391360a4c2b91") + assertThat(emittedStates.last().accountIdError).isNull() + assertThat(emittedStates.last().isNextEnabled).isTrue() } @Test diff --git a/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberScreenTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberScreenTest.kt index 3239677648..2faeadc594 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberScreenTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/phonenumber/PhoneNumberScreenTest.kt @@ -17,7 +17,10 @@ import androidx.compose.ui.test.onNodeWithTag import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performImeAction +import androidx.compose.ui.test.performTextInput import androidx.test.core.app.ApplicationProvider +import assertk.assertThat +import assertk.assertions.hasLength import org.junit.Rule import org.junit.Test import org.junit.runner.RunWith @@ -26,7 +29,7 @@ import org.robolectric.annotation.Config import org.signal.core.ui.CoreUiDependenciesRule import org.signal.core.ui.compose.theme.SignalTheme import org.signal.registration.R -import org.signal.registration.screens.shared.AccountIdError +import org.signal.registration.screens.shared.AccountIdFormat import org.signal.registration.test.TestTags /** @@ -253,19 +256,25 @@ class PhoneNumberScreenTest { } @Test - fun `an over-long account ID says why it can't be submitted`() { + fun `account ID entry turns away anything typed past a complete ID`() { // Given + var emittedEvent: PhoneNumberEntryScreenEvents? = null + composeTestRule.setContent { SignalTheme { PhoneNumberScreen( - state = accountIdState().copy(accountIdError = AccountIdError.TooLong(34)), - onEvent = {} + state = accountIdState(), + onEvent = { emittedEvent = it } ) } } + // When + composeTestRule.onNodeWithTag(TestTags.PHONE_NUMBER_PHONE_FIELD).performTextInput("ff") + // Then - composeTestRule.onNodeWithText(context.getString(R.string.AccountIdField__too_long, 34, 32)).assertExists() + val newValue = (emittedEvent as PhoneNumberEntryScreenEvents.NationalNumberChanged).newValue + assertThat(newValue).hasLength(AccountIdFormat.ACCOUNT_ID_LENGTH) } @Test diff --git a/feature/registration/src/test/java/org/signal/registration/screens/shared/AccountIdFormatTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/shared/AccountIdFormatTest.kt index 1377058c77..fa0be2b960 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/shared/AccountIdFormatTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/shared/AccountIdFormatTest.kt @@ -22,6 +22,13 @@ class AccountIdFormatTest { assertThat(AccountIdFormat.normalize(" a6b28482 2e32 ")).isEqualTo("a6b284822e32") } + @Test + fun `normalizeAndTruncate cuts off anything past a complete ID`() { + assertThat(AccountIdFormat.normalizeAndTruncate(FULL_ID + "ff")).isEqualTo(FULL_ID) + assertThat(AccountIdFormat.normalizeAndTruncate("A6B28482-2E32-83D0-7F23-91360A4C2B91-FF")).isEqualTo(FULL_ID) + assertThat(AccountIdFormat.normalizeAndTruncate("a6b28482")).isEqualTo("a6b28482") + } + @Test fun `text containing a letter reads as an account ID at any length`() { assertThat(AccountIdFormat.asAccountIdOrNull("a")).isEqualTo("a") @@ -34,6 +41,11 @@ class AccountIdFormatTest { assertThat(AccountIdFormat.asAccountIdOrNull("1".repeat(16))).isEqualTo("1".repeat(16)) } + @Test + fun `text longer than a complete account ID reads as one, cut off at the maximum length`() { + assertThat(AccountIdFormat.asAccountIdOrNull(FULL_ID + "ff")).isEqualTo(FULL_ID) + } + @Test fun `a phone number never reads as an account ID`() { assertThat(AccountIdFormat.asAccountIdOrNull("+1 555 123 4567")).isNull() @@ -54,10 +66,9 @@ class AccountIdFormatTest { } @Test - fun `validate only complains about length and alphabet, never about being mid-entry`() { + fun `validate only complains about the alphabet, never about being mid-entry`() { assertThat(AccountIdFormat.validate("a6b28482")).isNull() assertThat(AccountIdFormat.validate(FULL_ID)).isNull() - assertThat(AccountIdFormat.validate(FULL_ID + "ff")).isEqualTo(AccountIdError.TooLong(34)) assertThat(AccountIdFormat.validate("a6b28482g")).isEqualTo(AccountIdError.Invalid) } diff --git a/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModelTest.kt index 806845a7fc..09dfa2c893 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryViewModelTest.kt @@ -162,11 +162,11 @@ class SignalLoginCredentialEntryViewModelTest { } @Test - fun `AccountIdChanged reports an over-long ID as too long`() = runTest(testDispatcher) { + fun `AccountIdChanged ignores anything typed past a complete ID`() = runTest(testDispatcher) { val state = applyAccountId(VALID_ACCOUNT_ID + "ab") - assertThat(state.accountIdError).isEqualTo(AccountIdError.TooLong(34)) - assertThat(state.isNextEnabled).isFalse() + assertThat(state.accountId).isEqualTo(VALID_ACCOUNT_ID) + assertThat(state.accountIdError).isNull() } @Test @@ -177,6 +177,15 @@ class SignalLoginCredentialEntryViewModelTest { assertThat(emittedStates.last().recoveryKey.isValid).isTrue() } + @Test + fun `RecoveryKeyChanged ignores anything typed past a complete key`() = runTest(testDispatcher) { + applyEvent(SignalLoginCredentialEntryState(), SignalLoginCredentialEntryScreenEvents.RecoveryKeyChanged(VALID_AEP + "abc")) + + assertThat(emittedStates.last().recoveryKey.normalized).isEqualTo(VALID_AEP) + assertThat(emittedStates.last().recoveryKey.error).isNull() + assertThat(emittedStates.last().recoveryKey.isValid).isTrue() + } + @Test fun `editing either half clears a rejected login`() = runTest(testDispatcher) { val rejected = completeState().copy(areCredentialsIncorrect = true)