From ac20105b9bbd364f28ec785f18c00e0321a59366 Mon Sep 17 00:00:00 2001 From: Kirill Kamakin Date: Sat, 10 Dec 2022 10:32:13 +0100 Subject: [PATCH] Create OkHttp authenticator without token invalidation --- .../datasource/AuthenticationProvider.kt | 1 - .../mealient/datasource/DataSourceModule.kt | 8 +- .../datasource/impl/AuthInterceptor.kt | 58 ------- .../datasource/impl/MealieAuthenticator.kt | 40 +++++ .../datasource/impl/OkHttpBuilderImpl.kt | 3 + .../datasource/AuthInterceptorTest.kt | 164 ------------------ .../datasource/MealieAuthenticatorTest.kt | 90 ++++++++++ 7 files changed, 136 insertions(+), 228 deletions(-) delete mode 100644 datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/AuthInterceptor.kt create mode 100644 datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieAuthenticator.kt delete mode 100644 datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/AuthInterceptorTest.kt create mode 100644 datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/MealieAuthenticatorTest.kt diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/AuthenticationProvider.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/AuthenticationProvider.kt index b43f93a..7df0e88 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/AuthenticationProvider.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/AuthenticationProvider.kt @@ -4,5 +4,4 @@ interface AuthenticationProvider { suspend fun getAuthHeader(): String? - suspend fun invalidateAuthHeader() } \ No newline at end of file diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/DataSourceModule.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/DataSourceModule.kt index 73ed120..2ffa990 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/DataSourceModule.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/DataSourceModule.kt @@ -6,9 +6,8 @@ import dagger.Module import dagger.Provides import dagger.hilt.InstallIn import dagger.hilt.components.SingletonComponent -import dagger.multibindings.IntoSet -import gq.kirmanak.mealient.datasource.impl.AuthInterceptor import gq.kirmanak.mealient.datasource.impl.CacheBuilderImpl +import gq.kirmanak.mealient.datasource.impl.MealieAuthenticator import gq.kirmanak.mealient.datasource.impl.NetworkRequestWrapperImpl import gq.kirmanak.mealient.datasource.impl.OkHttpBuilderImpl import gq.kirmanak.mealient.datasource.impl.RetrofitBuilder @@ -20,7 +19,7 @@ import gq.kirmanak.mealient.datasource.v1.MealieDataSourceV1Impl import gq.kirmanak.mealient.datasource.v1.MealieServiceV1 import kotlinx.serialization.ExperimentalSerializationApi import kotlinx.serialization.json.Json -import okhttp3.Interceptor +import okhttp3.Authenticator import okhttp3.MediaType.Companion.toMediaType import okhttp3.OkHttpClient import retrofit2.Converter @@ -91,6 +90,5 @@ interface DataSourceModule { @Binds @Singleton - @IntoSet - fun bindAuthInterceptor(authInterceptor: AuthInterceptor): Interceptor + fun bindAuthenticator(mealieAuthenticator: MealieAuthenticator): Authenticator } \ No newline at end of file diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/AuthInterceptor.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/AuthInterceptor.kt deleted file mode 100644 index 5f2fc71..0000000 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/AuthInterceptor.kt +++ /dev/null @@ -1,58 +0,0 @@ -package gq.kirmanak.mealient.datasource.impl - -import androidx.annotation.VisibleForTesting -import gq.kirmanak.mealient.datasource.AuthenticationProvider -import kotlinx.coroutines.runBlocking -import okhttp3.Interceptor -import okhttp3.Response -import retrofit2.HttpException -import javax.inject.Inject -import javax.inject.Provider -import javax.inject.Singleton - -@Singleton -class AuthInterceptor @Inject constructor( - private val authenticationProvider: Provider, -) : Interceptor { - - override fun intercept(chain: Interceptor.Chain): Response { - val token = getAuthHeader() - return if (token == null) { - proceedWithAuthHeader(chain) - } else { - try { - proceedWithAuthHeader(chain, token) - } catch (e: HttpException) { - if (e.code() in setOf(401, 403)) { - invalidateAuthHeader() - proceedWithAuthHeader(chain, getAuthHeader()) - } else { - throw e - } - } - } - } - - private fun proceedWithAuthHeader(chain: Interceptor.Chain, token: String? = null): Response { - val requestBuilder = chain.request().newBuilder() - val request = if (token == null) { - requestBuilder.removeHeader(HEADER_NAME).build() - } else { - requestBuilder.header(HEADER_NAME, token).build() - } - return chain.proceed(request) - } - - private fun getAuthHeader() = runBlocking { - authenticationProvider.get().getAuthHeader() - } - - private fun invalidateAuthHeader() = runBlocking { - authenticationProvider.get().invalidateAuthHeader() - } - - companion object { - @VisibleForTesting - const val HEADER_NAME = "Authorization" - } -} diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieAuthenticator.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieAuthenticator.kt new file mode 100644 index 0000000..bf5df70 --- /dev/null +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/MealieAuthenticator.kt @@ -0,0 +1,40 @@ +package gq.kirmanak.mealient.datasource.impl + +import androidx.annotation.VisibleForTesting +import gq.kirmanak.mealient.datasource.AuthenticationProvider +import kotlinx.coroutines.runBlocking +import okhttp3.Authenticator +import okhttp3.Request +import okhttp3.Response +import okhttp3.Route +import javax.inject.Inject +import javax.inject.Provider +import javax.inject.Singleton + +@Singleton +class MealieAuthenticator @Inject constructor( + private val authenticationProvider: Provider, +) : Authenticator { + + override fun authenticate(route: Route?, response: Response): Request? { + val supportsBearer = response.challenges().any { it.scheme == BEARER_SCHEME } + val request = response.request + return if (supportsBearer && request.header(HEADER_NAME) == null) { + getAuthHeader()?.let { request.copyWithHeader(HEADER_NAME, it) } + } else { + null // Either Bearer is not supported or we've already tried to authenticate + } + } + + private fun getAuthHeader() = runBlocking { authenticationProvider.get().getAuthHeader() } + + companion object { + @VisibleForTesting + const val HEADER_NAME = "Authorization" + private const val BEARER_SCHEME = "Bearer" + } +} + +private fun Request.copyWithHeader(name: String, value: String): Request { + return newBuilder().header(name, value).build() +} \ No newline at end of file diff --git a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/OkHttpBuilderImpl.kt b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/OkHttpBuilderImpl.kt index 21ca1d1..bb2a9a6 100644 --- a/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/OkHttpBuilderImpl.kt +++ b/datasource/src/main/kotlin/gq/kirmanak/mealient/datasource/impl/OkHttpBuilderImpl.kt @@ -2,6 +2,7 @@ package gq.kirmanak.mealient.datasource.impl import gq.kirmanak.mealient.datasource.CacheBuilder import gq.kirmanak.mealient.datasource.OkHttpBuilder +import okhttp3.Authenticator import okhttp3.Interceptor import okhttp3.OkHttpClient import javax.inject.Inject @@ -12,10 +13,12 @@ class OkHttpBuilderImpl @Inject constructor( private val cacheBuilder: CacheBuilder, // Use @JvmSuppressWildcards because otherwise dagger can't inject it (https://stackoverflow.com/a/43149382) private val interceptors: Set<@JvmSuppressWildcards Interceptor>, + private val authenticator: Authenticator, ) : OkHttpBuilder { override fun buildOkHttp(): OkHttpClient = OkHttpClient.Builder() .apply { interceptors.forEach(::addNetworkInterceptor) } .cache(cacheBuilder.buildCache()) + .authenticator(authenticator) .build() } \ No newline at end of file diff --git a/datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/AuthInterceptorTest.kt b/datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/AuthInterceptorTest.kt deleted file mode 100644 index b89c2d8..0000000 --- a/datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/AuthInterceptorTest.kt +++ /dev/null @@ -1,164 +0,0 @@ -package gq.kirmanak.mealient.datasource - -import com.google.common.truth.Truth.assertThat -import gq.kirmanak.mealient.datasource.impl.AuthInterceptor -import gq.kirmanak.mealient.test.BaseUnitTest -import io.mockk.CapturingSlot -import io.mockk.coEvery -import io.mockk.coVerify -import io.mockk.coVerifySequence -import io.mockk.every -import io.mockk.impl.annotations.MockK -import io.mockk.slot -import io.mockk.verify -import okhttp3.Interceptor -import okhttp3.Protocol -import okhttp3.Request -import okhttp3.Response -import okhttp3.ResponseBody.Companion.toResponseBody -import org.junit.Before -import org.junit.Test -import retrofit2.HttpException -import retrofit2.Response as RetrofitResponse - -class AuthInterceptorTest : BaseUnitTest() { - - private lateinit var subject: AuthInterceptor - - @MockK(relaxUnitFun = true) - lateinit var authenticationProvider: AuthenticationProvider - - @MockK(relaxUnitFun = true) - lateinit var chain: Interceptor.Chain - - @Before - override fun setUp() { - super.setUp() - subject = AuthInterceptor { authenticationProvider } - every { chain.request() } returns Request.Builder().url("http://localhost").build() - } - - @Test - fun `when no header then still proceeds`() { - mockProceedCallAndCaptureRequest() - coEvery { authenticationProvider.getAuthHeader() } returns null - subject.intercept(chain) - verify { chain.proceed(any()) } - } - - @Test - fun `when has header then adds header`() { - coEvery { authenticationProvider.getAuthHeader() } returns "token" - val requestSlot = mockProceedCallAndCaptureRequest() - subject.intercept(chain) - assertThat(requestSlot.captured.header(AuthInterceptor.HEADER_NAME)).isEqualTo("token") - } - - @Test(expected = HttpException::class) - fun `when unauthorized but didn't have token then throws`() { - coEvery { authenticationProvider.getAuthHeader() } returns null - mockUnauthorized() - subject.intercept(chain) - } - - @Test(expected = HttpException::class) - fun `when unauthorized and had token then invalidates it`() { - coEvery { authenticationProvider.getAuthHeader() } returns "token" - mockUnauthorized() - subject.intercept(chain) - coVerify { authenticationProvider.invalidateAuthHeader() } - } - - @Test(expected = HttpException::class) - fun `when not found and had token then throws exception`() { - coEvery { authenticationProvider.getAuthHeader() } returns "token" - val requestSlot = slot() - every { - chain.proceed(capture(requestSlot)) - } answers { - throw HttpException(RetrofitResponse.error(404, "".toResponseBody())) - } - subject.intercept(chain) - } - - @Test - fun `when not found and had token then does not invalidate it`() { - coEvery { authenticationProvider.getAuthHeader() } returns "token" - val requestSlot = slot() - every { - chain.proceed(capture(requestSlot)) - } answers { - throw HttpException(RetrofitResponse.error(404, "".toResponseBody())) - } - try { - subject.intercept(chain) - } catch (e: HttpException) { - coVerify(inverse = true) { authenticationProvider.invalidateAuthHeader() } - } - } - - @Test - fun `when unauthorized and had token then calls again with new token`() { - coEvery { authenticationProvider.getAuthHeader() } returns "token" andThen "newToken" - val requests = mutableListOf() - every { - chain.proceed(capture(requests)) - } answers { - throw HttpException(RetrofitResponse.error(401, "".toResponseBody())) - } andThenAnswer { - buildResponse(requests[1]) - } - subject.intercept(chain) - coVerifySequence { - authenticationProvider.getAuthHeader() - authenticationProvider.invalidateAuthHeader() - authenticationProvider.getAuthHeader() - } - assertThat(requests[0].header(AuthInterceptor.HEADER_NAME)).isEqualTo("token") - assertThat(requests[1].header(AuthInterceptor.HEADER_NAME)).isEqualTo("newToken") - } - - @Test - fun `when had token but now does not then removes it`() { - coEvery { authenticationProvider.getAuthHeader() } returns null - val mockRequest = Request.Builder() - .url("http://localhost") - .header(AuthInterceptor.HEADER_NAME, "token") - .build() - every { chain.request() } returns mockRequest - val requestSlot = mockProceedCallAndCaptureRequest() - subject.intercept(chain) - assertThat(requestSlot.captured.header(AuthInterceptor.HEADER_NAME)).isNull() - } - - private fun mockUnauthorized() { - val requestSlot = slot() - every { - chain.proceed(capture(requestSlot)) - } answers { - throw HttpException(RetrofitResponse.error(401, "".toResponseBody())) - } - } - - private fun mockProceedCallAndCaptureRequest(): CapturingSlot { - val requestSlot = slot() - every { - chain.proceed(capture(requestSlot)) - } answers { - buildResponse(requestSlot.captured) - } - return requestSlot - } - - private fun buildResponse( - request: Request, - code: Int = 200, - protocol: Protocol = Protocol.HTTP_2, - message: String = if (code == 200) "OK" else "NOT OK", - ) = Response.Builder() - .code(code) - .request(request) - .protocol(protocol) - .message(message) - .build() -} \ No newline at end of file diff --git a/datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/MealieAuthenticatorTest.kt b/datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/MealieAuthenticatorTest.kt new file mode 100644 index 0000000..41d8714 --- /dev/null +++ b/datasource/src/test/kotlin/gq/kirmanak/mealient/datasource/MealieAuthenticatorTest.kt @@ -0,0 +1,90 @@ +package gq.kirmanak.mealient.datasource + +import com.google.common.truth.Truth.assertThat +import gq.kirmanak.mealient.datasource.impl.MealieAuthenticator +import gq.kirmanak.mealient.datasource.impl.MealieAuthenticator.Companion.HEADER_NAME +import gq.kirmanak.mealient.test.BaseUnitTest +import io.mockk.coEvery +import io.mockk.coVerify +import io.mockk.impl.annotations.MockK +import okhttp3.Protocol +import okhttp3.Request +import okhttp3.Response +import org.junit.Before +import org.junit.Test + +class MealieAuthenticatorTest : BaseUnitTest() { + + private lateinit var subject: MealieAuthenticator + + @MockK + lateinit var authenticationProvider: AuthenticationProvider + + @Before + override fun setUp() { + super.setUp() + subject = MealieAuthenticator { authenticationProvider } + } + + @Test + fun `when bearer is not supported expect authenticate to return null`() { + val response = buildResponse(challenges = null) + assertThat(subject.authenticate(null, response)).isNull() + } + + @Test + fun `when no auth header exists expect authenticate to return null`() { + coEvery { authenticationProvider.getAuthHeader() } returns null + val response = buildResponse() + assertThat(subject.authenticate(null, response)).isNull() + } + + @Test + fun `when no auth header exists expect authenticate to call provider`() { + coEvery { authenticationProvider.getAuthHeader() } returns null + val response = buildResponse() + subject.authenticate(null, response) + coVerify { authenticationProvider.getAuthHeader() } + } + + @Test + fun `when no auth header was set expect authenticate to return null`() { + val response = buildResponse(authHeader = "token") + assertThat(subject.authenticate(null, response)).isNull() + } + + @Test + fun `when auth header exists expect authenticate to return request`() { + coEvery { authenticationProvider.getAuthHeader() } returns "token" + val response = buildResponse() + val actualHeader = subject.authenticate(null, response)?.header(HEADER_NAME) + assertThat(actualHeader).isEqualTo("token") + } + + private fun buildResponse( + url: String = "http://localhost", + code: Int = 401, + message: String = "Unauthorized", + protocol: Protocol = Protocol.HTTP_2, + challenges: String? = "Bearer", + authHeader: String? = null, + ): Response { + val request = buildRequest(authHeader, url) + return Response.Builder().apply { + request(request) + code(code) + message(message) + protocol(protocol) + if (challenges != null) header("WWW-Authenticate", challenges) + }.build() + } + + private fun buildRequest( + authHeader: String? = null, + url: String = "http://localhost", + ): Request = Request.Builder().apply { + url(url) + if (authHeader != null) header(HEADER_NAME, authHeader) + }.build() + +} \ No newline at end of file