diff --git a/CHANGELOG.md b/CHANGELOG.md index b233822..9f15c73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,3 +10,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Initial release. + +### Changed + +- Identifier handling now preserves issuer and resource identity. Issuers are stored and compared + byte-for-byte (RFC 8414 §3.3) with no trailing-slash reconciliation; the terminating slash is + stripped only when *deriving* a `.well-known` discovery URL (RFC 8414/9728 §3.1). The resource + identifier is likewise preserved verbatim: deriving the Protected Resource Metadata path now + strips the terminating slash of the resource path (`/mcp/` → + `/.well-known/oauth-protected-resource/mcp`, RFC 9728 §3.1) without altering the resource + identifier itself. + + **Migration:** If your configured issuer differs from your authorization server's actual + identifier by a trailing slash, correct the config — the SDK no longer silently reconciles them. diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java index 6fd7548..832c200 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc8414ConformanceTest.java @@ -58,6 +58,18 @@ void rfc8414_metadata_issuer_must_match_configured_issuer() { Map.of("issuer", "https://evil.example.com", "jwks_uri", baseUrl + "/jwks")); ConformanceTestSupport.stubJwks(wireMock, "/jwks", rsaKeys); + assertThatThrownBy(() -> ConformanceTestSupport.buildClient(baseUrl)) + .isInstanceOf(Exception.class) + .hasMessageContaining("issuer"); + + // Catalog variant: the §3.3 comparison is exact, so a metadata issuer differing from the + // configured issuer only by a terminating slash is rejected too. This is the case a + // normalizing comparison would silently accept. + wireMock.resetAll(); + ConformanceTestSupport.stubMetadata( + wireMock, Map.of("issuer", baseUrl + "/", "jwks_uri", baseUrl + "/jwks")); + ConformanceTestSupport.stubJwks(wireMock, "/jwks", rsaKeys); + assertThatThrownBy(() -> ConformanceTestSupport.buildClient(baseUrl)) .isInstanceOf(Exception.class) .hasMessageContaining("issuer"); diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9068ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9068ConformanceTest.java index 6e27360..2c63498 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9068ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9068ConformanceTest.java @@ -98,6 +98,30 @@ void rfc9068_issuer_must_match() { .cause() .isInstanceOf(InvalidClaimsException.class) .hasMessageContaining("Issuer mismatch"); + + // Catalog variant: an authorization server whose identifier genuinely ends in "/" mints + // tokens whose iss carries that slash. Matching is exact in both directions, so the token + // verifies — the pair is identical, not normalized into agreement. This is the leg a + // trailing-slash-stripping comparison broke: it compared the token's "…/" against a + // stripped configured issuer and rejected every token the AS issued. + String slashIssuer = baseUrl + "/"; + wireMock.resetAll(); + ConformanceTestSupport.stubMetadata( + wireMock, Map.of("issuer", slashIssuer, "jwks_uri", baseUrl + "/jwks")); + ConformanceTestSupport.stubJwks(wireMock, "/jwks", rsaKeys); + + AuthplaneResource slashVerifier = + assertDoesNotThrow( + () -> + ConformanceTestSupport.buildVerifier( + ConformanceTestSupport.buildClient(slashIssuer), + TestFixtures.RESOURCE, + List.of("read:data"))); + String slashToken = TestFixtures.token().rsaKey(rsaKeys).issuer(slashIssuer).build(); + + VerifiedClaims slashClaims = + assertDoesNotThrow(() -> slashVerifier.verify(slashToken).get().claims()); + assertThat(slashClaims.issuer()).isEqualTo(slashIssuer); } @Test diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java index cbb0569..6ed3626 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc9728ConformanceTest.java @@ -148,5 +148,13 @@ void rfc9728_well_known_path_must_derive_from_resource_uri() { ProtectedResourceMetadata.wellKnownPath( URI.create("https://api.example.com/v2/mcp"))) .isEqualTo("/.well-known/oauth-protected-resource/v2/mcp"); + + // Catalog variant: a resource identifier published with a terminating slash serves its + // metadata at the slash-less well-known path, so identifiers differing only by that slash + // resolve to the same document (RFC 9728 §3.1). + assertThat( + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/mcp/"))) + .isEqualTo("/.well-known/oauth-protected-resource/mcp"); } } diff --git a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java index 43811c0..2bd662c 100644 --- a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java +++ b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClientBuilder.java @@ -48,7 +48,9 @@ public final class AuthplaneClientBuilder { AuthplaneClientBuilder(String issuer) { Objects.requireNonNull(issuer, "issuer must not be null"); if (issuer.isBlank()) throw new IllegalArgumentException("issuer must not be blank"); - this.issuer = normalizeIssuer(issuer); + // Store the issuer verbatim (identity is preserved). Any trailing slash is stripped only + // where a URL is *derived* (RFC 8414/9728 §3.1), never on the stored/compared identifier. + this.issuer = issuer; } /** Sets development mode. When true, SSRF protection is relaxed. */ @@ -267,8 +269,4 @@ private void wireMetadataCallback( } }); } - - private static String normalizeIssuer(String issuer) { - return issuer.endsWith("/") ? issuer.substring(0, issuer.length() - 1) : issuer; - } } diff --git a/core/src/main/java/ai/authplane/sdk/core/CircuitPolicy.java b/core/src/main/java/ai/authplane/sdk/core/CircuitPolicy.java index cddd9f4..60f307f 100644 --- a/core/src/main/java/ai/authplane/sdk/core/CircuitPolicy.java +++ b/core/src/main/java/ai/authplane/sdk/core/CircuitPolicy.java @@ -10,8 +10,8 @@ /** * Decides whether a failure from AS token/introspection/revocation flows should increment the - * circuit breaker (Python {@code AuthplaneClient._handle_failure} semantics, extended for OAuth - * business errors vs infra). + * circuit breaker, distinguishing OAuth business errors (which do not trip it) from infrastructure + * failures (which do). */ public final class CircuitPolicy { diff --git a/core/src/main/java/ai/authplane/sdk/core/dpop/DPoPProofMissingException.java b/core/src/main/java/ai/authplane/sdk/core/dpop/DPoPProofMissingException.java index 296052c..c9d4e39 100644 --- a/core/src/main/java/ai/authplane/sdk/core/dpop/DPoPProofMissingException.java +++ b/core/src/main/java/ai/authplane/sdk/core/dpop/DPoPProofMissingException.java @@ -9,8 +9,7 @@ * depends on the exact semantics of this exception type: "the token is DPoP-bound ({@code cnf.jkt} * present) but the call site provided no {@code VerificationRequestContext} to bind a proof * against." The MCP adapter swallows this specific exception in its bearer-only pre-validation pass - * and defers proof binding to its second hook (the context extractor) — that is the Java equivalent - * of the TS SDK's FastMCP DPoP workaround. + * and defers proof binding to its second hook (the context extractor). * *

