From 4c381624bd5e72c2bde895c05b4b75a076746a51 Mon Sep 17 00:00:00 2001 From: Aitor Viana Date: Wed, 22 Jan 2025 17:32:50 +0000 Subject: [PATCH] Fix missing NETP_DEBUG_SERVER_TOKEN for internal builds (#5495) Task/Issue URL: https://app.asana.com/0/488551667048375/1209187729140301/f ### Description When we migrated to GHA, the `NETP_DEBUG_SERVER_TOKEN` was no longer passed as env variable, breaking VPN internal settings Incidentally I changed the way we intercept requests, using annotations rather than endpoint string matching ### Steps to test this PR - [ ] Build internal build and verify `VPN server` option in NetP dev settings works as expected --- .github/workflows/release_nightly.yml | 2 + common/common-test/build.gradle | 1 + .../duckduckgo/common/test/api/FakeChain.kt | 10 +- .../NetpControllerRequestInterceptor.kt | 25 ++- .../configuration/WgVpnControllerService.kt | 10 ++ .../NetpControllerRequestInterceptorTest.kt | 142 ++++-------------- 6 files changed, 65 insertions(+), 125 deletions(-) diff --git a/.github/workflows/release_nightly.yml b/.github/workflows/release_nightly.yml index bc7d2ed8d174..22b86683c5e6 100644 --- a/.github/workflows/release_nightly.yml +++ b/.github/workflows/release_nightly.yml @@ -100,6 +100,8 @@ jobs: gradle clean - name: Assemble the bundle + env: + NETP_DEBUG_SERVER_TOKEN: ${{ secrets.NETP_DEBUG_SERVER_TOKEN }} if: steps.check_for_changes.outputs.has_changes == 'true' run: gradle bundleInternalRelease -PversionNameSuffix=-nightly -PuseUploadSigning -PlatestTag=${{ steps.get_latest_tag.outputs.latest_tag }} -Pbuild-date-time diff --git a/common/common-test/build.gradle b/common/common-test/build.gradle index cc1099219493..b83f8eb25f32 100644 --- a/common/common-test/build.gradle +++ b/common/common-test/build.gradle @@ -20,6 +20,7 @@ dependencies { implementation "com.squareup.moshi:moshi-kotlin:_" implementation Square.okHttp3.okHttp implementation KotlinX.coroutines.core + implementation Square.retrofit2.retrofit // api because TestDispatcher is in the CoroutineTestRule public API api (KotlinX.coroutines.test) { // https://github.com/Kotlin/kotlinx.coroutines/issues/2023 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 index cf404c4b31c0..738c5dc44819 100644 --- 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 @@ -16,12 +16,15 @@ package com.duckduckgo.common.test.api +import java.lang.reflect.Method import java.util.concurrent.TimeUnit import okhttp3.* +import retrofit2.Invocation open class FakeChain( private val url: String, private val expectedResponseCode: Int? = null, + private val serviceMethod: Method? = null, ) : Interceptor.Chain { override fun call(): Call { TODO("Not yet implemented") @@ -50,7 +53,12 @@ open class FakeChain( } override fun request(): Request { - return Request.Builder().url(url).build() + return Request.Builder().apply { + url(url) + serviceMethod?.let { + tag(Invocation::class.java, Invocation.of(it, emptyList())) + } + }.build() } override fun withConnectTimeout( diff --git a/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptor.kt b/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptor.kt index 5b9be1e6765e..6651a4f56fc4 100644 --- a/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptor.kt +++ b/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptor.kt @@ -29,6 +29,7 @@ import logcat.logcat import okhttp3.Interceptor import okhttp3.Interceptor.Chain import okhttp3.Response +import retrofit2.Invocation @ContributesMultibinding( scope = AppScope::class, @@ -42,9 +43,16 @@ class NetpControllerRequestInterceptor @Inject constructor( override fun getInterceptor(): Interceptor = this override fun intercept(chain: Chain): Response { - val url = chain.request().url - val newRequest = chain.request().newBuilder() - return if (ENDPOINTS_PATTERN_MATCHER.any { url.toString().endsWith(it) }) { + val request = chain.request() + val url = request + + val authRequired = chain.request().tag(Invocation::class.java) + ?.method() + ?.isAnnotationPresent(AuthRequired::class.java) == true + + return if (authRequired) { + val newRequest = chain.request().newBuilder() + logcat { "Adding Authorization Bearer token to request $url" } newRequest.addHeader( name = "Authorization", @@ -63,20 +71,11 @@ class NetpControllerRequestInterceptor @Inject constructor( newRequest.build().also { logcat { "headers: ${it.headers}" } }, ) } else { - chain.proceed(newRequest.build()) + chain.proceed(request) } } private suspend fun authorizationHeaderValue(): String { return "bearer ddg:${subscriptions.getAccessToken()}" } - - companion object { - // The NetP environments are for now https://.netp.duckduckgo.com/ - private val ENDPOINTS_PATTERN_MATCHER = listOf( - "netp.duckduckgo.com/servers", - "netp.duckduckgo.com/register", - "netp.duckduckgo.com/locations", - ) - } } diff --git a/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/WgVpnControllerService.kt b/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/WgVpnControllerService.kt index 7bf948a77660..79c3808ccc99 100644 --- a/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/WgVpnControllerService.kt +++ b/network-protection/network-protection-impl/src/main/java/com/duckduckgo/networkprotection/impl/configuration/WgVpnControllerService.kt @@ -111,6 +111,7 @@ object WgVpnControllerServiceModule { @ProtectedVpnControllerService interface WgVpnControllerService { @GET("$NETP_ENVIRONMENT_URL/servers") + @AuthRequired suspend fun getServers(): List @GET("$NETP_ENVIRONMENT_URL/servers/{serverName}/status") @@ -119,12 +120,14 @@ interface WgVpnControllerService { ): ServerStatus @Headers("Content-Type: application/json") + @AuthRequired @POST("$NETP_ENVIRONMENT_URL/register") suspend fun registerKey( @Body registerKeyBody: RegisterKeyBody, ): List @GET("$NETP_ENVIRONMENT_URL/locations") + @AuthRequired suspend fun getEligibleLocations(): List } @@ -171,3 +174,10 @@ data class EligibleLocation( data class EligibleCity( val name: String, ) + +/** + * This annotation is used in interceptors to be able to intercept the annotated service calls + */ +@Target(AnnotationTarget.FUNCTION) +@Retention(AnnotationRetention.RUNTIME) +annotation class AuthRequired diff --git a/network-protection/network-protection-impl/src/test/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptorTest.kt b/network-protection/network-protection-impl/src/test/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptorTest.kt index de469939af3b..5e3d0264e00c 100644 --- a/network-protection/network-protection-impl/src/test/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptorTest.kt +++ b/network-protection/network-protection-impl/src/test/java/com/duckduckgo/networkprotection/impl/configuration/NetpControllerRequestInterceptorTest.kt @@ -2,14 +2,10 @@ package com.duckduckgo.networkprotection.impl.configuration import com.duckduckgo.appbuildconfig.api.AppBuildConfig import com.duckduckgo.appbuildconfig.api.BuildFlavor -import com.duckduckgo.appbuildconfig.api.BuildFlavor.INTERNAL -import com.duckduckgo.appbuildconfig.api.BuildFlavor.PLAY import com.duckduckgo.common.test.api.FakeChain import com.duckduckgo.subscriptions.api.Subscriptions import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.runBlocking -import kotlinx.coroutines.test.runTest -import org.junit.Assert import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Before @@ -38,54 +34,43 @@ class NetpControllerRequestInterceptorTest { } @Test - fun whenInterceptNotNetPUrlThenDoNothing() { - val chain = FakeChain("https://this.is.not.the.url/servers") + fun `when @AuthRequired annotation is not present then authorization header not is added`() { + val nonAnnotatedMethod = FakeApiService::class.java.getMethod("endpointNotRequiringAuthentication") - val headers = interceptor.intercept(chain).headers - - assertTrue(headers.size == 0) - } - - @Test - fun whenInterceptServersCallThenAddAuthHeader() { - val chain = FakeChain("https://staging.netp.duckduckgo.com/servers") - - val headers = interceptor.intercept(chain).headers - - assertTrue(headers.size == 1) - assertFalse(headers.map { it.first }.any { it.contains("NetP-Debug-Code") }) - assertTrue(headers.map { it.first }.any { it.contains("Authorization") }) - } - - @Test - fun whenInterceptRegisterCallThenAddAuthHeader() { - val chain = FakeChain("https://staging.netp.duckduckgo.com/register") + val chain = FakeChain( + url = "https://this.is.not.the.url/servers", + serviceMethod = nonAnnotatedMethod, + ) val headers = interceptor.intercept(chain).headers - assertTrue(headers.size == 1) - assertFalse(headers.map { it.first }.any { it.contains("NetP-Debug-Code") }) - assertTrue(headers.map { it.first }.any { it.contains("Authorization") }) + assertTrue(headers.size == 0) } @Test - fun whenInterceptServersCallInternalBuildThenAddAuthAndDebugHeaders() { + fun `when @AuthRequired annotation is not present and internal build then authorization header not is added`() { whenever(appBuildConfig.flavor).thenReturn(BuildFlavor.INTERNAL) - val chain = FakeChain("https://staging.netp.duckduckgo.com/servers") + val nonAnnotatedMethod = FakeApiService::class.java.getMethod("endpointNotRequiringAuthentication") + + val chain = FakeChain( + url = "https://this.is.not.the.url/servers", + serviceMethod = nonAnnotatedMethod, + ) val headers = interceptor.intercept(chain).headers - assertTrue(headers.size == 2) - assertTrue(headers.map { it.first }.any { it.contains("NetP-Debug-Code") }) - assertTrue(headers.map { it.first }.any { it.contains("Authorization") }) + assertTrue(headers.size == 0) } @Test - fun whenInterceptRegisterCallInternalBuildThenAddAuthAndDebugHeaders() { + fun `when @AuthRequired annotation is present and internal then authorization header is added`() { whenever(appBuildConfig.flavor).thenReturn(BuildFlavor.INTERNAL) - - val chain = FakeChain("https://staging.netp.duckduckgo.com/register") + val annotatedMethod = FakeApiService::class.java.getMethod("endpointRequiringAuthentication") + val chain = FakeChain( + url = "https://staging.netp.duckduckgo.com/servers", + serviceMethod = annotatedMethod, + ) val headers = interceptor.intercept(chain).headers @@ -95,23 +80,13 @@ class NetpControllerRequestInterceptorTest { } @Test - fun whenInterceptServersCallPlayBuildThenAddAuthHeader() { - whenever(appBuildConfig.flavor).thenReturn(BuildFlavor.PLAY) - - val chain = FakeChain("https://staging.netp.duckduckgo.com/servers") - - val headers = interceptor.intercept(chain).headers - - assertTrue(headers.size == 1) - assertFalse(headers.map { it.first }.any { it.contains("NetP-Debug-Code") }) - assertTrue(headers.map { it.first }.any { it.contains("Authorization") }) - } - - @Test - fun whenInterceptRegisterCallPlayBuildThenAddAuthHeader() { + fun `when @AuthRequired annotation is present and not internal then netp debug code not added`() { whenever(appBuildConfig.flavor).thenReturn(BuildFlavor.PLAY) - - val chain = FakeChain("https://staging.netp.duckduckgo.com/register") + val annotatedMethod = FakeApiService::class.java.getMethod("endpointRequiringAuthentication") + val chain = FakeChain( + url = "https://staging.netp.duckduckgo.com/servers", + serviceMethod = annotatedMethod, + ) val headers = interceptor.intercept(chain).headers @@ -120,64 +95,9 @@ class NetpControllerRequestInterceptorTest { assertTrue(headers.map { it.first }.any { it.contains("Authorization") }) } - // ---------------------------------------------------------------------------------------------------------------------------------- - // Here starts the tests for the subscriptions - // ---------------------------------------------------------------------------------------------------------------------------------- - @Test - fun whenUrlIsServersAndFlavorIsPlayThenOnlyAddTokenToHeader() = runTest { - val fakeChain = FakeChain(url = "https://controller.netp.duckduckgo.com/servers") - whenever(appBuildConfig.flavor).thenReturn(PLAY) - whenever(subscriptions.getAccessToken()).thenReturn("token123") - - interceptor.intercept(fakeChain).run { - Assert.assertEquals("bearer ddg:token123", headers["Authorization"]) - assertFalse(headers.names().contains("NetP-Debug-Code")) - } - } - - @Test - fun whenUrlIsLocationsAndFlavorIsPlayThenOnlyAddTokenToHeader() = runTest { - val fakeChain = FakeChain(url = "https://controller.netp.duckduckgo.com/locations") - whenever(appBuildConfig.flavor).thenReturn(PLAY) - whenever(subscriptions.getAccessToken()).thenReturn("token123") - - interceptor.intercept(fakeChain).run { - Assert.assertEquals("bearer ddg:token123", headers["Authorization"]) - assertFalse(headers.names().contains("NetP-Debug-Code")) - } - } - - @Test - fun whenUrlIsRegisterAndFlavorIsPlayThenOnlyAddTokenToHeader() = runTest { - val fakeChain = FakeChain(url = "https://staging1.netp.duckduckgo.com/register") - whenever(appBuildConfig.flavor).thenReturn(PLAY) - whenever(subscriptions.getAccessToken()).thenReturn("token123") - - interceptor.intercept(fakeChain).run { - Assert.assertEquals("bearer ddg:token123", headers["Authorization"]) - assertFalse(headers.names().contains("NetP-Debug-Code")) - } - } - - @Test - fun whenUrlIsNotNetPAndFlavorIsInternalThenDoNothingWithHeaders() = runTest { - val fakeChain = FakeChain(url = "https://improving.duckduckgo.com/t/m_netp_ev_enabled_android_phone?atb=v336-7&appVersion=5.131.0&test=1") - - interceptor.intercept(fakeChain).run { - Assert.assertNull(headers["Authorization"]) - assertFalse(headers.names().contains("NetP-Debug-Code")) - } - } - - @Test - fun whenUrlIsNetPAndFlavorIsInternalThenAddTokenAndDebugCodeToHeader() = runTest { - val fakeChain = FakeChain(url = "https://controller.netp.duckduckgo.com/servers") - whenever(appBuildConfig.flavor).thenReturn(INTERNAL) - whenever(subscriptions.getAccessToken()).thenReturn("token123") - - interceptor.intercept(fakeChain).run { - Assert.assertEquals("bearer ddg:token123", headers["Authorization"]) - assertTrue(headers.names().contains("NetP-Debug-Code")) - } + private interface FakeApiService { + @AuthRequired + fun endpointRequiringAuthentication() + fun endpointNotRequiringAuthentication() } }