From 3092ba1fc663487ccc631e569a57718677237065 Mon Sep 17 00:00:00 2001 From: David Perez Date: Wed, 20 Nov 2024 13:36:43 -0600 Subject: [PATCH] PM-15110: Ensure all network requests always use the current environment data (#4344) --- .../network/interceptor/BaseUrlInterceptor.kt | 30 ++---- .../interceptor/BaseUrlInterceptors.kt | 37 +++---- .../manager/NetworkConfigManagerImpl.kt | 5 - .../manager/di/PlatformManagerModule.kt | 3 - .../disk/FakeEnvironmentDiskSource.kt | 34 ++++++ .../interceptor/BaseUrlInterceptorTest.kt | 5 +- .../interceptor/BaseUrlInterceptorsTest.kt | 101 ------------------ .../manager/NetworkConfigManagerTest.kt | 15 +-- .../repository/EnvironmentRepositoryTest.kt | 32 +----- 9 files changed, 66 insertions(+), 196 deletions(-) create mode 100644 app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/FakeEnvironmentDiskSource.kt delete mode 100644 app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorsTest.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptor.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptor.kt index 6c2f61ab6..b098b9d7e 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptor.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptor.kt @@ -6,37 +6,27 @@ import okhttp3.Interceptor import okhttp3.Response /** - * A [Interceptor] that optionally takes the current base URL of a request and replaces it with - * the currently set [baseUrl] + * An [Interceptor] that optionally takes the current base URL of a request and replaces it with + * the currently set base URL from the [baseUrlProvider]. */ -class BaseUrlInterceptor : Interceptor { +class BaseUrlInterceptor( + private val baseUrlProvider: () -> String?, +) : Interceptor { - /** - * The base URL to use as an override, or `null` if no override should be performed. - */ - var baseUrl: String? = null - set(value) { - field = value - baseHttpUrl = baseUrl?.let { requireNotNull(it.toHttpUrlOrNull()) } - } - - private var baseHttpUrl: HttpUrl? = null + private val baseHttpUrl: HttpUrl? + get() = baseUrlProvider()?.let { requireNotNull(it.toHttpUrlOrNull()) } override fun intercept(chain: Interceptor.Chain): Response { val request = chain.request() // If no base URL is set, we can simply skip - val base = baseHttpUrl ?: return chain.proceed(request) + val base = baseHttpUrl ?: return chain.proceed(request = request) // Update the base URL used. return chain.proceed( - request + request = request .newBuilder() - .url( - request - .url - .replaceBaseUrlWith(base), - ) + .url(url = request.url.replaceBaseUrlWith(baseUrl = base)) .build(), ) } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptors.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptors.kt index f1f6bc042..d4b174724 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptors.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptors.kt @@ -1,47 +1,44 @@ package com.x8bit.bitwarden.data.platform.datasource.network.interceptor +import com.x8bit.bitwarden.data.platform.annotation.OmitFromCoverage +import com.x8bit.bitwarden.data.platform.datasource.disk.EnvironmentDiskSource import com.x8bit.bitwarden.data.platform.repository.model.Environment import com.x8bit.bitwarden.data.platform.repository.util.baseApiUrl import com.x8bit.bitwarden.data.platform.repository.util.baseEventsUrl import com.x8bit.bitwarden.data.platform.repository.util.baseIdentityUrl +import com.x8bit.bitwarden.data.platform.repository.util.toEnvironmentUrlsOrDefault import javax.inject.Inject import javax.inject.Singleton /** * An overall container for various [BaseUrlInterceptor] implementations for different API groups. */ +@OmitFromCoverage @Singleton -class BaseUrlInterceptors @Inject constructor() { - var environment: Environment = Environment.Us - set(value) { - field = value - updateBaseUrls(environment = value) - } +class BaseUrlInterceptors @Inject constructor( + private val environmentDiskSource: EnvironmentDiskSource, +) { + private val environment: Environment + get() = environmentDiskSource.preAuthEnvironmentUrlData.toEnvironmentUrlsOrDefault() /** * An interceptor for "/api" calls. */ - val apiInterceptor: BaseUrlInterceptor = BaseUrlInterceptor() + val apiInterceptor: BaseUrlInterceptor = BaseUrlInterceptor { + environment.environmentUrlData.baseApiUrl + } /** * An interceptor for "/identity" calls. */ - val identityInterceptor: BaseUrlInterceptor = BaseUrlInterceptor() + val identityInterceptor: BaseUrlInterceptor = BaseUrlInterceptor { + environment.environmentUrlData.baseIdentityUrl + } /** * An interceptor for "/events" calls. */ - val eventsInterceptor: BaseUrlInterceptor = BaseUrlInterceptor() - - init { - // Ensure all interceptors begin with a default value - environment = Environment.Us - } - - private fun updateBaseUrls(environment: Environment) { - val environmentUrlData = environment.environmentUrlData - apiInterceptor.baseUrl = environmentUrlData.baseApiUrl - identityInterceptor.baseUrl = environmentUrlData.baseIdentityUrl - eventsInterceptor.baseUrl = environmentUrlData.baseEventsUrl + val eventsInterceptor: BaseUrlInterceptor = BaseUrlInterceptor { + environment.environmentUrlData.baseEventsUrl } } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerImpl.kt index 6a7b9b0ef..e226a3413 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerImpl.kt @@ -2,7 +2,6 @@ package com.x8bit.bitwarden.data.platform.manager import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.RefreshAuthenticator -import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlInterceptors import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository import com.x8bit.bitwarden.data.platform.repository.ServerConfigRepository @@ -20,7 +19,6 @@ class NetworkConfigManagerImpl( authRepository: AuthRepository, environmentRepository: EnvironmentRepository, serverConfigRepository: ServerConfigRepository, - private val baseUrlInterceptors: BaseUrlInterceptors, refreshAuthenticator: RefreshAuthenticator, dispatcherManager: DispatcherManager, ) : NetworkConfigManager { @@ -31,9 +29,6 @@ class NetworkConfigManagerImpl( @Suppress("OPT_IN_USAGE") environmentRepository .environmentStateFlow - .onEach { environment -> - baseUrlInterceptors.environment = environment - } .debounce(timeoutMillis = ENVIRONMENT_DEBOUNCE_TIMEOUT_MS) .onEach { _ -> // This updates the stored service configuration by performing a network request. diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt index f09e31143..b275fc259 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/di/PlatformManagerModule.kt @@ -12,7 +12,6 @@ import com.x8bit.bitwarden.data.platform.datasource.disk.PushDiskSource import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource import com.x8bit.bitwarden.data.platform.datasource.disk.legacy.LegacyAppCenterMigrator import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.RefreshAuthenticator -import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlInterceptors import com.x8bit.bitwarden.data.platform.datasource.network.service.EventService import com.x8bit.bitwarden.data.platform.datasource.network.service.PushService import com.x8bit.bitwarden.data.platform.manager.AppStateManager @@ -198,7 +197,6 @@ object PlatformManagerModule { authRepository: AuthRepository, environmentRepository: EnvironmentRepository, serverConfigRepository: ServerConfigRepository, - baseUrlInterceptors: BaseUrlInterceptors, refreshAuthenticator: RefreshAuthenticator, dispatcherManager: DispatcherManager, ): NetworkConfigManager = @@ -206,7 +204,6 @@ object PlatformManagerModule { authRepository = authRepository, environmentRepository = environmentRepository, serverConfigRepository = serverConfigRepository, - baseUrlInterceptors = baseUrlInterceptors, refreshAuthenticator = refreshAuthenticator, dispatcherManager = dispatcherManager, ) diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/FakeEnvironmentDiskSource.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/FakeEnvironmentDiskSource.kt new file mode 100644 index 000000000..d2e28ef36 --- /dev/null +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/disk/FakeEnvironmentDiskSource.kt @@ -0,0 +1,34 @@ +package com.x8bit.bitwarden.data.platform.datasource.disk + +import com.x8bit.bitwarden.data.auth.datasource.disk.model.EnvironmentUrlDataJson +import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.onSubscription + +class FakeEnvironmentDiskSource : EnvironmentDiskSource { + private val storedEmailVerificationUrls = mutableMapOf() + + override var preAuthEnvironmentUrlData: EnvironmentUrlDataJson? = null + set(value) { + field = value + mutablePreAuthEnvironmentUrlDataFlow.tryEmit(value) + } + + override val preAuthEnvironmentUrlDataFlow: Flow + get() = mutablePreAuthEnvironmentUrlDataFlow + .onSubscription { emit(preAuthEnvironmentUrlData) } + + override fun getPreAuthEnvironmentUrlDataForEmail( + userEmail: String, + ): EnvironmentUrlDataJson? = storedEmailVerificationUrls[userEmail] + + override fun storePreAuthEnvironmentUrlDataForEmail( + userEmail: String, + urls: EnvironmentUrlDataJson, + ) { + storedEmailVerificationUrls[userEmail] = urls + } + + private val mutablePreAuthEnvironmentUrlDataFlow = + bufferedMutableSharedFlow(replay = 1) +} diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorTest.kt index 172fdd7b0..52b1f6da4 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorTest.kt @@ -6,7 +6,8 @@ import org.junit.jupiter.api.Assertions.assertNotEquals import org.junit.jupiter.api.Test class BaseUrlInterceptorTest { - private val baseUrlInterceptor = BaseUrlInterceptor() + private var baseUrl: String? = null + private val baseUrlInterceptor = BaseUrlInterceptor { baseUrl } @Test fun `intercept with a null base URL should proceed with the original request`() { @@ -22,7 +23,7 @@ class BaseUrlInterceptorTest { @Test fun `intercept with a non-null base URL should update the base URL used by the request`() { - baseUrlInterceptor.baseUrl = "https://api.bitwarden.com" + baseUrl = "https://api.bitwarden.com" val request = Request.Builder().url("http://www.fake.com/").build() val chain = FakeInterceptorChain(request) diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorsTest.kt deleted file mode 100644 index cbe0b79c1..000000000 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/BaseUrlInterceptorsTest.kt +++ /dev/null @@ -1,101 +0,0 @@ -package com.x8bit.bitwarden.data.platform.datasource.network.interceptor - -import com.x8bit.bitwarden.data.auth.datasource.disk.model.EnvironmentUrlDataJson -import com.x8bit.bitwarden.data.platform.repository.model.Environment -import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Test - -class BaseUrlInterceptorsTest { - private val baseUrlInterceptors = BaseUrlInterceptors() - - @Test - fun `the default environment should be US and all interceptors should have the correct URLs`() { - assertEquals( - Environment.Us, - baseUrlInterceptors.environment, - ) - assertEquals( - "https://vault.bitwarden.com/api", - baseUrlInterceptors.apiInterceptor.baseUrl, - ) - assertEquals( - "https://vault.bitwarden.com/identity", - baseUrlInterceptors.identityInterceptor.baseUrl, - ) - assertEquals( - "https://vault.bitwarden.com/events", - baseUrlInterceptors.eventsInterceptor.baseUrl, - ) - } - - @Suppress("MaxLineLength") - @Test - fun `setting the environment should update all the interceptors correctly for a non-blank base URL`() { - baseUrlInterceptors.environment = Environment.Eu - - assertEquals( - "https://vault.bitwarden.eu/api", - baseUrlInterceptors.apiInterceptor.baseUrl, - ) - assertEquals( - "https://vault.bitwarden.eu/identity", - baseUrlInterceptors.identityInterceptor.baseUrl, - ) - assertEquals( - "https://vault.bitwarden.eu/events", - baseUrlInterceptors.eventsInterceptor.baseUrl, - ) - } - - @Suppress("MaxLineLength") - @Test - fun `setting the environment should update all the interceptors correctly for a blank base URL and all URLs filled`() { - baseUrlInterceptors.environment = Environment.SelfHosted( - environmentUrlData = EnvironmentUrlDataJson( - base = " ", - api = "https://api.com", - identity = "https://identity.com", - events = "https://events.com", - ), - ) - - assertEquals( - "https://api.com", - baseUrlInterceptors.apiInterceptor.baseUrl, - ) - assertEquals( - "https://identity.com", - baseUrlInterceptors.identityInterceptor.baseUrl, - ) - assertEquals( - "https://events.com", - baseUrlInterceptors.eventsInterceptor.baseUrl, - ) - } - - @Suppress("MaxLineLength") - @Test - fun `setting the environment should update all the interceptors correctly for a blank base URL and some or all URLs absent`() { - baseUrlInterceptors.environment = Environment.SelfHosted( - environmentUrlData = EnvironmentUrlDataJson( - base = " ", - api = "", - identity = "", - icon = " ", - ), - ) - - assertEquals( - "https://api.bitwarden.com", - baseUrlInterceptors.apiInterceptor.baseUrl, - ) - assertEquals( - "https://identity.bitwarden.com", - baseUrlInterceptors.identityInterceptor.baseUrl, - ) - assertEquals( - "https://events.bitwarden.com", - baseUrlInterceptors.eventsInterceptor.baseUrl, - ) - } -} diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerTest.kt index ef3fa46d2..78de7a958 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/NetworkConfigManagerTest.kt @@ -4,7 +4,6 @@ import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.model.AuthState import com.x8bit.bitwarden.data.platform.base.FakeDispatcherManager import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.RefreshAuthenticator -import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.BaseUrlInterceptors import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository import com.x8bit.bitwarden.data.platform.repository.ServerConfigRepository @@ -17,7 +16,6 @@ import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.UnconfinedTestDispatcher -import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test @@ -42,7 +40,6 @@ class NetworkConfigManagerTest { } private val refreshAuthenticator = RefreshAuthenticator() - private val baseUrlInterceptors = BaseUrlInterceptors() private lateinit var networkConfigManager: NetworkConfigManager @@ -52,25 +49,15 @@ class NetworkConfigManagerTest { authRepository = authRepository, environmentRepository = environmentRepository, serverConfigRepository = serverConfigRepository, - baseUrlInterceptors = baseUrlInterceptors, refreshAuthenticator = refreshAuthenticator, dispatcherManager = dispatcherManager, ) } @Test - fun `changes in the Environment should update the BaseUrlInterceptors`() { + fun `changes in the Environment should call getServerConfig after debounce period`() { mutableEnvironmentStateFlow.value = Environment.Us - assertEquals( - Environment.Us, - baseUrlInterceptors.environment, - ) - mutableEnvironmentStateFlow.value = Environment.Eu - assertEquals( - Environment.Eu, - baseUrlInterceptors.environment, - ) testDispatcher.advanceTimeByAndRunCurrent(delayTimeMillis = 500L) coVerify(exactly = 1) { serverConfigRepository.getServerConfig(forceRefresh = true) diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/EnvironmentRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/EnvironmentRepositoryTest.kt index 3e6be00b0..281752a69 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/EnvironmentRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/EnvironmentRepositoryTest.kt @@ -6,17 +6,14 @@ import com.x8bit.bitwarden.data.auth.datasource.disk.model.EnvironmentUrlDataJso import com.x8bit.bitwarden.data.auth.datasource.disk.model.UserStateJson import com.x8bit.bitwarden.data.auth.datasource.disk.util.FakeAuthDiskSource import com.x8bit.bitwarden.data.platform.base.FakeDispatcherManager -import com.x8bit.bitwarden.data.platform.datasource.disk.EnvironmentDiskSource +import com.x8bit.bitwarden.data.platform.datasource.disk.FakeEnvironmentDiskSource import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager import com.x8bit.bitwarden.data.platform.repository.model.Environment -import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow import com.x8bit.bitwarden.data.platform.repository.util.toEnvironmentUrls import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkStatic -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.onSubscription import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertEquals @@ -218,30 +215,3 @@ class EnvironmentRepositoryTest { } private const val EMAIL = "email@example.com" - -private class FakeEnvironmentDiskSource : EnvironmentDiskSource { - private val storedEmailVerificationUrls = mutableMapOf() - - override var preAuthEnvironmentUrlData: EnvironmentUrlDataJson? = null - set(value) { - field = value - mutablePreAuthEnvironmentUrlDataFlow.tryEmit(value) - } - - override val preAuthEnvironmentUrlDataFlow: Flow - get() = mutablePreAuthEnvironmentUrlDataFlow - .onSubscription { emit(preAuthEnvironmentUrlData) } - - override fun getPreAuthEnvironmentUrlDataForEmail(userEmail: String): EnvironmentUrlDataJson? = - storedEmailVerificationUrls[userEmail] - - override fun storePreAuthEnvironmentUrlDataForEmail( - userEmail: String, - urls: EnvironmentUrlDataJson, - ) { - storedEmailVerificationUrls[userEmail] = urls - } - - private val mutablePreAuthEnvironmentUrlDataFlow = - bufferedMutableSharedFlow(replay = 1) -}