KTOR-7483 Allow auth header when client is not shared (#4368)

ktorio · Oct 7, 2024 · 0665736 · 0665736
1 parent e307968
commit 0665736
Show file tree

Hide file tree

Showing 4 changed files with 25 additions and 47 deletions.
diff --git a/ktor-client/ktor-client-core/api/ktor-client-core.api b/ktor-client/ktor-client-core/api/ktor-client-core.api
@@ -598,7 +598,7 @@ public final class io/ktor/client/plugins/api/TransformResponseBodyContext {
 
 public final class io/ktor/client/plugins/cache/HttpCache {
 	public static final field Companion Lio/ktor/client/plugins/cache/HttpCache$Companion;
-	public synthetic fun <init> (Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;Lio/ktor/client/plugins/cache/storage/CacheStorage;Lio/ktor/client/plugins/cache/storage/CacheStorage;ZZZLkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public synthetic fun <init> (Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;Lio/ktor/client/plugins/cache/storage/CacheStorage;Lio/ktor/client/plugins/cache/storage/CacheStorage;ZZLkotlin/jvm/internal/DefaultConstructorMarker;)V
 }
 
 public final class io/ktor/client/plugins/cache/HttpCache$Companion : io/ktor/client/plugins/HttpClientPlugin {
@@ -612,13 +612,11 @@ public final class io/ktor/client/plugins/cache/HttpCache$Companion : io/ktor/cl
 
 public final class io/ktor/client/plugins/cache/HttpCache$Config {
 	public fun <init> ()V
-	public final fun getCacheRequestWithAuth ()Z
 	public final fun getPrivateStorage ()Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;
 	public final fun getPublicStorage ()Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;
 	public final fun isShared ()Z
 	public final fun privateStorage (Lio/ktor/client/plugins/cache/storage/CacheStorage;)V
 	public final fun publicStorage (Lio/ktor/client/plugins/cache/storage/CacheStorage;)V
-	public final fun setCacheRequestWithAuth (Z)V
 	public final fun setPrivateStorage (Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;)V
 	public final fun setPublicStorage (Lio/ktor/client/plugins/cache/storage/HttpCacheStorage;)V
 	public final fun setShared (Z)V

diff --git a/ktor-client/ktor-client-core/api/ktor-client-core.klib.api b/ktor-client/ktor-client-core/api/ktor-client-core.klib.api
@@ -333,9 +333,6 @@ final class io.ktor.client.plugins.cache/HttpCache { // io.ktor.client.plugins.c
     final class Config { // io.ktor.client.plugins.cache/HttpCache.Config|null[0]
         constructor <init>() // io.ktor.client.plugins.cache/HttpCache.Config.<init>|<init>(){}[0]
 
-        final var cacheRequestWithAuth // io.ktor.client.plugins.cache/HttpCache.Config.cacheRequestWithAuth|{}cacheRequestWithAuth[0]
-            final fun <get-cacheRequestWithAuth>(): kotlin/Boolean // io.ktor.client.plugins.cache/HttpCache.Config.cacheRequestWithAuth.<get-cacheRequestWithAuth>|<get-cacheRequestWithAuth>(){}[0]
-            final fun <set-cacheRequestWithAuth>(kotlin/Boolean) // io.ktor.client.plugins.cache/HttpCache.Config.cacheRequestWithAuth.<set-cacheRequestWithAuth>|<set-cacheRequestWithAuth>(kotlin.Boolean){}[0]
         final var isShared // io.ktor.client.plugins.cache/HttpCache.Config.isShared|{}isShared[0]
             final fun <get-isShared>(): kotlin/Boolean // io.ktor.client.plugins.cache/HttpCache.Config.isShared.<get-isShared>|<get-isShared>(){}[0]
             final fun <set-isShared>(kotlin/Boolean) // io.ktor.client.plugins.cache/HttpCache.Config.isShared.<set-isShared>|<set-isShared>(kotlin.Boolean){}[0]

diff --git a/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt b/ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt
@@ -41,17 +41,18 @@ internal val LOGGER = KtorSimpleLogger("io.ktor.client.plugins.HttpCache")
  * You can learn more from [Caching](https://ktor.io/docs/client-caching.html).
  */
 public class HttpCache private constructor(
-    @Deprecated("This will become internal", level = DeprecationLevel.ERROR)
-    @Suppress("DEPRECATION_ERROR")
-    internal val publicStorage: HttpCacheStorage,
-    @Deprecated("This will become internal", level = DeprecationLevel.ERROR)
-    @Suppress("DEPRECATION_ERROR")
-    internal val privateStorage: HttpCacheStorage,
+    @Deprecated(
+        "This will become internal",
+        level = DeprecationLevel.ERROR
+    ) @Suppress("DEPRECATION_ERROR") internal val publicStorage: HttpCacheStorage,
+    @Deprecated(
+        "This will become internal",
+        level = DeprecationLevel.ERROR
+    ) @Suppress("DEPRECATION_ERROR") internal val privateStorage: HttpCacheStorage,
     private val publicStorageNew: CacheStorage,
     private val privateStorageNew: CacheStorage,
     private val useOldStorage: Boolean,
     internal val isSharedClient: Boolean,
-    internal val cacheRequestWithAuth: Boolean
 ) {
     /**
      * A configuration for the [HttpCache] plugin.
@@ -62,17 +63,6 @@ public class HttpCache private constructor(
         internal var privateStorageNew: CacheStorage = CacheStorage.Unlimited()
         internal var useOldStorage = false
 
-        /**
-         * Specifies if requests with Authorization header should be cached.
-         *
-         * According to specification, enabling this flag has security implications.
-         * See https://datatracker.ietf.org/doc/html/rfc2616#section-14.8,
-         * https://datatracker.ietf.org/doc/html/rfc7234#section-3,
-         * and https://datatracker.ietf.org/doc/html/rfc9111#section-3 for the details
-         */
-        @Deprecated("Changing this flag has security implication", level = DeprecationLevel.WARNING)
-        public var cacheRequestWithAuth: Boolean = false
-
         /**
          * Specifies if the client where this plugin is installed is shared among multiple users.
          * When set to true, all responses with `private` Cache-Control directive will not be cached.
@@ -143,15 +133,13 @@ public class HttpCache private constructor(
             val config = Config().apply(block)
 
             with(config) {
-                @Suppress("DEPRECATION_ERROR")
-                return HttpCache(
+                @Suppress("DEPRECATION_ERROR") return HttpCache(
                     publicStorage = publicStorage,
                     privateStorage = privateStorage,
                     publicStorageNew = publicStorageNew,
                     privateStorageNew = privateStorageNew,
                     useOldStorage = useOldStorage,
-                    isSharedClient = isShared,
-                    cacheRequestWithAuth = cacheRequestWithAuth
+                    isSharedClient = isShared
                 )
             }
         }
@@ -165,7 +153,7 @@ public class HttpCache private constructor(
                 if (content !is OutgoingContent.NoContent) return@intercept
                 if (context.method != HttpMethod.Get || !context.url.protocol.canStore()) return@intercept
 
-                if (!plugin.cacheRequestWithAuth && context.headers.contains(HttpHeaders.Authorization)) {
+                if (plugin.isSharedClient && context.headers.contains(HttpHeaders.Authorization)) {
                     return@intercept
                 }
 
@@ -187,9 +175,8 @@ public class HttpCache private constructor(
                 val validateStatus = shouldValidate(cache.expires, cache.headers, context)
 
                 if (validateStatus == ValidateStatus.ShouldNotValidate) {
-                    val cachedCall = cache
-                        .createResponse(scope, RequestForCache(context.build()), context.executionContext)
-                        .call
+                    val cachedCall =
+                        cache.createResponse(scope, RequestForCache(context.build()), context.executionContext).call
                     proceedWithCache(scope, cachedCall)
                     return@intercept
                 }
@@ -221,17 +208,19 @@ public class HttpCache private constructor(
                     LOGGER.trace("Caching response for ${response.call.request.url}")
                     val cachedData = plugin.cacheResponse(response)
                     if (cachedData != null) {
-                        val reusableResponse = cachedData
-                            .createResponse(scope, response.request, response.coroutineContext)
+                        val reusableResponse =
+                            cachedData.createResponse(scope, response.request, response.coroutineContext)
                         proceedWith(reusableResponse)
                         return@intercept
                     }
                 }
 
                 if (response.status == HttpStatusCode.NotModified) {
                     LOGGER.trace("Not modified response for ${response.call.request.url}, replying from cache")
-                    val responseFromCache = plugin.findAndRefresh(response.call.request, response)
-                        ?: throw InvalidCacheStateException(response.call.request.url)
+                    val responseFromCache =
+                        plugin.findAndRefresh(response.call.request, response) ?: throw InvalidCacheStateException(
+                            response.call.request.url
+                        )
 
                     scope.monitor.raise(HttpResponseFromCache, responseFromCache)
                     proceedWith(responseFromCache)
@@ -340,11 +329,9 @@ public class HttpCache private constructor(
 
         else -> {
             val requestHeaders = mergedHeadersLookup(request.content, request.headers::get, request.headers::getAll)
-            storage.findAll(url)
-                .sortedByDescending { it.responseTime }
-                .firstOrNull { cachedResponse ->
-                    cachedResponse.varyKeys.all { (key, value) -> requestHeaders(key) == value }
-                }
+            storage.findAll(url).sortedByDescending { it.responseTime }.firstOrNull { cachedResponse ->
+                cachedResponse.varyKeys.all { (key, value) -> requestHeaders(key) == value }
+            }
         }
     }
 

diff --git a/ktor-client/ktor-client-core/jvm/test/HttpCacheTest.kt b/ktor-client/ktor-client-core/jvm/test/HttpCacheTest.kt
@@ -70,7 +70,7 @@ class HttpCacheTest {
     }
 
     @Test
-    fun `should mix ETags when Authorization header is present and flag enabled`() = testApplication {
+    fun `should mix ETags when Authorization header is present and client is not shared`() = testApplication {
         application {
             routing {
                 get("/me") {
@@ -92,11 +92,7 @@ class HttpCacheTest {
         }
 
         val client = createClient {
-            install(HttpCache) {
-                isShared = true
-                @Suppress("DEPRECATION")
-                cacheRequestWithAuth = true
-            }
+            install(HttpCache)
         }
 
         assertEquals(