[flexiglass] Addresses comments from ag/23884810

1. Defaults AuthenticationRepositoryImpl.isAutoConfirmEnabled to false
   instead of true.
2. Moved hintedPinLength test cases from PinBouncerViewModelTest to
   AuthenticationInteractorTest.

Bug: 280883900
Test: All unit tests still pass
Test: Manually ran on device, made sure that the PIN bouncer works with
autoconfirm on and off.

Change-Id: I6c7b876a82ead8698723b0e53e69c7edff198fbc
This commit is contained in:
Alejandro Nijamkin
2023-07-11 16:59:24 -07:00
parent 778b918db2
commit bf7eccba44
8 changed files with 173 additions and 103 deletions

View File

@@ -71,10 +71,23 @@ interface AuthenticationRepository {
*/
val isBypassEnabled: StateFlow<Boolean>
/** Whether the auto confirm feature is enabled for the currently-selected user. */
/**
* Whether the auto confirm feature is enabled for the currently-selected user.
*
* Note that the length of the PIN is also important to take into consideration, please see
* [hintedPinLength].
*/
val isAutoConfirmEnabled: StateFlow<Boolean>
/** The length of the PIN for which we should show a hint. */
/**
* The exact length a PIN should be for us to enable PIN length hinting.
*
* A PIN that's shorter or longer than this is not eligible for the UI to render hints showing
* how many digits the current PIN is, even if [isAutoConfirmEnabled] is enabled.
*
* Note that PIN length hinting is only available if the PIN auto confirmation feature is
* available.
*/
val hintedPinLength: Int
/** Whether the pattern should be visible for the currently-selected user. */
@@ -171,10 +184,10 @@ constructor(
.stateIn(
scope = applicationScope,
started = SharingStarted.WhileSubscribed(),
initialValue = true,
initialValue = false,
)
override val hintedPinLength: Int = LockPatternUtils.MIN_AUTO_PIN_REQUIREMENT_LENGTH
override val hintedPinLength: Int = 6
override val isPatternVisible: StateFlow<Boolean> =
userRepository.selectedUserInfo

View File

@@ -37,7 +37,6 @@ import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn
import kotlinx.coroutines.launch
@@ -103,10 +102,11 @@ constructor(
/** The length of the hinted PIN, or `null` if pin length hint should not be shown. */
val hintedPinLength: StateFlow<Int?> =
flow { emit(repository.getPinLength()) }
.map { currentPinLength ->
// Hinting is enabled for 6-digit codes only
currentPinLength.takeIf { repository.hintedPinLength == it }
repository.isAutoConfirmEnabled
.map { isAutoConfirmEnabled ->
repository.getPinLength().takeIf {
isAutoConfirmEnabled && it == repository.hintedPinLength
}
}
.stateIn(
scope = applicationScope,

View File

@@ -88,7 +88,7 @@ constructor(
/** Whether the auto confirm feature is enabled for the currently-selected user. */
val isAutoConfirmEnabled: StateFlow<Boolean> = authenticationInteractor.isAutoConfirmEnabled
/** The length of the PIN for which we should show a hint. */
/** The length of the hinted PIN, or `null`, if pin length hint should not be shown. */
val hintedPinLength: StateFlow<Int?> = authenticationInteractor.hintedPinLength
/** Whether the pattern should be visible for the currently-selected user. */

View File

@@ -164,7 +164,7 @@ class AuthenticationInteractorTest : SysuiTestCase() {
testScope.runTest {
val isThrottled by collectLastValue(underTest.isThrottled)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
assertThat(underTest.authenticate(listOf(1, 2, 3, 4))).isTrue()
assertThat(underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)).isTrue()
assertThat(isThrottled).isFalse()
}
@@ -172,7 +172,7 @@ class AuthenticationInteractorTest : SysuiTestCase() {
fun authenticate_withIncorrectPin_returnsFalse() =
testScope.runTest {
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
assertThat(underTest.authenticate(listOf(9, 8, 7))).isFalse()
assertThat(underTest.authenticate(listOf(9, 8, 7, 6, 5, 4))).isFalse()
}
@Test(expected = IllegalArgumentException::class)
@@ -270,7 +270,15 @@ class AuthenticationInteractorTest : SysuiTestCase() {
val isThrottled by collectLastValue(underTest.isThrottled)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(underTest.authenticate(listOf(1, 2, 3), tryAutoConfirm = true)).isNull()
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN.toMutableList().apply {
removeLast()
},
tryAutoConfirm = true
)
)
.isNull()
assertThat(isThrottled).isFalse()
}
@@ -280,7 +288,13 @@ class AuthenticationInteractorTest : SysuiTestCase() {
val isUnlocked by collectLastValue(underTest.isUnlocked)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(underTest.authenticate(listOf(1, 2, 4, 4), tryAutoConfirm = true)).isFalse()
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN.map { it + 1 },
tryAutoConfirm = true
)
)
.isFalse()
assertThat(isUnlocked).isFalse()
}
@@ -290,7 +304,12 @@ class AuthenticationInteractorTest : SysuiTestCase() {
val isUnlocked by collectLastValue(underTest.isUnlocked)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(underTest.authenticate(listOf(1, 2, 3, 4, 5), tryAutoConfirm = true))
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN + listOf(7),
tryAutoConfirm = true
)
)
.isFalse()
assertThat(isUnlocked).isFalse()
}
@@ -301,7 +320,13 @@ class AuthenticationInteractorTest : SysuiTestCase() {
val isUnlocked by collectLastValue(underTest.isUnlocked)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(underTest.authenticate(listOf(1, 2, 3, 4), tryAutoConfirm = true)).isTrue()
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN,
tryAutoConfirm = true
)
)
.isTrue()
assertThat(isUnlocked).isTrue()
}
@@ -311,7 +336,13 @@ class AuthenticationInteractorTest : SysuiTestCase() {
val isUnlocked by collectLastValue(underTest.isUnlocked)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(false)
assertThat(underTest.authenticate(listOf(1, 2, 3, 4), tryAutoConfirm = true)).isNull()
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN,
tryAutoConfirm = true
)
)
.isNull()
assertThat(isUnlocked).isFalse()
}
@@ -334,7 +365,7 @@ class AuthenticationInteractorTest : SysuiTestCase() {
val throttling by collectLastValue(underTest.throttling)
val isThrottled by collectLastValue(underTest.isThrottled)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
underTest.authenticate(listOf(1, 2, 3, 4))
underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)
assertThat(isUnlocked).isTrue()
assertThat(isThrottled).isFalse()
assertThat(throttling).isEqualTo(AuthenticationThrottlingModel())
@@ -366,7 +397,7 @@ class AuthenticationInteractorTest : SysuiTestCase() {
)
// Correct PIN, but throttled, so doesn't attempt it:
assertThat(underTest.authenticate(listOf(1, 2, 3, 4))).isNull()
assertThat(underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)).isNull()
assertThat(isUnlocked).isFalse()
assertThat(isThrottled).isTrue()
assertThat(throttling)
@@ -412,9 +443,62 @@ class AuthenticationInteractorTest : SysuiTestCase() {
)
// Correct PIN and no longer throttled so unlocks successfully:
assertThat(underTest.authenticate(listOf(1, 2, 3, 4))).isTrue()
assertThat(underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)).isTrue()
assertThat(isUnlocked).isTrue()
assertThat(isThrottled).isFalse()
assertThat(throttling).isEqualTo(AuthenticationThrottlingModel())
}
@Test
fun hintedPinLength_withoutAutoConfirm_isNull() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(false)
assertThat(hintedPinLength).isNull()
}
@Test
fun hintedPinLength_withAutoConfirmPinTooShort_isNull() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.overrideCredential(
buildList {
repeat(utils.authenticationRepository.hintedPinLength - 1) { add(it + 1) }
}
)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(hintedPinLength).isNull()
}
@Test
fun hintedPinLength_withAutoConfirmPinAtRightLength_isSameLength() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
utils.authenticationRepository.overrideCredential(
buildList { repeat(utils.authenticationRepository.hintedPinLength) { add(it + 1) } }
)
assertThat(hintedPinLength).isEqualTo(utils.authenticationRepository.hintedPinLength)
}
@Test
fun hintedPinLength_withAutoConfirmPinTooLong_isNull() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.overrideCredential(
buildList {
repeat(utils.authenticationRepository.hintedPinLength + 1) { add(it + 1) }
}
)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(hintedPinLength).isNull()
}
}

