From 655beb9dd648ec0711a336a08b0549fda3902f07 Mon Sep 17 00:00:00 2001 From: David Perez Date: Wed, 16 Oct 2024 17:01:05 -0500 Subject: [PATCH] PM-13688: Remove race condition from AuthTokenInterceptor (#4108) --- .../network/di/PlatformNetworkModule.kt | 4 ++- .../interceptor/AuthTokenInterceptor.kt | 14 ++++++++-- .../manager/NetworkConfigManagerImpl.kt | 15 ----------- .../data/platform/manager/PushManagerImpl.kt | 7 +++-- .../manager/di/PlatformManagerModule.kt | 3 --- .../interceptor/AuthTokenInterceptorTest.kt | 26 +++++++++++++++--- .../manager/NetworkConfigManagerTest.kt | 27 ------------------- 7 files changed, 40 insertions(+), 56 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt index 1c8ba730b3..ff47617734 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/di/PlatformNetworkModule.kt @@ -58,7 +58,9 @@ object PlatformNetworkModule { @Provides @Singleton - fun providesAuthTokenInterceptor(): AuthTokenInterceptor = AuthTokenInterceptor() + fun providesAuthTokenInterceptor( + authDiskSource: AuthDiskSource, + ): AuthTokenInterceptor = AuthTokenInterceptor(authDiskSource = authDiskSource) @Provides @Singleton diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptor.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptor.kt index 2bb2805f98..0f8b2c9d5c 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptor.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptor.kt @@ -1,5 +1,6 @@ package com.x8bit.bitwarden.data.platform.datasource.network.interceptor +import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource import com.x8bit.bitwarden.data.platform.datasource.network.util.HEADER_KEY_AUTHORIZATION import com.x8bit.bitwarden.data.platform.datasource.network.util.HEADER_VALUE_BEARER_PREFIX import okhttp3.Interceptor @@ -11,11 +12,20 @@ import javax.inject.Singleton * Interceptor responsible for adding the auth token(Bearer) to API requests. */ @Singleton -class AuthTokenInterceptor : Interceptor { +class AuthTokenInterceptor( + private val authDiskSource: AuthDiskSource, +) : Interceptor { /** * The auth token to be added to API requests. + * + * Note: This is done on demand to ensure that no race conditions can exist when retrieving the + * token. */ - var authToken: String? = null + private val authToken: String? + get() = authDiskSource + .userState + ?.activeUserId + ?.let { userId -> authDiskSource.getAccountTokens(userId = userId)?.accessToken } private val missingTokenMessage = "Auth token is missing!" 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 10e017b06d..6a7b9b0efd 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 @@ -1,9 +1,7 @@ package com.x8bit.bitwarden.data.platform.manager import com.x8bit.bitwarden.data.auth.repository.AuthRepository -import com.x8bit.bitwarden.data.auth.repository.model.AuthState import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.RefreshAuthenticator -import com.x8bit.bitwarden.data.platform.datasource.network.interceptor.AuthTokenInterceptor 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 @@ -18,10 +16,8 @@ private const val ENVIRONMENT_DEBOUNCE_TIMEOUT_MS: Long = 500L /** * Primary implementation of [NetworkConfigManager]. */ -@Suppress("LongParameterList") class NetworkConfigManagerImpl( authRepository: AuthRepository, - private val authTokenInterceptor: AuthTokenInterceptor, environmentRepository: EnvironmentRepository, serverConfigRepository: ServerConfigRepository, private val baseUrlInterceptors: BaseUrlInterceptors, @@ -32,17 +28,6 @@ class NetworkConfigManagerImpl( private val collectionScope = CoroutineScope(dispatcherManager.unconfined) init { - authRepository - .authStateFlow - .onEach { authState -> - authTokenInterceptor.authToken = when (authState) { - is AuthState.Authenticated -> authState.accessToken - is AuthState.Unauthenticated -> null - is AuthState.Uninitialized -> null - } - } - .launchIn(collectionScope) - @Suppress("OPT_IN_USAGE") environmentRepository .environmentStateFlow diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/PushManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/PushManagerImpl.kt index 1f37ba9769..85ad1603eb 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/PushManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/PushManagerImpl.kt @@ -1,6 +1,7 @@ package com.x8bit.bitwarden.data.platform.manager import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource +import com.x8bit.bitwarden.data.auth.repository.util.activeUserIdChangesFlow import com.x8bit.bitwarden.data.platform.datasource.disk.PushDiskSource import com.x8bit.bitwarden.data.platform.datasource.network.model.PushTokenRequest import com.x8bit.bitwarden.data.platform.datasource.network.service.PushService @@ -21,7 +22,6 @@ import com.x8bit.bitwarden.data.platform.util.decodeFromStringOrNull import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.SharedFlow import kotlinx.coroutines.flow.asSharedFlow -import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.mapNotNull import kotlinx.coroutines.flow.onEach @@ -100,9 +100,8 @@ class PushManagerImpl @Inject constructor( init { authDiskSource - .userStateFlow - .mapNotNull { it?.activeUserId } - .distinctUntilChanged() + .activeUserIdChangesFlow + .mapNotNull { it } .onEach { registerStoredPushTokenIfNecessary() } .launchIn(unconfinedScope) } 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 3c3873ff8d..34e9e12c73 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 @@ -10,7 +10,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.AuthTokenInterceptor 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 @@ -185,7 +184,6 @@ object PlatformManagerModule { @Singleton fun provideNetworkConfigManager( authRepository: AuthRepository, - authTokenInterceptor: AuthTokenInterceptor, environmentRepository: EnvironmentRepository, serverConfigRepository: ServerConfigRepository, baseUrlInterceptors: BaseUrlInterceptors, @@ -194,7 +192,6 @@ object PlatformManagerModule { ): NetworkConfigManager = NetworkConfigManagerImpl( authRepository = authRepository, - authTokenInterceptor = authTokenInterceptor, environmentRepository = environmentRepository, serverConfigRepository = serverConfigRepository, baseUrlInterceptors = baseUrlInterceptors, diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptorTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptorTest.kt index 24f97faed5..a5420b980e 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptorTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/interceptor/AuthTokenInterceptorTest.kt @@ -1,5 +1,9 @@ package com.x8bit.bitwarden.data.platform.datasource.network.interceptor +import com.x8bit.bitwarden.data.auth.datasource.disk.model.AccountTokensJson +import com.x8bit.bitwarden.data.auth.datasource.disk.model.UserStateJson +import com.x8bit.bitwarden.data.auth.datasource.disk.util.FakeAuthDiskSource +import io.mockk.mockk import junit.framework.TestCase.assertEquals import okhttp3.Request import org.junit.Assert.assertThrows @@ -9,8 +13,10 @@ import javax.inject.Singleton @Singleton class AuthTokenInterceptorTest { - private val interceptor: AuthTokenInterceptor = AuthTokenInterceptor() - private val mockAuthToken = "yourAuthToken" + private val authDiskSource = FakeAuthDiskSource() + private val interceptor: AuthTokenInterceptor = AuthTokenInterceptor( + authDiskSource = authDiskSource, + ) private val request: Request = Request .Builder() .url("http://localhost") @@ -18,12 +24,13 @@ class AuthTokenInterceptorTest { @Test fun `intercept should add the auth token when set`() { - interceptor.authToken = mockAuthToken + authDiskSource.userState = USER_STATE + authDiskSource.storeAccountTokens(userId = USER_ID, ACCOUNT_TOKENS) val response = interceptor.intercept( chain = FakeInterceptorChain(request = request), ) assertEquals( - "Bearer $mockAuthToken", + "Bearer $ACCESS_TOKEN", response.request.header("Authorization"), ) } @@ -41,3 +48,14 @@ class AuthTokenInterceptorTest { ) } } + +private const val USER_ID: String = "user_id" +private const val ACCESS_TOKEN: String = "access_token" +private val USER_STATE: UserStateJson = UserStateJson( + activeUserId = USER_ID, + accounts = mapOf(USER_ID to mockk()), +) +private val ACCOUNT_TOKENS: AccountTokensJson = AccountTokensJson( + accessToken = ACCESS_TOKEN, + refreshToken = null, +) 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 60fd11e676..ef3fa46d2d 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.AuthTokenInterceptor 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 @@ -19,7 +18,6 @@ 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.Assertions.assertNull import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test @@ -44,7 +42,6 @@ class NetworkConfigManagerTest { } private val refreshAuthenticator = RefreshAuthenticator() - private val authTokenInterceptor = AuthTokenInterceptor() private val baseUrlInterceptors = BaseUrlInterceptors() private lateinit var networkConfigManager: NetworkConfigManager @@ -53,7 +50,6 @@ class NetworkConfigManagerTest { fun setUp() { networkConfigManager = NetworkConfigManagerImpl( authRepository = authRepository, - authTokenInterceptor = authTokenInterceptor, environmentRepository = environmentRepository, serverConfigRepository = serverConfigRepository, baseUrlInterceptors = baseUrlInterceptors, @@ -62,29 +58,6 @@ class NetworkConfigManagerTest { ) } - @Test - fun `authenticatorProvider should be set on initialization`() { - assertEquals( - authRepository, - refreshAuthenticator.authenticatorProvider, - ) - } - - @Test - fun `changes in the AuthState should update the AuthTokenInterceptor`() { - mutableAuthStateFlow.value = AuthState.Uninitialized - assertNull(authTokenInterceptor.authToken) - - mutableAuthStateFlow.value = AuthState.Authenticated(accessToken = "accessToken") - assertEquals( - "accessToken", - authTokenInterceptor.authToken, - ) - - mutableAuthStateFlow.value = AuthState.Unauthenticated - assertNull(authTokenInterceptor.authToken) - } - @Test fun `changes in the Environment should update the BaseUrlInterceptors`() { mutableEnvironmentStateFlow.value = Environment.Us