From 7bd004876ab22b949473a6458f4d51e5faca3103 Mon Sep 17 00:00:00 2001 From: Craig Russell Date: Thu, 23 Nov 2023 16:26:10 +0000 Subject: [PATCH] Ensure autofill disable prompt disabled no matter how a credential is saved (#3856) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task/Issue URL: https://app.asana.com/0/1203822806345703/1205939031559534/f ### Description Change the way the Autofill decline counter works to ensure it is disabled when credentials are added regardless of how they were added. Previously, this required the decline counter to be manually disabled at the point in the code that was adding a credential (e.g., when handing the result of save credential prompt) but wasn't handling the other places that a credential could be created (sync + manually created). This PR introduces a dedicated observer on the credential count that will automatically disable the decline counter if it detects one or more credentials exist. ### Steps to test this PR Logcat filter: `package:mine & (message~:"CredentialEverPopulatedObserver" | message~:"Permanently disabling Autofill decline counter" | message~:"User declined to save credentials" | message~:"Determined if decline counter should be active")` #### Decline counter disabled when manually adding a credential - [x] Fresh install (`internal`) - [x] Visit Overflow->Logins - [x] Click the ➕ and add details for a login. tap the ✔️ to save - [x] Verify you see the following in the logs: ``` Permanently disabling Autofill decline counter CredentialEverPopulatedObserver stopped ``` #### Decline counter disabled from sync - [x] Fresh install (`internal`) - [x] Go to sync, and sync saved logins from elsewhere - [x] Verify you see the following in the logs: ``` Permanently disabling Autofill decline counter CredentialEverPopulatedObserver stopped ``` #### Decline counter disabled from saving a login - [x] Fresh install (`internal`) - [x] Visit https://fill.dev/form/login-simple. Enter username and password (>= 4 chars). Tap login button - [x] Save when prompted - [x] Verify you see the following in the logs: ``` Permanently disabling Autofill decline counter CredentialEverPopulatedObserver stopped ``` #### Ensure decline counter still works - [x] Fresh install (`internal`) - [x] Visit https://fill.dev/form/login-simple. Enter username and password (>= 4 chars). Tap login button. **Don't save**. - [x] Login on another page( e.g., https://privacy-test-pages.glitch.me/autofill/form-submission.html). **Don't save.** - [x] Verify you see the prompt to "Keep saving logins" --- .../ResultHandlerSaveLoginCredentialsTest.kt | 20 ----- .../ResultHandlerSaveLoginCredentials.kt | 4 - .../AutofillDisablingDeclineCounter.kt | 29 ++++--- .../CredentialEverPopulatedObserver.kt | 68 +++++++++++++++ .../AutofillDisablingDeclineCounterTest.kt | 9 +- .../CredentialEverPopulatedObserverTest.kt | 82 +++++++++++++++++++ 6 files changed, 171 insertions(+), 41 deletions(-) create mode 100644 autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserver.kt create mode 100644 autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserverTest.kt diff --git a/app/src/test/java/com/duckduckgo/app/browser/autofill/ResultHandlerSaveLoginCredentialsTest.kt b/app/src/test/java/com/duckduckgo/app/browser/autofill/ResultHandlerSaveLoginCredentialsTest.kt index 34561daa41bc..a21885c73a76 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/autofill/ResultHandlerSaveLoginCredentialsTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/autofill/ResultHandlerSaveLoginCredentialsTest.kt @@ -27,7 +27,6 @@ import com.duckduckgo.autofill.api.domain.app.LoginCredentials import com.duckduckgo.autofill.api.store.AutofillStore import com.duckduckgo.autofill.impl.AutofillFireproofDialogSuppressor import com.duckduckgo.autofill.impl.ui.credential.saving.ResultHandlerSaveLoginCredentials -import com.duckduckgo.autofill.impl.ui.credential.saving.declines.AutofillDeclineCounter import com.duckduckgo.common.test.CoroutineTestRule import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runTest @@ -46,14 +45,12 @@ class ResultHandlerSaveLoginCredentialsTest { private val callback: AutofillEventListener = mock() private val autofillFireproofDialogSuppressor: AutofillFireproofDialogSuppressor = mock() - private val declineCounter: AutofillDeclineCounter = mock() private val autofillStore: AutofillStore = mock() private val appBuildConfig: AppBuildConfig = mock() private val testee = ResultHandlerSaveLoginCredentials( autofillFireproofDialogSuppressor = autofillFireproofDialogSuppressor, dispatchers = coroutineTestRule.testDispatcherProvider, - declineCounter = declineCounter, autofillStore = autofillStore, appBuildConfig = appBuildConfig, appCoroutineScope = coroutineTestRule.testScope, @@ -85,23 +82,6 @@ class ResultHandlerSaveLoginCredentialsTest { verify(callback).onSavedCredentials(loginCredentials) } - @Test - fun whenSaveCredentialsForFirstTimeThenDisableDeclineCountMonitoringFlag() = runTest { - val loginCredentials = LoginCredentials(domain = "example.com", username = "foo", password = "bar") - val bundle = bundle("example.com", loginCredentials) - whenever(autofillStore.saveCredentials(any(), any())).thenReturn(loginCredentials) - testee.processResult(bundle, context, "tab-id-123", Fragment(), callback) - verify(declineCounter).disableDeclineCounter() - } - - @Test - fun whenSaveCredentialsUnsuccessfulThenDoesNotDisableDeclineCountMonitoringFlag() = runTest { - val bundle = bundle("example.com", someLoginCredentials()) - whenever(autofillStore.saveCredentials(any(), any())).thenReturn(null) - testee.processResult(bundle, context, "tab-id-123", Fragment(), callback) - verify(declineCounter, never()).disableDeclineCounter() - } - private suspend fun verifySaveNeverCalled() { verify(autofillStore, never()).saveCredentials(any(), any()) } diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/ResultHandlerSaveLoginCredentials.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/ResultHandlerSaveLoginCredentials.kt index 162ccb71cace..905f7bd5989e 100644 --- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/ResultHandlerSaveLoginCredentials.kt +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/ResultHandlerSaveLoginCredentials.kt @@ -30,7 +30,6 @@ import com.duckduckgo.autofill.api.CredentialSavePickerDialog import com.duckduckgo.autofill.api.domain.app.LoginCredentials import com.duckduckgo.autofill.api.store.AutofillStore import com.duckduckgo.autofill.impl.AutofillFireproofDialogSuppressor -import com.duckduckgo.autofill.impl.ui.credential.saving.declines.AutofillDeclineCounter import com.duckduckgo.common.utils.DispatcherProvider import com.duckduckgo.di.scopes.AppScope import com.squareup.anvil.annotations.ContributesMultibinding @@ -44,7 +43,6 @@ import timber.log.Timber class ResultHandlerSaveLoginCredentials @Inject constructor( private val autofillFireproofDialogSuppressor: AutofillFireproofDialogSuppressor, private val dispatchers: DispatcherProvider, - private val declineCounter: AutofillDeclineCounter, private val autofillStore: AutofillStore, private val appBuildConfig: AppBuildConfig, @AppCoroutineScope private val appCoroutineScope: CoroutineScope, @@ -68,8 +66,6 @@ class ResultHandlerSaveLoginCredentials @Inject constructor( appCoroutineScope.launch(dispatchers.io()) { val savedCredentials = autofillStore.saveCredentials(originalUrl, selectedCredentials) if (savedCredentials != null) { - declineCounter.disableDeclineCounter() - withContext(dispatchers.main()) { autofillCallback.onSavedCredentials(savedCredentials) } diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounter.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounter.kt index c166e35d291a..dc573a142b7d 100644 --- a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounter.kt +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounter.kt @@ -31,15 +31,15 @@ import kotlinx.coroutines.withContext import timber.log.Timber /** -* Repeated prompts to use Autofill (e.g., save login credentials) might annoy a user who doesn't want to use Autofill. -* If the user has declined too many times without using it, we will prompt them to disable. -* -* This class is used to track the number of times a user has declined to use Autofill when prompted. -* It should be permanently disabled, by calling disableDeclineCounter(), when user: -* - saves a credential, or -* - chooses to disable autofill when prompted to disable autofill, or -* - chooses to keep using autofill when prompted to disable autofill -*/ + * Repeated prompts to use Autofill (e.g., save login credentials) might annoy a user who doesn't want to use Autofill. + * If the user has declined too many times without using it, we will prompt them to disable. + * + * This class is used to track the number of times a user has declined to use Autofill when prompted. + * It should be permanently disabled, by calling disableDeclineCounter(), when user: + * - saves a credential, or + * - chooses to disable autofill when prompted to disable autofill, or + * - chooses to keep using autofill when prompted to disable autofill + */ interface AutofillDeclineCounter { /** @@ -68,8 +68,7 @@ class AutofillDisablingDeclineCounter @Inject constructor( private val dispatchers: DispatcherProvider = DefaultDispatcherProvider(), ) : AutofillDeclineCounter { - @VisibleForTesting - var isActive = false + private var isActive = false /** * The previous domain for which we have recorded a decline, held in-memory only. @@ -105,6 +104,8 @@ class AutofillDisablingDeclineCounter @Inject constructor( private fun shouldRecordDecline(domain: String) = domain != currentSessionPreviousDeclinedDomain override suspend fun disableDeclineCounter() { + Timber.d("Permanently disabling Autofill decline counter") + isActive = false currentSessionPreviousDeclinedDomain = null @@ -131,10 +132,14 @@ class AutofillDisablingDeclineCounter @Inject constructor( private suspend fun determineIfDeclineCounterIsActive(): Boolean { return withContext(dispatchers.io()) { - autofillStore.autofillEnabled && autofillStore.monitorDeclineCounts && autofillStore.autofillAvailable + autofillStore.monitorDeclineCounts && autofillStore.autofillAvailable } } + fun isActive(): Boolean { + return isActive + } + companion object { private const val GLOBAL_DECLINE_COUNT_THRESHOLD = 2 } diff --git a/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserver.kt b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserver.kt new file mode 100644 index 000000000000..a01461d17e3e --- /dev/null +++ b/autofill/autofill-impl/src/main/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserver.kt @@ -0,0 +1,68 @@ +/* + * Copyright (c) 2023 DuckDuckGo + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.duckduckgo.autofill.impl.ui.credential.saving.declines + +import androidx.lifecycle.LifecycleOwner +import com.duckduckgo.app.di.AppCoroutineScope +import com.duckduckgo.app.lifecycle.MainProcessLifecycleObserver +import com.duckduckgo.autofill.api.store.AutofillStore +import com.duckduckgo.common.utils.DispatcherProvider +import com.duckduckgo.di.scopes.AppScope +import com.squareup.anvil.annotations.ContributesMultibinding +import dagger.SingleInstanceIn +import javax.inject.Inject +import kotlin.coroutines.coroutineContext +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.flow.takeWhile +import kotlinx.coroutines.job +import kotlinx.coroutines.launch +import timber.log.Timber + +@ContributesMultibinding( + scope = AppScope::class, + boundType = MainProcessLifecycleObserver::class, +) +@SingleInstanceIn(AppScope::class) +class CredentialEverPopulatedObserver @Inject constructor( + private val declineCounter: AutofillDisablingDeclineCounter, + private val autofillStore: AutofillStore, + @AppCoroutineScope private val appCoroutineScope: CoroutineScope, + private val dispatchers: DispatcherProvider, +) : MainProcessLifecycleObserver { + + override fun onCreate(owner: LifecycleOwner) { + super.onCreate(owner) + + appCoroutineScope.launch(dispatchers.io()) { + observeForCredentials() + } + } + + private suspend fun observeForCredentials() { + autofillStore.getCredentialCount() + .takeWhile { declineCounter.isActive() } + .collect { numberCredentials -> + Timber.d("CredentialEverPopulatedObserver received number of stored credentials: $numberCredentials") + + // when any credentials added, we can permanently disable the decline counter and stop observing the credential count + if (numberCredentials > 0) { + declineCounter.disableDeclineCounter() + coroutineContext.job.cancel() + } + } + } +} diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounterTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounterTest.kt index 2d37e704aca7..298a6477a5b7 100644 --- a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounterTest.kt +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/AutofillDisablingDeclineCounterTest.kt @@ -126,7 +126,7 @@ class AutofillDisablingDeclineCounterTest { @Test fun whenCounterNotActiveThenShouldNeverPromptToDisableAutofill() = runTest { initialiseDeclineCounter() - testee.isActive = false + testee.disableDeclineCounter() configureGlobalDeclineCountAtThreshold() assertFalse(testee.shouldPromptToDisableAutofill()) } @@ -135,14 +135,13 @@ class AutofillDisablingDeclineCounterTest { fun whenAutofillNotAvailableThenCounterNotActive() = runTest { whenever(autofillStore.autofillAvailable).thenReturn(false) initialiseDeclineCounter() - assertFalse(testee.isActive) + assertFalse(testee.isActive()) } @Test - fun whenAutofillNotEnabledThenCounterNotActive() = runTest { - whenever(autofillStore.autofillEnabled).thenReturn(false) + fun whenAutofillAvailableThenCounterStartsAsActive() = runTest { initialiseDeclineCounter() - assertFalse(testee.isActive) + assertTrue(testee.isActive()) } private fun configureGlobalDeclineCountAtThreshold() { diff --git a/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserverTest.kt b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserverTest.kt new file mode 100644 index 000000000000..0beecc3bbd94 --- /dev/null +++ b/autofill/autofill-impl/src/test/java/com/duckduckgo/autofill/impl/ui/credential/saving/declines/CredentialEverPopulatedObserverTest.kt @@ -0,0 +1,82 @@ +package com.duckduckgo.autofill.impl.ui.credential.saving.declines + +import androidx.lifecycle.LifecycleOwner +import com.duckduckgo.autofill.api.store.AutofillStore +import com.duckduckgo.common.test.CoroutineTestRule +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever + +@ExperimentalCoroutinesApi +class CredentialEverPopulatedObserverTest { + + @get:Rule + val coroutineTestRule: CoroutineTestRule = CoroutineTestRule() + + private val autofillStore: AutofillStore = mock() + private val credentialCountFlow = MutableSharedFlow() + + @Before + fun setup() { + runTest { + whenever(autofillStore.monitorDeclineCounts).thenReturn(true) + whenever(autofillStore.autofillAvailable).thenReturn(true) + whenever(autofillStore.getCredentialCount()).thenReturn(credentialCountFlow) + } + + declineCounter = AutofillDisablingDeclineCounter( + autofillStore = autofillStore, + appCoroutineScope = coroutineTestRule.testScope, + dispatchers = coroutineTestRule.testDispatcherProvider, + ) + + testee = CredentialEverPopulatedObserver( + declineCounter = declineCounter, + autofillStore = autofillStore, + appCoroutineScope = coroutineTestRule.testScope, + dispatchers = coroutineTestRule.testDispatcherProvider, + ) + } + + private lateinit var declineCounter: AutofillDisablingDeclineCounter + private lateinit var testee: CredentialEverPopulatedObserver + + @Test + fun whenUpdatedCredentialCountIsAbove0ThenDeclineCounterDisabled() = runTest { + testee.onCreate(mock()) + assertTrue(declineCounter.isActive()) + + simulateFlowEmissionUpdatedCredentialCount(1, this) + + assertFalse(declineCounter.isActive()) + } + + @Test + fun whenUpdatedCredentialCountIs0ThenDeclineCounterNotDisabled() = runTest { + testee.onCreate(mock()) + assertTrue(declineCounter.isActive()) + + simulateFlowEmissionUpdatedCredentialCount(credentialCount = 0, this) + + assertTrue(declineCounter.isActive()) + } + + private suspend fun simulateFlowEmissionUpdatedCredentialCount( + credentialCount: Int, + scope: TestScope, + ) { + // simulate emitting an updated credential count + scope.launch { + credentialCountFlow.emit(credentialCount) + }.join() + } +}