View File

@@ -94,7 +94,7 @@ class BouncerInteractorTest : SysuiTestCase() {
assertThat(message).isEqualTo(MESSAGE_ENTER_YOUR_PIN)
// Correct input.
assertThat(underTest.authenticate(listOf(1, 2, 3, 4))).isTrue()
assertThat(underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)).isTrue()
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Gone))
}
@@ -118,13 +118,20 @@ class BouncerInteractorTest : SysuiTestCase() {
assertThat(message).isEmpty()
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
// Wrong 4-digit pin
assertThat(underTest.authenticate(listOf(1, 2, 3, 5), tryAutoConfirm = true)).isFalse()
// Wrong 6-digit pin
assertThat(underTest.authenticate(listOf(1, 2, 3, 5, 5, 6), tryAutoConfirm = true))
.isFalse()
assertThat(message).isEqualTo(MESSAGE_WRONG_PIN)
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
// Correct input.
assertThat(underTest.authenticate(listOf(1, 2, 3, 4), tryAutoConfirm = true)).isTrue()
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN,
tryAutoConfirm = true
)
)
.isTrue()
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Gone))
}
@@ -147,7 +154,13 @@ class BouncerInteractorTest : SysuiTestCase() {
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
// Correct input.
assertThat(underTest.authenticate(listOf(1, 2, 3, 4), tryAutoConfirm = true)).isNull()
assertThat(
underTest.authenticate(
FakeAuthenticationRepository.DEFAULT_PIN,
tryAutoConfirm = true
)
)
.isNull()
assertThat(message).isEmpty()
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
}
@@ -309,7 +322,7 @@ class BouncerInteractorTest : SysuiTestCase() {
)
// Correct PIN, but throttled, so doesn't change away from the bouncer scene:
assertThat(underTest.authenticate(listOf(1, 2, 3, 4))).isNull()
assertThat(underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)).isNull()
assertThat(currentScene?.key).isEqualTo(SceneKey.Bouncer)
assertTryAgainMessage(
message,
@@ -339,7 +352,7 @@ class BouncerInteractorTest : SysuiTestCase() {
assertThat(currentScene?.key).isEqualTo(SceneKey.Bouncer)
// Correct PIN and no longer throttled so changes to the Gone scene:
assertThat(underTest.authenticate(listOf(1, 2, 3, 4))).isTrue()
assertThat(underTest.authenticate(FakeAuthenticationRepository.DEFAULT_PIN)).isTrue()
assertThat(currentScene?.key).isEqualTo(SceneKey.Gone)
assertThat(isThrottled).isFalse()
assertThat(throttling).isEqualTo(AuthenticationThrottlingModel())

View File

@@ -18,6 +18,7 @@ package com.android.systemui.bouncer.ui.viewmodel
import androidx.test.filters.SmallTest
import com.android.systemui.SysuiTestCase
import com.android.systemui.authentication.data.repository.FakeAuthenticationRepository
import com.android.systemui.authentication.shared.model.AuthenticationMethodModel
import com.android.systemui.coroutines.collectLastValue
import com.android.systemui.scene.SceneTestUtils
@@ -60,10 +61,9 @@ class AuthMethodBouncerViewModelTest : SysuiTestCase() {
assertThat(animateFailure).isFalse()
// Wrong PIN:
underTest.onPinButtonClicked(3)
underTest.onPinButtonClicked(4)
underTest.onPinButtonClicked(5)
underTest.onPinButtonClicked(6)
FakeAuthenticationRepository.DEFAULT_PIN.drop(2).forEach { digit ->
underTest.onPinButtonClicked(digit)
}
underTest.onAuthenticateButtonClicked()
assertThat(animateFailure).isTrue()
@@ -71,10 +71,9 @@ class AuthMethodBouncerViewModelTest : SysuiTestCase() {
assertThat(animateFailure).isFalse()
// Correct PIN:
underTest.onPinButtonClicked(1)
underTest.onPinButtonClicked(2)
underTest.onPinButtonClicked(3)
underTest.onPinButtonClicked(4)
FakeAuthenticationRepository.DEFAULT_PIN.forEach { digit ->
underTest.onPinButtonClicked(digit)
}
underTest.onAuthenticateButtonClicked()
assertThat(animateFailure).isFalse()
}

View File

@@ -19,6 +19,7 @@ package com.android.systemui.bouncer.ui.viewmodel
import androidx.test.filters.SmallTest
import com.android.systemui.R
import com.android.systemui.SysuiTestCase
import com.android.systemui.authentication.data.repository.FakeAuthenticationRepository
import com.android.systemui.authentication.shared.model.AuthenticationMethodModel
import com.android.systemui.coroutines.collectLastValue
import com.android.systemui.scene.SceneTestUtils
@@ -211,10 +212,9 @@ class PinBouncerViewModelTest : SysuiTestCase() {
)
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
underTest.onShown()
underTest.onPinButtonClicked(1)
underTest.onPinButtonClicked(2)
underTest.onPinButtonClicked(3)
underTest.onPinButtonClicked(4)
FakeAuthenticationRepository.DEFAULT_PIN.forEach { digit ->
underTest.onPinButtonClicked(digit)
}
underTest.onAuthenticateButtonClicked()
@@ -275,10 +275,9 @@ class PinBouncerViewModelTest : SysuiTestCase() {
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
// Enter the correct PIN:
underTest.onPinButtonClicked(1)
underTest.onPinButtonClicked(2)
underTest.onPinButtonClicked(3)
underTest.onPinButtonClicked(4)
FakeAuthenticationRepository.DEFAULT_PIN.forEach { digit ->
underTest.onPinButtonClicked(digit)
}
assertThat(message?.text).isEmpty()
underTest.onAuthenticateButtonClicked()
@@ -300,10 +299,9 @@ class PinBouncerViewModelTest : SysuiTestCase() {
)
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
underTest.onShown()
underTest.onPinButtonClicked(1)
underTest.onPinButtonClicked(2)
underTest.onPinButtonClicked(3)
underTest.onPinButtonClicked(4)
FakeAuthenticationRepository.DEFAULT_PIN.forEach { digit ->
underTest.onPinButtonClicked(digit)
}
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Gone))
}
@@ -324,10 +322,12 @@ class PinBouncerViewModelTest : SysuiTestCase() {
)
assertThat(currentScene).isEqualTo(SceneModel(SceneKey.Bouncer))
underTest.onShown()
underTest.onPinButtonClicked(1)
underTest.onPinButtonClicked(2)
underTest.onPinButtonClicked(3)
underTest.onPinButtonClicked(5) // PIN is now wrong!
FakeAuthenticationRepository.DEFAULT_PIN.dropLast(1).forEach { digit ->
underTest.onPinButtonClicked(digit)
}
underTest.onPinButtonClicked(
FakeAuthenticationRepository.DEFAULT_PIN.last() + 1
) // PIN is now wrong!
assertThat(entries).hasSize(0)
assertThat(message?.text).isEqualTo(WRONG_PIN)
@@ -386,47 +386,6 @@ class PinBouncerViewModelTest : SysuiTestCase() {
assertThat(confirmButtonAppearance).isEqualTo(ActionButtonAppearance.Hidden)
}
@Test
fun hintedPinLength_withoutAutoConfirm_isNull() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(false)
assertThat(hintedPinLength).isNull()
}
@Test
fun hintedPinLength_withAutoConfirmPinLessThanSixDigits_isNull() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(hintedPinLength).isNull()
}
@Test
fun hintedPinLength_withAutoConfirmPinExactlySixDigits_isSix() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
utils.authenticationRepository.overrideCredential(listOf(1, 2, 3, 4, 5, 6))
assertThat(hintedPinLength).isEqualTo(6)
}
@Test
fun hintedPinLength_withAutoConfirmPinMoreThanSixDigits_isNull() =
testScope.runTest {
val hintedPinLength by collectLastValue(underTest.hintedPinLength)
utils.authenticationRepository.setAuthenticationMethod(AuthenticationMethodModel.Pin)
utils.authenticationRepository.setAutoConfirmEnabled(true)
assertThat(hintedPinLength).isNull()
}
companion object {
private const val ENTER_YOUR_PIN = "Enter your pin"
private const val WRONG_PIN = "Wrong pin"

View File

@@ -40,7 +40,7 @@ class FakeAuthenticationRepository(
private val _isUnlocked = MutableStateFlow(false)
override val isUnlocked: StateFlow<Boolean> = _isUnlocked.asStateFlow()
override val hintedPinLength: Int = 6
override val hintedPinLength: Int = HINTING_PIN_LENGTH
private val _isPatternVisible = MutableStateFlow(true)
override val isPatternVisible: StateFlow<Boolean> = _isPatternVisible.asStateFlow()
@@ -82,7 +82,7 @@ class FakeAuthenticationRepository(
}
override suspend fun getPinLength(): Int {
return (credentialOverride ?: listOf(1, 2, 3, 4)).size
return (credentialOverride ?: DEFAULT_PIN).size
}
override fun setBypassEnabled(isBypassEnabled: Boolean) {
@@ -148,6 +148,15 @@ class FakeAuthenticationRepository(
}
}
private fun getExpectedCredential(securityMode: SecurityMode): List<Any> {
return when (val credentialType = getCurrentCredentialType(securityMode)) {
LockPatternUtils.CREDENTIAL_TYPE_PIN -> credentialOverride ?: DEFAULT_PIN
LockPatternUtils.CREDENTIAL_TYPE_PASSWORD -> "password".toList()
LockPatternUtils.CREDENTIAL_TYPE_PATTERN -> PATTERN.toCells()
else -> error("Unsupported credential type $credentialType!")
}
}
companion object {
val DEFAULT_AUTHENTICATION_METHOD = AuthenticationMethodModel.Pin
val PATTERN =
@@ -162,6 +171,8 @@ class FakeAuthenticationRepository(
)
const val MAX_FAILED_AUTH_TRIES_BEFORE_THROTTLING = 5
const val THROTTLE_DURATION_MS = 30000
const val HINTING_PIN_LENGTH = 6
val DEFAULT_PIN = buildList { repeat(HINTING_PIN_LENGTH) { add(it + 1) } }
private fun AuthenticationMethodModel.toSecurityMode(): SecurityMode {
return when (this) {
@@ -188,15 +199,6 @@ class FakeAuthenticationRepository(
}
}
private fun getExpectedCredential(securityMode: SecurityMode): List<Any> {
return when (val credentialType = getCurrentCredentialType(securityMode)) {
LockPatternUtils.CREDENTIAL_TYPE_PIN -> listOf(1, 2, 3, 4)
LockPatternUtils.CREDENTIAL_TYPE_PASSWORD -> "password".toList()
LockPatternUtils.CREDENTIAL_TYPE_PATTERN -> PATTERN.toCells()
else -> error("Unsupported credential type $credentialType!")
}
}
private fun LockscreenCredential.matches(expectedCredential: List<Any>): Boolean {
@Suppress("UNCHECKED_CAST")
return when {