From 74d391d95e2ce79b3c47260019e71fb913a2be3b Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Wed, 29 Jul 2026 15:45:14 -0400 Subject: [PATCH] Block creation of weak pins. --- .../screens/pincreation/PinCreationScreen.kt | 7 +- .../screens/pincreation/PinCreationState.kt | 3 +- .../pincreation/PinCreationViewModel.kt | 10 +- .../src/main/res/values/strings.xml | 2 + .../pincreation/PinCreationScreenTest.kt | 17 ++++ .../pincreation/PinCreationViewModelTest.kt | 99 ++++++++++++++++++- 6 files changed, 131 insertions(+), 7 deletions(-) diff --git a/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationScreen.kt b/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationScreen.kt index 65582bf61d..d0596211bb 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationScreen.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationScreen.kt @@ -401,7 +401,8 @@ private fun PinInputSection( isConfirm = isConfirm, isAlphanumericKeyboard = state.isAlphanumericKeyboard, isMismatch = state.pinMismatch, - matchesVerificationCode = state.pinMatchesVerificationCode + matchesVerificationCode = state.pinMatchesVerificationCode, + isTooWeak = state.pinTooWeak ) Spacer(modifier = Modifier.height(16.dp)) KeyboardToggleButton( @@ -445,14 +446,16 @@ private fun PinInputLabel( isAlphanumericKeyboard: Boolean, isMismatch: Boolean, matchesVerificationCode: Boolean, + isTooWeak: Boolean, modifier: Modifier = Modifier ) { - val isError = !isConfirm && (isMismatch || matchesVerificationCode) + val isError = !isConfirm && (isMismatch || matchesVerificationCode || isTooWeak) Text( text = when { isConfirm -> stringResource(R.string.PinCreationScreen__reenter_pin) matchesVerificationCode -> stringResource(R.string.PinCreationScreen__reentered_verification_code) + isTooWeak -> stringResource(R.string.PinCreationScreen__choose_a_stronger_pin) isMismatch -> stringResource(R.string.PinCreationScreen__pins_dont_match) isAlphanumericKeyboard -> stringResource(R.string.PinCreationScreen__pin_at_least_4_characters) else -> stringResource(R.string.PinCreationScreen__pin_at_least_4_digits) diff --git a/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationState.kt b/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationState.kt index ba0e8152a4..6279e6dcc3 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationState.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationState.kt @@ -14,6 +14,7 @@ data class PinCreationState( val isConfirmEnabled: Boolean = false, val pinMismatch: Boolean = false, val pinMatchesVerificationCode: Boolean = false, + val pinTooWeak: Boolean = false, val loading: Boolean = false, val firstPin: String? = null, val submittedVerificationCode: String? = null, @@ -21,7 +22,7 @@ data class PinCreationState( val dialogs: Dialogs = Dialogs() ) { override fun toString(): String { - return "PinCreationState(isAlphanumericKeyboard=$isAlphanumericKeyboard, isConfirmEnabled=$isConfirmEnabled, pinMismatch=$pinMismatch, pinMatchesVerificationCode=$pinMatchesVerificationCode, loading=$loading, firstPin=${firstPin?.let { "${it.length} chars" }}, submittedVerificationCode=${submittedVerificationCode?.censor()}, accountEntropyPool=${accountEntropyPool?.displayValue?.censor()}, dialogs=$dialogs)" + return "PinCreationState(isAlphanumericKeyboard=$isAlphanumericKeyboard, isConfirmEnabled=$isConfirmEnabled, pinMismatch=$pinMismatch, pinMatchesVerificationCode=$pinMatchesVerificationCode, pinTooWeak=$pinTooWeak, loading=$loading, firstPin=${firstPin?.let { "${it.length} chars" }}, submittedVerificationCode=${submittedVerificationCode?.censor()}, accountEntropyPool=${accountEntropyPool?.displayValue?.censor()}, dialogs=$dialogs)" } data class Dialogs( diff --git a/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationViewModel.kt b/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationViewModel.kt index 58e05087b4..01781a4124 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationViewModel.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/pincreation/PinCreationViewModel.kt @@ -22,6 +22,7 @@ import org.signal.registration.RegistrationFlowEvent import org.signal.registration.RegistrationFlowState import org.signal.registration.RegistrationRepository import org.signal.registration.RestoreDecision +import org.whispersystems.signalservice.api.kbs.PinValidityChecker import kotlin.time.toKotlinDuration /** @@ -66,12 +67,17 @@ class PinCreationViewModel( when { !state.isConfirmEnabled && event.pin == state.submittedVerificationCode -> { Log.w(TAG, "[PinSubmitted] User entered their verification code as their PIN. Prompting them to choose a different PIN.") - _state.value = state.copy(pinMatchesVerificationCode = true, pinMismatch = false) + _state.value = state.copy(pinMatchesVerificationCode = true, pinMismatch = false, pinTooWeak = false) + } + + !state.isConfirmEnabled && !PinValidityChecker.valid(event.pin) -> { + Log.w(TAG, "[PinSubmitted] User entered a PIN that is too common. Prompting them to choose a stronger PIN.") + _state.value = state.copy(pinTooWeak = true, pinMismatch = false, pinMatchesVerificationCode = false) } !state.isConfirmEnabled -> { Log.d(TAG, "[PinSubmitted] First PIN entered. Asking the user to confirm it.") - _state.value = state.copy(firstPin = event.pin, isConfirmEnabled = true, pinMismatch = false, pinMatchesVerificationCode = false) + _state.value = state.copy(firstPin = event.pin, isConfirmEnabled = true, pinMismatch = false, pinMatchesVerificationCode = false, pinTooWeak = false) } event.pin != state.firstPin -> { diff --git a/feature/registration/src/main/res/values/strings.xml b/feature/registration/src/main/res/values/strings.xml index 4219c48937..9cea5c7ba6 100644 --- a/feature/registration/src/main/res/values/strings.xml +++ b/feature/registration/src/main/res/values/strings.xml @@ -377,6 +377,8 @@ Re-enter PIN PINs don\'t match. Try again. + + Choose a stronger PIN You re-entered the code that was already used to verify your phone number. Choose a new and unique Signal PIN. diff --git a/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationScreenTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationScreenTest.kt index 9a629c0530..234781d01a 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationScreenTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationScreenTest.kt @@ -10,6 +10,7 @@ import androidx.compose.ui.test.assertIsDisplayed import androidx.compose.ui.test.assertIsNotEnabled import androidx.compose.ui.test.junit4.createComposeRule import androidx.compose.ui.test.onNodeWithTag +import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performTextInput import androidx.test.core.app.ApplicationProvider @@ -20,6 +21,7 @@ import org.robolectric.RobolectricTestRunner 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.test.TestTags /** @@ -101,4 +103,19 @@ class PinCreationScreenTest { assert(emittedEvent is PinCreationScreenEvents.PinSubmitted) } + + @Test + fun `when the pin is too weak, the stronger pin error is displayed`() { + composeTestRule.setContent { + SignalTheme { + PinCreationScreen( + state = PinCreationState(pinTooWeak = true), + onEvent = {} + ) + } + } + + val context = ApplicationProvider.getApplicationContext() + composeTestRule.onNodeWithText(context.getString(R.string.PinCreationScreen__choose_a_stronger_pin)).assertIsDisplayed() + } } diff --git a/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationViewModelTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationViewModelTest.kt index 6737127fdb..fdbbaf6240 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationViewModelTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/pincreation/PinCreationViewModelTest.kt @@ -82,7 +82,7 @@ class PinCreationViewModelTest { val states = collectStates() val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate()) - viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("123456")) + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("148502")) coVerify(exactly = 0) { mockRepository.setNewlyCreatedPin(any(), any(), any()) } assertThat(emittedParentEvents).hasSize(0) @@ -142,12 +142,107 @@ class PinCreationViewModelTest { val states = collectStates() val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate(), submittedVerificationCode = "123456") - viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("987654")) + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("148502")) assertThat(states.last().isConfirmEnabled).isTrue() assertThat(states.last().pinMatchesVerificationCode).isFalse() } + // ==================== Weak PIN Tests ==================== + + @Test + fun `first PinSubmitted with an ascending sequential PIN is rejected as too weak`() = runTest(testDispatcher) { + val states = collectStates() + val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate()) + + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("1234")) + + coVerify(exactly = 0) { mockRepository.setNewlyCreatedPin(any(), any(), any()) } + assertThat(emittedParentEvents).hasSize(0) + assertThat(states.last().pinTooWeak).isTrue() + assertThat(states.last().isConfirmEnabled).isFalse() + assertThat(states.last().firstPin).isNull() + } + + @Test + fun `first PinSubmitted with a descending sequential PIN is rejected as too weak`() = runTest(testDispatcher) { + val states = collectStates() + val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate()) + + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("43210")) + + assertThat(states.last().pinTooWeak).isTrue() + assertThat(states.last().isConfirmEnabled).isFalse() + } + + @Test + fun `first PinSubmitted with a repeated-digit PIN is rejected as too weak`() = runTest(testDispatcher) { + val states = collectStates() + val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate()) + + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("9999")) + + assertThat(states.last().pinTooWeak).isTrue() + assertThat(states.last().isConfirmEnabled).isFalse() + } + + @Test + fun `first PinSubmitted with a sequential non-arabic-numeral PIN is rejected as too weak`() = runTest(testDispatcher) { + val states = collectStates() + val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate()) + + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("١٢٣٤٥")) + + assertThat(states.last().pinTooWeak).isTrue() + assertThat(states.last().isConfirmEnabled).isFalse() + } + + @Test + fun `first PinSubmitted with a sequential alphanumeric PIN is allowed`() = runTest(testDispatcher) { + val states = collectStates() + val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate(), isAlphanumericKeyboard = true) + + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("abcd")) + + assertThat(states.last().pinTooWeak).isFalse() + assertThat(states.last().isConfirmEnabled).isTrue() + } + + @Test + fun `first PinSubmitted matching the verification code is reported as such rather than as too weak`() = runTest(testDispatcher) { + val states = collectStates() + val initialState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate(), submittedVerificationCode = "123456") + + viewModel.applyEvent(initialState, PinCreationScreenEvents.PinSubmitted("123456")) + + assertThat(states.last().pinMatchesVerificationCode).isTrue() + assertThat(states.last().pinTooWeak).isFalse() + } + + @Test + fun `first PinSubmitted with a strong PIN clears a previous too-weak error`() = runTest(testDispatcher) { + val states = collectStates() + val weakState = PinCreationState(accountEntropyPool = AccountEntropyPool.generate(), pinTooWeak = true) + + viewModel.applyEvent(weakState, PinCreationScreenEvents.PinSubmitted("148502")) + + assertThat(states.last().pinTooWeak).isFalse() + assertThat(states.last().isConfirmEnabled).isTrue() + } + + @Test + fun `weak PIN check is skipped on the confirmation step`() = runTest(testDispatcher) { + val aep = AccountEntropyPool.generate() + val confirmState = PinCreationState(accountEntropyPool = aep, isConfirmEnabled = true, firstPin = "148502") + + coEvery { mockRepository.setNewlyCreatedPin(any(), any(), any()) } returns RequestResult.Success(null) + + viewModel.applyEvent(confirmState, PinCreationScreenEvents.PinSubmitted("148502")) + + coVerify { mockRepository.setNewlyCreatedPin("148502", any(), any()) } + assertThat(emittedParentEvents).contains(RegistrationFlowEvent.RegistrationComplete) + } + // ==================== PinSubmitted Success Tests ==================== @Test