From 9e58a42de334ba941daf07c0c496f95fb357cef2 Mon Sep 17 00:00:00 2001 From: burakov Date: Mon, 4 Dec 2023 16:53:12 +0000 Subject: [flexiglass] Always round up the remaining throttle milliseconds. This fixes a bug where we show the user there are 0 seconds remaining before they can try to re-authenticate during throttling. Bonus: since we only store this value in the model for presenting it to the user, we should store it in seconds directly rather than converting from milliseconds when formatting the message (which happens every second during throttling). Fix: 314308690 Bug: 314757822 Test: Unit tests updated and still pass. Flag: ACONFIG com.android.systemui.scene_container DEVELOPMENT Change-Id: Ifbf4708b687c037877784b29427c56ee2f20ab37 --- .../interactor/AuthenticationInteractorTest.kt | 16 +++++----------- .../domain/interactor/BouncerInteractorTest.kt | 6 ++---- .../bouncer/ui/viewmodel/BouncerViewModelTest.kt | 9 +++++++-- .../ui/viewmodel/PasswordBouncerViewModelTest.kt | 6 +++--- .../domain/interactor/AuthenticationInteractor.kt | 3 ++- .../shared/model/AuthenticationThrottlingModel.kt | 9 ++++++--- .../bouncer/domain/interactor/BouncerInteractor.kt | 3 +-- .../bouncer/ui/viewmodel/BouncerViewModel.kt | 3 +-- .../interactor/BouncerMessageInteractorTest.kt | 20 ++++++++++---------- .../data/repository/FakeAuthenticationRepository.kt | 3 ++- 10 files changed, 39 insertions(+), 39 deletions(-) diff --git a/packages/SystemUI/multivalentTests/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractorTest.kt b/packages/SystemUI/multivalentTests/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractorTest.kt index d968c1bb54bb..661c3458574e 100644 --- a/packages/SystemUI/multivalentTests/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractorTest.kt +++ b/packages/SystemUI/multivalentTests/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractorTest.kt @@ -27,8 +27,6 @@ import com.android.systemui.authentication.shared.model.AuthenticationThrottling import com.android.systemui.coroutines.collectLastValue import com.android.systemui.scene.SceneTestUtils import com.google.common.truth.Truth.assertThat -import kotlin.time.Duration.Companion.milliseconds -import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy import kotlinx.coroutines.test.runCurrent @@ -335,7 +333,7 @@ class AuthenticationInteractorTest : SysuiTestCase() { AuthenticationThrottlingModel( failedAttemptCount = FakeAuthenticationRepository.MAX_FAILED_AUTH_TRIES_BEFORE_THROTTLING, - remainingMs = FakeAuthenticationRepository.THROTTLE_DURATION_MS, + remainingSeconds = FakeAuthenticationRepository.THROTTLE_DURATION_SECONDS, ) ) @@ -347,15 +345,12 @@ class AuthenticationInteractorTest : SysuiTestCase() { AuthenticationThrottlingModel( failedAttemptCount = FakeAuthenticationRepository.MAX_FAILED_AUTH_TRIES_BEFORE_THROTTLING, - remainingMs = FakeAuthenticationRepository.THROTTLE_DURATION_MS, + remainingSeconds = FakeAuthenticationRepository.THROTTLE_DURATION_SECONDS, ) ) // Move the clock forward to ALMOST skip the throttling, leaving one second to go: - val throttleTimeoutSec = - FakeAuthenticationRepository.THROTTLE_DURATION_MS.milliseconds.inWholeSeconds - .toInt() - repeat(throttleTimeoutSec - 1) { time -> + repeat(FakeAuthenticationRepository.THROTTLE_DURATION_SECONDS - 1) { time -> advanceTimeBy(1000) assertThat(throttling) .isEqualTo( @@ -363,9 +358,8 @@ class AuthenticationInteractorTest : SysuiTestCase() { failedAttemptCount = FakeAuthenticationRepository .MAX_FAILED_AUTH_TRIES_BEFORE_THROTTLING, - remainingMs = - ((throttleTimeoutSec - (time + 1)).seconds.inWholeMilliseconds) - .toInt(), + remainingSeconds = + FakeAuthenticationRepository.THROTTLE_DURATION_SECONDS - (time + 1), ) ) } diff --git a/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractorTest.kt b/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractorTest.kt index 04f6cd30fc32..3e3a1a947bd4 100644 --- a/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractorTest.kt +++ b/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractorTest.kt @@ -29,7 +29,6 @@ import com.android.systemui.keyguard.domain.interactor.KeyguardFaceAuthInteracto import com.android.systemui.res.R import com.android.systemui.scene.SceneTestUtils import com.google.common.truth.Truth.assertThat -import kotlin.math.ceil import kotlin.time.Duration.Companion.milliseconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy @@ -268,7 +267,7 @@ class BouncerInteractorTest : SysuiTestCase() { AuthenticationThrottlingModel( failedAttemptCount = FakeAuthenticationRepository.MAX_FAILED_AUTH_TRIES_BEFORE_THROTTLING, - remainingMs = FakeAuthenticationRepository.THROTTLE_DURATION_MS, + remainingSeconds = FakeAuthenticationRepository.THROTTLE_DURATION_SECONDS, ) ) assertTryAgainMessage( @@ -286,8 +285,7 @@ class BouncerInteractorTest : SysuiTestCase() { .toInt() ) - throttling?.remainingMs?.let { remainingMs -> - val seconds = ceil(remainingMs / 1000f).toInt() + throttling?.remainingSeconds?.let { seconds -> repeat(seconds) { time -> advanceTimeBy(1000) val remainingTimeSec = seconds - time - 1 diff --git a/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModelTest.kt b/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModelTest.kt index 75d6a007b4aa..2b64d8eca032 100644 --- a/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModelTest.kt +++ b/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModelTest.kt @@ -26,6 +26,7 @@ import com.android.systemui.flags.Flags import com.android.systemui.scene.SceneTestUtils import com.google.common.truth.Truth.assertThat import com.google.common.truth.Truth.assertWithMessage +import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.emptyFlow import kotlinx.coroutines.flow.flatMapLatest @@ -144,7 +145,9 @@ class BouncerViewModelTest : SysuiTestCase() { } assertThat(message?.isUpdateAnimated).isFalse() - throttling?.remainingMs?.let { remainingMs -> advanceTimeBy(remainingMs.toLong()) } + throttling?.remainingSeconds?.let { remainingSeconds -> + advanceTimeBy(remainingSeconds.seconds.inWholeMilliseconds) + } assertThat(message?.isUpdateAnimated).isTrue() } @@ -167,7 +170,9 @@ class BouncerViewModelTest : SysuiTestCase() { } assertThat(isInputEnabled).isFalse() - throttling?.remainingMs?.let { milliseconds -> advanceTimeBy(milliseconds.toLong()) } + throttling?.remainingSeconds?.let { remainingSeconds -> + advanceTimeBy(remainingSeconds.seconds.inWholeMilliseconds) + } assertThat(isInputEnabled).isTrue() } diff --git a/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/PasswordBouncerViewModelTest.kt b/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/PasswordBouncerViewModelTest.kt index 64f294600be2..a217d93c79ae 100644 --- a/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/PasswordBouncerViewModelTest.kt +++ b/packages/SystemUI/multivalentTests/src/com/android/systemui/bouncer/ui/viewmodel/PasswordBouncerViewModelTest.kt @@ -335,12 +335,12 @@ class PasswordBouncerViewModelTest : SysuiTestCase() { repeat(failedAttemptCount) { authenticationRepository.reportAuthenticationAttempt(false) } - val remainingTimeMs = 30_000 - authenticationRepository.setThrottleDuration(remainingTimeMs) + val remainingTimeSeconds = 30 + authenticationRepository.setThrottleDuration(remainingTimeSeconds * 1000) authenticationRepository.throttling.value = AuthenticationThrottlingModel( failedAttemptCount = failedAttemptCount, - remainingMs = remainingTimeMs, + remainingSeconds = remainingTimeSeconds, ) } else { authenticationRepository.reportAuthenticationAttempt(true) diff --git a/packages/SystemUI/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractor.kt b/packages/SystemUI/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractor.kt index 1ba0220bdae7..4e67771cba82 100644 --- a/packages/SystemUI/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/authentication/domain/interactor/AuthenticationInteractor.kt @@ -29,6 +29,7 @@ import com.android.systemui.dagger.qualifiers.Background import com.android.systemui.user.data.repository.UserRepository import com.android.systemui.util.time.SystemClock import javax.inject.Inject +import kotlin.math.ceil import kotlin.math.max import kotlin.time.Duration.Companion.seconds import kotlinx.coroutines.CoroutineDispatcher @@ -283,7 +284,7 @@ constructor( if (remainingMs > 0) { AuthenticationThrottlingModel( failedAttemptCount = failedAttemptCount.await(), - remainingMs = remainingMs.toInt(), + remainingSeconds = ceil(remainingMs / 1000f).toInt(), ) } else { null // Throttling ended. diff --git a/packages/SystemUI/src/com/android/systemui/authentication/shared/model/AuthenticationThrottlingModel.kt b/packages/SystemUI/src/com/android/systemui/authentication/shared/model/AuthenticationThrottlingModel.kt index d0d398e31859..8392528b86aa 100644 --- a/packages/SystemUI/src/com/android/systemui/authentication/shared/model/AuthenticationThrottlingModel.kt +++ b/packages/SystemUI/src/com/android/systemui/authentication/shared/model/AuthenticationThrottlingModel.kt @@ -23,10 +23,13 @@ data class AuthenticationThrottlingModel( val failedAttemptCount: Int = 0, /** - * Remaining amount of time, in milliseconds, before another authentication attempt can be done. - * If not throttling this will be `0`. + * Remaining amount of time, in seconds, before another authentication attempt can be done. If + * not throttling this will be `0`. * * This number is changed throughout the timeout. + * + * Note: this isn't precise (in milliseconds), but rounded up to ensure "at most" this amount of + * seconds remains. */ - val remainingMs: Int = 0, + val remainingSeconds: Int = 0, ) diff --git a/packages/SystemUI/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractor.kt b/packages/SystemUI/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractor.kt index 1122877929a0..677f60d405b8 100644 --- a/packages/SystemUI/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractor.kt +++ b/packages/SystemUI/src/com/android/systemui/bouncer/domain/interactor/BouncerInteractor.kt @@ -32,7 +32,6 @@ import com.android.systemui.res.R import com.android.systemui.scene.shared.flag.SceneContainerFlags import com.android.systemui.util.kotlin.pairwise import javax.inject.Inject -import kotlin.time.Duration.Companion.milliseconds import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.async import kotlinx.coroutines.flow.MutableSharedFlow @@ -259,7 +258,7 @@ constructor( throttlingModel != null -> applicationContext.getString( com.android.internal.R.string.lockscreen_too_many_failed_attempts_countdown, - throttlingModel.remainingMs.milliseconds.inWholeSeconds, + throttlingModel.remainingSeconds, ) message != null -> message else -> "" diff --git a/packages/SystemUI/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModel.kt b/packages/SystemUI/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModel.kt index 58fa85781ca1..fefc3e3411dd 100644 --- a/packages/SystemUI/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModel.kt +++ b/packages/SystemUI/src/com/android/systemui/bouncer/ui/viewmodel/BouncerViewModel.kt @@ -36,7 +36,6 @@ import com.android.systemui.user.ui.viewmodel.UserSwitcherViewModel import com.android.systemui.user.ui.viewmodel.UserViewModel import dagger.Module import dagger.Provides -import kotlin.math.ceil import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.SupervisorJob @@ -205,7 +204,7 @@ class BouncerViewModel( applicationContext.getString( authMethodViewModel.throttlingMessageId, throttling.failedAttemptCount, - ceil(throttling.remainingMs / 1000f).toInt(), + throttling.remainingSeconds, ) } else { null diff --git a/packages/SystemUI/tests/src/com/android/systemui/bouncer/domain/interactor/BouncerMessageInteractorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/bouncer/domain/interactor/BouncerMessageInteractorTest.kt index 094616f0682c..aa0d7b621793 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/bouncer/domain/interactor/BouncerMessageInteractorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/bouncer/domain/interactor/BouncerMessageInteractorTest.kt @@ -35,7 +35,7 @@ import com.android.systemui.bouncer.shared.model.BouncerMessageModel import com.android.systemui.bouncer.ui.BouncerView import com.android.systemui.classifier.FalsingCollector import com.android.systemui.coroutines.collectLastValue -import com.android.systemui.flags.FakeFeatureFlags +import com.android.systemui.flags.FakeFeatureFlagsClassic import com.android.systemui.flags.Flags import com.android.systemui.flags.SystemPropertiesHelper import com.android.systemui.keyguard.DismissCallbackRegistry @@ -62,7 +62,6 @@ import org.junit.Test import org.junit.runner.RunWith import org.mockito.ArgumentMatchers.eq import org.mockito.Mock -import org.mockito.Mockito import org.mockito.Mockito.mock import org.mockito.Mockito.verify import org.mockito.MockitoAnnotations @@ -108,17 +107,18 @@ class BouncerMessageInteractorTest : SysuiTestCase() { suspend fun TestScope.init() { userRepository.setSelectedUserInfo(PRIMARY_USER) - val featureFlags = FakeFeatureFlags().apply { set(Flags.REVAMPED_BOUNCER_MESSAGES, true) } + val featureFlags = + FakeFeatureFlagsClassic().apply { set(Flags.REVAMPED_BOUNCER_MESSAGES, true) } primaryBouncerInteractor = PrimaryBouncerInteractor( bouncerRepository, - Mockito.mock(BouncerView::class.java), - Mockito.mock(Handler::class.java), - Mockito.mock(KeyguardStateController::class.java), - Mockito.mock(KeyguardSecurityModel::class.java), - Mockito.mock(PrimaryBouncerCallbackInteractor::class.java), - Mockito.mock(FalsingCollector::class.java), - Mockito.mock(DismissCallbackRegistry::class.java), + mock(BouncerView::class.java), + mock(Handler::class.java), + mock(KeyguardStateController::class.java), + mock(KeyguardSecurityModel::class.java), + mock(PrimaryBouncerCallbackInteractor::class.java), + mock(FalsingCollector::class.java), + mock(DismissCallbackRegistry::class.java), context, keyguardUpdateMonitor, fakeTrustRepository, diff --git a/packages/SystemUI/tests/utils/src/com/android/systemui/authentication/data/repository/FakeAuthenticationRepository.kt b/packages/SystemUI/tests/utils/src/com/android/systemui/authentication/data/repository/FakeAuthenticationRepository.kt index 4fdea9713ab0..03270870bccd 100644 --- a/packages/SystemUI/tests/utils/src/com/android/systemui/authentication/data/repository/FakeAuthenticationRepository.kt +++ b/packages/SystemUI/tests/utils/src/com/android/systemui/authentication/data/repository/FakeAuthenticationRepository.kt @@ -166,7 +166,8 @@ class FakeAuthenticationRepository( AuthenticationPatternCoordinate(0, 2), ) const val MAX_FAILED_AUTH_TRIES_BEFORE_THROTTLING = 5 - const val THROTTLE_DURATION_MS = 30000 + const val THROTTLE_DURATION_SECONDS = 30 + const val THROTTLE_DURATION_MS = THROTTLE_DURATION_SECONDS * 1000 const val HINTING_PIN_LENGTH = 6 val DEFAULT_PIN = buildList { repeat(HINTING_PIN_LENGTH) { add(it + 1) } } -- cgit v1.2.3-59-g8ed1b