If you refactor {@code AuthplaneResource.validateDpop} so that this exception is thrown for a * different reason (e.g. proof present but malformed), update the swallow logic in {@code diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java b/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java index ad08bd7..a2f920b 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/DocumentCache.java @@ -1,5 +1,6 @@ package ai.authplane.sdk.core.fetching; +import java.time.Clock; import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.locks.ReentrantLock; @@ -28,6 +29,7 @@ public class DocumentCache { private final String url; private final int configuredRefreshSeconds; private final String documentType; // "JWKS" or "metadata" — for log messages + private final Clock clock; private volatile BiConsumer, Map> onChangeCallback; private final ReentrantLock fetchLock = new ReentrantLock(); @@ -36,7 +38,10 @@ public class DocumentCache { private Map cachedDocument; private long cachedAtEpochSeconds; // when the current cache was stored private Long serverExpiresAtSeconds; // from HTTP cache headers, or null - private CompletableFuture bgRefreshFuture; + + // Written under fetchLock; volatile so the package-private accessor can read it without + // taking fetchLock. + private volatile CompletableFuture bgRefreshFuture; /** * @param fetcher document fetcher (SSRF-safe or direct) @@ -52,11 +57,43 @@ public DocumentCache( String documentType, BiConsumer, Map> onChangeCallback) { + this( + fetcher, + url, + configuredRefreshSeconds, + documentType, + onChangeCallback, + Clock.systemUTC()); + } + + /** + * Test seam. Same as the public constructor, but with the time source injected so TTL expiry + * can be driven by advancing a clock rather than by sleeping against wall time — the difference + * between a deterministic assertion and a race with the CI runner. + * + *

The seam is {@link Clock} rather than a {@code LongSupplier} of epoch seconds, even though + * this class represents time as {@code long} epoch seconds throughout. {@code Clock} is the + * platform idiom, it composes ({@code Clock.fixed}, {@code Clock.offset}), and the conversion + * cost is one call in {@code nowEpochSeconds()} — not one per use site. Several other classes + * in the SDK still read the wall clock directly and will want the same seam; this is the shape + * to copy. + * + * @param clock the time source + */ + DocumentCache( + DocumentFetcher fetcher, + String url, + int configuredRefreshSeconds, + String documentType, + BiConsumer, Map> onChangeCallback, + Clock clock) { + this.fetcher = fetcher; this.url = url; this.configuredRefreshSeconds = configuredRefreshSeconds; this.documentType = documentType; this.onChangeCallback = onChangeCallback; + this.clock = clock; } /** Returns the URL this cache fetches from. */ @@ -207,6 +244,15 @@ private boolean backgroundRefreshScheduled() { return bgRefreshFuture != null && !bgRefreshFuture.isDone(); } + /** + * Test seam: the in-flight background refresh, or {@code null} if none has been scheduled. Lets + * a test await the refresh it just triggered instead of guessing how long the async fetch will + * take. + */ + CompletableFuture backgroundRefreshFuture() { + return bgRefreshFuture; + } + private void scheduleBackgroundRefresh() { bgRefreshFuture = CompletableFuture.runAsync( @@ -227,7 +273,7 @@ private void scheduleBackgroundRefresh() { }); } - private static long nowEpochSeconds() { - return System.currentTimeMillis() / 1000L; + private long nowEpochSeconds() { + return clock.instant().getEpochSecond(); } } diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java b/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java index a9d4441..914088f 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/MetadataCache.java @@ -2,6 +2,7 @@ import java.net.URI; import java.util.Map; +import java.util.Objects; import java.util.function.BiConsumer; import java.util.logging.Logger; @@ -11,9 +12,8 @@ * Cache for OAuth Authorization Server Metadata (RFC 8414). * *

Extracts and validates the {@code jwks_uri} field. Validates issuer and endpoint URLs - * internally when metadata is fetched, matching the Python/Go pattern. Triggers the change callback - * when the document changes, allowing the caller to detect jwks_uri rotation and restart the - * JwksCache. + * internally when metadata is fetched. Triggers the change callback when the document changes, + * allowing the caller to detect jwks_uri rotation and restart the JwksCache. */ public class MetadataCache extends DocumentCache { @@ -45,7 +45,11 @@ public MetadataCache( boolean allowHttp, BiConsumer, Map> onChangeCallback) { super(fetcher, metadataUrl, refreshSeconds, "metadata", onChangeCallback); - this.expectedIssuer = expectedIssuer; + // Required: the RFC 8414 §3.3 comparison in getJwksUri() dereferences this. Without the + // check a null surfaces as a bare NPE from the first metadata read rather than as a + // contract violation at construction. + this.expectedIssuer = + Objects.requireNonNull(expectedIssuer, "expectedIssuer must not be null"); this.allowHttp = allowHttp; } @@ -93,14 +97,14 @@ private void validateMetadata(Map metadata) throws MetadataFetch "OAuth server metadata is missing or has empty 'issuer' field"); } - String normalizedMetadataIssuer = normalizeIssuer(issuer); - String normalizedExpectedIssuer = normalizeIssuer(expectedIssuer); - if (!normalizedExpectedIssuer.equals(normalizedMetadataIssuer)) { + // RFC 8414 §3.3: the issuer is compared byte-for-byte against the configured value. + // No trailing-slash reconciliation — a difference in the terminating slash is a mismatch. + if (!expectedIssuer.equals(issuer)) { throw new MetadataFetchException( "OAuth server metadata issuer mismatch: expected '" - + normalizedExpectedIssuer + + expectedIssuer + "', got '" - + normalizedMetadataIssuer + + issuer + "'"); } @@ -150,11 +154,4 @@ private void validateEndpointUrl(String field, String value) throws MetadataFetc + "'"); } } - - private static String normalizeIssuer(String issuer) { - if (issuer == null) { - return null; - } - return issuer.endsWith("/") ? issuer.substring(0, issuer.length() - 1) : issuer; - } } diff --git a/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java b/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java index ac979e3..b715cd0 100644 --- a/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java +++ b/core/src/main/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadata.java @@ -61,38 +61,85 @@ private ProtectedResourceMetadata( * 

      * "https://api.example.com"        → "/.well-known/oauth-protected-resource"
      * "https://api.example.com/mcp"    → "/.well-known/oauth-protected-resource/mcp"
+     * "https://api.example.com/mcp/"   → "/.well-known/oauth-protected-resource/mcp"
+     * "https://api.example.com/mcp//"  → "/.well-known/oauth-protected-resource/mcp"
      * "https://api.example.com/v2/mcp" → "/.well-known/oauth-protected-resource/v2/mcp"
+     * "https://api.example.com/a%2Fb"  → "/.well-known/oauth-protected-resource/a%2Fb"
      *
* - * @param resourceUri the resource server URI + *

Per RFC 9728 §3.1 every terminating slash of the resource path is stripped when deriving + * the well-known path; it does not affect the resource identifier itself. The derivation reads + * the raw (percent-encoded) path, so an encoded octet such as {@code %2F} is carried through + * verbatim rather than decoded into a path separator — decoding it would name a different path + * than the resource identifier does. + * + * @param resourceUri the resource server URI; must be hierarchical and carry an authority * @return the URL path (including leading slash) where the PRM should be served + * @throws IllegalArgumentException if {@code resourceUri} is opaque or has no authority */ public static String wellKnownPath(URI resourceUri) { - String path = resourceUri.getPath(); - if (path == null || path.isEmpty() || path.equals("/")) { + requireDerivable(resourceUri); + + // Read the raw path: URI.getPath() percent-decodes, which would turn a resource + // identifier of ".../a%2Fb" into the well-known path ".../a/b" — a different path than + // the identifier names, and the silent rewrite this derivation exists to avoid. + String path = resourceUri.getRawPath(); + if (path == null || path.isEmpty()) { + return WELL_KNOWN_PREFIX; + } + + // Strip every terminating slash before deriving the well-known path (RFC 9728 §3.1): + // the resource identity is preserved elsewhere, but the derived .well-known path must + // not carry a trailing slash ("/mcp/" and "/mcp//" both → ".../mcp"). Stripping only one + // would make this helper and wellKnownUrl disagree on a doubled slash. + String derivedPath = path.replaceAll("/+$", ""); + if (derivedPath.isEmpty()) { return WELL_KNOWN_PREFIX; } // Strip leading slash — WELL_KNOWN_PREFIX already starts with / - String cleanPath = path.startsWith("/") ? path.substring(1) : path; + String cleanPath = derivedPath.startsWith("/") ? derivedPath.substring(1) : derivedPath; return WELL_KNOWN_PREFIX + "/" + cleanPath; } /** * Computes the full URL of the PRM document for the given resource URI. * - * @param resourceUri the resource server URI string + *

The path component is derived by {@link #wellKnownPath(URI)}, so both helpers agree by + * construction: the slash stripping happens in exactly one place. + * + * @param resourceUri the resource server URI string; must be hierarchical and carry an + * authority * @return the full PRM document URL + * @throws IllegalArgumentException if {@code resourceUri} is opaque or has no authority */ public static String wellKnownUrl(String resourceUri) { - String stripped = - resourceUri.endsWith("/") - ? resourceUri.substring(0, resourceUri.length() - 1) - : resourceUri; - URI uri = URI.create(stripped); + URI uri = URI.create(resourceUri); + requireDerivable(uri); return uri.getScheme() + "://" + uri.getAuthority() + wellKnownPath(uri); } + /** + * Guards the PRM derivation helpers against identifiers they cannot derive from. + * + *

RFC 8707 §2 permits a resource indicator that is any absolute URI, and this class stores + * whatever it is given verbatim — {@code urn:example:api} is a valid resource identifier. But + * an opaque URI has no authority and no hierarchical path, so there is no PRM URL to publish + * for it: the derivation would otherwise emit {@code urn://null/.well-known/...} and hand that + * to the {@code resource_metadata} parameter of the 401 challenge. + */ + private static void requireDerivable(URI resourceUri) { + if (resourceUri.isOpaque() || resourceUri.getAuthority() == null) { + throw new IllegalArgumentException( + "Cannot derive a Protected Resource Metadata URL from \"" + + resourceUri + + "\": PRM derivation requires a hierarchical resource identifier with" + + " an authority (e.g. https://api.example.com/mcp). The resource" + + " identifier itself may be any absolute URI permitted by RFC 8707 §2" + + " and is stored verbatim; only the derivation is restricted."); + } + } + // ----------------------------------------------------------------------- // Document serialization // ----------------------------------------------------------------------- diff --git a/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java b/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java index 24012a0..12f2c16 100644 --- a/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java @@ -190,6 +190,48 @@ void build_metadataIssuerMismatch_failsFuture() { }); } + @Test + void build_issuerWithTrailingSlash_reachesMetadataCacheAndValidatorVerbatim() throws Exception { + // The builder stores the configured issuer verbatim and passes it to MetadataCache (and, + // via the client, to each resource's JwtValidator) as the expected issuer. Metadata and + // token validation compare byte-for-byte (RFC 8414 §3.3), so a trailing-slash issuer + // verifies only against a metadata document — and a token — whose `iss` carries the same + // trailing slash. If the builder silently normalized the issuer (stripping the slash), the + // metadata comparison would fail and build() would throw; a green build therefore proves + // the configured value reached MetadataCache unmodified. + String issuerWithSlash = baseUrl + "/"; + wireMock.resetAll(); + wireMock.stubFor( + get(urlEqualTo("/.well-known/oauth-authorization-server")) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody( + TestFixtures.serializeMap( + Map.of( + "issuer", + issuerWithSlash, + "jwks_uri", + baseUrl + "/jwks"))))); + stubJwks(); + + AuthplaneClient client = + AuthplaneClient.builder(issuerWithSlash).devMode(true).build().get(); + + // Builder stored the issuer byte-for-byte. + assertThat(client.issuer()).isEqualTo(issuerWithSlash); + + // The JwtValidator built for a resource inherits the same verbatim issuer: a token whose + // `iss` carries the identical trailing slash verifies. + AuthplaneResource verifier = client.resource(TestFixtures.RESOURCE, TestFixtures.SCOPES); + String token = TestFixtures.token().rsaKey(rsaKeys).issuer(issuerWithSlash).build(); + VerifiedClaims claims = verifier.verify(token).get().claims(); + assertThat(claims.issuer()).isEqualTo(issuerWithSlash); + + client.close(); + } + // ----------------------------------------------------------------------- // resource() factory creates working resources // ----------------------------------------------------------------------- diff --git a/core/src/test/java/ai/authplane/sdk/core/JwtValidatorTest.java b/core/src/test/java/ai/authplane/sdk/core/JwtValidatorTest.java index f65ba5f..d409ba6 100644 --- a/core/src/test/java/ai/authplane/sdk/core/JwtValidatorTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/JwtValidatorTest.java @@ -259,6 +259,25 @@ void verify_wrongIssuer_throwsInvalidClaims() { .hasMessageContaining("Issuer mismatch"); } + @Test + void verify_issuerWithConfiguredTrailingSlash_verifies() throws Exception { + // Identifiers are compared verbatim (RFC 8414 §3.3): the configured issuer is compared + // byte-for-byte. When the validator is configured with a trailing-slash issuer and the + // token's `iss` carries the same trailing slash, verification succeeds — the SDK does not + // silently reconcile slashes. + String issuerWithSlash = TestFixtures.ISSUER + "/"; + JwtValidator validator = + new JwtValidator( + issuerWithSlash, + TestFixtures.RESOURCE, + Set.of("RS256", "ES256"), + 30, + rsaKeyLookup()); + String token = TestFixtures.token().rsaKey(rsaKeys).issuer(issuerWithSlash).build(); + VerifiedClaims claims = validator.verify(token); + assertThat(claims.issuer()).isEqualTo(issuerWithSlash); + } + @Test void verify_wrongAudience_throwsInvalidClaims() { JwtValidator validator = validatorWith(rsaKeyLookup()); diff --git a/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java b/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java index 57909fb..5dc970a 100644 --- a/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/fetching/DocumentCacheTest.java @@ -3,10 +3,16 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneId; +import java.time.ZoneOffset; import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; +import java.util.function.BiConsumer; import org.junit.jupiter.api.Test; @@ -15,6 +21,9 @@ class DocumentCacheTest { private static final Map DOC_V1 = Map.of("version", "1"); private static final Map DOC_V2 = Map.of("version", "2"); + /** Arbitrary fixed start time; only the deltas matter. */ + private static final long T0 = 1_700_000_000L; + private DocumentCache cache; @Test @@ -47,16 +56,15 @@ void get_usesStaleCacheOnFetchFailure() throws Exception { throw new CompletionException( new RuntimeException("Network down")); }); - // Very short TTL so it expires quickly - cache = cacheWith(fetcher, 1); + TestClock clock = new TestClock(); + cache = cacheWith(fetcher, 100, clock); cache.fetch(); - // Wait for TTL to expire - Thread.sleep(1500); + clock.advanceSeconds(101); // past the TTL - // Next get() should try to refresh, fail, but return stale - Map result = cache.get(); - assertThat(result).isEqualTo(DOC_V1); + // get() refreshes synchronously, the refresh fails, stale is returned + assertThat(cache.get()).isEqualTo(DOC_V1); + assertThat(calls.get()).isEqualTo(2); } @Test @@ -80,19 +88,17 @@ void onChangeCallback_calledWhenDocumentChanges() throws Exception { return new FetchResult(n == 1 ? DOC_V1 : DOC_V2, null); }); - cache = - new DocumentCache( - fetcher, - "https://example.com/jwks", - 1, - "JWKS", - (old, next) -> calls.incrementAndGet()); + TestClock clock = new TestClock(); + cache = cacheWith(fetcher, 100, (old, next) -> calls.incrementAndGet(), clock); cache.fetch(); // DOC_V1 - Thread.sleep(1500); - cache.get(); // triggers refresh → DOC_V2 → callback called - // Wait for async callback - Thread.sleep(100); - assertThat(calls.get()).isGreaterThanOrEqualTo(1); + assertThat(calls.get()).isEqualTo(0); // no change on the first fetch + + clock.advanceSeconds(101); // past the TTL + + // Expired, so get() refreshes synchronously — the callback fires inline, not on a + // background thread, so there is nothing to wait for. + assertThat(cache.get()).isEqualTo(DOC_V2); + assertThat(calls.get()).isEqualTo(1); } @Test @@ -107,30 +113,50 @@ void forceRefresh_alwaysFetches() throws Exception { @Test void get_triggersBackgroundRefreshAt80PercentTtl() throws Exception { AtomicInteger fetchCount = new AtomicInteger(); - cache = cacheWith(countingFetcher(DOC_V1, fetchCount), 1); - cache.fetch(); // initial fetch - // Wait for 80% of 1s TTL - Thread.sleep(900); - cache.get(); // should trigger background refresh - Thread.sleep(300); // wait for async refresh - assertThat(fetchCount.get()).isGreaterThanOrEqualTo(2); + TestClock clock = new TestClock(); + cache = cacheWith(countingFetcher(DOC_V1, fetchCount), 100, clock); + cache.fetch(); + assertThat(fetchCount.get()).isEqualTo(1); + + clock.advanceSeconds(79); // just under the 80% threshold + cache.get(); + assertThat(cache.backgroundRefreshFuture()).isNull(); + assertThat(fetchCount.get()).isEqualTo(1); + + clock.advanceSeconds(1); // exactly 80% of the 100s TTL + cache.get(); + + CompletableFuture refresh = cache.backgroundRefreshFuture(); + assertThat(refresh).as("a background refresh must have been scheduled").isNotNull(); + refresh.join(); // await the refresh itself rather than guessing a duration + + assertThat(fetchCount.get()).isEqualTo(2); } @Test void get_serverExpiresTtl_usesMinOfConfiguredAndServer() throws Exception { - // Return a FetchResult with server-expires that is shorter than configured - long serverExpires = System.currentTimeMillis() / 1000 + 1; // 1 second + // Server says the document expires 10s from now; the configured TTL is 300s. The + // effective TTL must be the server's, so the cache expires at +10 rather than +300. + AtomicInteger fetchCount = new AtomicInteger(); + TestClock clock = new TestClock(); DocumentFetcher fetcher = - url -> CompletableFuture.completedFuture(new FetchResult(DOC_V1, serverExpires)); - cache = cacheWith(fetcher, 300); // configured 300s, server says 1s + url -> { + fetchCount.incrementAndGet(); + return CompletableFuture.completedFuture( + new FetchResult(DOC_V1, clock.instant().getEpochSecond() + 10)); + }; + cache = cacheWith(fetcher, 300, clock); cache.fetch(); - // Wait for server TTL to expire - Thread.sleep(1500); + clock.advanceSeconds(5); // inside both TTLs + cache.get(); + assertThat(fetchCount.get()).as("still fresh under the server TTL").isEqualTo(1); - // Should trigger synchronous refresh (returns stale on failure since no network) - Map result = cache.get(); - assertThat(result).isEqualTo(DOC_V1); + clock.advanceSeconds(6); // past the server TTL, far short of the configured one + assertThat(cache.get()).isEqualTo(DOC_V1); + assertThat(fetchCount.get()) + .as("the server expiry, not the configured TTL, governs") + .isEqualTo(2); } @Test @@ -146,10 +172,46 @@ void setOnChangeCallback_replacesCallback() throws Exception { // Helpers // ----------------------------------------------------------------------- + /** Manually advanced clock, so TTL expiry is driven rather than waited on. */ + private static final class TestClock extends Clock { + private final AtomicLong nowSeconds = new AtomicLong(T0); + + @Override + public ZoneId getZone() { + return ZoneOffset.UTC; + } + + @Override + public Clock withZone(ZoneId zone) { + return this; + } + + @Override + public Instant instant() { + return Instant.ofEpochSecond(nowSeconds.get()); + } + + void advanceSeconds(long seconds) { + nowSeconds.addAndGet(seconds); + } + } + private static DocumentCache cacheWith(DocumentFetcher fetcher, int ttl) { return new DocumentCache(fetcher, "https://example.com/jwks", ttl, "JWKS", null); } + private static DocumentCache cacheWith(DocumentFetcher fetcher, int ttl, TestClock clock) { + return cacheWith(fetcher, ttl, null, clock); + } + + private static DocumentCache cacheWith( + DocumentFetcher fetcher, + int ttl, + BiConsumer, Map> onChange, + TestClock clock) { + return new DocumentCache(fetcher, "https://example.com/jwks", ttl, "JWKS", onChange, clock); + } + private static DocumentFetcher successFetcher(Map doc) { return url -> CompletableFuture.completedFuture(new FetchResult(doc, null)); } diff --git a/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java b/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java index 9744506..677c581 100644 --- a/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/fetching/MetadataCacheTest.java @@ -42,14 +42,43 @@ void getJwksUri_validMetadata_returnsJwksUri() throws Exception { } @Test - void getJwksUri_normalizesTrailingSlashOnIssuer() throws Exception { - // Metadata declares issuer with trailing slash; configured value doesn't. - // normalizeIssuer should strip the slash before comparison. + void getJwksUri_trailingSlashIssuerMismatch_rejected() { + // RFC 8414 §3.3: issuer is compared byte-for-byte. Metadata declares the issuer with a + // trailing slash; the configured value does not — the SDK must NOT reconcile them. MetadataCache cache = cacheWith( Map.of("issuer", ISSUER + "/", "jwks_uri", "https://auth.example.com/jwks"), false); + assertThatThrownBy(cache::getJwksUri) + .isInstanceOf(MetadataFetchException.class) + .hasMessageContaining("issuer mismatch"); + } + + @Test + void getJwksUri_issuerWithConfiguredTrailingSlash_verifies() throws Exception { + // Identifiers are compared verbatim (RFC 8414 §3.3): when the configured issuer itself + // carries a trailing slash and the metadata's iss matches it byte-for-byte, verification + // succeeds. + DocumentFetcher fetcher = + url -> + CompletableFuture.completedFuture( + new FetchResult( + Map.of( + "issuer", + ISSUER + "/", + "jwks_uri", + "https://auth.example.com/jwks"), + null)); + MetadataCache cache = + new MetadataCache( + fetcher, + ISSUER + "/.well-known/oauth-authorization-server", + 300, + ISSUER + "/", + false, + null); + assertThat(cache.getJwksUri()).isEqualTo("https://auth.example.com/jwks"); } diff --git a/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java b/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java index b7c5804..b63022e 100644 --- a/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/prm/ProtectedResourceMetadataTest.java @@ -3,6 +3,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatNullPointerException; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.InstanceOfAssertFactories.LIST; import java.net.URI; @@ -27,6 +28,78 @@ void wellKnownPath_resourceWithPath() { .isEqualTo("/.well-known/oauth-protected-resource/mcp"); } + @Test + void wellKnownPath_resourceWithTrailingSlash_stripped() { + // The terminating slash is stripped only when deriving the well-known path (RFC 9728 + // §3.1); the resource identifier itself is compared verbatim. "/mcp/" → + // ".../oauth-protected-resource/mcp". + assertThat( + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/mcp/"))) + .isEqualTo("/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void wellKnownPath_rootResourceWithTrailingSlash() { + // A root resource carrying only a terminating slash (path "/") derives the bare + // well-known path — exercises the path.equals("/") branch (RFC 9728 §3.1). + assertThat(ProtectedResourceMetadata.wellKnownPath(URI.create("https://api.example.com/"))) + .isEqualTo("/.well-known/oauth-protected-resource"); + } + + @Test + void urnStyleResource_isAccepted() { + // RFC 8707 §2 permits non-http(s) resource indicators. A urn: identifier must not be + // rejected by any http(s)+authority validator — it is stored verbatim. + var prm = + ProtectedResourceMetadata.builder() + .resource("urn:example:api") + .authorizationServer("https://auth.example.com") + .build(); + assertThat(prm.getResource()).isEqualTo("urn:example:api"); + assertThat(prm.toMap().get("resource")).isEqualTo("urn:example:api"); + } + + @Test + void urnStyleResource_cannotDeriveAPrmUrl() { + // The identifier is stored verbatim (above), but there is no PRM URL to derive from an + // opaque URI: it has no authority and no hierarchical path. Deriving anyway produced + // "urn://null/.well-known/oauth-protected-resource", which AuthplaneResource.prmUrl() + // hands straight to the resource_metadata parameter of the 401 challenge. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.wellKnownPath( + URI.create("urn:example:api"))) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("hierarchical resource identifier"); + + assertThatThrownBy(() -> ProtectedResourceMetadata.wellKnownUrl("urn:example:api")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("hierarchical resource identifier"); + } + + @Test + void wellKnownPath_stripsEveryTerminatingSlash() { + // Stripping only one slash made wellKnownPath and wellKnownUrl disagree on a doubled + // slash — the exact invariant this pair is supposed to hold. + assertThat( + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/mcp//"))) + .isEqualTo("/.well-known/oauth-protected-resource/mcp"); + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp//")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void wellKnownPath_preservesPercentEncodedOctets() { + // getPath() decodes, so "%2F" collapsed to "/" and the derived path named a different + // resource than the identifier does (RFC 3986 §3.3). + assertThat( + ProtectedResourceMetadata.wellKnownPath( + URI.create("https://api.example.com/a%2Fb"))) + .isEqualTo("/.well-known/oauth-protected-resource/a%2Fb"); + } + @Test void wellKnownPath_resourceWithDeepPath() { assertThat( @@ -41,6 +114,14 @@ void wellKnownUrl_returnsFullUrl() { .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource"); } + @Test + void wellKnownUrl_pathWithTrailingSlash_stripped() { + // wellKnownUrl applies its own terminating-slash strip before deriving the path, so a + // trailing-slash resource URL still resolves to the slash-less well-known document. + assertThat(ProtectedResourceMetadata.wellKnownUrl("https://api.example.com/mcp/")) + .isEqualTo("https://api.example.com/.well-known/oauth-protected-resource/mcp"); + } + @Test void toMap_containsAllRequiredFields() { var prm = diff --git a/mcp/docs/user-guide.md b/mcp/docs/user-guide.md index 8a5584f..0ed4746 100644 --- a/mcp/docs/user-guide.md +++ b/mcp/docs/user-guide.md @@ -384,7 +384,7 @@ The MCP Java SDK splits transport-level auth into two hooks with different capab ### DPoP-bound tokens (`cnf.jkt` present) -`AuthplaneMcpAdapter` handles this asymmetry internally so DPoP-bound tokens work end-to-end (the TypeScript SDK's FastMCP integration has the same `authenticate`-called-twice hook split and applies the equivalent workaround): +`AuthplaneMcpAdapter` handles this asymmetry internally so DPoP-bound tokens work end-to-end (the MCP Java SDK splits header validation and request extraction into two hooks, so the adapter defers the DPoP proof check to the hook that has the request): - **`validateHeaders`** runs `resource.verify(token)` in bearer-only mode. For a DPoP-bound token this throws `DPoPProofMissingException` by design — the adapter **swallows that specific exception** and lets the request flow through. All other failures (expired, bad signature, revoked, DPoP unsupported, scope insufficient) still surface as 401/403. - **`extract`** runs `resource.verify(token, context)` with the full request context. This is the authoritative DPoP validation: proof signature, `htm`/`htu`/`ath` claims, replay store check, binding to `cnf.jkt`. Failures here bubble as the typed `AuthplaneException` (visible as 500 with diagnostic body — see "DPoP failure surface" below). @@ -404,7 +404,7 @@ For the SSE GET path the MCP Java SDK calls only `validateHeaders`, never `extra `validateHeaders` and `extract` each call `resource.verify(...)` for bearer-only tokens, so the verifier runs twice per request. When RFC 7662 introspection-based revocation checking is enabled, this triggers **two introspection calls per request** to the authorization server. DPoP-bound tokens incur only one full verify (the validateHeaders pass throws `DPoPProofMissingException` before reaching introspection). -A per-request memo (analogous to the TypeScript SDK's `AsyncLocalStorage`-keyed cache used in its FastMCP integration) would collapse the bearer-only path back to one introspection per request without changing the public contract. It would need either an upstream MCP SDK change to give `validateHeaders` access to the request, or a separately registered servlet `Filter` that sets a request attribute the adapter can read. Tracked as a noted follow-up; not in this version. +A per-request memo (a request-scoped cache of the verify result, keyed by the in-flight request) would collapse the bearer-only path back to one introspection per request without changing the public contract. It would need either an upstream MCP SDK change to give `validateHeaders` access to the request, or a separately registered servlet `Filter` that sets a request attribute the adapter can read. Tracked as a noted follow-up; not in this version. ### DPoP failure surface diff --git a/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapter.java b/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapter.java index 81b43d7..0a109e0 100644 --- a/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapter.java +++ b/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapter.java @@ -52,9 +52,8 @@ *

API Note — Double introspection on authenticated paths: {@code validateHeaders} and * {@code extract} each invoke {@code resource.verify(...)}, so when RFC 7662 introspection-based * revocation checking is enabled a bearer-only request triggers two introspection calls to the - * authorization server. A per-request memo (analogous to the TypeScript SDK's {@code - * AsyncLocalStorage} cache) would collapse it to one without changing the public contract — left as - * a noted follow-up. + * authorization server. A per-request memo (a request-scoped cache of the verify result) would + * collapse it to one without changing the public contract — left as a noted follow-up. * * @see AuthplaneMcpSetup */ @@ -122,12 +121,10 @@ public AuthplaneResource resource() { * through POST and receive full DPoP + revocation validation via {@code extract}. See * user-guide §13 for the full rationale. * - *

The TypeScript SDK applies the equivalent workaround in its FastMCP integration ({@code - * authenticate} is called twice per request and the verify result is cached across calls via - * {@code AsyncLocalStorage}). The Java MCP SDK exposes two hooks that cannot share state - * because {@code validateHeaders} receives only headers (no request), so the equivalent - * invariant ("DPoP proof validated exactly once per request") is reached by deferring proof - * binding to {@code extract} rather than caching across calls. + *

The Java MCP SDK exposes two hooks that cannot share state because {@code validateHeaders} + * receives only headers (no request), so the invariant ("DPoP proof validated exactly once per + * request") is reached by deferring proof binding to {@code extract} rather than caching the + * verify result across the two calls. * * @param headers request headers (multi-valued, case-insensitive lookup) * @throws ServerTransportSecurityException HTTP 401 if the Authorization header is missing, diff --git a/mcp/src/test/java/ai/authplane/sdk/mcp/UrlElicitationSupportTest.java b/mcp/src/test/java/ai/authplane/sdk/mcp/UrlElicitationSupportTest.java index 04d2b42..a851650 100644 --- a/mcp/src/test/java/ai/authplane/sdk/mcp/UrlElicitationSupportTest.java +++ b/mcp/src/test/java/ai/authplane/sdk/mcp/UrlElicitationSupportTest.java @@ -34,8 +34,8 @@ void toUrlElicitationRequiredError_mapsConsentRequired() { assertThat(mapped.getJsonRpcError().code()).isEqualTo(-32042); // The admin-configured consent message is preserved at the top-level JSON-RPC - // message (parity with the python/ts SDKs), and the human-readable per-elicitation - // message carries the same text plus service context. + // message, and the human-readable per-elicitation message carries the same text + // plus service context. assertThat(mapped.getJsonRpcError().message()).isEqualTo("User must grant access"); @SuppressWarnings("unchecked") diff --git a/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapter.java b/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapter.java index 8d03c08..c62adc5 100644 --- a/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapter.java +++ b/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapter.java @@ -62,9 +62,8 @@ *

API Note — Double introspection on authenticated paths: {@code validateHeaders} and * {@code extract} each invoke {@code resource.verify(...)}, so when RFC 7662 introspection-based * revocation checking is enabled a bearer-only request triggers two introspection calls to the - * authorization server. A per-request memo (analogous to the TypeScript SDK's {@code - * AsyncLocalStorage} cache) would collapse it to one without changing the public contract — left as - * a noted follow-up. + * authorization server. A per-request memo (a request-scoped cache of the verify result) would + * collapse it to one without changing the public contract — left as a noted follow-up. * * @see AuthplaneMcpServerConfig */