From 116bf6cd0923629fd67ec00ff2445112fa950012 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Wed, 2 Sep 2026 12:44:17 +0000 Subject: [PATCH] Don't let a prefilled account ID suppress the password manager prompt. --- .../SignalLoginCredentialEntryScreen.kt | 7 ++- .../SignalLoginCredentialEntryState.kt | 8 +++- .../SignalLoginCredentialEntryViewModel.kt | 12 +++-- .../SignalLoginCredentialEntryScreenTest.kt | 22 +++++++++ ...SignalLoginCredentialEntryViewModelTest.kt | 48 +++++++++++++++++++ 5 files changed, 89 insertions(+), 8 deletions(-) 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 f78dd029f2..dacc38fedb 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 @@ -252,8 +252,8 @@ private fun CredentialTextFields( /** * Builds a modifier that prompts the password manager the first time either credential field is tapped, so a saved - * login can fill both halves at once. Only fires while the fields are still empty, and only once per screen so a - * dismissed prompt doesn't keep coming back. + * login can fill both halves at once. Only fires while the user hasn't filled anything in themselves, and only once + * per screen so a dismissed prompt doesn't keep coming back. */ @Composable private fun passwordManagerPromptOnFocus( @@ -265,8 +265,7 @@ private fun passwordManagerPromptOnFocus( var hasPrompted by rememberSaveable { mutableStateOf(false) } return Modifier.onFocusChanged { focusState -> - val fieldsAreEmpty = state.accountId.isEmpty() && state.recoveryKey.enteredText.isEmpty() - if (focusState.isFocused && !hasPrompted && fieldsAreEmpty && SignalCredentialManager.isSupported(context)) { + if (focusState.isFocused && !hasPrompted && state.canPromptPasswordManager && SignalCredentialManager.isSupported(context)) { hasPrompted = true coroutineScope.launch { val credential = SignalCredentialManager.getCredential(context) diff --git a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryState.kt b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryState.kt index be25139da3..6aeb12e032 100644 --- a/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryState.kt +++ b/feature/registration/src/main/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryState.kt @@ -20,6 +20,8 @@ import org.signal.registration.screens.shared.AccountIdFormat data class SignalLoginCredentialEntryState( val accountId: String = "", val accountIdError: AccountIdError? = null, + /** Whether [accountId] was handed to the screen by the flow rather than typed by the user, and so isn't user input. */ + val isAccountIdPrefilled: Boolean = false, val recoveryKey: AepInput = AepInput(), /** Whether the recovery key is spelled out rather than masked like a password. */ val isRecoveryKeyRevealed: Boolean = false, @@ -38,7 +40,11 @@ data class SignalLoginCredentialEntryState( !areCredentialsIncorrect && !isLoggingIn - override fun toString(): String = "SignalLoginCredentialEntryState(accountId=${accountId.censor()}, accountIdError=$accountIdError, recoveryKey=$recoveryKey, isRecoveryKeyRevealed=$isRecoveryKeyRevealed, areCredentialsIncorrect=$areCredentialsIncorrect, isLoggingIn=$isLoggingIn, loginError=$loginError)" + /** Allow prompting if it's empty or came pre-filled */ + val canPromptPasswordManager: Boolean + get() = recoveryKey.enteredText.isEmpty() && (accountId.isEmpty() || isAccountIdPrefilled) + + override fun toString(): String = "SignalLoginCredentialEntryState(accountId=${accountId.censor()}, accountIdError=$accountIdError, isAccountIdPrefilled=$isAccountIdPrefilled, recoveryKey=$recoveryKey, isRecoveryKeyRevealed=$isRecoveryKeyRevealed, areCredentialsIncorrect=$areCredentialsIncorrect, isLoggingIn=$isLoggingIn, loginError=$loginError)" } /** A login failure that the text fields can't express, so it gets a dialog instead. */ 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 6f43f614c5..8db76682be 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 @@ -49,7 +49,12 @@ class SignalLoginCredentialEntryViewModel( private val TAG = Log.tag(SignalLoginCredentialEntryViewModel::class) } - private val _state = MutableStateFlow(SignalLoginCredentialEntryState(accountId = prefilledAccountId ?: "")) + private val _state = MutableStateFlow( + SignalLoginCredentialEntryState( + accountId = prefilledAccountId ?: "", + isAccountIdPrefilled = !prefilledAccountId.isNullOrEmpty() + ) + ) val state: StateFlow = _state.asStateFlow() private val _actions = Channel(Channel.BUFFERED) @@ -79,7 +84,7 @@ class SignalLoginCredentialEntryViewModel( is SignalLoginCredentialEntryScreenEvents.AccountIdChanged -> { val accountId = AccountIdFormat.normalize(event.value) - stateEmitter(state.copy(accountId = accountId, accountIdError = AccountIdFormat.validate(accountId), areCredentialsIncorrect = false)) + stateEmitter(state.copy(accountId = accountId, accountIdError = AccountIdFormat.validate(accountId), isAccountIdPrefilled = false, areCredentialsIncorrect = false)) } is SignalLoginCredentialEntryScreenEvents.RecoveryKeyChanged -> { @@ -126,10 +131,11 @@ class SignalLoginCredentialEntryViewModel( parentEventEmitter: (RegistrationFlowEvent) -> Unit, stateEmitter: (SignalLoginCredentialEntryState) -> Unit ) { - val accountId = AccountIdFormat.normalize(event.accountId) + val accountId = AccountIdFormat.normalize(event.accountId).ifEmpty { state.accountId } val filledState = state.copy( accountId = accountId, accountIdError = AccountIdFormat.validate(accountId), + isAccountIdPrefilled = false, recoveryKey = AepInput.from(event.recoveryKey), areCredentialsIncorrect = false ) diff --git a/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenTest.kt b/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenTest.kt index 74c9a65a1a..63caaa4172 100644 --- a/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenTest.kt +++ b/feature/registration/src/test/java/org/signal/registration/screens/signallogincredentials/SignalLoginCredentialEntryScreenTest.kt @@ -83,6 +83,28 @@ class SignalLoginCredentialEntryScreenTest { coVerify(exactly = 0) { SignalCredentialManager.getCredential(any()) } } + @Test + fun `when a field is tapped with only a prefilled account ID, the password manager is still prompted`() { + stubPasswordManager() + setContent(SignalLoginCredentialEntryState(accountId = VALID_ACCOUNT_ID, isAccountIdPrefilled = true)) + + composeTestRule.onNodeWithTag(TestTags.SIGNAL_LOGIN_CREDENTIAL_ACCOUNT_ID_FIELD).performClick() + composeTestRule.waitForIdle() + + assertThat(events).contains(SignalLoginCredentialEntryScreenEvents.PasswordManagerCredentialSelected(accountId = VALID_ACCOUNT_ID, recoveryKey = VALID_RECOVERY_KEY)) + } + + @Test + fun `when a field is tapped with a user-typed account ID, the password manager is not prompted`() { + stubPasswordManager() + setContent(SignalLoginCredentialEntryState(accountId = VALID_ACCOUNT_ID)) + + composeTestRule.onNodeWithTag(TestTags.SIGNAL_LOGIN_CREDENTIAL_ACCOUNT_ID_FIELD).performClick() + composeTestRule.waitForIdle() + + coVerify(exactly = 0) { SignalCredentialManager.getCredential(any()) } + } + @Test fun `the account ID field is tagged for autofill as the username`() { setContent(SignalLoginCredentialEntryState()) 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 f248168797..0daf06e4af 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 @@ -95,6 +95,41 @@ class SignalLoginCredentialEntryViewModelTest { assertThat(prefilled.state.value.accountId).isEqualTo(VALID_ACCOUNT_ID) } + @Test + fun `a prefilled account ID does not count as user input, so the password manager can still be offered`() = runTest(testDispatcher) { + val prefilled = SignalLoginCredentialEntryViewModel(repository = mockRepository, parentEventEmitter = parentEventEmitter, prefilledAccountId = VALID_ACCOUNT_ID) + + assertThat(prefilled.state.value.isAccountIdPrefilled).isTrue() + assertThat(prefilled.state.value.canPromptPasswordManager).isTrue() + } + + @Test + fun `without a prefilled account ID the screen starts empty and the password manager can be offered`() = runTest(testDispatcher) { + assertThat(viewModel.state.value.isAccountIdPrefilled).isFalse() + assertThat(viewModel.state.value.canPromptPasswordManager).isTrue() + } + + @Test + fun `an account ID the user typed themselves stops the password manager from being offered`() = runTest(testDispatcher) { + applyEvent( + SignalLoginCredentialEntryState(accountId = VALID_ACCOUNT_ID, isAccountIdPrefilled = true), + SignalLoginCredentialEntryScreenEvents.AccountIdChanged("a6b28482") + ) + + assertThat(emittedStates.last().isAccountIdPrefilled).isFalse() + assertThat(emittedStates.last().canPromptPasswordManager).isFalse() + } + + @Test + fun `an entered recovery key stops the password manager from being offered`() = runTest(testDispatcher) { + applyEvent( + SignalLoginCredentialEntryState(accountId = VALID_ACCOUNT_ID, isAccountIdPrefilled = true), + SignalLoginCredentialEntryScreenEvents.RecoveryKeyChanged("uy38") + ) + + assertThat(emittedStates.last().canPromptPasswordManager).isFalse() + } + @Test fun `BackClicked navigates back`() = runTest(testDispatcher) { applyEvent(SignalLoginCredentialEntryState(), SignalLoginCredentialEntryScreenEvents.BackClicked) @@ -191,6 +226,19 @@ class SignalLoginCredentialEntryViewModelTest { coVerify(exactly = 0) { mockRepository.reRegisterAccountWithoutPhoneNumber(any(), any(), any(), any(), any()) } } + @Test + fun `PasswordManagerCredentialSelected keeps a prefilled account ID when the credential has no username`() = runTest(testDispatcher) { + stubSuccessfulLogin(AccountEntropyPool(VALID_AEP)) + + applyEvent( + SignalLoginCredentialEntryState(accountId = VALID_ACCOUNT_ID, isAccountIdPrefilled = true), + SignalLoginCredentialEntryScreenEvents.PasswordManagerCredentialSelected(accountId = "", recoveryKey = VALID_AEP) + ) + + assertThat(emittedStates.first().accountId).isEqualTo(VALID_ACCOUNT_ID) + coVerify { mockRepository.reRegisterAccountWithoutPhoneNumber(any(), any(), any(), any(), any()) } + } + @Test fun `PasswordManagerCredentialSelected clears a previously rejected login`() = runTest(testDispatcher) { stubSuccessfulLogin(AccountEntropyPool(VALID_AEP))