Fix Disconnect/re-connect from notification when both VPN and AppTP a…

…re enabled (#5154) Task/Issue URL: https://app.asana.com/0/488551667048375/1208536273997127/f ### Description Fix bug when both AppTP and VPN are enabled and user `disconnects` from the notification. When that happens two notifications are showed with CTAs to re-enable AppTP and VPN resp. Expected: * AppTP enable enables just ApptP * VPN enable just enables VPN Actual * either ApptP or VPN enable enable both ### Steps to test this PR _Test Fix_ - [x] Enable both AppTP and VPN - [x] In the combined AppTP/VPN notification press `Disconnect` - [x] Verify two notifications, for AppTP and VPN are shown - [x] Click on AppTP notification, enable CTA - [x] verify only AppTP is enabled - [x] Click on VPN notification, enable CTA - [x] verify only VPN is enabled - [x] repeat this test in the inverse order, before VPN then AppTP _Test combined notification_ - [x] After doing _Test Fix_ above - [x] open apps with trackers - [x] verify AppTP blocks trackers and combined notification works as expected - [x] smoke test _Test AppTP_ - [x] smoke test AppTP only _Test VPN_ - [x] smoke test VPN only
duckduckgo · Oct 23, 2024 · a0f65ad · a0f65ad
1 parent f6be916
commit a0f65ad
Show file tree

Hide file tree

Showing 11 changed files with 100 additions and 76 deletions.
diff --git a/app-tracking-protection/vpn-api/src/main/java/com/duckduckgo/mobile/android/vpn/Vpn.kt b/app-tracking-protection/vpn-api/src/main/java/com/duckduckgo/mobile/android/vpn/Vpn.kt
@@ -26,7 +26,15 @@ interface Vpn {
     suspend fun start()
 
     /**
-     * Disable the device VPN by stopping the VPN service
+     * Pauses the VPN tunnel.
+     * All features that were registered to use the VPN tunnel (eg. AppTP, NetP) continue to be registered and so a subsequent
+     * [start] call will re-enable them all
+     */
+    suspend fun pause()
+
+    /**
+     * Stops the VPN tunnel AND all features registered to use the VPN tunnel (eg. AppTP, NetP). A subsequent call to [start]
+     * will not re-start the VPN because no feature would be registered.
      */
     suspend fun stop()
 

diff --git a/...ction/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/VpnFeaturesRegistryImpl.kt b/...ction/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/VpnFeaturesRegistryImpl.kt
@@ -21,20 +21,33 @@ import android.content.SharedPreferences
 import androidx.core.content.edit
 import com.duckduckgo.common.utils.DispatcherProvider
 import com.duckduckgo.data.store.api.SharedPreferencesProvider
+import com.duckduckgo.di.scopes.AppScope
 import com.duckduckgo.mobile.android.vpn.service.TrackerBlockingVpnService
+import com.squareup.anvil.annotations.ContributesBinding
+import dagger.SingleInstanceIn
 import java.util.UUID
+import javax.inject.Inject
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.withContext
 import logcat.logcat
 
 private const val PREFS_FILENAME = "com.duckduckgo.mobile.android.vpn.feature.registry.v1"
 private const val IS_INITIALIZED = "IS_INITIALIZED"
 
-internal class VpnFeaturesRegistryImpl(
+@ContributesBinding(
+    scope = AppScope::class,
+    boundType = VpnFeaturesRegistry::class,
+)
+@ContributesBinding(
+    scope = AppScope::class,
+    boundType = Vpn::class,
+)
+@SingleInstanceIn(AppScope::class)
+class VpnFeaturesRegistryImpl @Inject constructor(
     private val vpnServiceWrapper: VpnServiceWrapper,
     private val sharedPreferencesProvider: SharedPreferencesProvider,
     private val dispatcherProvider: DispatcherProvider,
-) : VpnFeaturesRegistry {
+) : VpnFeaturesRegistry, Vpn {
 
     private val mutex = Mutex()
 
@@ -103,6 +116,34 @@ internal class VpnFeaturesRegistryImpl(
     private fun registeredFeatures(): Map<String, Any?> {
         return preferences.all.filter { it.key != IS_INITIALIZED }
     }
+
+    override suspend fun start() = withContext(dispatcherProvider.io()) {
+        vpnServiceWrapper.startService()
+    }
+
+    override suspend fun pause() {
+        vpnServiceWrapper.stopService()
+    }
+
+    override suspend fun stop() {
+        try {
+            mutex.lock()
+            // unregister all features
+            getRegisteredFeatures().onEach {
+                preferences.edit(commit = true) {
+                    remove(it.featureName)
+                }
+            }
+            // stop VPN
+            vpnServiceWrapper.stopService()
+        } finally {
+            mutex.unlock()
+        }
+    }
+
+    override suspend fun snooze(triggerAtMillis: Long) {
+        vpnServiceWrapper.snoozeService(triggerAtMillis)
+    }
 }
 
 /**
@@ -111,10 +152,9 @@ internal class VpnFeaturesRegistryImpl(
  *
  * The class is marked as open to be able to mock it in tests.
  */
-internal open class VpnServiceWrapper(
+open class VpnServiceWrapper @Inject constructor(
     private val context: Context,
-    private val dispatcherProvider: DispatcherProvider,
-) : Vpn {
+) {
     open fun restartVpnService(forceRestart: Boolean) {
         TrackerBlockingVpnService.restartVpnService(context, forceRestart = forceRestart)
     }
@@ -127,19 +167,11 @@ internal open class VpnServiceWrapper(
         TrackerBlockingVpnService.startService(context)
     }
 
-    open fun isServiceRunning(): Boolean {
-        return TrackerBlockingVpnService.isServiceRunning(context)
-    }
-
-    override suspend fun start() = withContext(dispatcherProvider.io()) {
-        startService()
-    }
-
-    override suspend fun stop() = withContext(dispatcherProvider.io()) {
-        stopService()
+    open fun snoozeService(triggerAtMillis: Long) {
+        TrackerBlockingVpnService.snoozeService(context, triggerAtMillis)
     }
 
-    override suspend fun snooze(triggerAtMillis: Long) {
-        TrackerBlockingVpnService.snoozeService(context, triggerAtMillis)
+    open fun isServiceRunning(): Boolean {
+        return TrackerBlockingVpnService.isServiceRunning(context)
     }
 }
diff --git a/...ng-protection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/di/VpnAppModule.kt b/...ng-protection/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/di/VpnAppModule.kt
@@ -21,12 +21,7 @@ import android.content.res.Resources
 import android.net.ConnectivityManager
 import androidx.room.Room
 import com.duckduckgo.common.utils.DispatcherProvider
-import com.duckduckgo.data.store.api.SharedPreferencesProvider
 import com.duckduckgo.di.scopes.AppScope
-import com.duckduckgo.mobile.android.vpn.Vpn
-import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistry
-import com.duckduckgo.mobile.android.vpn.VpnFeaturesRegistryImpl
-import com.duckduckgo.mobile.android.vpn.VpnServiceWrapper
 import com.duckduckgo.mobile.android.vpn.stats.AppTrackerBlockingStatsRepository
 import com.duckduckgo.mobile.android.vpn.stats.RealAppTrackerBlockingStatsRepository
 import com.duckduckgo.mobile.android.vpn.store.*
@@ -88,25 +83,6 @@ object VpnAppModule {
         return context.resources
     }
 
-    @Provides
-    @SingleInstanceIn(AppScope::class)
-    fun provideVpnFeaturesRegistry(
-        context: Context,
-        sharedPreferencesProvider: SharedPreferencesProvider,
-        dispatcherProvider: DispatcherProvider,
-    ): VpnFeaturesRegistry {
-        return VpnFeaturesRegistryImpl(VpnServiceWrapper(context, dispatcherProvider), sharedPreferencesProvider, dispatcherProvider)
-    }
-
-    @Provides
-    @SingleInstanceIn(AppScope::class)
-    fun provideVpnServiceWrapper(
-        context: Context,
-        dispatcherProvider: DispatcherProvider,
-    ): Vpn {
-        return VpnServiceWrapper(context, dispatcherProvider)
-    }
-
     @Provides
     fun provideAppTrackerBlockingStatsRepository(
         vpnDatabase: VpnDatabase,

diff --git a/...ion/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/service/VpnActionReceiver.kt b/...ion/vpn-impl/src/main/java/com/duckduckgo/mobile/android/vpn/service/VpnActionReceiver.kt
@@ -25,8 +25,10 @@ import android.os.SystemClock
 import com.duckduckgo.anvil.annotations.InjectWith
 import com.duckduckgo.common.utils.DispatcherProvider
 import com.duckduckgo.di.scopes.ReceiverScope
+import com.duckduckgo.mobile.android.app.tracking.AppTrackingProtection
 import com.duckduckgo.mobile.android.vpn.Vpn
 import com.duckduckgo.mobile.android.vpn.pixels.DeviceShieldPixels
+import com.duckduckgo.networkprotection.api.NetworkProtectionState
 import dagger.android.AndroidInjection
 import java.util.concurrent.TimeUnit
 import javax.inject.Inject
@@ -38,6 +40,10 @@ import logcat.logcat
 class VpnActionReceiver : BroadcastReceiver() {
     @Inject lateinit var vpn: Vpn
 
+    @Inject lateinit var networkProtectionState: NetworkProtectionState
+
+    @Inject lateinit var appTrackingProtection: AppTrackingProtection
+
     @Inject lateinit var dispatcherProvider: DispatcherProvider
 
     @Inject lateinit var context: Context

diff --git a/...o/mobile/android/vpn/service/notification/AppTPAndNetPEnabledNotificationContentPlugin.kt b/...o/mobile/android/vpn/service/notification/AppTPAndNetPEnabledNotificationContentPlugin.kt
@@ -33,6 +33,7 @@ import com.squareup.anvil.annotations.ContributesMultibinding
 import dagger.SingleInstanceIn
 import javax.inject.Inject
 import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.map
 import kotlinx.coroutines.runBlocking
 
@@ -77,6 +78,7 @@ class AppTPAndNetPEnabledNotificationContentPlugin @Inject constructor(
         }
 
         return repository.getVpnTrackers({ dateOfLastHour() })
+            .filter { isActive() } // make sure we only emit when this plugin is active
             .map { trackersBlocked ->
                 val trackingApps = trackersBlocked.trackingApps()
                 val location = networkProtectionState.serverLocation()

diff --git a/...ckduckgo/mobile/android/vpn/service/notification/AppTpEnabledNotificationContentPlugin.kt b/...ckduckgo/mobile/android/vpn/service/notification/AppTpEnabledNotificationContentPlugin.kt
@@ -79,6 +79,7 @@ class AppTpEnabledNotificationContentPlugin @Inject constructor(
         }
 
         return repository.getVpnTrackers({ dateOfLastHour() })
+            .filter { isActive() } // make sure we only emit when this plugin is active
             .map { trackersBlocked ->
                 val trackingApps = trackersBlocked.trackingApps()
                 val isEnabled = appTrackingProtection.isEnabled()

diff --git a/...n/vpn-impl/src/test/java/com/duckduckgo/mobile/android/vpn/VpnFeaturesRegistryImplTest.kt b/...n/vpn-impl/src/test/java/com/duckduckgo/mobile/android/vpn/VpnFeaturesRegistryImplTest.kt
@@ -20,7 +20,6 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
 import com.duckduckgo.common.test.CoroutineTestRule
 import com.duckduckgo.common.test.api.InMemorySharedPreferences
-import com.duckduckgo.common.utils.DispatcherProvider
 import com.duckduckgo.data.store.api.SharedPreferencesProvider
 import kotlinx.coroutines.test.runTest
 import org.junit.Assert.*
@@ -44,7 +43,7 @@ class VpnFeaturesRegistryImplTest {
     @Before
     fun setup() {
         val prefs = InMemorySharedPreferences()
-        vpnServiceWrapper = TestVpnServiceWrapper(coroutineTestRule.testDispatcherProvider)
+        vpnServiceWrapper = TestVpnServiceWrapper()
 
         whenever(
             sharedPreferencesProvider.getSharedPreferences(eq("com.duckduckgo.mobile.android.vpn.feature.registry.v1"), eq(true), eq(false)),
@@ -163,9 +162,7 @@ class VpnFeaturesRegistryImplTest {
         BAR("BAR"),
     }
 
-    private class TestVpnServiceWrapper constructor(
-        dispatcher: DispatcherProvider,
-    ) : VpnServiceWrapper(InstrumentationRegistry.getInstrumentation().context, dispatcher) {
+    private class TestVpnServiceWrapper constructor() : VpnServiceWrapper(InstrumentationRegistry.getInstrumentation().context) {
         private var isRunning = false
         var restartCount = 0
 

diff --git a/...bile/android/vpn/service/notification/AppTPAndNetPEnabledNotificationContentPluginTest.kt b/...bile/android/vpn/service/notification/AppTPAndNetPEnabledNotificationContentPluginTest.kt
@@ -137,6 +137,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentThenReturnsCorrectInitialUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn("Stockholm, SE")
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             val item = awaitItem()
@@ -151,6 +153,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentOneCompanyThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn("Stockholm, SE")
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             val trackers = listOf(aTrackerAndCompany())
@@ -169,6 +173,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentMultipleDifferentAppsThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn("Stockholm, SE")
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             val trackers = listOf(
@@ -193,6 +199,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentMultipleDifferentAppsNoLocationThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn(null)
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             val trackers = listOf(
@@ -217,6 +225,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentTrackersWithoutEntityThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn("Stockholm, SE")
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             appTrackerBlockingStatsRepository.insert(listOf(aTrackerAndCompany(), aTrackerAndCompany()))
@@ -232,6 +242,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentTrackersWithoutEntityNoLocationThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn(null)
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             appTrackerBlockingStatsRepository.insert(listOf(aTrackerAndCompany(), aTrackerAndCompany()))
@@ -247,6 +259,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentMultipleSameThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn("Stockholm, SE")
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             appTrackerBlockingStatsRepository.insert(listOf(aTrackerAndCompany(), aTrackerAndCompany()))
@@ -265,6 +279,8 @@ class AppTPAndNetPEnabledNotificationContentPluginTest {
     @Test
     fun getUpdateContentMultipleSameNoLocationThenReturnsCorrectUpdatedNotificationContent() = runTest {
         whenever(networkProtectionState.serverLocation()).thenReturn(null)
+        whenever(appTrackingProtection.isEnabled()).thenReturn(true)
+        whenever(networkProtectionState.isEnabled()).thenReturn(true)
 
         plugin.getUpdatedContent().test {
             appTrackerBlockingStatsRepository.insert(listOf(aTrackerAndCompany(), aTrackerAndCompany()))

diff --git a/...ckgo/mobile/android/vpn/service/notification/AppTpEnabledNotificationContentPluginTest.kt b/...ckgo/mobile/android/vpn/service/notification/AppTpEnabledNotificationContentPluginTest.kt
@@ -130,15 +130,12 @@ class AppTpEnabledNotificationContentPluginTest {
     }
 
     @Test
-    fun getUpdateContentAppTpNotEnabledThenReturnsCorrectInitialUpdatedNotificationContent() = runTest {
+    fun getUpdateContentAppTpNotEnabledThenReturnsNoContent() = runTest {
         whenever(appTrackingProtection.isEnabled()).thenReturn(false)
+        whenever(networkProtectionState.isEnabled()).thenReturn(false)
 
         plugin.getUpdatedContent().test {
-            val item = awaitItem()
-
-            item.assertTextEquals("")
-
-            cancelAndConsumeRemainingEvents()
+            expectNoEvents()
         }
     }
 
@@ -162,20 +159,16 @@ class AppTpEnabledNotificationContentPluginTest {
     }
 
     @Test
-    fun getUpdateContentOneCompanyAppTpNotEnabledThenReturnsCorrectUpdatedNotificationContent() = runTest {
+    fun getUpdateContentOneCompanyAppTpNotEnabledThenReturnsNoContent() = runTest {
         whenever(appTrackingProtection.isEnabled()).thenReturn(false)
+        whenever(networkProtectionState.isEnabled()).thenReturn(false)
 
         plugin.getUpdatedContent().test {
             val trackers = listOf(aTrackerAndCompany())
             appTrackerBlockingStatsRepository.insert(trackers)
             db.vpnAppTrackerBlockingDao().insertTrackerEntities(trackers.map { it.asEntity() })
 
-            skipItems(1)
-            val item = awaitItem()
-
-            item.assertTextEquals("")
-
-            cancelAndConsumeRemainingEvents()
+            expectNoEvents()
         }
     }
 
@@ -205,8 +198,9 @@ class AppTpEnabledNotificationContentPluginTest {
     }
 
     @Test
-    fun getUpdateContentMultipleDifferentAppsAppTpNotEnabledThenReturnsCorrectUpdatedNotificationContent() = runTest {
+    fun getUpdateContentMultipleDifferentAppsAppTpNotEnabledThenReturnsNoContent() = runTest {
         whenever(appTrackingProtection.isEnabled()).thenReturn(false)
+        whenever(networkProtectionState.isEnabled()).thenReturn(false)
 
         plugin.getUpdatedContent().test {
             val trackers = listOf(
@@ -220,12 +214,7 @@ class AppTpEnabledNotificationContentPluginTest {
             appTrackerBlockingStatsRepository.insert(trackers)
             db.vpnAppTrackerBlockingDao().insertTrackerEntities(trackers.map { it.asEntity() })
 
-            skipItems(1)
-            val item = awaitItem()
-
-            item.assertTextEquals("")
-
-            cancelAndConsumeRemainingEvents()
+            expectNoEvents()
         }
     }
 
@@ -265,16 +254,13 @@ class AppTpEnabledNotificationContentPluginTest {
     }
 
     @Test
-    fun getUpdateContentMultipleSameAppTpNotEnabledThenReturnsCorrectUpdatedNotificationContent() = runTest {
+    fun getUpdateContentMultipleSameAppTpNotEnabledThenReturnsNoContent() = runTest {
         whenever(appTrackingProtection.isEnabled()).thenReturn(false)
+        whenever(networkProtectionState.isEnabled()).thenReturn(false)
         plugin.getUpdatedContent().test {
             appTrackerBlockingStatsRepository.insert(listOf(aTrackerAndCompany(), aTrackerAndCompany()))
 
-            val item = expectMostRecentItem()
-
-            item.assertTextEquals("")
-
-            cancelAndConsumeRemainingEvents()
+            expectNoEvents()
         }
     }