Address comments

duckduckgo · Dec 19, 2023 · b9f69b1 · b9f69b1
1 parent 9772d94
commit b9f69b1
Show file tree

Hide file tree

Showing 13 changed files with 212 additions and 116 deletions.
diff --git a/common/common-test/src/main/java/com/duckduckgo/common/test/api/FakeChain.kt b/common/common-test/src/main/java/com/duckduckgo/common/test/api/FakeChain.kt
@@ -19,7 +19,10 @@ package com.duckduckgo.common.test.api
 import java.util.concurrent.TimeUnit
 import okhttp3.*
 
-class FakeChain(private val url: String) : Interceptor.Chain {
+class FakeChain(
+    private val url: String,
+    private val expectedResponseCode: Int? = null,
+) : Interceptor.Chain {
     override fun call(): Call {
         TODO("Not yet implemented")
     }
@@ -37,7 +40,7 @@ class FakeChain(private val url: String) : Interceptor.Chain {
             .request(request)
             .headers(request.headers) // echo the headers
             .protocol(Protocol.HTTP_2)
-            .code(200)
+            .code(expectedResponseCode ?: 200)
             .message("")
             .build()
     }

diff --git a/...ion-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/WgServerApi.kt b/...ion-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/WgServerApi.kt
@@ -21,7 +21,6 @@ import com.duckduckgo.di.scopes.VpnScope
 import com.duckduckgo.networkprotection.impl.configuration.WgServerApi.WgServerData
 import com.duckduckgo.networkprotection.impl.di.UnprotectedVpnControllerService
 import com.duckduckgo.networkprotection.impl.settings.geoswitching.NetpEgressServersProvider
-import com.duckduckgo.networkprotection.impl.store.NetworkProtectionRepository
 import com.squareup.anvil.annotations.ContributesBinding
 import javax.inject.Inject
 import logcat.logcat
@@ -45,54 +44,44 @@ class RealWgServerApi @Inject constructor(
     @UnprotectedVpnControllerService private val wgVpnControllerService: WgVpnControllerService,
     private val serverDebugProvider: WgServerDebugProvider,
     private val netNetpEgressServersProvider: NetpEgressServersProvider,
-    private val netpRepository: NetworkProtectionRepository,
 ) : WgServerApi {
 
     override suspend fun registerPublicKey(publicKey: String): WgServerData? {
-        val registerKeyBody = try {
-            // This bit of code gets all possible egress servers which should be order by proximity, caches them for internal builds and then
-            // returns the closest one or null if list is empty
-            val selectedServer = wgVpnControllerService.getServers().map { it.server }
-                .also { fetchedServers ->
-                    logcat { "Fetched servers ${fetchedServers.map { it.name }}" }
-                    serverDebugProvider.cacheServers(fetchedServers)
-                }
-                .map { it.name }
-                .firstOrNull { serverName ->
-                    serverDebugProvider.getSelectedServerName()?.let { userSelectedServer ->
-                        serverName == userSelectedServer
-                    } ?: false
-                }
-
-            logcat { "Register key in $selectedServer" }
-            val userPreferredLocation = netNetpEgressServersProvider.updateServerLocationsAndReturnPreferred()
-            if (selectedServer != null) {
-                RegisterKeyBody(publicKey = publicKey, server = selectedServer)
-            } else if (userPreferredLocation != null) {
-                if (userPreferredLocation.cityName != null) {
-                    RegisterKeyBody(publicKey = publicKey, country = userPreferredLocation.countryCode, city = userPreferredLocation.cityName)
-                } else {
-                    RegisterKeyBody(publicKey = publicKey, country = userPreferredLocation.countryCode)
-                }
+        // This bit of code gets all possible egress servers which should be order by proximity, caches them for internal builds and then
+        // returns the closest one or null if list is empty
+        val selectedServer = wgVpnControllerService.getServers().map { it.server }
+            .also { fetchedServers ->
+                logcat { "Fetched servers ${fetchedServers.map { it.name }}" }
+                serverDebugProvider.cacheServers(fetchedServers)
+            }
+            .map { it.name }
+            .firstOrNull { serverName ->
+                serverDebugProvider.getSelectedServerName()?.let { userSelectedServer ->
+                    serverName == userSelectedServer
+                } ?: false
+            }
+
+        logcat { "Register key in $selectedServer" }
+        val userPreferredLocation = netNetpEgressServersProvider.updateServerLocationsAndReturnPreferred()
+        val registerKeyBody = if (selectedServer != null) {
+            RegisterKeyBody(publicKey = publicKey, server = selectedServer)
+        } else if (userPreferredLocation != null) {
+            if (userPreferredLocation.cityName != null) {
+                RegisterKeyBody(publicKey = publicKey, country = userPreferredLocation.countryCode, city = userPreferredLocation.cityName)
             } else {
-                RegisterKeyBody(publicKey = publicKey, server = "*")
+                RegisterKeyBody(publicKey = publicKey, country = userPreferredLocation.countryCode)
             }
-        } catch (e: Exception) {
+        } else {
             RegisterKeyBody(publicKey = publicKey, server = "*")
         }
 
         return wgVpnControllerService.registerKey(registerKeyBody)
             .run {
-                if (isSuccessful) {
-                    logcat { "Register key returned ${this.body()?.map { it.server.name }}" }
-                    val server = this.body()?.firstOrNull()?.toWgServerData()
-                    logcat { "Selected Egress server is $server" }
-                    netpRepository.disabledDueToAccessRevoked = false
-                    server
-                } else {
-                    netpRepository.disabledDueToAccessRevoked = this.code() == 403
-                    null
-                }
+                logcat { "Register key in $selectedServer" }
+                logcat { "Register key returned ${this.map { it.server.name }}" }
+                val server = this.firstOrNull()?.toWgServerData()
+                logcat { "Selected Egress server is $server" }
+                server
             }
     }
 

diff --git a/...c/main/java/com/duckduckgo/networkprotection/impl/configuration/WgVpnControllerService.kt b/...c/main/java/com/duckduckgo/networkprotection/impl/configuration/WgVpnControllerService.kt
@@ -37,7 +37,6 @@ import javax.net.ssl.TrustManagerFactory
 import javax.net.ssl.X509TrustManager
 import okhttp3.OkHttpClient
 import org.conscrypt.Conscrypt
-import retrofit2.Response
 import retrofit2.Retrofit
 import retrofit2.http.Body
 import retrofit2.http.GET
@@ -121,7 +120,7 @@ interface WgVpnControllerService {
     @POST("$NETP_ENVIRONMENT_URL/register")
     suspend fun registerKey(
         @Body registerKeyBody: RegisterKeyBody,
-    ): Response<List<EligibleServerInfo>>
+    ): List<EligibleServerInfo>
 
     @GET("$NETP_ENVIRONMENT_URL/locations")
     suspend fun getEligibleLocations(): List<EligibleLocation>

diff --git a/.../src/main/java/com/duckduckgo/networkprotection/impl/store/NetworkProtectionRepository.kt b/.../src/main/java/com/duckduckgo/networkprotection/impl/store/NetworkProtectionRepository.kt
@@ -36,7 +36,7 @@ interface NetworkProtectionRepository {
     var enabledTimeInMillis: Long
     var serverDetails: ServerDetails?
     var clientInterface: ClientInterface?
-    var disabledDueToAccessRevoked: Boolean
+    var vpnAccessRevoked: Boolean
 
     enum class ReconnectStatus {
         NotReconnecting,
@@ -145,10 +145,10 @@ class RealNetworkProtectionRepository @Inject constructor(
             }
         }
 
-    override var disabledDueToAccessRevoked: Boolean
-        get() = networkProtectionPrefs.getBoolean(KEY_DISABLED_DUE_TO_ACCESS_REVOKED, false)
+    override var vpnAccessRevoked: Boolean
+        get() = networkProtectionPrefs.getBoolean(KEY_VPN_ACCESS_REVOKED, false)
         set(value) {
-            networkProtectionPrefs.putBoolean(KEY_DISABLED_DUE_TO_ACCESS_REVOKED, value)
+            networkProtectionPrefs.putBoolean(KEY_VPN_ACCESS_REVOKED, value)
         }
 
     companion object {
@@ -160,6 +160,6 @@ class RealNetworkProtectionRepository @Inject constructor(
         private const val KEY_WG_SERVER_ENABLE_TIME = "wg_server_enable_time"
         private const val KEY_WG_RECONNECT_STATUS = "wg_reconnect_status"
         private const val KEY_WG_CLIENT_IFACE_TUNNEL_IP = "wg_client_iface_tunnel_ip"
-        private const val KEY_DISABLED_DUE_TO_ACCESS_REVOKED = "key_disabled_due_to_access_revoked"
+        private const val KEY_VPN_ACCESS_REVOKED = "key_vpn_access_revoked"
     }
 }
diff --git a/...st/java/com/duckduckgo/networkprotection/impl/configuration/FakeWgVpnControllerService.kt b/...st/java/com/duckduckgo/networkprotection/impl/configuration/FakeWgVpnControllerService.kt
@@ -19,7 +19,6 @@ package com.duckduckgo.networkprotection.impl.configuration
 import com.squareup.moshi.JsonAdapter
 import com.squareup.moshi.Moshi
 import com.squareup.moshi.Types
-import retrofit2.Response
 
 class FakeWgVpnControllerService : WgVpnControllerService {
     private val moshi: Moshi = Moshi.Builder().build()
@@ -44,7 +43,7 @@ class FakeWgVpnControllerService : WgVpnControllerService {
         return serverLocations
     }
 
-    override suspend fun registerKey(registerKeyBody: RegisterKeyBody): Response<List<EligibleServerInfo>> {
+    override suspend fun registerKey(registerKeyBody: RegisterKeyBody): List<EligibleServerInfo> {
         return servers.filter {
             return@filter if (registerKeyBody.server != "*") {
                 it.server.name == registerKeyBody.server
@@ -57,9 +56,7 @@ class FakeWgVpnControllerService : WgVpnControllerService {
             } else {
                 true
             }
-        }.map {
-            it.toEligibleServerInfo()
-        }.run { Response.success(this) }
+        }.map { it.toEligibleServerInfo() }
     }
 
     private fun RegisteredServerInfo.toEligibleServerInfo(): EligibleServerInfo {

diff --git a/.../src/test/java/com/duckduckgo/networkprotection/impl/configuration/RealWgServerApiTest.kt b/.../src/test/java/com/duckduckgo/networkprotection/impl/configuration/RealWgServerApiTest.kt
@@ -17,22 +17,16 @@
 package com.duckduckgo.networkprotection.impl.configuration
 
 import com.duckduckgo.networkprotection.impl.configuration.WgServerApi.WgServerData
-import com.duckduckgo.networkprotection.impl.fakes.FakeNetworkProtectionRepository
 import com.duckduckgo.networkprotection.impl.settings.geoswitching.NetpEgressServersProvider
 import com.duckduckgo.networkprotection.impl.settings.geoswitching.NetpEgressServersProvider.PreferredLocation
-import com.duckduckgo.networkprotection.impl.store.NetworkProtectionRepository
 import kotlinx.coroutines.test.runTest
-import okhttp3.ResponseBody.Companion.toResponseBody
 import org.junit.Assert.*
 import org.junit.Before
 import org.junit.Test
 import org.mockito.Mock
-import org.mockito.Mockito
 import org.mockito.MockitoAnnotations
-import org.mockito.kotlin.any
 import org.mockito.kotlin.verify
 import org.mockito.kotlin.whenever
-import retrofit2.Response
 
 class RealWgServerApiTest {
     private val wgVpnControllerService = FakeWgVpnControllerService()
@@ -41,7 +35,6 @@ class RealWgServerApiTest {
     private lateinit var internalWgServerDebugProvider: FakeWgServerDebugProvider
     private lateinit var productionApi: RealWgServerApi
     private lateinit var internalApi: RealWgServerApi
-    private lateinit var networkProtectionRepository: NetworkProtectionRepository
 
     @Mock
     private lateinit var netpEgressServersProvider: NetpEgressServersProvider
@@ -52,19 +45,16 @@ class RealWgServerApiTest {
 
         productionWgServerDebugProvider = DefaultWgServerDebugProvider()
         internalWgServerDebugProvider = FakeWgServerDebugProvider()
-        networkProtectionRepository = FakeNetworkProtectionRepository()
 
         internalApi = RealWgServerApi(
             wgVpnControllerService,
             internalWgServerDebugProvider,
             netpEgressServersProvider,
-            networkProtectionRepository,
         )
         productionApi = RealWgServerApi(
             wgVpnControllerService,
             productionWgServerDebugProvider,
             netpEgressServersProvider,
-            networkProtectionRepository,
         )
     }
 
@@ -224,45 +214,6 @@ class RealWgServerApiTest {
             internalApi.registerPublicKey("testpublickey"),
         )
     }
-
-    @Test
-    fun whenServerThrowExceptionAndRegisterReturns403ThenSetDisabledDueToInvalidSubscription() = runTest {
-        val mockService = Mockito.mock(WgVpnControllerService::class.java)
-        val testee = RealWgServerApi(
-            mockService,
-            internalWgServerDebugProvider,
-            netpEgressServersProvider,
-            networkProtectionRepository,
-        )
-        whenever(mockService.getServers()).thenThrow(RuntimeException())
-        whenever(mockService.registerKey(any())).thenReturn(
-            Response.error(403, "Test".toResponseBody()),
-        )
-
-        testee.registerPublicKey("test")
-
-        verify(mockService).registerKey(RegisterKeyBody(publicKey = "test", server = "*"))
-        assertTrue(networkProtectionRepository.disabledDueToAccessRevoked)
-    }
-
-    @Test
-    fun whenServerThrowExceptionAndRegisterReturns400ThenDoNotSetDisabledDueToInvalidSubscription() = runTest {
-        val mockService = Mockito.mock(WgVpnControllerService::class.java)
-        val testee = RealWgServerApi(
-            mockService,
-            internalWgServerDebugProvider,
-            netpEgressServersProvider,
-            networkProtectionRepository,
-        )
-        whenever(mockService.getServers()).thenThrow(RuntimeException())
-        whenever(mockService.registerKey(any())).thenReturn(
-            Response.error(400, "Test".toResponseBody()),
-        )
-
-        testee.registerPublicKey("test")
-
-        assertFalse(networkProtectionRepository.disabledDueToAccessRevoked)
-    }
 }
 
 private class FakeWgServerDebugProvider() : WgServerDebugProvider {

diff --git a/.../test/java/com/duckduckgo/networkprotection/impl/fakes/FakeNetworkProtectionRepository.kt b/.../test/java/com/duckduckgo/networkprotection/impl/fakes/FakeNetworkProtectionRepository.kt
@@ -24,7 +24,7 @@ import com.duckduckgo.networkprotection.impl.store.NetworkProtectionRepository.S
 class FakeNetworkProtectionRepository : NetworkProtectionRepository {
     private var _reconnectStatus: ReconnectStatus? = null
     private var _serverDetails: ServerDetails? = null
-    private var _disabledDueToInvalidSubscription: Boolean = false
+    private var _vpnAccessRevoked: Boolean = false
 
     override var reconnectStatus: ReconnectStatus
         get() = _reconnectStatus ?: NotReconnecting
@@ -50,9 +50,9 @@ class FakeNetworkProtectionRepository : NetworkProtectionRepository {
         get() = null
         set(_) {}
 
-    override var disabledDueToAccessRevoked: Boolean
-        get() = _disabledDueToInvalidSubscription
+    override var vpnAccessRevoked: Boolean
+        get() = _vpnAccessRevoked
         set(value) {
-            _disabledDueToInvalidSubscription = value
+            _vpnAccessRevoked = value
         }
 }
diff --git a/network-protection/network-protection-subscription-internal/build.gradle b/network-protection/network-protection-subscription-internal/build.gradle
@@ -50,7 +50,7 @@ dependencies {
     implementation "com.squareup.logcat:logcat:_"
 
     // Testing dependencies
-    testImplementation project(path: ':common-test')
+    testImplementation project(':common-test')
     testImplementation "org.mockito.kotlin:mockito-kotlin:_"
     testImplementation Testing.junit4
     testImplementation AndroidX.archCore.testing
@@ -59,7 +59,6 @@ dependencies {
     testImplementation "androidx.test:runner:_"
     testImplementation Testing.robolectric
     testImplementation 'app.cash.turbine:turbine:_'
-    testImplementation project(path: ':common-test')
     testImplementation (KotlinX.coroutines.test) {
         // https://github.com/Kotlin/kotlinx.coroutines/issues/2023
         // conflicts with mockito due to direct inclusion of byte buddy

diff --git a/...al/src/main/java/com/duckduckgo/networkprotection/subscription/NetpSubscriptionChecker.kt b/...al/src/main/java/com/duckduckgo/networkprotection/subscription/NetpSubscriptionChecker.kt
@@ -109,7 +109,7 @@ class NetpSubscriptionCheckWorker(
         logcat { "Sub check: checking entitlement" }
         if (!netpSubscriptionManager.hasValidEntitlement() && networkProtectionState.isEnabled()) {
             logcat { "Sub check: disabling" }
-            netpRepository.disabledDueToAccessRevoked = true
+            netpRepository.vpnAccessRevoked = true
             networkProtectionState.stop()
         } else if (!networkProtectionState.isEnabled()) {
             logcat { "Sub check: cancelling scheduled checker" }

diff --git a/...duckgo/networkprotection/subscription/configuration/NetpSubscriptionRequestInterceptor.kt b/...duckgo/networkprotection/subscription/configuration/NetpSubscriptionRequestInterceptor.kt
@@ -21,6 +21,7 @@ import com.duckduckgo.appbuildconfig.api.isInternalBuild
 import com.duckduckgo.di.scopes.AppScope
 import com.duckduckgo.networkprotection.impl.BuildConfig
 import com.duckduckgo.networkprotection.impl.configuration.NetpRequestInterceptor
+import com.duckduckgo.networkprotection.impl.store.NetworkProtectionRepository
 import com.duckduckgo.networkprotection.subscription.NetpSubscriptionManager
 import com.squareup.anvil.annotations.ContributesBinding
 import com.squareup.anvil.annotations.ContributesBinding.Priority.HIGHEST
@@ -37,12 +38,13 @@ import okhttp3.Response
 class NetpSubscriptionRequestInterceptor @Inject constructor(
     private val appBuildConfig: AppBuildConfig,
     private val netpSubscriptionManager: NetpSubscriptionManager,
+    private val networkProtectionRepository: NetworkProtectionRepository,
 ) : NetpRequestInterceptor {
 
     override fun intercept(chain: Chain): Response {
         val url = chain.request().url
         val newRequest = chain.request().newBuilder()
-        if (ENDPOINTS_PATTERN_MATCHER.any { url.toString().endsWith(it) }) {
+        return if (ENDPOINTS_PATTERN_MATCHER.any { url.toString().endsWith(it) }) {
             logcat { "Adding Authorization Bearer token to request $url" }
             newRequest.addHeader(
                 name = "Authorization",
@@ -56,11 +58,15 @@ class NetpSubscriptionRequestInterceptor @Inject constructor(
                     value = BuildConfig.NETP_DEBUG_SERVER_TOKEN,
                 )
             }
-        }
 
-        return chain.proceed(
-            newRequest.build().also { logcat { "headers: ${it.headers}" } },
-        )
+            chain.proceed(
+                newRequest.build().also { logcat { "headers: ${it.headers}" } },
+            ).also {
+                networkProtectionRepository.vpnAccessRevoked = it.code == 403
+            }
+        } else {
+            chain.proceed(newRequest.build())
+        }
     }
 
     companion object {

diff --git a/...ckgo/networkprotection/subscription/notification/NetAccessRevokedNotificationScheduler.kt b/...ckgo/networkprotection/subscription/notification/NetAccessRevokedNotificationScheduler.kt
@@ -48,7 +48,7 @@ class NetAccessRevokedNotificationScheduler @Inject constructor(
 
     override fun onVpnStartFailed(coroutineScope: CoroutineScope) {
         super.onVpnStartFailed(coroutineScope)
-        if (networkProtectionRepository.disabledDueToAccessRevoked) {
+        if (networkProtectionRepository.vpnAccessRevoked) {
             notificationManager.notify(
                 NetPDisabledNotificationScheduler.NETP_REMINDER_NOTIFICATION_ID,
                 buildVpnAccessRevokedNotification(context),

diff --git a/...duckgo/networkprotection/subscription/notification/SubsNetpDisabledNotificationBuilder.kt b/...duckgo/networkprotection/subscription/notification/SubsNetpDisabledNotificationBuilder.kt
@@ -36,7 +36,7 @@ class SubsNetpDisabledNotificationBuilder @Inject constructor(
 ) : NetPDisabledNotificationBuilder {
 
     override fun buildDisabledNotification(context: Context): Notification {
-        return if (netpRepository.disabledDueToAccessRevoked) {
+        return if (netpRepository.vpnAccessRevoked) {
             buildVpnAccessRevokedNotification(context)
         } else {
             realNetPDisabledNotificationBuilder.buildDisabledNotification(context)