diff --git a/sdk-core/api/sdk-core.api b/sdk-core/api/sdk-core.api index d8b4eae4..7ebcc312 100644 --- a/sdk-core/api/sdk-core.api +++ b/sdk-core/api/sdk-core.api @@ -761,6 +761,8 @@ public class org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep : org/ public fun (Lorg/dexpace/sdk/core/http/auth/BearerTokenProvider;Ljava/util/List;Ljava/time/Duration;Lorg/dexpace/sdk/core/util/Clock;)V public synthetic fun (Lorg/dexpace/sdk/core/http/auth/BearerTokenProvider;Ljava/util/List;Ljava/time/Duration;Lorg/dexpace/sdk/core/util/Clock;ILkotlin/jvm/internal/DefaultConstructorMarker;)V protected fun authorizeRequest (Lorg/dexpace/sdk/core/http/request/Request;)Lorg/dexpace/sdk/core/http/request/Request; + protected fun authorizeRequestOnChallenge (Lorg/dexpace/sdk/core/http/request/Request;Lorg/dexpace/sdk/core/http/response/Response;)Lorg/dexpace/sdk/core/http/request/Request; + protected fun bearerHeaderValue (Ljava/lang/String;)Ljava/lang/String; } public final class org/dexpace/sdk/core/http/pipeline/steps/DefaultAsyncInstrumentationStep : org/dexpace/sdk/core/http/pipeline/AsyncHttpStep { diff --git a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt index f514f200..ab9c1500 100644 --- a/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt +++ b/sdk-core/src/main/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStep.kt @@ -7,10 +7,12 @@ package org.dexpace.sdk.core.http.pipeline.steps +import org.dexpace.sdk.core.http.auth.AuthChallengeParser import org.dexpace.sdk.core.http.auth.BearerToken import org.dexpace.sdk.core.http.auth.BearerTokenProvider import org.dexpace.sdk.core.http.common.HttpHeaderName import org.dexpace.sdk.core.http.request.Request +import org.dexpace.sdk.core.http.response.Response import org.dexpace.sdk.core.util.Clock import java.time.Duration import java.util.concurrent.locks.ReentrantLock @@ -37,6 +39,29 @@ import kotlin.concurrent.withLock * to decide when to refresh pre-emptively (default 30 seconds). A token whose `expiresAt` * is `now + 29s` with the default margin is considered expired and is refreshed. * + * ## Eviction on 401 + * + * The refresh margin only covers *local* expiry. A server may revoke a token before its + * advertised expiry, and the cached token would otherwise keep being stamped until the + * margin elapsed. To handle this, [authorizeRequestOnChallenge] evicts the rejected token + * on a 401 + `WWW-Authenticate` response and re-stamps the request with a freshly fetched + * one, so the single retry driven by [AuthStep] carries a new credential. Eviction is an + * *additional* trigger layered on top of the margin check — it does not change pre-emptive + * refresh. Only the token that produced the 401 is evicted; a token a concurrent request + * already refreshed in the meantime is left in place. The retry runs regardless of HTTP + * method (the in-step hook is not gated by retry-safety), so a non-idempotent POST is + * re-authenticated too. + * + * Eviction only fires when the `WWW-Authenticate` header actually advertises a `Bearer` + * challenge. A 401 carrying only a non-bearer challenge (e.g. `Basic`) — or an unparseable + * one — is surfaced unchanged, because refreshing the bearer token would not satisfy it: the + * retry would just be rejected again, at the cost of a wasted token fetch and round trip. + * + * A request that reaches the challenge hook carrying **no** `Authorization` header is a + * cross-origin redirect re-issue whose credential the AUTH stage deliberately suppressed + * (see [CrossOriginRedirectMarker]). In that case the hook re-stamps nothing and surfaces the + * 401 unchanged, so the caller's token is never attached to a server-chosen foreign host. + * * ## Errors from [provider] * * - Throws → propagated. The exception is **not** cached, so a subsequent request @@ -46,8 +71,11 @@ import kotlin.concurrent.withLock * * ## Open for subclassing * - * Users wanting to override [authorizeRequestOnChallenge] (e.g. force a token refresh on - * 401) can extend this class. + * Eviction-on-401 is built in. Users wanting to customise challenge handling further (e.g. + * inspect the `WWW-Authenticate` scheme before deciding to refresh) can override + * [authorizeRequestOnChallenge]. A subclass that changes the stamped header format by + * overriding [authorizeRequest] must also override [bearerHeaderValue] so the eviction match + * keeps recognising the rejected token. * * Thread-safety: see Caching above. * Cancellation: token fetch may block; the [provider] is expected to respect interrupts. @@ -68,10 +96,84 @@ public open class BearerTokenAuthStep override fun authorizeRequest(request: Request): Request { val token = currentToken() return request.newBuilder() - .setHeader(HttpHeaderName.AUTHORIZATION.caseSensitiveName, "Bearer ${token.token}") + .setHeader(HttpHeaderName.AUTHORIZATION.caseSensitiveName, bearerHeaderValue(token.token)) .build() } + /** + * On a 401 challenge, evict the token that was just rejected and re-stamp [request] + * with a freshly fetched one so [AuthStep]'s single retry carries a new credential. + * + * Returns `null` — surfacing the 401 unchanged with no retry — in two cases: + * + * - [request] carried no `Authorization` header. A request reaches this hook + * credential-free only when the AUTH stage deliberately suppressed stamping, i.e. a + * cross-origin redirect re-issue (see [CrossOriginRedirectMarker]). Re-stamping there + * would attach the caller's bearer token to a server-chosen foreign host and re-drive it + * through the chain, leaking the token cross-origin and bypassing the HTTPS guard that + * only [AuthStep.process]'s first pass enforces. Refusing to re-stamp preserves that + * suppression. + * - [response]'s `WWW-Authenticate` header does not advertise a `Bearer` challenge. + * Refreshing the bearer token cannot satisfy a `Basic`/`Digest`-only (or unparseable) + * challenge, so the retry would only earn a second 401 after a wasted fetch and round + * trip; the original 401 is surfaced instead. + * + * Otherwise eviction is scoped to the exact token that produced the 401 (matched against + * the `Authorization` header [request] carried): if a concurrent request already refreshed + * the cache, that newer token is preserved and reused for the retry rather than being + * discarded. The subsequent [currentToken] call then re-fetches only when the cache is + * actually empty, so a 401 storm across threads still funnels through the same + * double-checked-locking fetch as the expiry-margin path. + */ + override fun authorizeRequestOnChallenge( + request: Request, + response: Response, + ): Request? { + // No credential on the rejected request means stamping was suppressed (cross-origin + // redirect). Do not re-attach one: surface the 401 unchanged. + val rejectedHeader = request.headers.get(HttpHeaderName.AUTHORIZATION) ?: return null + // A token refresh can only satisfy a Bearer challenge; surface anything else unchanged + // rather than burning a fetch + retry that the server will just reject again. + if (!offersBearerChallenge(response)) return null + evictRejectedToken(rejectedHeader) + return authorizeRequest(request) + } + + /** + * Returns `true` when [response]'s `WWW-Authenticate` header advertises a `Bearer` + * challenge. A header with only non-bearer challenges (or one that does not parse) returns + * `false`. [AuthStep] guarantees the header is present before this hook runs; the explicit + * null-guard keeps the method correct if called from elsewhere. + */ + private fun offersBearerChallenge(response: Response): Boolean { + val header = response.headers.get(HttpHeaderName.WWW_AUTHENTICATE) ?: return false + return AuthChallengeParser.parse(header).any { it.scheme == BEARER_SCHEME } + } + + /** + * Clears [cachedToken] iff it is still the token whose stamped header is [rejectedHeader]. + * Guarded by the same [lock] as the refresh path so the read-compare-clear is atomic + * against a concurrent refresh. Routes the comparison through [bearerHeaderValue] so it + * stays in lock-step with the value [authorizeRequest] stamps. + */ + private fun evictRejectedToken(rejectedHeader: String) { + lock.withLock { + val current = cachedToken ?: return + if (bearerHeaderValue(current.token) == rejectedHeader) { + cachedToken = null + } + } + } + + /** + * The `Authorization` header value for [token]. Single source of truth shared by + * [authorizeRequest] (stamping) and [evictRejectedToken] (the eviction match), so the two + * never drift. A subclass that overrides [authorizeRequest] to emit a different header + * format must override this too, or eviction will stop matching and a rejected token will + * be re-fetched but never cleared. + */ + protected open fun bearerHeaderValue(token: String): String = "Bearer $token" + private fun currentToken(): BearerToken { // Fast path: lock-free volatile read; return the cached token if it's still valid. cachedToken?.takeIf { !it.isExpiredAt(clock.now(), refreshMargin) }?.let { return it } @@ -122,5 +224,9 @@ public open class BearerTokenAuthStep // Default refresh margin: refresh the bearer token 30 seconds before its expiry // so an in-flight request never carries a near-expired credential. private const val DEFAULT_REFRESH_MARGIN_SECONDS = 30L + + // Lower-cased `Bearer` scheme name; AuthChallengeParser normalises schemes to lower + // case, so the eviction gate compares against this constant. + private const val BEARER_SCHEME = "bearer" } } diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt index d55dab0e..5105405a 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/AuthStepTest.kt @@ -519,15 +519,16 @@ class AuthStepTest { // ----------------- 401 + WWW-Authenticate handling ----------------- @Test - fun `401 with WWW-Authenticate returns the 401 unchanged by default`() { - val provider = BearerTokenProvider { _, _ -> BearerToken("tk", null) } + fun `401 with WWW-Authenticate returns the 401 unchanged when the step does not override the challenge hook`() { + // KeyCredentialAuthStep does not override authorizeRequestOnChallenge, so it exercises + // the AuthStep base default (returns null → no retry). The 401 surfaces unchanged. val fake = FakeHttpClient() .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } val pipeline = HttpPipelineBuilder(fake) - .append(BearerTokenAuthStep(provider, listOf("scope"))) + .append(KeyCredentialAuthStep(KeyCredential("k"))) .build() val response = pipeline.send(getHttpsRequest()) diff --git a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt index 033787c6..ad0254f3 100644 --- a/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt +++ b/sdk-core/src/test/kotlin/org/dexpace/sdk/core/http/pipeline/steps/BearerTokenAuthStepTest.kt @@ -13,14 +13,20 @@ import org.dexpace.sdk.core.http.common.HttpHeaderName import org.dexpace.sdk.core.http.pipeline.HttpPipelineBuilder import org.dexpace.sdk.core.http.request.Method import org.dexpace.sdk.core.http.request.Request +import org.dexpace.sdk.core.http.request.RequestBody +import org.dexpace.sdk.core.io.Io import org.dexpace.sdk.core.testing.FakeHttpClient import org.dexpace.sdk.core.testing.FixedClock +import org.dexpace.sdk.io.OkioIoProvider import java.time.Duration import java.time.Instant +import java.util.concurrent.atomic.AtomicInteger +import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith import kotlin.test.assertNotNull +import kotlin.test.assertNull import kotlin.test.assertTrue class BearerTokenAuthStepTest { @@ -28,6 +34,13 @@ class BearerTokenAuthStepTest { private val nowInstant: Instant = Instant.parse("2026-01-01T12:00:00Z") private val clock = FixedClock(nowInstant) + @BeforeTest + fun setUp() { + // The 401-challenge retry path closes the 401 response body; tests that exercise it + // need an installed IoProvider so body materialization succeeds. + Io.installProvider(OkioIoProvider) + } + // ---- fresh-token validation does NOT apply refreshMargin ---- /** @@ -172,6 +185,203 @@ class BearerTokenAuthStepTest { assertEquals(2, calls, "provider must have been called again after prior exception") } + // ---- Evict-on-401 ---- + + @Test + fun `401 challenge evicts the cached token and the retry carries a freshly fetched one`() { + // A non-expiring token would normally be cached forever (no expiry margin ever fires). + // The server rejects the first attempt with a 401 + WWW-Authenticate; the step must + // evict the stale token and re-fetch so the single retry carries the fresh credential. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount, "exactly one retry after the 401") + assertEquals(2, fetches.get(), "the 401 must trigger a re-fetch (eviction), not reuse the cached token") + // First attempt carries the stale token; the retry carries the freshly fetched one. + assertEquals("Bearer token-1", fake.requests[0].headers.get(HttpHeaderName.AUTHORIZATION)) + assertEquals("Bearer token-2", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `401 eviction works for a non-idempotent POST`() { + // The in-step challenge hook is NOT gated by retry-safety, so a POST (non-idempotent) + // still re-authenticates after a 401. A replayable body lets the retry re-send. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(postRequest()) + + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount, "the POST must be retried once after re-authentication") + assertEquals(2, fetches.get()) + assertEquals("Bearer token-2", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `after a 401 eviction the next independent request still sees the refreshed token cached`() { + // The retry re-fetches and re-caches; a subsequent unrelated request must reuse that + // refreshed token rather than fetching a third time. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(200) } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + pipeline.send(getRequest()) // 401 → evict → retry with token-2 + pipeline.send(getRequest()) // reuses cached token-2 + + assertEquals(2, fetches.get(), "the refreshed token is cached; no third fetch") + assertEquals("Bearer token-2", fake.requests[2].headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `repeated 401s surface the second 401 without a second retry`() { + // The base AuthStep retries exactly once. If the freshly fetched token is ALSO rejected, + // the second 401 is surfaced as the operation result (no infinite refresh loop), and the + // provider is consulted once per attempt (twice total). + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(401, response.status.code, "the second 401 is surfaced, not retried again") + assertEquals(2, fake.callCount) + assertEquals(2, fetches.get()) + } + + @Test + fun `cross-origin-marked request that 401s is surfaced unchanged without stamping a token`() { + // A cross-origin redirect re-issue is stripped of its Authorization header and tagged + // with the internal marker, so it reaches the foreign host with no credential. If that + // host 401s, the challenge path must NOT stamp the caller's bearer token onto the + // foreign host (that would leak the token cross-origin and bypass the HTTPS/cross-origin + // suppression). The 401 must surface unchanged with no token fetch. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Bearer realm=\"x\"") } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + // Mimic DefaultRedirectStep's cross-origin re-issue: no Authorization, marker present. + val markedRequest = + Request.builder() + .method(Method.GET) + .url("https://foreign.example.org/resource") + .addHeader(CrossOriginRedirectMarker.MARKER_HEADER, CrossOriginRedirectMarker.MARKER_VALUE) + .build() + + val response = pipeline.send(markedRequest) + + assertEquals(401, response.status.code, "the cross-origin 401 must surface unchanged") + assertEquals(1, fake.callCount, "no retry: the credential-free request must not be re-stamped") + assertEquals(0, fetches.get(), "no token must be fetched for a credential-free cross-origin request") + // The (single) request that reached the foreign host carried no Authorization header. + assertNull(fake.requests.single().headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `401 advertising only a non-bearer challenge is surfaced unchanged without eviction or refetch`() { + // A token refresh cannot satisfy a Basic-only challenge, so the step must NOT evict, + // refetch, or retry: the 401 surfaces unchanged after the single initial fetch. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Basic realm=\"x\"") } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(401, response.status.code, "a non-bearer 401 must surface unchanged") + assertEquals(1, fake.callCount, "no retry for a challenge a token refresh cannot satisfy") + assertEquals(1, fetches.get(), "only the initial stamp fetched; the 401 must not trigger a refetch") + assertEquals("Bearer token-1", fake.requests.single().headers.get(HttpHeaderName.AUTHORIZATION)) + } + + @Test + fun `401 advertising both a non-bearer and a bearer challenge still evicts and retries`() { + // The eviction gate matches the Bearer challenge anywhere in a multi-challenge header, + // so a `Basic, Bearer` 401 still refreshes the token and retries. + val fetches = AtomicInteger(0) + val provider = + BearerTokenProvider { _, _ -> + BearerToken("token-${fetches.incrementAndGet()}", futureExpiry) + } + val fake = + FakeHttpClient() + .enqueue { status(401).header("WWW-Authenticate", "Basic realm=\"x\", Bearer realm=\"y\"") } + .enqueue { status(200) } + val pipeline = + HttpPipelineBuilder(fake) + .append(BearerTokenAuthStep(provider, listOf("scope"), clock = clock)) + .build() + + val response = pipeline.send(getRequest()) + + assertEquals(200, response.status.code) + assertEquals(2, fake.callCount, "the bearer challenge in the multi-challenge header triggers one retry") + assertEquals(2, fetches.get()) + assertEquals("Bearer token-2", fake.requests[1].headers.get(HttpHeaderName.AUTHORIZATION)) + } + // ---- Helpers ---- private fun getRequest(): Request = @@ -179,4 +389,12 @@ class BearerTokenAuthStepTest { .method(Method.GET) .url("https://api.example.com/resource") .build() + + private fun postRequest(): Request = + Request.builder() + .method(Method.POST) + .url("https://api.example.com/resource") + // RequestBody.create(String) is replayable, so the non-idempotent retry can re-send. + .body(RequestBody.create("payload")) + .build() }