diff --git a/.github/scripts/conformance-case-body-drift.sh b/.github/scripts/conformance-case-body-drift.sh new file mode 100755 index 0000000..07622fd --- /dev/null +++ b/.github/scripts/conformance-case-body-drift.sh @@ -0,0 +1,361 @@ +#!/usr/bin/env bash +# +# Detect semantic drift in the conformance catalog: a case whose BODY changed +# under an unchanged id. +# +# Everything else in the alignment machinery compares case IDS. The pinned ref +# protects against new cases arriving unannounced, and the drift job's id-set +# comparison against the catalog tip reports cases added or removed. Neither +# looks at the body of a case, so a case that is re-tightened in place — same +# id, stricter requirement — is invisible end to end: the SDK bumps its pin and +# starts declaring conformance to a requirement nothing verified it against. +# That has already happened once, to the metadata jwks_uri rotation case. +# +# This script closes that gap. It compares the body of every case the SDK +# actually registers between two checkouts of the catalog — normally the pinned +# ref and the tip — and fails naming any case whose body changed. +# +# Scoped to the ids the SDK registers on purpose. Diffing the whole catalog, or +# asserting catalog_version, fires on every catalog edit including cases this +# SDK does not cover; the noise is what gets a guard ignored. +# +# Inputs (environment): +# PINNED_CATALOG path to the catalog YAML at the ref this repo pins +# TIP_CATALOG path to the catalog YAML to compare against +# REGISTERED_IDS path to a file holding one case id per line — the ids this +# SDK registers. Produced per-repo; for this repo, by +# conformance-registered-case-ids.sh. +# DRIFT_SUMMARY optional path to append a Markdown summary to +# COVERAGE_DIR optional directory to name in that summary as the place the +# conformance coverage lives, so the one human-facing pointer +# in this script is not the thing that has to be edited when +# it is dropped into a tree laid out differently. Defaults to +# this repo's core/conformancetests/. +# +# Exit status: +# 0 no body drift in any registered case +# 1 body drift found, OR this check could not do its job +# +# The second half of that exit code matters as much as the first. A guard that +# under-checks while reporting green is the defect class this script exists to +# close, so every input it cannot read, every catalog shape it cannot parse and +# every empty intermediate result is a hard failure rather than a quiet pass. + +set -euo pipefail + +: "${PINNED_CATALOG:?PINNED_CATALOG must be set}" +: "${TIP_CATALOG:?TIP_CATALOG must be set}" +: "${REGISTERED_IDS:?REGISTERED_IDS must be set}" + +DRIFT_SUMMARY="${DRIFT_SUMMARY:-}" +COVERAGE_DIR="${COVERAGE_DIR:-core/conformancetests/}" + +fail() { + echo "::error::$1" >&2 + exit 1 +} + +for input in "$PINNED_CATALOG" "$TIP_CATALOG" "$REGISTERED_IDS"; do + if [[ ! -f "$input" || ! -r "$input" ]]; then + fail "case-body drift check: '$input' is not a readable regular file. The check cannot run and is not reporting a clean result." + fi + if [[ ! -s "$input" ]]; then + fail "case-body drift check: '$input' is empty. The check cannot run and is not reporting a clean result." + fi +done + +WORK="$(mktemp -d)" +trap 'rm -rf "$WORK"' EXIT + +# --------------------------------------------------------------------------- +# Case body extraction +# --------------------------------------------------------------------------- +# +# Emits one line per body line, as "", for every case in +# the catalog's top-level `cases:` block. The id prefix keeps each body +# addressable without opening a file per case, and leaves the body text after +# the tab byte-for-byte as the catalog has it. +# +# Only the `cases:` block is read. `standards_in_scope` earlier in the file +# also holds `- id:` entries, and matching those would compare bodies that are +# not cases at all. +# +# Normalization is deliberately minimal: trailing whitespace is stripped, blank +# lines are dropped, and comment lines are dropped only at structural positions +# — at or above the case-item indent. Nothing else is touched — no attempt is +# made to unfold YAML line continuations, because that needs real YAML semantics +# and is out of scope here. The consequence is stated rather than hidden: +# re-wrapping a folded scalar reports as drift even when the meaning is +# unchanged. That direction is the safe one. A re-wrap costs a human one look at +# the diff; the opposite error is the silent under-check that produced this +# check. +# +# The indent condition on comment dropping is that same trade-off. Below the +# case-item indent a leading `#` is not necessarily a comment: inside a block +# scalar (`use_case: |`) or a multi-line double-quoted scalar it is ordinary +# text, and dropping it would let an edit to that line report clean — the one +# normalization that erred toward silence rather than noise. Deeper lines are +# compared as body text instead, so a genuine comment edit there shows as drift. +# +# Any shape the extractor cannot read with certainty is a failure, not a skip. +extract_case_bodies() { + awk -v src="$1" ' + function die(lineno, msg) { + printf("%s:%s: %s\n", src, lineno, msg) > "/dev/stderr" + err = 1 + exit 1 + } + + BEGIN { + state = 0; itemind = -1; ncases = 0; casekeys = 0 + # A single quote, as an octal escape. Writing the character itself would + # mean breaking out of the shell quoting around this program for it. + SQ = "\047" + } + + # A column-0, non-blank, non-comment line either opens the cases block or, + # once inside it, closes it. + /^[^[:space:]#]/ { + if ($0 ~ /^cases:[[:space:]]*(#.*)?$/) { + casekeys++ + if (casekeys > 1) { + die(FNR, "a second top-level cases: key; the catalog shape is not what this check parses") + } + state = 1 + next + } + if (state == 1) { state = 2 } + next + } + + state != 1 { next } + + # A blank line carries nothing to compare wherever it sits. + /^[[:space:]]*$/ { next } + + { + match($0, /^[[:space:]]*/) + ind = RLENGTH + rest = substr($0, ind + 1) + isitem = (rest ~ /^-([[:space:]]|$)/) + + # A leading `#` is a comment only at a structural position: at or above + # the case-item indent, and anywhere before the first item has fixed that + # indent. Deeper than that it can be content — a line inside a block + # scalar or a multi-line quoted scalar — and dropping it would hide an + # edit to it. Those fall through and are compared as body text. + if (rest ~ /^#/ && (itemind < 0 || ind <= itemind)) { next } + + if (itemind < 0) { + if (!isitem) { + die(FNR, "content inside the cases: block before the first case item; the catalog shape is not what this check parses") + } + itemind = ind + } + + if (ind < itemind) { + die(FNR, "a line inside the cases: block indented less than the case items; the catalog shape is not what this check parses") + } + + # At the item indent, anything that is not an item start would be + # appended to the previous case body and mis-attributed to it. + if (ind == itemind && !isitem) { + die(FNR, "a non-item line at the case-item indent; the catalog shape is not what this check parses") + } + + if (ind == itemind) { + # New case. Its id must be the first key of the item: the id is what + # every other check keys on, and an item whose id sits further down is + # a shape this extractor would silently mis-attribute. + if (!match($0, /^[[:space:]]*-[[:space:]]+id:[[:space:]]*/)) { + die(FNR, "case item does not open with an id: key; the catalog shape is not what this check parses") + } + raw = substr($0, RLENGTH + 1) + sub(/[[:space:]]+$/, "", raw) + + if (substr(raw, 1, 1) == "\"") { + if (!match(raw, /^"[^"\\]+"([[:space:]]*#.*)?$/)) { + die(FNR, "case id is a double-quoted scalar this check will not read unambiguously (an escape, or an unterminated quote)") + } + id = raw + sub(/^"/, "", id) + sub(/"([[:space:]]*#.*)?$/, "", id) + } else if (substr(raw, 1, 1) == SQ) { + if (!match(raw, "^" SQ "[^" SQ "\\\\]+" SQ "([[:space:]]*#.*)?$")) { + die(FNR, "case id is a single-quoted scalar this check will not read unambiguously (an escape, or an unterminated quote)") + } + id = raw + sub("^" SQ, "", id) + sub(SQ "([[:space:]]*#.*)?$", "", id) + } else { + id = raw + sub(/[[:space:]]+#.*$/, "", id) + sub(/[[:space:]]+$/, "", id) + } + + # The id has to be a plain token: it names the case in every error + # message this check emits, and it is compared as an exact string. + if (id !~ /^[A-Za-z0-9][A-Za-z0-9._-]*$/) { + die(FNR, "case id is not a plain token; this check compares ids as exact strings and will not guess at this one") + } + if (id in seen) { + die(FNR, "duplicate case id " id "; ids key this comparison, so the second case would be invisible to it") + } + seen[id] = FNR + ncases++ + curid = id + } + + line = $0 + sub(/[[:space:]]+$/, "", line) + printf("%s\t%s\n", curid, line) + } + + END { + if (err) { exit 1 } + if (casekeys == 0) { + printf("%s: no top-level cases: key found\n", src) > "/dev/stderr" + exit 1 + } + if (ncases == 0) { + printf("%s: the cases: block parsed to zero cases; this check would compare nothing and report clean\n", src) > "/dev/stderr" + exit 1 + } + } + ' "$1" +} + +if ! extract_case_bodies "$PINNED_CATALOG" > "$WORK/pinned.tsv"; then + fail "case-body drift check: could not read the case bodies out of the pinned catalog '$PINNED_CATALOG' (see the parse error above). Not reporting a clean result." +fi + +if ! extract_case_bodies "$TIP_CATALOG" > "$WORK/tip.tsv"; then + fail "case-body drift check: could not read the case bodies out of the comparison catalog '$TIP_CATALOG' (see the parse error above). Not reporting a clean result." +fi + +# --------------------------------------------------------------------------- +# The registered ids to restrict the comparison to +# --------------------------------------------------------------------------- + +# Tolerate blank lines and surrounding whitespace in the id list; reject +# anything else, because an id this check silently drops is a case it silently +# stops guarding. +# The `|| true` is load-bearing. grep exits 1 when it selects no lines, so on an +# id list that is entirely blank the pipeline would fail under `pipefail` and +# `set -e` would end the script right here — exit 1 with nothing printed. The +# exit code would be the right one for the wrong reason, and the next +# maintainer would get a silent red with no message to act on. Let the pipeline +# succeed and let the explicit emptiness check below do the reporting. +{ tr -d '\r' < "$REGISTERED_IDS" \ + | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//' \ + | grep -v '^$' \ + | sort -u || true ; } > "$WORK/ids" + +if [[ ! -s "$WORK/ids" ]]; then + fail "case-body drift check: '$REGISTERED_IDS' holds no case ids. An empty id list makes this check vacuously green, which is the failure it exists to prevent." +fi + +if malformed="$(grep -vE '^[A-Za-z0-9][A-Za-z0-9._-]*$' "$WORK/ids" || true)"; [[ -n "$malformed" ]]; then + fail "case-body drift check: '$REGISTERED_IDS' holds entries that are not plain case ids: $(echo "$malformed" | tr '\n' ' '). Not reporting a clean result." +fi + +case_body() { + # $1 = stream, $2 = id + awk -F '\t' -v id="$2" '$1 == id { print substr($0, length(id) + 2) }' "$1" +} + +# --------------------------------------------------------------------------- +# Compare +# --------------------------------------------------------------------------- + +drifted=0 +missing_from_pin=0 +absent_from_tip=0 +compared=0 +: > "$WORK/report" + +while IFS= read -r id; do + case_body "$WORK/pinned.tsv" "$id" > "$WORK/body.pinned" + case_body "$WORK/tip.tsv" "$id" > "$WORK/body.tip" + + if [[ ! -s "$WORK/body.pinned" ]]; then + # The SDK registers a case its own pinned catalog does not hold. Whatever + # else is true, this check cannot vouch for that case, so it says so. + echo "::error::This SDK registers conformance case '$id', which the PINNED catalog does not contain. The case-body drift check cannot compare it." >&2 + missing_from_pin=$((missing_from_pin + 1)) + continue + fi + + if [[ ! -s "$WORK/body.tip" ]]; then + # Removal is id-level drift and the id-set check reports it. Named here so + # it is not mistaken for a compared-and-clean case, but not counted as body + # drift, to keep one catalog change from being reported twice. + echo "::warning::Conformance case '$id' is registered by this SDK and present in the pinned catalog, but absent from the comparison catalog. That is id-level drift; the alignment check is what reports it." >&2 + absent_from_tip=$((absent_from_tip + 1)) + continue + fi + + compared=$((compared + 1)) + + if ! diff -u -L "pinned/$id" -L "tip/$id" "$WORK/body.pinned" "$WORK/body.tip" > "$WORK/diff"; then + drifted=$((drifted + 1)) + echo "::error::Pinned conformance case '$id' changed shape under the same id. This SDK's coverage for it was written against the pinned wording and nothing has verified it against the new wording." >&2 + cat "$WORK/diff" >&2 + { + echo "" + echo "### \`$id\`" + echo "" + echo '```diff' + cat "$WORK/diff" + echo '```' + } >> "$WORK/report" + fi +done < "$WORK/ids" + +# A run that compared nothing is not a clean run. Reached when every registered +# id is missing from the pinned catalog, or when the id list and the catalog +# have no id in common at all — a mismatched pair of inputs, say, or an id +# source that produced plausible-looking nonsense. +if [[ "$compared" -eq 0 ]]; then + fail "case-body drift check: not one registered case id could be compared ($(wc -l < "$WORK/ids" | tr -d ' ') ids read). The inputs do not line up; this is not a clean result." +fi + +summary_head="" +if [[ "$drifted" -gt 0 ]]; then + summary_head="## Conformance case-body drift detected" +elif [[ "$missing_from_pin" -gt 0 ]]; then + summary_head="## Conformance case-body drift check could not verify every registered case" +fi + +if [[ -n "$DRIFT_SUMMARY" && -n "$summary_head" ]]; then + { + echo "$summary_head" + echo "" + echo "Compared $compared registered case(s) between the pinned catalog and the catalog tip." + if [[ "$drifted" -gt 0 ]]; then + echo "" + echo "$drifted registered case(s) changed body under an unchanged id. The id-level" + echo "alignment check cannot see this: the id is the same, so the case looks adopted" + echo "while its requirement has moved." + echo "" + echo "**Next steps:** re-read the coverage in \`$COVERAGE_DIR\` against the new" + echo "wording. Either the coverage still holds and the pin can be bumped, or it does not" + echo "and the registration should be downgraded to the level actually demonstrated." + cat "$WORK/report" + fi + if [[ "$missing_from_pin" -gt 0 ]]; then + echo "" + echo "$missing_from_pin registered case(s) are absent from the pinned catalog, so their" + echo "bodies could not be compared at all." + fi + } >> "$DRIFT_SUMMARY" +fi + +if [[ "$drifted" -gt 0 || "$missing_from_pin" -gt 0 ]]; then + exit 1 +fi + +echo "Conformance case bodies: compared $compared registered case(s) between the pinned catalog and the catalog tip; no case changed shape under an unchanged id." +if [[ "$absent_from_tip" -gt 0 ]]; then + echo "($absent_from_tip registered case(s) are absent from the comparison catalog; the alignment check reports those.)" +fi diff --git a/.github/scripts/conformance-case-body-drift.test.sh b/.github/scripts/conformance-case-body-drift.test.sh new file mode 100755 index 0000000..d6ec274 --- /dev/null +++ b/.github/scripts/conformance-case-body-drift.test.sh @@ -0,0 +1,703 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Tests for conformance-case-body-drift.sh and conformance-registered-case-ids.sh. +# +# Both scripts run only on the weekly drift schedule, so a break in either +# surfaces late and quietly — and the way it surfaces is a green run, because +# what they guard against is a check that under-reports. Shellcheck cannot see +# that class at all: a loosened id regex that silently drops cases, or a `diff` +# whose exit code stops being read, is valid shell. These tests pin the +# behaviour instead, so such an edit fails at PR time rather than the next time +# the catalog is re-tightened in place. +# +# The headline case is the real one the drift check exists for: the metadata +# jwks_uri rotation case was re-tightened under an unchanged id between two +# catalog revisions. The fixtures carry a trimmed form of both wordings, so the +# suite asserts against the change that actually happened rather than an +# invented one. +# +# Every fixture is a handful of YAML and JSON written to a temp dir. Nothing +# here checks out the catalog, runs the conformance suite, or needs Maven — the +# point is that these controls stay runnable and fast on a PR. +# +# Run: .github/scripts/conformance-case-body-drift.test.sh + +SCRIPTDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +DRIFT="$SCRIPTDIR/conformance-case-body-drift.sh" +IDSCRIPT="$SCRIPTDIR/conformance-registered-case-ids.sh" + +# conformance-registered-case-ids.sh reads its report with jq and fails cleanly +# when jq is absent — which would turn half this suite into a check that the +# absence message is right, silently dropping the cases it is here to cover. +# Refuse to run instead of passing for that reason. +if ! command -v jq > /dev/null 2>&1; then + echo "error: these tests need jq; conformance-registered-case-ids.sh reads its report with it" >&2 + exit 1 +fi + +failures=0 + +# One root that an EXIT trap removes, so a fixture still gets cleaned up when +# `set -e` kills the shell from inside a helper — the moment a leak is least +# welcome. The per-case RETURN traps below do not fire then. +TESTROOT="$(mktemp -d)" +trap 'rm -rf "$TESTROOT"' EXIT + +pass() { printf ' ok %s\n' "$1"; } +fail() { printf ' FAIL %s\n %s\n' "$1" "$2"; failures=$((failures + 1)); } + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + +# Writes a catalog to $1. $2 picks the wording of the jwks_uri rotation case: +# "pinned" for the one the SDK's coverage was written against, "tip" for the +# re-tightening that replaced it under the same id. Trimmed to the keys that +# carry the change — surface, requirement_summary, stimulus, rationale — because +# the point of the fixture is the shape of the edit, not its length. +# +# standards_in_scope is present on purpose. It holds `- id:` entries that are +# not cases, and a fixture without it would not exercise the scoping that keeps +# them out of the comparison. +write_catalog() { + local out="$1" jwks="$2" + + cat > "$out" <<'YAML' +--- +schema_version: "1.0" +catalog_id: "oauth-sdk-conformance-catalog" +catalog_version: "2026-08-04" + +standards_in_scope: + - id: "RFC8414" + title: "OAuth 2.0 Authorization Server Metadata" + - id: "RFC7009" + title: "OAuth 2.0 Token Revocation" + +cases: +YAML + + if [[ "$jwks" == "pinned" ]]; then + cat >> "$out" <<'YAML' + - id: "rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache" + title: "Reconfigure JWKS resolution when metadata jwks_uri changes" + surface: "sdk-client.discovery" + priority: "medium" + requirement_summary: "When trusted metadata changes jwks_uri, the SDK SHOULD rebind JWKS fetching to the new URI." + stimulus: + operation: "client._on_metadata_changed" + expected: + outcome: "accept" + side_effect: + - "jwks_uri updated to new metadata value" + rationale: "Keeps key discovery aligned with metadata rotation without requiring client recreation." +YAML + else + cat >> "$out" <<'YAML' + - id: "rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache" + title: "Follow a metadata jwks_uri rotation using only ordinary verification traffic" + surface: "sdk-verifier.jwks" + priority: "medium" + requirement_summary: "A verifier SHOULD re-read metadata on its configured refresh interval and rebind JWKS fetching\ + \ to the rotated jwks_uri. Following the rotation MUST require nothing beyond ordinary verification traffic: no\ + \ force-refresh argument, no test-only hook, and no reflective access to internals." + stimulus: + operation: "verifier.verify, repeated as ordinary traffic spanning the metadata refresh interval" + expected: + outcome: "accept" + side_effect: + - "metadata re-fetched after the refresh interval elapses, without an explicit refresh call" + - "JWKS fetched from new_metadata.jwks_uri" + rationale: "Keeps key discovery aligned with metadata rotation without requiring client recreation. The mechanism\ + \ restriction is the substance of the case: a verify-only resource server never repeats client-side discovery." +YAML + fi + + cat >> "$out" <<'YAML' + - id: "rfc7009-revocation-server-errors-must-surface" + title: "Surface revocation endpoint server errors as failures" + surface: "sdk-client.revocation" + priority: "high" + requirement_summary: "A 5xx from the revocation endpoint MUST surface as a failure rather than be swallowed." + expected: + outcome: "reject" + rationale: "A revocation the caller believes succeeded is worse than one that visibly failed." +YAML +} + +# A pinned/tip pair that differs only in the jwks case, in $1/pinned.yaml and +# $1/tip.yaml, with both case ids registered in $1/ids.txt. +make_pair() { + local root="$1" + write_catalog "$root/pinned.yaml" pinned + write_catalog "$root/tip.yaml" tip + cat > "$root/ids.txt" <<'IDS' +rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache +rfc7009-revocation-server-errors-must-surface +IDS +} + +# Runs the drift script against $1/{pinned,tip}.yaml and $1/ids.txt, capturing +# stdout and stderr together into $out and the exit status into $rc. Both are +# declared `local` by the caller. +run_drift() { + local root="$1" + rc=0 + out="$(PINNED_CATALOG="$root/pinned.yaml" \ + TIP_CATALOG="$root/tip.yaml" \ + REGISTERED_IDS="$root/ids.txt" \ + DRIFT_SUMMARY="$root/summary.md" \ + "$DRIFT" 2>&1)" || rc=$? +} + +# --------------------------------------------------------------------------- +# conformance-case-body-drift.sh +# --------------------------------------------------------------------------- + +# --- the positive control ------------------------------------------------------ +# Without this every assertion below could be satisfied by a script that fails +# unconditionally, and the suite would look green while guarding nothing. +t_identical_catalogs_are_clean() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + cp "$root/pinned.yaml" "$root/tip.yaml" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 0 ]]; then + fail "identical catalogs are clean" "exit $rc, want 0: ${out##*$'\n'}" + elif ! grep -q "compared 2 registered case(s)" <<<"$out"; then + fail "identical catalogs are clean" "it did not report comparing both cases: ${out##*$'\n'}" + else + pass "identical catalogs report clean, having compared both registered cases" + fi +} + +# --- the case this check exists for -------------------------------------------- +# The jwks_uri rotation case was re-tightened in place: same id, new surface, new +# requirement, new mechanism restriction. Every id-level check in the alignment +# machinery sees an unchanged id set and reports clean. This is the one that must +# not, and it must name the case — a red run that does not say which case sends +# the maintainer back into the catalog diff by hand. +t_retightened_case_is_drift() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "a re-tightened case is drift" "exit $rc, want 1" + elif ! grep -q "rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache" <<<"$out"; then + fail "a re-tightened case is drift" "it failed without naming the case: ${out##*$'\n'}" + elif ! grep -q "sdk-verifier.jwks" <<<"$out"; then + fail "a re-tightened case is drift" "it named the case but printed no diff of the change" + elif ! grep -q "rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache" "$root/summary.md"; then + fail "a re-tightened case is drift" "the case is missing from the drift summary" + else + pass "a case re-tightened under an unchanged id fails, names the case and diffs it" + fi +} + +# --- scoping to the ids this SDK registers ------------------------------------- +# The whole reason the check takes an id list: diffing the entire catalog fires +# on cases this SDK does not cover, and noise is what gets a guard ignored. Same +# drifted catalog as above, with only the untouched case registered. +t_unregistered_drift_is_ignored() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + echo "rfc7009-revocation-server-errors-must-surface" > "$root/ids.txt" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 0 ]]; then + fail "drift outside the registered ids is ignored" "exit $rc, want 0: ${out##*$'\n'}" + else + pass "a case that drifted but is not registered does not fail the check" + fi +} + +# --- standards_in_scope ids are not cases -------------------------------------- +# `standards_in_scope` earlier in the catalog carries its own `- id:` entries, and +# they are plain tokens that pass every id check. If the extractor ever reached +# them, "RFC8414" would compare clean against itself and this check would vouch +# for a case that does not exist. +t_standards_in_scope_is_not_a_case() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + cat > "$root/ids.txt" <<'IDS' +RFC8414 +rfc7009-revocation-server-errors-must-surface +IDS + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "standards_in_scope ids are not cases" "exit $rc, want 1" + elif ! grep -q "registers conformance case 'RFC8414', which the PINNED catalog does not contain" <<<"$out"; then + fail "standards_in_scope ids are not cases" "it did not report RFC8414 as absent: ${out##*$'\n'}" + else + pass "an id from standards_in_scope is not found as a case" + fi +} + +# --- a `#` line deep in a scalar is body, not a comment ------------------------ +# Inside a block scalar a leading `#` is text. Dropping such lines as comments +# made an edit to one report clean, which is the single normalization in the +# script that erred toward silence. Both fixtures here keep the structural shape +# identical so nothing but the scalar line can account for the result. +t_comment_shaped_scalar_line_is_body() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + cp "$root/pinned.yaml" "$root/tip.yaml" + + cat >> "$root/pinned.yaml" <<'YAML' + - id: "rfc8414-metadata-refresh-sequence" + use_case: | + Rotation sequence, in order: + # step 2: the AS begins serving new_metadata + expected: + outcome: "accept" +YAML + cat >> "$root/tip.yaml" <<'YAML' + - id: "rfc8414-metadata-refresh-sequence" + use_case: | + Rotation sequence, in order: + # step 2: the AS withdraws jwks-v1.json entirely + expected: + outcome: "accept" +YAML + echo "rfc8414-metadata-refresh-sequence" > "$root/ids.txt" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "a comment-shaped line inside a scalar is body text" "exit $rc, want 1 — the edit was dropped as a comment" + elif ! grep -q "withdraws jwks-v1.json" <<<"$out"; then + fail "a comment-shaped line inside a scalar is body text" "it failed without diffing the edited line" + else + pass "an edit to a #-leading line inside a block scalar reports as drift" + fi +} + +# --- a comment at a structural position stays a comment ------------------------ +# The other half of that trade-off. Comments around and between the case items +# are YAML comments by construction, and treating a re-worded one as drift would +# be the noise the scoping above exists to avoid. +t_structural_comment_is_not_body() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + cp "$root/pinned.yaml" "$root/tip.yaml" + + # At the case-item indent, and at column 0 inside the cases block. + cat >> "$root/tip.yaml" <<'YAML' + # Revisit this grouping once the verifier surfaces settle. +# Catalog maintainers: keep the cases sorted by RFC number. +YAML + + local out rc + run_drift "$root" + if [[ "$rc" -ne 0 ]]; then + fail "a structural comment is not body" "exit $rc, want 0: ${out##*$'\n'}" + else + pass "comments at and above the case-item indent do not register as drift" + fi +} + +# --- a registered case the pin does not hold ----------------------------------- +# Nothing can be said about that case's body either way, so the check says so +# rather than counting it as compared-and-clean. +t_registered_id_absent_from_pin() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + cp "$root/pinned.yaml" "$root/tip.yaml" + printf 'rfc9999-a-case-the-pin-never-had\n' >> "$root/ids.txt" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "a registered id absent from the pin fails" "exit $rc, want 1" + elif ! grep -q "rfc9999-a-case-the-pin-never-had" <<<"$out"; then + fail "a registered id absent from the pin fails" "it did not name the case: ${out##*$'\n'}" + else + pass "a registered case the pinned catalog does not hold fails, naming it" + fi +} + +# --- a case removed at the tip is id-level drift, reported elsewhere ----------- +# Removal is what the alignment check reports. Counting it here too would put one +# catalog change in two red steps, so it warns and stays out of the exit status — +# but it must still be named, not folded into the compared-and-clean count. +t_case_absent_from_tip_warns() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + cp "$root/pinned.yaml" "$root/tip.yaml" + # Drop the last case from the tip only — it runs to the end of the file, so + # deleting from its `- id:` line onward removes the whole item rather than + # orphaning its keys onto the case before it. + sed '/- id: "rfc7009-revocation-server-errors-must-surface"/,$d' "$root/tip.yaml" > "$root/tip.trimmed" + mv "$root/tip.trimmed" "$root/tip.yaml" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 0 ]]; then + fail "a case absent from the tip warns" "exit $rc, want 0: ${out##*$'\n'}" + elif ! grep -q "absent from the comparison catalog" <<<"$out"; then + fail "a case absent from the tip warns" "it passed without mentioning the removal" + else + pass "a case removed at the tip warns and leaves the exit status to the alignment check" + fi +} + +# --- an all-blank id list ------------------------------------------------------ +# An empty id list makes this check vacuously green, which is the failure it +# exists to prevent. The wording assertion is not decoration: the grep that +# filters blank lines exits 1 when it selects nothing, and under `pipefail` that +# used to end the script right here — the right exit code with nothing printed. +t_blank_id_list() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + printf '\n \n\t\n\n' > "$root/ids.txt" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "an all-blank id list fails" "exit $rc, want 1" + elif ! grep -q "holds no case ids" <<<"$out"; then + fail "an all-blank id list fails" "it failed silently, with no message to act on: ${out:-(empty)}" + else + pass "an all-blank id list fails with a message rather than a bare exit 1" + fi +} + +# --- an id list holding something that is not an id ---------------------------- +# An entry this check silently drops is a case it silently stops guarding. +t_malformed_id_list() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + echo 'rfc8414-jwks-uri rotation' > "$root/ids.txt" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "a malformed id list fails" "exit $rc, want 1" + elif ! grep -q "not plain case ids" <<<"$out"; then + fail "a malformed id list fails" "unexpected message: ${out##*$'\n'}" + else + pass "an id list entry that is not a plain case id fails" + fi +} + +# --- no id in common with the catalog ------------------------------------------ +# A mismatched pair of inputs, or an id source that produced plausible-looking +# nonsense. Nothing was compared, so nothing is clean. +t_nothing_compared() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + echo 'some-other-catalogs-case-id' > "$root/ids.txt" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "comparing nothing is not a clean run" "exit $rc, want 1" + elif ! grep -q "not one registered case id could be compared" <<<"$out"; then + fail "comparing nothing is not a clean run" "unexpected message: ${out##*$'\n'}" + else + pass "an id list with nothing in common with the catalog fails" + fi +} + +# --- a missing input ----------------------------------------------------------- +# The checkout that produces these files can fail; reading a clean result out of +# a file that is not there is the shape of failure this script refuses. +t_missing_input() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + rm -f "$root/pinned.yaml" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "a missing input fails" "exit $rc, want 1" + elif ! grep -q "is not a readable regular file" <<<"$out"; then + fail "a missing input fails" "unexpected message: ${out##*$'\n'}" + else + pass "a missing catalog fails rather than reporting a clean result" + fi +} + +# --- an empty input ------------------------------------------------------------ +# A truncated checkout leaves a file that exists and parses to nothing. +t_empty_input() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + make_pair "$root" + : > "$root/tip.yaml" + + local out rc + run_drift "$root" + if [[ "$rc" -ne 1 ]]; then + fail "an empty input fails" "exit $rc, want 1" + elif ! grep -q "is empty" <<<"$out"; then + fail "an empty input fails" "unexpected message: ${out##*$'\n'}" + else + pass "an empty catalog file fails rather than reporting a clean result" + fi +} + +# --- catalog shapes the extractor will not guess at ----------------------------- +# Each of these would otherwise be mis-attributed to a neighbouring case, which +# is the quiet kind of wrong: the comparison still runs and still reports. +# Driven off one table because the contract is identical for all of them — fail, +# and say which line and why. +t_malformed_catalog_shapes() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + + local name shape expect + while IFS='|' read -r name expect shape; do + [[ -n "$name" ]] || continue + local dir; dir="$(mktemp -d "$root/XXXXXX")" + make_pair "$dir" + # Only the tip is malformed, so a pass here cannot come from both sides + # being equally unreadable. + printf '%b' "$shape" >> "$dir/tip.yaml" + + local out rc + run_drift "$dir" + if [[ "$rc" -ne 1 ]]; then + fail "$name" "exit $rc, want 1" + elif ! grep -q "$expect" <<<"$out"; then + fail "$name" "unexpected message: ${out##*$'\n'}" + else + pass "$name" + fi + done <<'SHAPES' +a second top-level cases: key fails|a second top-level cases: key|cases:\n - id: "rfc7009-a-second-block"\n title: "x"\n +a non-item line at the case-item indent fails|a non-item line at the case-item indent| title: "orphaned, and it would land on the previous case"\n +a case item whose first key is not id: fails|does not open with an id: key| - title: "id further down"\n id: "rfc7009-id-not-first"\n +a duplicate case id fails|duplicate case id| - id: "rfc7009-revocation-server-errors-must-surface"\n title: "the same id again"\n +a case id that is not a plain token fails|not a plain token| - id: "rfc7009 revocation errors"\n title: "spaces in the id"\n +SHAPES +} + +# --------------------------------------------------------------------------- +# conformance-registered-case-ids.sh +# --------------------------------------------------------------------------- +# +# The report lists one entry per CATALOG case, so presence in it is not +# registration — `test_id` is. These fixtures are hand-written JSON rather than a +# suite run: the point is what the filter does with each shape, and running the +# suite to produce one would make these controls depend on Maven, on the catalog +# checkout, and on the suite passing. + +run_ids() { + rc=0 + out="$(CONFORMANCE_REPORT="$1" "$IDSCRIPT" 2>&1)" || rc=$? +} + +# --- only cases with a test_id are registered ----------------------------------- +# The placeholder entries the report synthesizes for uncovered catalog cases +# carry `case_id` and `status` and no `test_id` at all. Letting one through would +# put a case this SDK never registered into the comparison, where its absence +# from the pin then fails the whole check for no reason. +t_ids_filters_unregistered() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + cat > "$root/report.json" <<'JSON' +{ + "cases": [ + {"case_id": "rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache", "test_id": "ai.authplane.sdk.core.conformance.Rfc8414ConformanceTest#jwksRotation", "status": "passed"}, + {"case_id": "rfc7009-revocation-server-errors-must-surface", "test_id": "ai.authplane.sdk.core.conformance.Rfc7009ConformanceTest#serverErrors", "status": "failed"}, + {"case_id": "rfc9999-not-covered-here", "status": "not_run"} + ], + "uncatalogued_tests": [ + {"test_id": "ai.authplane.sdk.core.conformance.ConformanceCatalogTest#catalogCasesAndConformanceMappingsAgree", "status": "passed"} + ] +} +JSON + + local out rc + run_ids "$root/report.json" + if [[ "$rc" -ne 0 ]]; then + fail "only cases with a test_id are registered" "exit $rc, want 0: ${out##*$'\n'}" + elif [[ "$out" != "rfc8414-jwks-uri-rotation-must-reconfigure-jwks-cache +rfc7009-revocation-server-errors-must-surface" ]]; then + fail "only cases with a test_id are registered" "printed: ${out//$'\n'/, }" + else + pass "an absent test_id is filtered out and a failed test still counts as registered" + fi +} + +# --- uncatalogued_tests are not a source of case ids ---------------------------- +# They carry a `test_id` too, but they are tests with no @ConformanceCase mapping +# at all — they have no case id to contribute, and reading them would put test +# method names into a list the drift check compares as catalog case ids. +# Asserted above by the expected output; kept explicit here so a filter that +# started reading `..` rather than `.cases[]` fails on its own. +t_ids_ignores_uncatalogued_tests() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + cat > "$root/report.json" <<'JSON' +{ + "cases": [ + {"case_id": "rfc7009-revocation-server-errors-must-surface", "test_id": "ai.authplane.sdk.core.conformance.Rfc7009ConformanceTest#serverErrors", "status": "passed"} + ], + "uncatalogued_tests": [ + {"test_id": "ai.authplane.sdk.core.conformance.ConformanceRunStateTest#writesReport", "status": "passed"} + ] +} +JSON + + local out rc + run_ids "$root/report.json" + if [[ "$rc" -ne 0 ]]; then + fail "uncatalogued tests contribute no case ids" "exit $rc, want 0: ${out##*$'\n'}" + elif [[ "$out" != "rfc7009-revocation-server-errors-must-surface" ]]; then + fail "uncatalogued tests contribute no case ids" "printed: ${out//$'\n'/, }" + else + pass "entries under uncatalogued_tests contribute no case ids" + fi +} + +# --- a report where nothing registered ----------------------------------------- +# Every entry a placeholder: the suite was cut short, or it died before any +# @ConformanceCase test ran. An empty list downstream is a vacuously green drift +# check. +t_ids_nothing_registered() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + cat > "$root/report.json" <<'JSON' +{"cases": [{"case_id": "rfc9999-not-covered-here", "status": "not_run"}]} +JSON + + local out rc + run_ids "$root/report.json" + if [[ "$rc" -ne 1 ]]; then + fail "a report with no registration fails" "exit $rc, want 1" + elif ! grep -q "records no case with a test_id" <<<"$out"; then + fail "a report with no registration fails" "unexpected message: ${out##*$'\n'}" + else + pass "a report whose entries are all placeholders fails" + fi +} + +# --- an entry with no case_id --------------------------------------------------- +# It would drop out of the filter silently and take a real registration with it, +# so the count would be short by one with nothing to show for it. +t_ids_missing_case_id() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + cat > "$root/report.json" <<'JSON' +{ + "cases": [ + {"case_id": "rfc7009-revocation-server-errors-must-surface", "test_id": "ai.authplane.sdk.core.conformance.Rfc7009ConformanceTest#serverErrors", "status": "passed"}, + {"test_id": "ai.authplane.sdk.core.conformance.Rfc7009ConformanceTest#somethingElse", "status": "passed"} + ] +} +JSON + + local out rc + run_ids "$root/report.json" + if [[ "$rc" -ne 1 ]]; then + fail "an entry with no case_id fails" "exit $rc, want 1" + elif ! grep -q "missing or non-string case_id" <<<"$out"; then + fail "an entry with no case_id fails" "unexpected message: ${out##*$'\n'}" + else + pass "a case entry without a case_id fails instead of being dropped" + fi +} + +# --- a test_id that is present but empty or null -------------------------------- +# Absence is what marks an uncovered case. A present-but-unusable test_id is a +# shape the harness does not emit, so reading it either way would be a guess: as +# a registration it vouches for a case on the strength of a value nothing can +# name, and as a non-registration it drops a case that may well be covered. +t_ids_unusable_test_id() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + + local out rc + cat > "$root/null.json" <<'JSON' +{ + "cases": [ + {"case_id": "rfc7009-revocation-server-errors-must-surface", "test_id": null, "status": "not_run"} + ] +} +JSON + run_ids "$root/null.json" + if [[ "$rc" -ne 1 ]] || ! grep -q "not a non-empty string" <<<"$out"; then + fail "a null test_id fails" "exit $rc: ${out##*$'\n'}" + else + pass "a case entry whose test_id is null fails rather than being guessed at" + fi + + cat > "$root/empty.json" <<'JSON' +{ + "cases": [ + {"case_id": "rfc7009-revocation-server-errors-must-surface", "test_id": "", "status": "not_run"} + ] +} +JSON + run_ids "$root/empty.json" + if [[ "$rc" -ne 1 ]] || ! grep -q "not a non-empty string" <<<"$out"; then + fail "an empty test_id fails" "exit $rc: ${out##*$'\n'}" + else + pass "a case entry whose test_id is the empty string fails" + fi +} + +# --- a report that is not there, not JSON, or has no cases ---------------------- +# The workflow deletes the previous report before the run that writes the one +# this reads, so "absent" is a state that really occurs and must not read as +# "nothing registered, carry on". +t_ids_unusable_reports() { + local root; root="$(mktemp -d "$TESTROOT/XXXXXX")"; trap 'rm -rf "$root"' RETURN + + local out rc + run_ids "$root/absent.json" + if [[ "$rc" -ne 1 ]] || ! grep -q "does not exist" <<<"$out"; then + fail "a missing report fails" "exit $rc: ${out##*$'\n'}" + else + pass "a missing report fails, naming the path" + fi + + echo 'not json at all' > "$root/bad.json" + run_ids "$root/bad.json" + if [[ "$rc" -ne 1 ]] || ! grep -q "not valid JSON" <<<"$out"; then + fail "an unparseable report fails" "exit $rc: ${out##*$'\n'}" + else + pass "a report that is not JSON fails" + fi + + echo '{"cases": []}' > "$root/empty.json" + run_ids "$root/empty.json" + if [[ "$rc" -ne 1 ]] || ! grep -q "no non-empty .cases array" <<<"$out"; then + fail "a report with no cases fails" "exit $rc: ${out##*$'\n'}" + else + pass "a report with an empty cases array fails" + fi +} + +echo "conformance-case-body-drift.sh — case body comparison" +t_identical_catalogs_are_clean +t_retightened_case_is_drift +t_unregistered_drift_is_ignored +t_standards_in_scope_is_not_a_case +t_comment_shaped_scalar_line_is_body +t_structural_comment_is_not_body +t_registered_id_absent_from_pin +t_case_absent_from_tip_warns +t_blank_id_list +t_malformed_id_list +t_nothing_compared +t_missing_input +t_empty_input +t_malformed_catalog_shapes + +echo "conformance-registered-case-ids.sh — registration filter" +t_ids_filters_unregistered +t_ids_ignores_uncatalogued_tests +t_ids_nothing_registered +t_ids_missing_case_id +t_ids_unusable_test_id +t_ids_unusable_reports + +if [[ "$failures" -gt 0 ]]; then + echo "$failures failing" + exit 1 +fi +echo "all passing" diff --git a/.github/scripts/conformance-registered-case-ids.sh b/.github/scripts/conformance-registered-case-ids.sh new file mode 100755 index 0000000..c5df9c7 --- /dev/null +++ b/.github/scripts/conformance-registered-case-ids.sh @@ -0,0 +1,85 @@ +#!/usr/bin/env bash +# +# Print the conformance case ids THIS SDK registers, one per line. +# +# This is the repo-specific half of the case-body drift check: the id source +# depends on how this repo's harness records registrations, so it lives here and +# conformance-case-body-drift.sh stays generic. +# +# For this repo the ids come out of conformance-report.json, which the +# conformance suite writes when the JUnit run closes (ConformanceRunState) into +# the directory named by the `conformance.report.dir` system property — set in +# the root pom to the reactor root, so the file lands beside CHANGELOG / README. +# That report is the harness's own record of what registered, so it cannot +# disagree with what the suite actually did — the reason for reading it rather +# than grepping @ConformanceCase("...") annotations out of the test sources, +# which would be a second, weaker id extractor that reports what it matched and +# stays silent about what it missed. +# +# The report lists one entry per CATALOG case, so presence in it is not +# registration. `test_id` is: ConformanceExtension records it when a test +# carrying @ConformanceCase runs, and the placeholder entries the report +# synthesizes for uncovered catalog cases carry only `case_id` and `status`. +# Reading `test_id` rather than `status` matters — a registered case whose test +# failed or was skipped is still registered, and still needs its body watched, +# but its status is not "passed". +# +# Only `.cases` is read. `.uncatalogued_tests` alongside it also carries +# `test_id` entries, but those are tests with no @ConformanceCase mapping at +# all — they have no case id to contribute. +# +# Requires the suite to have run, so the report on disk belongs to this commit. +# +# Inputs (environment): +# CONFORMANCE_REPORT path to conformance-report.json +# (default: $GITHUB_WORKSPACE/conformance-report.json) +# +# Exit status: +# 0 ids printed on stdout +# 1 the report is missing, unreadable, or holds no registered case + +set -euo pipefail + +REPORT="${CONFORMANCE_REPORT:-${GITHUB_WORKSPACE:-.}/conformance-report.json}" + +fail() { + echo "::error::$1" >&2 + exit 1 +} + +if ! command -v jq > /dev/null 2>&1; then + fail "registered case ids: jq is not available, so the conformance report cannot be read." +fi + +if [[ ! -f "$REPORT" ]]; then + fail "registered case ids: '$REPORT' does not exist. The conformance suite writes it when the JUnit run closes, so either the suite did not run or it failed before the report was written." +fi + +if ! jq -e . "$REPORT" > /dev/null 2>&1; then + fail "registered case ids: '$REPORT' is not valid JSON." +fi + +if ! jq -e '(.cases | type) == "array" and (.cases | length) > 0' "$REPORT" > /dev/null 2>&1; then + fail "registered case ids: '$REPORT' has no non-empty .cases array." +fi + +# An entry with a case_id that is not a string, or empty, would silently drop +# out of the filter below and take a real registration with it. +if ! jq -e 'all(.cases[]; (.case_id | type) == "string" and (.case_id | length) > 0)' "$REPORT" > /dev/null 2>&1; then + fail "registered case ids: '$REPORT' holds a case entry with a missing or non-string case_id." +fi + +# A test_id that is present but not a non-empty string is a shape this filter +# has no reading for: absent means "not registered", and anything else would be +# treated as a registration on the strength of a value nothing can name. +if ! jq -e 'all(.cases[]; (has("test_id") | not) or ((.test_id | type) == "string" and (.test_id | length) > 0))' "$REPORT" > /dev/null 2>&1; then + fail "registered case ids: '$REPORT' holds a case entry whose test_id is present but is not a non-empty string." +fi + +ids="$(jq -r '.cases[] | select(has("test_id")) | .case_id' "$REPORT")" + +if [[ -z "$ids" ]]; then + fail "registered case ids: '$REPORT' records no case with a test_id, so no @ConformanceCase mapping registered. Any check restricted to this list would be vacuously green." +fi + +printf '%s\n' "$ids" diff --git a/.github/scripts/fetch-conformance-catalog.sh b/.github/scripts/fetch-conformance-catalog.sh new file mode 100755 index 0000000..7280544 --- /dev/null +++ b/.github/scripts/fetch-conformance-catalog.sh @@ -0,0 +1,76 @@ +#!/usr/bin/env bash +# +# Check out the shared conformance catalog at the revision this repo pins. +# +# Copies of this script exist outside this repo. They are not byte-identical — +# each one describes its own build — but the guards are meant to stay in step: a +# guard tightened in one copy and not the rest is how the copies drift apart. +# +# The catalog lives in github.com/AuthPlane/conformance and is updated +# independently of this repo, so cloning its default branch would let a catalog +# change turn an unrelated PR red here. The ref is pinned instead, single-sourced +# from the tracked .conformance-catalog-ref at the repo root — bump it there when +# adopting new catalog cases, together with the SDK-side coverage for them, so a +# catalog change can never break CI on its own. +# +# Uses the runner's built-in git rather than actions/checkout: equivalent trust +# for a public repo, no third-party action surface to SHA-pin. +# +# This script exists because the read/guard/fetch sequence is needed by more than +# one workflow (ci.yml, release.yml and conformance-catalog-drift.yml). Keeping it +# inline in each meant the guard could be tightened in one and not the others; the +# pin was single-sourced but the logic reading it was not. +# +# Checks out into $RUNNER_TEMP — outside $GITHUB_WORKSPACE — so the catalog stays +# out of the working tree: it must never be picked up as a module or a resource by +# the reactor, and `git add -A` in the release commit must never stage it as an +# embedded gitlink. +# +# Requires: GITHUB_WORKSPACE, RUNNER_TEMP. +# +# Optional: CONFORMANCE_CATALOG_DEST overrides the checkout directory. The drift +# workflow needs the pinned catalog and the catalog tip side by side in the same +# job to compare case bodies, so it cannot let both land on the default path. +# Every other caller leaves it unset and gets $RUNNER_TEMP/conformance. + +set -euo pipefail + +: "${GITHUB_WORKSPACE:?GITHUB_WORKSPACE must be set}" +: "${RUNNER_TEMP:?RUNNER_TEMP must be set}" + +REF_FILE="$GITHUB_WORKSPACE/.conformance-catalog-ref" +DEST="${CONFORMANCE_CATALOG_DEST:-$RUNNER_TEMP/conformance}" +CATALOG_REPO="https://github.com/AuthPlane/conformance.git" +CATALOG_FILE="oauth-sdk-conformance-catalog.yaml" + +if [[ ! -f "$REF_FILE" ]]; then + echo "::error::$REF_FILE is missing; the conformance catalog revision is unpinned" + exit 1 +fi + +CONFORMANCE_CATALOG_REF="$(tr -d '[:space:]' < "$REF_FILE")" + +# Guard against un-pinning: the ref must be a full commit SHA, not a branch or +# tag name, either of which would silently track a moving target. +if ! grep -Eq '^[0-9a-f]{40}$' <<< "$CONFORMANCE_CATALOG_REF"; then + echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA, got '$CONFORMANCE_CATALOG_REF'" + exit 1 +fi + +git init -q "$DEST" +if ! git -C "$DEST" fetch --depth=1 "$CATALOG_REPO" "$CONFORMANCE_CATALOG_REF"; then + echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable" + exit 1 +fi +git -C "$DEST" checkout -q FETCH_HEAD + +# The alignment assertion hard-fails when CONFORMANCE_CATALOG_PATH points at a +# missing file, but it reports that as a harness problem rather than drift. Fail +# here instead, where the cause is unambiguous: the fetch succeeded and the +# catalog still is not where every caller expects it. +if [[ ! -f "$DEST/$CATALOG_FILE" ]]; then + echo "::error::$CATALOG_FILE is not in the catalog at $CONFORMANCE_CATALOG_REF; the fetch succeeded but produced no catalog in $DEST" + exit 1 +fi + +echo "Conformance catalog checked out at $CONFORMANCE_CATALOG_REF in $DEST" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 781d2f4..4c5c2d5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,29 +23,20 @@ jobs: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - # Uses the runner's built-in git instead of actions/checkout: equivalent - # trust for a public repo (AuthPlane/conformance), no third-party action - # surface to SHA-pin. Keeps the catalog outside the workspace so + # Conformance catalog pinned by SHA (was: clone of the latest default + # branch). The single source of truth for the ref is the tracked + # `.conformance-catalog-ref` file at the repo root — bump it when adopting + # new catalog cases, together with the SDK-side conformance coverage, so a + # catalog change can never break CI on its own. The Checkout step above + # must precede this, which reads that file out of the workspace. + # + # The read/guard/fetch sequence lives in the script rather than inline + # here: more than one workflow needs it, and inline copies meant the + # 40-hex-SHA guard could be tightened in one and not the others. It checks + # the catalog out to $RUNNER_TEMP/conformance, outside the workspace, so # release.yml's `git add -A` cannot stage it as an embedded gitlink. - name: Check out shared conformance catalog (outside workspace) - # Conformance catalog pinned by SHA (was: clone of the latest default - # branch). The single source of truth for the ref is the tracked - # `.conformance-catalog-ref` file at the repo root — bump it when - # adopting new catalog cases, together with the SDK-side conformance - # coverage, so a catalog change can never break CI on its own. The - # Checkout step above must precede this read. Source: - # github.com/AuthPlane/conformance. - run: | - CONFORMANCE_CATALOG_REF="$(cat "$GITHUB_WORKSPACE/.conformance-catalog-ref")" - # Guard the pin: a non-SHA value would silently un-pin CI to whatever - # ref resolves at fetch time. - grep -Eq '^[0-9a-f]{40}$' <<<"$CONFORMANCE_CATALOG_REF" \ - || { echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA"; exit 1; } - git init -q "${{ runner.temp }}/conformance" - git -C "${{ runner.temp }}/conformance" \ - fetch --depth=1 https://github.com/AuthPlane/conformance.git "$CONFORMANCE_CATALOG_REF" \ - || { echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable"; exit 1; } - git -C "${{ runner.temp }}/conformance" checkout -q FETCH_HEAD + run: .github/scripts/fetch-conformance-catalog.sh - name: Setup Java uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4.8.0 diff --git a/.github/workflows/conformance-catalog-drift.yml b/.github/workflows/conformance-catalog-drift.yml index bc843a0..8784043 100644 --- a/.github/workflows/conformance-catalog-drift.yml +++ b/.github/workflows/conformance-catalog-drift.yml @@ -15,6 +15,21 @@ name: Conformance catalog drift # on drift so the run turns red and the `::warning::` plus job-summary line are # not buried in an otherwise-green run — prompting a coordinated ref bump plus # SDK-side coverage. +# +# The job runs TWO checks, because they see different things: +# +# 1. Case-id alignment against the tip. Reports cases added to the catalog +# that this SDK does not yet cover, and mappings pointing at case ids the +# catalog no longer has. This is a set comparison over ids. +# +# 2. Case-body drift for the cases this SDK registers. Reports a case that was +# re-tightened *in place* — same id, changed requirement. Check 1 is blind +# to this by construction: the id set is unchanged, so the case still looks +# adopted while the requirement underneath it has moved, and the SDK keeps +# declaring conformance to wording nothing verified it against. That has +# already happened once, to the metadata jwks_uri rotation case. Check 2 +# needs the catalog at *both* refs in the same job, which is why the pinned +# catalog is checked out here alongside the tip. on: schedule: @@ -65,27 +80,141 @@ jobs: CONFORMANCE_CATALOG_PATH: ${{ runner.temp }}/conformance/oauth-sdk-conformance-catalog.yaml run: mvn -B -ntp -pl core test -Dtest=ConformanceCatalogTest + # Snapshot what the alignment step actually found, before anything else + # runs. The evidence `Report drift` classifies on is the surefire report + # for ConformanceCatalogTest, and surefire keeps exactly one report file + # per test class and rewrites it in place. The pinned-catalog run below is + # the whole core module — ConformanceCatalogTest included, since + # src/conformance/java is a test-source root of that module — so it + # re-runs the same class against the pinned catalog, where it passes, and + # overwrites the drifted run's report. Reading the marker two Maven runs + # later would therefore find nothing on exactly the week this job exists + # to announce, and report real drift as a build problem while telling the + # reader not to touch the pin. Capturing it here is what keeps the two + # checks from destroying each other's evidence. + - name: Snapshot the alignment outcome + id: align_marker + if: always() + run: | + if grep -rqF 'Conformance-catalog drift:' core/target/surefire-reports 2>/dev/null; then + echo "marker=true" >> "$GITHUB_OUTPUT" + else + echo "marker=false" >> "$GITHUB_OUTPUT" + fi + + # The catalog at the ref this repo *pins*, alongside the tip cloned above. + # The body check needs both to compare anything; with only the tip it + # would have nothing to compare against and could report nothing but + # green. Reuses the same checkout ci.yml and release.yml use — including + # its 40-hex-SHA guard and its unreachable-ref failure — rather than a + # third copy of that logic that could be tightened in one place and not + # the others. CONFORMANCE_CATALOG_DEST keeps it off the default path, + # which the tip clone above already occupies. + # + # Placed AFTER the alignment check, with `if: always()`, so the two checks + # fail independently. Run before `align`, a failed checkout — a transient + # network error, or a pinned SHA that has become unreachable — would skip + # `Setup Java` and `align` (neither carries an `if:`, so both default to + # `success()`) and silently drop the id-level check that ran on its own + # before check 2 existed. `Report drift` would then classify the skip as + # an infrastructure problem, which is the wrong diagnosis for a check that + # never started. + - name: Check out pinned conformance catalog (outside workspace) + if: always() + env: + CONFORMANCE_CATALOG_DEST: ${{ runner.temp }}/conformance-pinned + run: .github/scripts/fetch-conformance-catalog.sh + + # The ids for check 2. They come from the harness's own report rather than + # from a grep over the test sources, so they cannot disagree with what + # actually registered. + # + # Run against the PINNED catalog, not the tip. The question the body check + # asks is "which cases does this SDK declare conformance to under its + # current pin", and anchoring the list to the pin keeps it stable while the + # tip moves. It also keeps this step independent of the alignment step, + # which is EXPECTED to go red whenever the tip has drifted — hence + # `if: always()` here too. + - name: Collect the case ids this SDK registers + if: always() + env: + CONFORMANCE_CATALOG_PATH: ${{ runner.temp }}/conformance-pinned/oauth-sdk-conformance-catalog.yaml + run: | + # Delete the report the alignment step wrote against the TIP catalog. + # If the run below fails to produce a new one, the id script must find + # nothing rather than silently read the earlier step's report and + # describe the wrong catalog. + rm -f "$GITHUB_WORKSPACE/conformance-report.json" + + # The whole core suite, not `-Dtest=ConformanceCatalogTest`: the + # report records a case as registered when a test carrying its + # @ConformanceCase annotation runs, so a filtered run would leave + # every case it skipped looking uncovered. + status=0 + mvn -B -ntp -pl core test > "$RUNNER_TEMP/pinned-suite.log" 2>&1 || status=$? + if [ "$status" -ne 0 ]; then + # Not this job's signal to raise: a suite that fails against its own + # pinned catalog is red on every PR in ci.yml already. Surfaced, and + # the id list still comes from THIS run's report, never an older one. + echo "::warning::The conformance suite did not pass against the pinned catalog (exit $status). This job reports catalog drift, not suite health — see ci.yml. Log tail follows." + tail -n 40 "$RUNNER_TEMP/pinned-suite.log" + fi + + "$GITHUB_WORKSPACE/.github/scripts/conformance-registered-case-ids.sh" \ + > "$RUNNER_TEMP/registered-case-ids.txt" + echo "This SDK registers $(wc -l < "$RUNNER_TEMP/registered-case-ids.txt") conformance case(s)." + + # Check 2. Fails the job when a case this SDK registers changed body + # between the pinned ref and the tip, and fails just as loudly when it + # cannot do that comparison at all. + - name: Check pinned case bodies against the catalog tip + id: bodies + if: always() + env: + PINNED_CATALOG: ${{ runner.temp }}/conformance-pinned/oauth-sdk-conformance-catalog.yaml + TIP_CATALOG: ${{ runner.temp }}/conformance/oauth-sdk-conformance-catalog.yaml + REGISTERED_IDS: ${{ runner.temp }}/registered-case-ids.txt + COVERAGE_DIR: core/src/conformance/java/ + run: | + DRIFT_SUMMARY="$GITHUB_STEP_SUMMARY" \ + .github/scripts/conformance-case-body-drift.sh + # Runs even when the alignment step fails the job, so the `::warning::` # and job summary are always written on drift. A `failure` outcome alone # does not mean drift — the step also fails on a compile error, a Maven # resolution failure, or a failure of the other test in the class. Real # drift is identified by the `Conformance-catalog drift:` marker the - # assertion writes into the surefire report; anything else that failed the - # step is reported as an infrastructure problem, as are skipped/cancelled - # outcomes (the alignment step never ran because an earlier step failed). + # assertion writes into the surefire report, read from the snapshot taken + # right after that step rather than from the report directory, which later + # steps overwrite; anything else that failed the step is reported as an + # infrastructure problem, as are skipped/cancelled outcomes (the alignment + # step never ran because an earlier step failed). + # + # The body check is recorded here too, and on success as well as failure. + # It prints its own detail into the step summary only when it finds + # something, so without this line a green run would say nothing about it + # at all — and a check whose silence is indistinguishable from its absence + # is how this class of drift went unnoticed in the first place. - name: Report drift if: always() run: | pinned="$(cat "$GITHUB_WORKSPACE/.conformance-catalog-ref")" grep -Eq '^[0-9a-f]{40}$' <<<"$pinned" \ || { echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA"; exit 1; } + + if [ "${{ steps.bodies.outcome }}" = "success" ]; then + echo "Conformance case bodies: no registered case changed shape under an unchanged id." >> "$GITHUB_STEP_SUMMARY" + else + echo "::warning::The pinned case-body check did not pass (outcome: ${{ steps.bodies.outcome }}). Either a case this SDK registers was re-tightened under the same id, or the check could not run. Read its step log — it names the case." + fi + case "${{ steps.align.outcome }}" in success) echo "No conformance-catalog drift detected against the latest catalog." >> "$GITHUB_STEP_SUMMARY" echo "Pinned ref: \`$pinned\`" >> "$GITHUB_STEP_SUMMARY" ;; failure) - if grep -rqF 'Conformance-catalog drift:' core/target/surefire-reports 2>/dev/null; then + if [ "${{ steps.align_marker.outputs.marker }}" = "true" ]; then echo "::warning::Conformance-catalog drift: the catalog-alignment check fails against the latest catalog. New or changed cases exist since the pinned ref ($pinned). Review github.com/AuthPlane/conformance, add SDK-side coverage, then bump .conformance-catalog-ref." { echo "### ⚠️ Conformance-catalog drift detected" @@ -96,7 +225,7 @@ jobs: echo "**Next steps:** review [AuthPlane/conformance](https://github.com/AuthPlane/conformance), add SDK-side coverage for any new cases, then bump \`.conformance-catalog-ref\` in the same change." } >> "$GITHUB_STEP_SUMMARY" else - echo "::warning::The catalog-alignment step failed without the drift marker (no 'Conformance-catalog drift:' in the surefire report). This is a build or harness problem — a compile error, a Maven resolution failure, or the other test in the class — not catalog drift." + echo "::warning::The catalog-alignment step failed without the drift marker (no 'Conformance-catalog drift:' in its surefire report). This is a build or harness problem — a compile error, a Maven resolution failure, or the other test in the class — not catalog drift." { echo "### ⚠️ Conformance drift check failed for another reason" echo "" diff --git a/.github/workflows/cut-release.yml b/.github/workflows/cut-release.yml index 3a8dd86..b48c22e 100644 --- a/.github/workflows/cut-release.yml +++ b/.github/workflows/cut-release.yml @@ -282,8 +282,17 @@ jobs: labels: release,automated delete-branch: true + # Best-effort. Auto-merge requires `allow_auto_merge` on the repository, + # which GitHub gates behind a paid plan for a private repo — this one is + # on the free plan, so the call fails and without this guard it took the + # whole job down after the cut had already done its work. ts-sdk and + # cs-sdk carry the same guard. The summary below reads this step's outcome + # rather than assuming it failed, so it stays accurate wherever this + # workflow is ported. - name: Enable auto-merge on bump PR + id: automerge if: steps.validate.outputs.mode == 'release' && steps.cpr.outputs.pull-request-number + continue-on-error: true env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: gh pr merge ${{ steps.cpr.outputs.pull-request-number }} --auto --squash @@ -294,6 +303,14 @@ jobs: v="${{ steps.validate.outputs.release }}" branch="${{ steps.validate.outputs.branch }}" mode="${{ steps.validate.outputs.mode }}" + case "${{ steps.automerge.outcome }}" in + success) bumpNote="bump PR opened — auto-merge armed" + bumpStep="Confirm the bump PR against \`$default\` merged: auto-merge was armed on it." ;; + skipped) bumpNote="no bump PR opened" + bumpStep="No bump PR was opened, so \`$default\` still targets its current version — bump it by hand if that is wrong." ;; + *) bumpNote="bump PR opened — merge it manually" + bumpStep="Merge the bump PR against \`$default\` by hand: auto-merge did not arm, which needs \`allow_auto_merge\` on the repository." ;; + esac { if [[ "$mode" == "hotfix" ]]; then echo "### Hotfix branch cut" @@ -310,10 +327,12 @@ jobs: echo "" echo "- **Release version**: \`$v\`" echo "- **Branch**: \`$branch\` (version \`${{ steps.validate.outputs.releaseSnapshot }}\`)" - echo "- **Next dev version on \`$default\`**: \`${{ steps.validate.outputs.nextDev }}\` (auto-merge PR opened)" + echo "- **Next dev version on \`$default\`**: \`${{ steps.validate.outputs.nextDev }}\` ($bumpNote)" echo "" echo "**Next steps**:" echo "1. On \`$branch\`: rename \`## [Unreleased]\` to \`## [$v]\` (or add a new dated entry), land any stabilization fixes." echo "2. When ready, run the **Release** workflow from \`$branch\` (optionally with dry-run first)." + echo "3. $bumpStep" + echo "4. After publication, copy the \`## [$v]\` section from tag \`v$v\` back onto \`$default\` verbatim, and delete from \`$default\`'s \`[Unreleased]\` every bullet that section now carries — the rename in step 1 happens only on the release branch and nothing back-merges it, so \`$default\` otherwise never records the release and re-announces those bullets at the next cut. Copy from the tag, not \`$branch\`: **Release** deletes the branch once publication succeeds." fi } >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/publish-maven.yml b/.github/workflows/publish-maven.yml index 3ff66f9..6f6d17a 100644 --- a/.github/workflows/publish-maven.yml +++ b/.github/workflows/publish-maven.yml @@ -39,6 +39,13 @@ jobs: with: ref: ${{ github.ref }} + # `-P release verify` below re-runs the full suite as a last-chance gate, + # and the conformance alignment tests read the catalog through + # CONFORMANCE_CATALOG_PATH. Same shared script ci.yml and release.yml use, + # so the 40-hex-SHA guard cannot be tightened in one and not the others. + - name: Check out shared conformance catalog (outside workspace) + run: .github/scripts/fetch-conformance-catalog.sh + - name: Set up JDK 21 uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4.8.0 with: @@ -75,25 +82,6 @@ jobs: exit 1 fi - # Uses the runner's built-in git instead of actions/checkout: equivalent - # trust for a public repo (AuthPlane/conformance), no third-party action - # surface to SHA-pin. Keeps the catalog outside the workspace. Same step - # as ci.yml and release.yml — the verify below re-runs the full test - # suite as a last-chance gate, and the conformance alignment tests read - # the catalog through CONFORMANCE_CATALOG_PATH. - - name: Check out shared conformance catalog (outside workspace) - run: | - CONFORMANCE_CATALOG_REF="$(cat "$GITHUB_WORKSPACE/.conformance-catalog-ref")" - # Guard the pin: a non-SHA value would silently un-pin CI to whatever - # ref resolves at fetch time. - grep -Eq '^[0-9a-f]{40}$' <<<"$CONFORMANCE_CATALOG_REF" \ - || { echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA"; exit 1; } - git init -q "${{ runner.temp }}/conformance" - git -C "${{ runner.temp }}/conformance" \ - fetch --depth=1 https://github.com/AuthPlane/conformance.git "$CONFORMANCE_CATALOG_REF" \ - || { echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable"; exit 1; } - git -C "${{ runner.temp }}/conformance" checkout -q FETCH_HEAD - - name: Build, test, sign # Same `-P release` profile the old single-workflow used: enables # source + javadoc jars + GPG signing + Central publishing plugin diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 0986909..381471a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -90,29 +90,20 @@ jobs: ref: ${{ github.ref }} token: ${{ steps.app_token.outputs.token }} - # Uses the runner's built-in git instead of actions/checkout: equivalent - # trust for a public repo (AuthPlane/conformance), no third-party action - # surface to SHA-pin. Keeps the catalog outside the workspace so the - # `git add -A` below cannot stage it as an embedded gitlink. + # Conformance catalog pinned by SHA (was: clone of the latest default + # branch). The single source of truth for the ref is the tracked + # `.conformance-catalog-ref` file at the repo root — bump it when adopting + # new catalog cases, together with the SDK-side conformance coverage, so a + # catalog change can never break CI on its own. The checkout step above + # must precede this, which reads that file out of the workspace. + # + # The read/guard/fetch sequence lives in the script rather than inline + # here: more than one workflow needs it, and inline copies meant the + # 40-hex-SHA guard could be tightened in one and not the others. It checks + # the catalog out to $RUNNER_TEMP/conformance, outside the workspace, so + # the `git add -A` below cannot stage it as an embedded gitlink. - name: Check out shared conformance catalog (outside workspace) - # Conformance catalog pinned by SHA (was: clone of the latest default - # branch). The single source of truth for the ref is the tracked - # `.conformance-catalog-ref` file at the repo root — bump it when - # adopting new catalog cases, together with the SDK-side conformance - # coverage, so a catalog change can never break CI on its own. The - # checkout step above must precede this read. Source: - # github.com/AuthPlane/conformance. - run: | - CONFORMANCE_CATALOG_REF="$(cat "$GITHUB_WORKSPACE/.conformance-catalog-ref")" - # Guard the pin: a non-SHA value would silently un-pin CI to whatever - # ref resolves at fetch time. - grep -Eq '^[0-9a-f]{40}$' <<<"$CONFORMANCE_CATALOG_REF" \ - || { echo "::error::.conformance-catalog-ref must be a 40-hex commit SHA"; exit 1; } - git init -q "${{ runner.temp }}/conformance" - git -C "${{ runner.temp }}/conformance" \ - fetch --depth=1 https://github.com/AuthPlane/conformance.git "$CONFORMANCE_CATALOG_REF" \ - || { echo "::error::Pinned conformance catalog ref $CONFORMANCE_CATALOG_REF is unreachable"; exit 1; } - git -C "${{ runner.temp }}/conformance" checkout -q FETCH_HEAD + run: .github/scripts/fetch-conformance-catalog.sh - name: Set up JDK 21 uses: actions/setup-java@c1e323688fd81a25caa38c78aa6df2d33d3e20d9 # v4.8.0 diff --git a/.github/workflows/workflows-lint.yml b/.github/workflows/workflows-lint.yml index dfadf36..5f57722 100644 --- a/.github/workflows/workflows-lint.yml +++ b/.github/workflows/workflows-lint.yml @@ -2,23 +2,44 @@ name: Lint workflows # Catches workflow YAML / shell-in-`run:` regressions at PR time so a # typo can't reach a release tag and surface only when a publish run -# fails. Scoped to changes under `.github/workflows/**` to keep CI -# overhead off unrelated PRs. +# fails. The shell scripts under scripts/ are in the same category — a +# break in them surfaces only when someone reaches for them after a +# release, which is the worst moment to discover it — so they are +# shellchecked and, where they have a suite, tested here too. +# +# `.github/scripts/*.sh` is in scope for the same reason. Those scripts +# serve the weekly conformance drift workflow, so a break in them surfaces +# late and quietly — and the way it surfaces is a green run, because what +# they guard against is a check that under-reports. Shellcheck alone would +# not have been enough for them, so they carry their own tests here as +# well, next to backport-fixes.test.sh. +# +# Scoped to `.github/workflows/**`, `.github/scripts/*.sh` and +# `scripts/*.sh` to keep CI overhead off unrelated PRs. on: pull_request: paths: - ".github/workflows/**" + - ".github/scripts/*.sh" + - "scripts/*.sh" push: branches: - main paths: - ".github/workflows/**" + - ".github/scripts/*.sh" + - "scripts/*.sh" permissions: contents: read jobs: + # This job runs actionlint, shellcheck and the backport-fixes suite — the id + # is narrower than the work. Do not rename it: the required status check is + # named after the job id, exactly as the branch protection rule depends on + # the workflow `name:` above. Renaming either silently drops the check from + # the required set. Add steps here instead. actionlint: runs-on: ubuntu-latest steps: @@ -62,3 +83,31 @@ jobs: # job fails loudly instead of silently degrading. - name: Run actionlint run: actionlint -color -shellcheck=shellcheck + + # actionlint only reaches shell inside `run:` blocks. The scripts + # those blocks invoke need shellcheck run against them directly. + - name: Shellcheck the repo scripts + run: shellcheck scripts/*.sh + + # backport-fixes.sh resolves --from as a branch or a tag against + # origin, and its failure modes (a fetch that fails for a reason + # other than "no such ref") are only reachable with a real remote. + # The suite builds throwaway repos in a temp dir — no network. + - name: Test backport-fixes.sh + run: scripts/backport-fixes.test.sh + + # The scripts the drift workflow invokes. Same argument as the repo + # scripts above: actionlint only reaches shell inside `run:` blocks. + - name: Shellcheck the workflow support scripts + run: shellcheck .github/scripts/*.sh + + # Shellcheck is the only other gate on the conformance drift scripts, + # and it cannot see the way they break: a loosened id regex that drops + # cases, or a `diff` whose exit code stops being read, is valid shell. + # The result is a check that reports green while guarding nothing, on a + # weekly schedule where nobody is watching — and it would stay unnoticed + # until a real re-tightening slipped through, which is the failure that + # check exists to prevent. These controls pin the detection itself. They + # need neither Maven nor the catalog checkout, so they run here. + - name: Test conformance-case-body-drift.sh + run: .github/scripts/conformance-case-body-drift.test.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index a36f6ef..28bd343 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,27 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `AccessDeniedException` and `InvalidTargetException` (subtypes of `TokenExchangeException`) for the `access_denied` (403) and `invalid_target` (400) token errors authserver 0.2.0 returns on a non-allowlisted cross-client exchange and on a `resource` that does not match a granted resource exactly; neither counts toward the circuit breaker. +- `ResourceOptions.builder().resourceMetadataUrl(...)`, Spring's `authplane.resource-metadata-url` and `AuthplaneMcpSetup.Builder.resourceMetadataUrl(...)` point the challenge's `resource_metadata` at an AS-hosted RFC 9728 document; `AuthplaneResource.resourceMetadataUrl()` is what adapters advertise. Gated at construction like the resource identifier: absolute `http(s)` with a host, no fragment, no userinfo, and a valid RFC 3986 query. +- New `WwwAuthenticate.descriptionFor(errorCode)` and `FALLBACK_ERROR_DESCRIPTION`, plus `verboseDescription` overloads on `WwwAuthenticate.of(...)` and `FailureResponse.of(...)` that restore the exception message for local debugging. + +### Changed + +- **BREAKING** `WwwAuthenticate.of(...)` and `FailureResponse.of(...)` now emit a fixed `error_description` chosen by the `error` code — on the challenge and in the JSON body alike — instead of the exception message. **Migration**: log `getMessage()` server-side, or pass `verboseDescription: true`. +- **BREAKING** The `ServerTransportSecurityException` the `mcp` and `spring` adapters raise now carries the fixed per-code sentence, not the exception message, which the MCP SDK's transport renders into the response. `extract(...)` still throws the typed exception with its own message. +- **BREAKING** `AuthplaneAuthenticationProvider` no longer reflects the exception message into the `OAuth2AuthenticationException` it raises; both the `OAuth2Error` description and the exception message now carry the fixed per-code sentence. The SDK's own entry point discarded both, but an application wiring this provider under Spring's `oauth2ResourceServer` gets `BearerTokenAuthenticationEntryPoint`, which renders the description straight into `error_description` — and `server.error.include-message=always` put the message in the error body. **Migration:** read `getCause()` for the original exception, which is unchanged. +- **BREAKING** `ASCredentials` now rejects a blank `clientSecret` at construction: authserver ≥ 0.1.2 answers `active: false` to unauthenticated introspection, so a public client would silently reject every token as revoked. Register a confidential client and pass its secret. +- The built-in introspection checker warns at construction when the client has no `AuthProvider`, and once per checker when the AS answers `active: false` for a token that passed local verification, pointing at the runtime-client requirement (`authserver admin resource runtime-client add`). +- **BREAKING** A resource identifier must now name a host at construction, not just carry a scheme: `urn:example:api`, `https:///mcp` and `https://:8443/mcp` are rejected by the new `ProtectedResourceMetadata.requireAuthority(String)` gate, which every construction path calls. An opaque identifier bound every DPoP request to the literal origin `urn://null`. **Migration:** configure the absolute URL clients address, e.g. `https://api.example.com/mcp`. +- `DocumentCache.forceRefresh()` now waits for a refresh already in flight instead of returning the document it holds as 0.2.0 did — its caller reaches it precisely because that document lacks the `kid`. +- Documentation correction for 0.2.0: `DocumentCache`'s constructor has refused a non-positive refresh interval and a null clock since that release — both reached the same permanent-expiry state the 0.2.0 cache-directive fix closed — and the published 0.2.0 notes never recorded it. An embedder constructing `JwksCache` or `MetadataCache` directly with a refresh interval of `0` upgrades from 0.1.0 and gets an `IllegalArgumentException` at construction with nothing in the changelog explaining it. + +### Deprecated + +- `VerifiedClaims.mayAct()`: authserver 0.2.0 no longer issues `may_act`; removed in the next minor. + ## [0.2.0] - 2026-09-09 ### Added diff --git a/README.md b/README.md index 723e62c..4a35845 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,10 @@ OAuth 2.1 JWT validation and token operations for Java resource servers, with fi Requires Java 21+. +## Compatibility + +Tested against authserver 0.2.0. Introspection-based revocation (`useBuiltinRevocationChecker()`) requires authserver 0.1.2 or later, which only answers the token's issuing client or a runtime-client of the resource — see the user guides. + ## Quickstart — MCP server with auth Using the [`authplane-mcp`](mcp/README.md) adapter for the [MCP Java SDK](https://github.com/modelcontextprotocol/java-sdk)'s servlet transport: diff --git a/core/docs/user-guide.md b/core/docs/user-guide.md index 4b248cf..1e04701 100644 --- a/core/docs/user-guide.md +++ b/core/docs/user-guide.md @@ -125,7 +125,8 @@ List scopes = claims.scopes(); | `CompletableFuture verify(String token, VerificationRequestContext context)` | `CompletableFuture` | Verify a JWT and apply inbound DPoP validation when configured | | `Map prmResponse()` | `Map` | RFC 9728 Protected Resource Metadata document for this resource | | `String prmPath()` | `String` | URL path at which this resource's RFC 9728 PRM document should be served | -| `String prmUrl()` | `String` | Absolute URL of this resource's RFC 9728 PRM document, for the `resource_metadata` challenge parameter (returned verbatim — not header-escaped) | +| `String prmUrl()` | `String` | Absolute URL of the resource-hosted RFC 9728 PRM document, always derived from the resource identifier (returned verbatim — not header-escaped) | +| `String resourceMetadataUrl()` | `String` | URL to advertise in the `resource_metadata` challenge parameter: the configured override, else `prmUrl()` (returned verbatim — not header-escaped) | | `String resourceUri()` | `String` | The scoped resource URI | | `List scopes()` | `List` | The configured scope list | | `AuthplaneClient client()` | `AuthplaneClient` | The parent client | @@ -157,7 +158,7 @@ Immutable snapshot of the validated claims. Accessor names match the record comp | `boolean hasClaim(String key)` | Check presence of any raw claim | | `boolean hasClaim(String key, Object value)` | Check a raw claim equals the expected value | | `Map act()` | `act` (actor) claim, or `null` | -| `Map mayAct()` | `may_act` claim, or `null` | +| `Map mayAct()` | **Deprecated** — authserver 0.2.0 no longer issues `may_act`; removed in the next minor. Returns the claim, or `null` | | `Map cnf()` | `cnf` claim as an immutable map, or `Map.of()` | | `boolean hasCnf()` | True when the token carries a `cnf` claim | | `boolean isDpopBound()` | True when `cnf.jkt` is present and non-blank | @@ -235,6 +236,7 @@ Per-resource configuration. Supply to `client.resource(resourceUri, scopes, opti | `allowedAlgorithms(List)` | `["RS256", "ES256"]` | JWT signing algorithms; only `RS256` and `ES256` (asymmetric) are supported, `none` and HMAC (HS256/384/512) are always rejected | | `clockSkewSeconds(int)` | `30` | Leeway applied to `exp`, `nbf`, `iat` | | `inboundDPoP(InboundDPoPOptions)` | `null` | Enables inbound DPoP proof validation | +| `resourceMetadataUrl(String)` | derived | Advertises this URL in the `resource_metadata` challenge parameter instead of the derived, resource-hosted one; must be an absolute `http(s)` URL naming a host, with no fragment, no userinfo, and a valid RFC 3986 query | | `useBuiltinRevocationChecker()` | disabled | Enables RFC 7662 introspection-based revocation checking; mutually exclusive with `revocationChecker(...)` | | `revocationChecker(RevocationChecker)` | `null` | Plug in a custom checker (e.g. Redis blocklist); mutually exclusive with `useBuiltinRevocationChecker()` | | `failClosed()` | fail-open | Reject tokens if the revocation check throws | @@ -286,6 +288,14 @@ AuthplaneResource verifier = client.resource(resourceUri, scopes, options); Uses the client's metadata cache to discover the introspection endpoint, the client's AS credentials for HTTP Basic auth, and the client's SSRF-safe transport. **Fails open** by default on transport or endpoint errors. Add `.failClosed()` to reject tokens when the checker throws. +**Who may introspect (authserver ≥ 0.1.2).** The client behind `ASCredentials` must be confidential (a public client cannot introspect at all) and must be either the client the token was issued to or a runtime-client of the Resource named in `aud`. Anyone else gets `{"active": false}`, so a resource server introspecting with the wrong credentials rejects every token as revoked. Register the resource server on its Resource with: + +```bash +authserver admin resource runtime-client add --client-id --slug +``` + +The checker logs a warning at construction when the client has no `AuthProvider`, and once when introspection answers `active: false` for a token that already passed local JWT verification. + ### Custom revocation checker ```java @@ -348,6 +358,25 @@ TokenResponse exchanged = client.exchange( | `actorToken(String)` | Actor token for delegation (RFC 8693 §2.1) | | `actorTokenType(String)` | Actor token type URN | +##### Exchange errors + +All three are subtypes of `TokenExchangeException`, and none of them trips the circuit breaker — the AS answered correctly: + +- `ConsentRequiredException` (`consent_required` / `interaction_required`): the user has not granted the downstream service; surface `consentUrl()` and retry after consent. +- `AccessDeniedException` (`access_denied`, HTTP 403): on a cross-client exchange, the operator has not allowlisted the exchanging client on the target Resource. Re-prompting the user will not fix it — the Resource policy must change (next section). +- `InvalidTargetException` (`invalid_target`, HTTP 400, RFC 8707 §2.2): the `resource` string does not match a granted resource exactly — a trailing slash or a different scheme is enough. Send the resource identifier byte for byte as it was granted. + +##### Operator step: allowlist the exchanging client + +For each MCP server that exchanges for a downstream resource it does not itself act as, allowlist its client ID on that Resource: + +```http +PATCH /admin/resources/{id} +{"policy": {"exchange": {"allowed_client_ids": [""]}}} +``` + +A client exchanging a token issued to itself, fronted exchanges, and Broker resources need nothing. + #### Token cache behaviour A shared in-memory cache backs both `clientCredentials` and `exchange`. @@ -469,6 +498,29 @@ Well-known path derivation: | `https://api.example.com/mcp` | `/.well-known/oauth-protected-resource/mcp` | | `https://api.example.com/v2/mcp` | `/.well-known/oauth-protected-resource/v2/mcp` | +#### Where the PRM document lives + +The document can be hosted in either of two places, and the resource decides which one its challenges point at: + +| Topology | Who serves the document | What the challenge advertises | +|---|---|---| +| Resource-hosted (default) | This server, at `prmPath()` | `prmUrl()` — the derived `/.well-known/oauth-protected-resource[/path]` | +| AS-hosted | The authorization server, which publishes one document per registered resource (authserver 0.2.0 and later serves `/.well-known/oauth-protected-resource/{ref}`, `ref` being the RFC 9728 §3.1 path suffix of the resource URI, or its slug) | The URL configured via `ResourceOptions.builder().resourceMetadataUrl(...)` | + +Use the second when the resource server cannot serve well-known paths — a platform that owns them, or a gateway that routes only the resource path. Nothing else changes: the SDK still derives `prmPath()`/`prmUrl()`, and `resourceMetadataUrl()` is what adapters put in the challenge. + +Whichever you pick, RFC 9728 §3.3 binds the document to the identifier: the `resource` member the client reads must equal, byte for byte, the identifier it derived the metadata request from. The resource URI registered at the authorization server, the `resourceUri` configured here, and the URL clients actually call therefore all have to be the same string — a trailing slash, a different host, or an extra path segment makes it a different resource and the client discards the document. + +```java +ResourceOptions options = ResourceOptions.builder() + .resourceMetadataUrl("https://auth.example.com/.well-known/oauth-protected-resource/mcp") + .build(); +AuthplaneResource resource = client.resource("https://mcp.example.com/mcp", scopes, options); + +resource.resourceMetadataUrl(); // the AS-hosted URL above — what challenges advertise +resource.prmUrl(); // still the derived https://mcp.example.com/.well-known/... +``` + `ProtectedResourceMetadata.wellKnownUrl(String resourceUri)` returns the full URL. If the resource identifier carries a query component, the returned URL carries it verbatim (`https://api.example.com/mcp?tenant=a` → `https://api.example.com/.well-known/oauth-protected-resource/mcp?tenant=a`) while routing stays path-keyed — the derivation table above is unaffected. The framework adapters (`authplane-mcp`, `authplane-spring`) register the servlet/router automatically — this is only needed when writing your own adapter. ### Dev mode @@ -501,6 +553,8 @@ All SDK exceptions extend `AuthplaneException` (unchecked). | `MetadataFetchException` | 503 | Metadata endpoint unreachable or missing `jwks_uri` | | `TokenExchangeException` | 500 | AS token / exchange / introspection / revocation call failed | | `ConsentRequiredException` | 500 | Token exchange needs user consent (subtype of `TokenExchangeException`); carries an optional `consentUrl` to drive the consent flow | +| `AccessDeniedException` | 500 | AS returned `access_denied` (subtype of `TokenExchangeException`): the exchanging client is not allowlisted on the target Resource | +| `InvalidTargetException` | 500 | AS returned `invalid_target` (subtype of `TokenExchangeException`): `resource` does not match a granted resource exactly | DPoP subclasses (all extend `DPoPException`): @@ -533,6 +587,19 @@ try { `HttpStatus.of(AuthplaneException)` returns `401`, `403`, `500`, or `503` per the table above. `WwwAuthenticate.of(AuthplaneException)` produces an RFC 6750 header value using the `DPoP` scheme for DPoP errors and `Bearer` for everything else; `WwwAuthenticate.of(error, realm)` adds a realm; `WwwAuthenticate.of(error, ChallengeOptions)` additionally emits the `resource_metadata` (RFC 9728 §5.3) and `scope` (RFC 6750 §3) parameters. All parameter values are escaped for header safety. +**`error_description` is fixed, not the exception message.** The challenge and the JSON body are both served to a caller who by definition has not authenticated, so the description is chosen by the `error` code: + +| `error` | `error_description` | +|---|---| +| `invalid_token` | `The access token is missing or not valid for this resource` | +| `insufficient_scope` | `The access token does not carry the scope this operation requires` | +| `invalid_dpop_proof` | `The DPoP proof is missing or not valid for this request` | +| anything else | `The request could not be authenticated` | + +The SDK's own messages name the failing detail — the unknown `kid`, the claim that did not validate, the `typ` that was rejected — and an audience mismatch would hand the caller the exact `aud` the resource expects, which is the value they would need in order to request a token for it. RFC 6750 §3 does not require `error_description` to be diagnostic; the `error` code already carries what a conforming client acts on. The message stays on the exception, so log it server-side. + +`WwwAuthenticate.of(error, options, true)` and `FailureResponse.of(error, options, true)` restore the message for local debugging — on both halves together, since an escape hatch that opened only one would let you believe the message was suppressed while it still shipped. They disclose SDK internals to unauthenticated callers; do not enable them in production. + ### Fail-open vs fail-closed revocation Default is fail-open: exceptions from the revocation checker are logged and the token is accepted. Opt in to fail-closed so checker failures become `TokenRevokedException`: diff --git a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc6750ConformanceTest.java b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc6750ConformanceTest.java index b8483aa..d5d3215 100644 --- a/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc6750ConformanceTest.java +++ b/core/src/conformance/java/ai/authplane/sdk/core/conformance/Rfc6750ConformanceTest.java @@ -22,7 +22,13 @@ void rfc6750_error_response_must_map_error_codes() { String expired = WwwAuthenticate.of(new TokenExpiredException("expired")); assertThat(expired).startsWith("Bearer "); assertThat(expired).contains("error=\"invalid_token\""); - assertThat(expired).contains("error_description=\"expired\""); + // RFC 6750 §3 does not require error_description to be diagnostic, and the + // challenge answers an unauthenticated caller, so it is the fixed per-code + // sentence rather than the exception's message. + assertThat(expired) + .contains( + "error_description=\"The access token is missing or not valid for this resource\""); + assertThat(expired).doesNotContain("expired\""); // invalid_signature → Bearer invalid_token String badSig = WwwAuthenticate.of(new InvalidSignatureException("bad sig")); 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 9a11886..0eddb54 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 @@ -202,11 +202,21 @@ void rfc9728_well_known_url_must_preserve_the_resource_query_component() { + " scheme but no authority still constructs and throws later, on the" + " 401 challenge path, which is the shape of failure moving these" + " gates to construction was meant to remove.") - void rfc9728_resource_identifier_must_be_an_absolute_url_with_scheme_and_host() { + void rfc9728_resource_identifier_must_be_an_absolute_url_with_scheme_and_host() + throws Exception { + // resource.create is the operation the catalog names as the stimulus, so drive the factory + // itself and not only the gate it delegates to. Asserting requireScheme alone would keep + // passing if the gate stayed intact but stopped being wired into construction, which is + // the regression this case exists to catch. + AuthplaneClient client = ConformanceTestSupport.buildClient(baseUrl); + // Each value rejects on its own — the case is explicit that rejecting one does not satisfy // it, because a guard that only asks "opaque or authority-less?" catches "/mcp" while // letting the scheme-relative form through. for (String identifier : List.of("/mcp", "//api.example.com/mcp")) { + assertThatThrownBy(() -> client.resource(identifier, List.of("read:data"))) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy(() -> ProtectedResourceMetadata.requireScheme(identifier)) .isInstanceOf(IllegalArgumentException.class); diff --git a/core/src/main/java/ai/authplane/sdk/core/ASCredentials.java b/core/src/main/java/ai/authplane/sdk/core/ASCredentials.java index 5ceb6e2..daa8857 100644 --- a/core/src/main/java/ai/authplane/sdk/core/ASCredentials.java +++ b/core/src/main/java/ai/authplane/sdk/core/ASCredentials.java @@ -15,15 +15,24 @@ * {@code client_id} and {@code client_secret} form-urlencoded before being Base64-encoded. Supply * it anywhere an {@link AuthProvider} is expected. * + *

Both parts must be non-blank: a public (secret-less) client cannot introspect, revoke or + * exchange against authserver, which answers {@code active: false} to unauthenticated + * introspection. + * * @see AuthplaneClientBuilder#authProvider(AuthProvider) */ public record ASCredentials(String clientId, String clientSecret) implements AuthProvider { - /** Validates that clientId is non-null and non-blank, and clientSecret is non-null. */ + /** Validates that clientId and clientSecret are both non-null and non-blank. */ public ASCredentials { Objects.requireNonNull(clientId, "clientId must not be null"); if (clientId.isBlank()) throw new IllegalArgumentException("clientId must not be blank"); Objects.requireNonNull(clientSecret, "clientSecret must not be null"); + if (clientSecret.isBlank()) { + throw new IllegalArgumentException( + "clientSecret must not be blank: a public client cannot authenticate to the" + + " token, introspection or revocation endpoint"); + } } @Override diff --git a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java index 7fbd8d9..1f11289 100644 --- a/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java +++ b/core/src/main/java/ai/authplane/sdk/core/AuthplaneClient.java @@ -198,6 +198,9 @@ public AuthplaneResource resource( ProtectedResourceMetadata.requireValidQuery(resourceUri); // RFC 8707 §2: an absolute URI always carries a scheme. Same reason, same boundary. ProtectedResourceMetadata.requireScheme(resourceUri); + // RFC 9728 §3 derives the metadata URL by inserting the well-known string after the host + // component, and the DPoP htu origin is built from the same authority. Same boundary. + ProtectedResourceMetadata.requireAuthority(resourceUri); // RFC 9110 §4.2.4: no userinfo. The identifier is published to unauthenticated callers // verbatim, so a credential in the authority is disclosed. Same reason, same boundary. ProtectedResourceMetadata.requireNoUserinfo(resourceUri); diff --git a/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java b/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java index 51ef6ba..fee0c7c 100644 --- a/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java +++ b/core/src/main/java/ai/authplane/sdk/core/AuthplaneResource.java @@ -56,6 +56,9 @@ public class AuthplaneResource { private final boolean failClosed; private final InboundDPoPOptions inboundDPoP; + // Operator-configured override for the advertised PRM URL — null means "derive it". + private final String resourceMetadataUrl; + // ----------------------------------------------------------------------- // Package-private constructor — created by AuthplaneClient.resource() // ----------------------------------------------------------------------- @@ -73,6 +76,10 @@ public class AuthplaneResource { // spliced into the DPoP htu binding target in normalizeRequestUrl below, where a missing // one reads as the literal text "null" and fails every DPoP-bound request. ProtectedResourceMetadata.requireScheme(resourceUri); + // Authoritative authority gate, for the other half of the same splice: normalizeRequestUrl + // reads base.getRawAuthority(), which is null for an identifier that names no host, so + // "urn:example:api" bound every request to the literal origin "urn://null". + ProtectedResourceMetadata.requireAuthority(resourceUri); // Authoritative userinfo gate: the identifier is published verbatim as the PRM `resource` // member and in the resource_metadata parameter of the 401 challenge, both of which reach // unauthenticated callers, so a credential in the authority must not get this far. @@ -92,6 +99,7 @@ public class AuthplaneResource { } this.failClosed = options.failClosed(); this.inboundDPoP = options.inboundDPoP(); + this.resourceMetadataUrl = options.resourceMetadataUrl(); // KeyLookup reads through the client's JWKS cache, after giving the metadata cache the // chance to re-read: verification is the only traffic a verify-only resource server has, @@ -292,6 +300,18 @@ private void checkRevocation(String token, VerifiedClaims claims) { } } catch (TokenRevokedException e) { throw e; + } catch (InterruptedException e) { + // The check never produced a verdict: restore the flag the catch cleared so the + // caller's shutdown path still sees the interrupt, and report it as a failed check + // rather than letting the fail-closed branch below call the token revoked. + Thread.currentThread().interrupt(); + if (failClosed) { + throw new TokenRevokedException( + "Token with jti='" + + claims.jti() + + "' rejected: revocation check was interrupted"); + } + LOG.warning("Revocation check interrupted (fail-open) for jti='" + claims.jti() + "'"); } catch (Exception e) { if (failClosed) { throw new TokenRevokedException( @@ -372,6 +392,31 @@ public String prmUrl() { return ProtectedResourceMetadata.wellKnownUrl(resourceUri); } + /** + * Returns the URL to advertise in the {@code resource_metadata} parameter of a {@code + * WWW-Authenticate} challenge (RFC 9728 §5.3): the operator-configured {@link + * ResourceOptions.Builder#resourceMetadataUrl(String)} when one is set, otherwise the derived + * resource-hosted {@link #prmUrl()}. + * + *

Adapters emit this rather than {@link #prmUrl()}, so the two RFC 9728 topologies are one + * decision made once per resource: the document is hosted by this resource server at {@link + * #prmPath()} (the default), or it is hosted elsewhere — typically by the authorization server, + * which publishes one per registered resource — and this server only points at it. + * + *

Either way RFC 9728 §3.3 binds the document that URL returns: its {@code resource} member + * must equal the identifier the client derived the request from, byte for byte, or the client + * discards the document. The resource registered at the AS, {@link #resourceUri()} and the URL + * clients actually call therefore have to be the same string. + * + *

Not header-safe, for the same reason {@link #prmUrl()} is not: escape it + * with {@link ai.authplane.sdk.core.errors.WwwAuthenticate#escapeQuotedString(String)} (as + * {@code FailureResponse}/{@code WwwAuthenticate} already do) before interpolating it into a + * header value. + */ + public String resourceMetadataUrl() { + return resourceMetadataUrl != null ? resourceMetadataUrl : prmUrl(); + } + /** * Returns the path component of this resource's URI (e.g. {@code /mcp} for {@code * https://mcp.example.com/mcp}), i.e. the endpoint path this resource is served at. Empty 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 60f307f..27ac111 100644 --- a/core/src/main/java/ai/authplane/sdk/core/CircuitPolicy.java +++ b/core/src/main/java/ai/authplane/sdk/core/CircuitPolicy.java @@ -21,12 +21,14 @@ public final class CircuitPolicy { /** OAuth {@code error} values where the AS responded correctly — do not trip the breaker. */ private static final Set OAUTH_ERRORS_NO_CIRCUIT = Set.of( + "access_denied", "consent_required", "interaction_required", "invalid_grant", "invalid_scope", "invalid_dpop_proof", "invalid_request", + "invalid_target", "unsupported_grant_type"); private static final int MAX_CAUSE_DEPTH = 8; diff --git a/core/src/main/java/ai/authplane/sdk/core/IntrospectionChecker.java b/core/src/main/java/ai/authplane/sdk/core/IntrospectionChecker.java index 3571f26..cb67fb7 100644 --- a/core/src/main/java/ai/authplane/sdk/core/IntrospectionChecker.java +++ b/core/src/main/java/ai/authplane/sdk/core/IntrospectionChecker.java @@ -1,5 +1,6 @@ package ai.authplane.sdk.core; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.logging.Logger; /** @@ -12,6 +13,11 @@ * successful introspection response rejects the token unless {@code active} is explicitly {@code * true}. * + *

authserver 0.1.2 and later answer {@code active: false} unless the introspecting client is + * confidential and is either the client the token was issued to or a runtime-client of the resource + * named in {@code aud}. A checker built without credentials, or with the wrong ones, therefore + * rejects every token as revoked; both cases are logged so the cause is not silent. + * *

Package-private — created by {@link AuthplaneResource} when {@link * ResourceOptions#useBuiltinRevocationChecker()} is set. */ @@ -20,9 +26,19 @@ class IntrospectionChecker implements RevocationChecker { private static final Logger LOG = Logger.getLogger(IntrospectionChecker.class.getName()); private final AuthplaneClient client; + private final AtomicBoolean ownershipWarned = new AtomicBoolean(); IntrospectionChecker(AuthplaneClient client) { this.client = client; + if (client.authProvider == null) { + LOG.warning( + "Built-in introspection checker configured without an AuthProvider: authserver" + + " >= 0.1.2 answers active=false to unauthenticated introspection, so" + + " every token will be rejected as revoked. Set authProvider(new" + + " ASCredentials(clientId, clientSecret)) on the client builder to a" + + " confidential client that is the issuing client or a runtime-client" + + " of this resource."); + } } /** @@ -31,12 +47,32 @@ class IntrospectionChecker implements RevocationChecker { *

Exceptions propagate to the verifier, which applies the fail-open/closed policy configured * via {@link ResourceOptions.Builder#failClosed()}. * + *

The token reaching this method has already passed local JWT verification, so an {@code + * active=false} answer is either a real revocation or the AS not recognising this resource + * server as the token's owner. The first such answer is logged once per checker with the + * runtime-client requirement; later ones are not, so a busy server is not flooded. + * * @param rawToken the raw JWT string to introspect * @param jti the {@code jti} claim (for logging) */ @Override public boolean isRevoked(String rawToken, String jti) throws Exception { var resp = client.introspect(rawToken).get(); - return !resp.active(); + if (resp.active()) { + return false; + } + if (ownershipWarned.compareAndSet(false, true)) { + LOG.warning( + "Introspection returned active=false for jti='" + + jti + + "' although the token passed local JWT verification. Unless the" + + " token was revoked, the AS did not recognise this resource server" + + " as the token's owner: authserver >= 0.1.2 answers active=false" + + " unless the introspecting client is the issuing client or a" + + " runtime-client of the resource named in aud. Register it with:" + + " authserver admin resource runtime-client add --client-id" + + " --slug . Logged once per checker."); + } + return true; } } diff --git a/core/src/main/java/ai/authplane/sdk/core/ResourceOptions.java b/core/src/main/java/ai/authplane/sdk/core/ResourceOptions.java index a2bca26..a0dde63 100644 --- a/core/src/main/java/ai/authplane/sdk/core/ResourceOptions.java +++ b/core/src/main/java/ai/authplane/sdk/core/ResourceOptions.java @@ -1,11 +1,13 @@ package ai.authplane.sdk.core; +import java.net.URI; import java.util.List; import java.util.Objects; import ai.authplane.sdk.core.dpop.InboundDPoPOptions; import ai.authplane.sdk.core.dpop.VerificationRequestContext; import ai.authplane.sdk.core.errors.TokenRevokedException; +import ai.authplane.sdk.core.prm.ProtectedResourceMetadata; /** * Per-resource configuration beyond the required resource and scopes. @@ -32,6 +34,7 @@ public final class ResourceOptions { private final boolean useBuiltinRevocationChecker; private final boolean failClosed; private final InboundDPoPOptions inboundDPoP; + private final String resourceMetadataUrl; private ResourceOptions(Builder builder) { this.allowedAlgorithms = List.copyOf(builder.allowedAlgorithms); @@ -40,6 +43,7 @@ private ResourceOptions(Builder builder) { this.useBuiltinRevocationChecker = builder.useBuiltinRevocationChecker; this.failClosed = builder.failClosed; this.inboundDPoP = builder.inboundDPoP; + this.resourceMetadataUrl = builder.resourceMetadataUrl; } /** Default options: RS256+ES256, 30s clock skew, no revocation checking. */ @@ -98,6 +102,15 @@ public InboundDPoPOptions inboundDPoP() { return inboundDPoP; } + /** + * The URL the {@code resource_metadata} parameter of a {@code WWW-Authenticate} challenge + * points at, or {@code null} (the default) to advertise the resource-hosted document the SDK + * derives from the resource identifier. See {@link Builder#resourceMetadataUrl(String)}. + */ + public String resourceMetadataUrl() { + return resourceMetadataUrl; + } + /** Builder for constructing {@link ResourceOptions} instances. */ public static final class Builder { @@ -107,6 +120,7 @@ public static final class Builder { private boolean useBuiltinRevocationChecker = false; private boolean failClosed = false; private InboundDPoPOptions inboundDPoP = null; + private String resourceMetadataUrl = null; private Builder() {} @@ -132,6 +146,95 @@ public Builder inboundDPoP(InboundDPoPOptions options) { return this; } + /** + * Points the {@code resource_metadata} parameter of every {@code WWW-Authenticate} + * challenge at {@code url} instead of the resource-hosted RFC 9728 document the SDK derives + * from the resource identifier ({@code /.well-known/oauth-protected-resource[/path]}). + * + *

Set this when the document lives somewhere else — typically the copy the authorization + * server publishes for a registered resource — and the resource server cannot, or does not + * want to, serve the well-known path itself. The document that URL returns must still carry + * the exact resource identifier this resource is configured with as its {@code resource} + * member (RFC 9728 §3.3), or clients discard it. + * + *

The URL must be absolute, with an {@code http} or {@code https} scheme and a host, no + * fragment, no userinfo, and a query that is a valid RFC 3986 §3.4 query. Plain {@code + * http} is accepted on any host: the derived PRM URL this value replaces is not + * scheme-narrowed either. + * + * @param url absolute URL of the Protected Resource Metadata document + * @throws IllegalArgumentException if {@code url} is not an absolute http(s) URL naming a + * host, or carries a fragment, userinfo, or an out-of-grammar query + */ + public Builder resourceMetadataUrl(String url) { + Objects.requireNonNull(url, "url must not be null"); + this.resourceMetadataUrl = requireAbsoluteHttpUrl(url); + return this; + } + + /** + * Shape gate for {@link #resourceMetadataUrl(String)}: the value is spliced verbatim into a + * header that reaches unauthenticated callers, so anything that is not an absolute {@code + * http(s)} URL naming a host is refused where the operator wrote it rather than advertised. + * + *

{@code http} is accepted on any host, with no comparison against the resource + * identifier's own scheme: the derived PRM URL this value replaces is not scheme-narrowed + * either, and a narrower gate refuses the in-cluster and docker-compose topologies dev mode + * exists to serve. + * + *

The userinfo, fragment and query gates are the identifier's own, reused verbatim: this + * value reaches the same {@code resource_metadata} sink the identifier does, and {@code + * URI.getHost()} is non-null for {@code https://svc:secret@host/x}, so without them a + * credential in the authority would be advertised in every 401 and 403. The query gate is + * the one {@code URI.create} cannot stand in for — it rejects only space, {@code "}, {@code + * \}, {@code |}, {@code ^}, {, }, {@code <} and {@code >}, so a + * raw non-ASCII octet would otherwise ship into the header. Their messages elide secrets, + * which is why the failure is re-reported around them rather than through {@link + * #rejection(String, String)} — that one embeds the raw value. + */ + private static String requireAbsoluteHttpUrl(String url) { + try { + ProtectedResourceMetadata.requireNoFragment(url); + ProtectedResourceMetadata.requireValidQuery(url); + ProtectedResourceMetadata.requireNoUserinfo(url); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException( + "resourceMetadataUrl is not usable as the resource_metadata parameter of a" + + " WWW-Authenticate challenge: " + + e.getMessage(), + e); + } + URI uri; + try { + uri = URI.create(url); + } catch (IllegalArgumentException e) { + throw new IllegalArgumentException(rejection(url, "it does not parse as a URI"), e); + } + String scheme = uri.getScheme(); + if (scheme == null) { + throw new IllegalArgumentException(rejection(url, "it has no scheme")); + } + if (!"https".equalsIgnoreCase(scheme) && !"http".equalsIgnoreCase(scheme)) { + throw new IllegalArgumentException( + rejection(url, "its scheme is \"" + scheme + "\", not http or https")); + } + if (uri.getHost() == null || uri.getHost().isBlank()) { + throw new IllegalArgumentException(rejection(url, "it names no host")); + } + return url; + } + + private static String rejection(String url, String reason) { + return "resourceMetadataUrl \"" + + url + + "\" is not usable as the resource_metadata parameter of a WWW-Authenticate" + + " challenge: " + + reason + + ". Configure the absolute URL clients should fetch the RFC 9728 document" + + " from, e.g. https://auth.example.com/.well-known/oauth-protected-resource/mcp."; + } + /** * Configures the verifier to reject tokens when the revocation check fails with an * exception. diff --git a/core/src/main/java/ai/authplane/sdk/core/VerifiedClaims.java b/core/src/main/java/ai/authplane/sdk/core/VerifiedClaims.java index 54721a6..10b2b39 100644 --- a/core/src/main/java/ai/authplane/sdk/core/VerifiedClaims.java +++ b/core/src/main/java/ai/authplane/sdk/core/VerifiedClaims.java @@ -175,7 +175,10 @@ public Map act() { * Returns the {@code may_act} claim as an immutable map, or {@code null} when absent. * *

Indicates which actors are authorized to act on behalf of the subject. + * + * @deprecated authserver 0.2.0 no longer issues {@code may_act}; removed in the next minor. */ + @Deprecated(forRemoval = true) @SuppressWarnings("unchecked") public Map mayAct() { Object mayActClaim = raw.get("may_act"); diff --git a/core/src/main/java/ai/authplane/sdk/core/errors/AccessDeniedException.java b/core/src/main/java/ai/authplane/sdk/core/errors/AccessDeniedException.java new file mode 100644 index 0000000..81ee35a --- /dev/null +++ b/core/src/main/java/ai/authplane/sdk/core/errors/AccessDeniedException.java @@ -0,0 +1,29 @@ +package ai.authplane.sdk.core.errors; + +import java.io.Serial; + +/** + * Thrown when the Authorization Server answers a token request with {@code access_denied} (HTTP + * 403). + * + *

On a cross-client token exchange this means the operator has not allowlisted the exchanging + * client on the target Resource ({@code policy.exchange.allowed_client_ids}). Unlike {@link + * ConsentRequiredException}, re-prompting the user does not fix it; the Resource policy must be + * changed. The AS responded correctly, so this does not count toward the circuit breaker. + * + *

The simple name collides with Spring Security's {@code + * org.springframework.security.access.AccessDeniedException}, which the Spring adapter throws for + * an authorization failure. Both are unchecked, so catching the wrong one compiles and catches + * nothing; import this one by its full name in any class that also uses Spring Security's. + */ +public final class AccessDeniedException extends TokenExchangeException { + + @Serial private static final long serialVersionUID = 1L; + + /** + * @param message human-readable message (the AS {@code error_description} when present) + */ + public AccessDeniedException(String message) { + super(message, "access_denied"); + } +} diff --git a/core/src/main/java/ai/authplane/sdk/core/errors/FailureResponse.java b/core/src/main/java/ai/authplane/sdk/core/errors/FailureResponse.java index 79d1e20..5ca10b1 100644 --- a/core/src/main/java/ai/authplane/sdk/core/errors/FailureResponse.java +++ b/core/src/main/java/ai/authplane/sdk/core/errors/FailureResponse.java @@ -38,10 +38,37 @@ public record Challenge(int status, String wwwAuthenticate, String jsonBody) {} * @return the status, {@code WWW-Authenticate} header, and JSON body */ public static Challenge of(AuthplaneException error, ChallengeOptions options) { + return of(error, options, false); + } + + /** + * {@link #of(AuthplaneException, ChallengeOptions)} with {@code verboseDescription} restoring + * the exception's own message in {@code error_description} — on both the challenge and the + * body, which is the point: the two travel in the same response to the same caller, so an + * escape hatch that opened only one of them would be a way to think the message was suppressed + * while it still shipped. + * + *

A development aid; do not enable it in production. + * + * @param error the verification/authorization failure + * @param options challenge parameters; use {@link ChallengeOptions#empty()} when none + * @param verboseDescription whether to emit the exception message instead of the fixed sentence + * @return the status, {@code WWW-Authenticate} header, and JSON body + */ + public static Challenge of( + AuthplaneException error, ChallengeOptions options, boolean verboseDescription) { int status = HttpStatus.of(error); - String header = WwwAuthenticate.of(error, options); + String header = WwwAuthenticate.of(error, options, verboseDescription); String code = WwwAuthenticate.errorCodeFor(error); - String description = error.getMessage() != null ? error.getMessage() : code; + + // The body carries the same fixed sentence the challenge does. It used to carry + // error.getMessage(), which named the unknown kid, the claim that did not validate, or the + // aud the resource expects — to a caller who by definition has not authenticated, and who + // reads whichever half of the response it looks at first. + String description = + verboseDescription && error.getMessage() != null + ? error.getMessage() + : WwwAuthenticate.descriptionFor(code); Map body = new LinkedHashMap<>(); body.put("error", code); diff --git a/core/src/main/java/ai/authplane/sdk/core/errors/InsufficientScopeException.java b/core/src/main/java/ai/authplane/sdk/core/errors/InsufficientScopeException.java index 1c33b28..4e01c32 100644 --- a/core/src/main/java/ai/authplane/sdk/core/errors/InsufficientScopeException.java +++ b/core/src/main/java/ai/authplane/sdk/core/errors/InsufficientScopeException.java @@ -22,9 +22,10 @@ public InsufficientScopeException(String requiredScope, List availableSc /** * Creates an exception indicating the token is missing one or more of {@code requiredScopes} - * (logical AND). The message names every missing scope so the RFC 6750 {@code - * error_description} (derived from {@link #getMessage()}) doesn't surface just the first one, - * while {@link #getRequiredScopes()} carries the full requested set. + * (logical AND). The message names every missing scope for the server-side log; the + * RFC 6750 {@code error_description} is no longer derived from it, and carries the fixed + * caller-safe sentence instead. {@link #getRequiredScopes()} carries the full requested set, + * which is what a challenge's {@code scope} parameter is built from. * * @param requiredScopes all scopes the caller required; must be non-empty * @param availableScopes the scopes actually present on the token diff --git a/core/src/main/java/ai/authplane/sdk/core/errors/InvalidTargetException.java b/core/src/main/java/ai/authplane/sdk/core/errors/InvalidTargetException.java new file mode 100644 index 0000000..33467c3 --- /dev/null +++ b/core/src/main/java/ai/authplane/sdk/core/errors/InvalidTargetException.java @@ -0,0 +1,23 @@ +package ai.authplane.sdk.core.errors; + +import java.io.Serial; + +/** + * Thrown when the Authorization Server answers a token request with {@code invalid_target} (RFC + * 8707 §2.2, HTTP 400). + * + *

The {@code resource} parameter did not match a resource granted to the token byte for byte — a + * trailing slash or a different scheme is enough. The AS responded correctly, so this does not + * count toward the circuit breaker. + */ +public final class InvalidTargetException extends TokenExchangeException { + + @Serial private static final long serialVersionUID = 1L; + + /** + * @param message human-readable message (the AS {@code error_description} when present) + */ + public InvalidTargetException(String message) { + super(message, "invalid_target"); + } +} diff --git a/core/src/main/java/ai/authplane/sdk/core/errors/WwwAuthenticate.java b/core/src/main/java/ai/authplane/sdk/core/errors/WwwAuthenticate.java index 433b5ae..606a6ee 100644 --- a/core/src/main/java/ai/authplane/sdk/core/errors/WwwAuthenticate.java +++ b/core/src/main/java/ai/authplane/sdk/core/errors/WwwAuthenticate.java @@ -1,6 +1,7 @@ package ai.authplane.sdk.core.errors; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.regex.Pattern; @@ -28,7 +29,8 @@ * quoted-string anyway, and letting CR/LF through would allow header injection) and backslash + * double-quote escaped. * - *

Example output: {@code Bearer error="invalid_token", error_description="Token expired"} + *

Example output: {@code Bearer error="invalid_token", error_description="The access token is + * missing or not valid for this resource"} */ public final class WwwAuthenticate { @@ -77,6 +79,54 @@ public ChallengeOptions withScope(List newScope) { } } + /** + * The {@code error_description} emitted for an error code. + * + *

The challenge and the JSON body are both served to a caller who by definition has not + * authenticated, so the description is built from the RFC 6750 §3.1 / RFC 9449 §7.1 error code + * and never from the exception's own message. The SDK's messages name the failing detail — the + * unknown {@code kid}, the claim that did not validate, the {@code typ} that was rejected — and + * an audience mismatch in particular would hand the caller the exact {@code aud} the resource + * expects, which is the value they need in order to request a token for it. RFC 6750 §3 does + * not require {@code error_description} to be diagnostic: the error code already carries + * everything a conforming client needs in order to decide what to do next. + * + *

The descriptions carry no comma, so the same text stays safe to emit as a {@code + * WWW-Authenticate} quoted-string, where a comma separates challenge parameters and is what a + * lenient client-side parser splits on. + */ + private static final Map SAFE_ERROR_DESCRIPTIONS = + Map.of( + "invalid_token", "The access token is missing or not valid for this resource", + "insufficient_scope", + "The access token does not carry the scope this operation requires", + "invalid_dpop_proof", + "The DPoP proof is missing or not valid for this request"); + + /** + * Covers an error code with no entry in {@code SAFE_ERROR_DESCRIPTIONS} — any code added + * without a matching row. Kept deliberately contentless for the same reason the table exists. + */ + public static final String FALLBACK_ERROR_DESCRIPTION = + "The request could not be authenticated"; + + /** + * The fixed, caller-safe {@code error_description} for an error code. + * + * @param errorCode an RFC 6750 §3.1 / RFC 9449 §7.1 error code, as {@link + * #errorCodeFor(AuthplaneException)} returns + * @return the sentence to emit for it + */ + public static String descriptionFor(String errorCode) { + // SAFE_ERROR_DESCRIPTIONS is a Map.of, whose getOrDefault probes the key's hash and throws + // on null. The fallback covers "any code with no matching row", and an adapter deriving a + // code from its own mapping may hold none. + if (errorCode == null) { + return FALLBACK_ERROR_DESCRIPTION; + } + return SAFE_ERROR_DESCRIPTIONS.getOrDefault(errorCode, FALLBACK_ERROR_DESCRIPTION); + } + /** C0 control characters (incl. CR/LF) and DEL — illegal in an HTTP header field-value. */ private static final Pattern CONTROL_CHARS = Pattern.compile("[\\x00-\\x1f\\x7f]"); @@ -132,6 +182,25 @@ public static String of(AuthplaneException error, String realm) { * @return the header value */ public static String of(AuthplaneException error, ChallengeOptions options) { + return of(error, options, false); + } + + /** + * {@link #of(AuthplaneException, ChallengeOptions)} with {@code verboseDescription} restoring + * the exception's own message in {@code error_description}, which is what this builder emitted + * before the description became a fixed per-code sentence. + * + *

A development aid. The challenge reaches a caller who has not authenticated and the + * message names SDK internals, so do not enable it in production. Scheme, status and error code + * are identical either way. + * + * @param error the SDK exception (must not be null) + * @param options optional challenge parameters; use {@link ChallengeOptions#empty()} when none + * @param verboseDescription whether to emit the exception message instead of the fixed sentence + * @return the header value + */ + public static String of( + AuthplaneException error, ChallengeOptions options, boolean verboseDescription) { Objects.requireNonNull(error, "error must not be null"); Objects.requireNonNull(options, "options must not be null"); @@ -144,7 +213,11 @@ public static String of(AuthplaneException error, ChallengeOptions options) { } sb.append("error=\"").append(errorCode).append("\""); sb.append(", error_description=\"") - .append(escapeQuotedString(error.getMessage())) + .append( + escapeQuotedString( + verboseDescription && error.getMessage() != null + ? error.getMessage() + : descriptionFor(errorCode))) .append("\""); if (!options.scope().isEmpty()) { sb.append(", scope=\"") diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java b/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java index e91f093..736ed72 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/CacheHeaderParser.java @@ -13,8 +13,7 @@ * configured interval governs. That is what {@code no-store} and {@code no-cache} return: they say * the response should not be reused, which for a document this SDK must keep serving is not an * expiry it can honour — the caller falls back to its own interval rather than treating the - * document as permanently stale. Both siblings model it the same way, as go's zero {@code - * time.Time} and ts's {@code undefined}. + * document as permanently stale. * *

A non-null return is an absolute expiry, and {@code DocumentCache} shortens its configured TTL * to it when it is in the future. An expiry already in the past — a stale {@code Expires:}, or 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 01ced82..f9b0603 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 @@ -257,21 +257,33 @@ public Map forceRefreshIgnoringFailureBackoff() throws Exception } private Map doForceRefresh(boolean ignoreFailureBackoff) throws Exception { - // tryLock, for the same reason {@link #get()} uses it: this is a request-path caller. The - // backoff above keeps a *failing* endpoint from costing a fetch per request, but it does - // nothing for the burst that arrives before the first failure records retryNotBefore — - // those would all queue on an exclusive lock for one HTTP timeout. A caller that finds a - // fetch already in flight is served the document currently held; the two methods now - // agree that no request-path caller blocks on another thread's fetch. - Map inFlight = cachedDocument; - if (inFlight != null && !fetchLock.tryLock()) { - LOG.fine(() -> documentType + " refresh in flight elsewhere; serving the current copy"); - return inFlight; - } - if (inFlight == null) { - fetchLock.lock(); - } + // Block, then re-check — deliberately not the tryLock {@link #get()} uses. get()'s caller + // wants *a* document, so the copy in hand is a valid answer. This caller wants a *newer* + // one: JwksCache.getKeyByKid(kid, true) reaches forceRefresh() precisely because the held + // document does not carry the kid. Returning that same document while a fetch is in + // flight guarantees the lookup finds nothing, so every concurrent kid-miss caller would + // see a spurious invalid_token for the length of every healthy rotation's fetch window. + // + // Blocking is bounded by the fetch already running, and the re-check is what keeps the + // burst cheap: whoever holds the lock publishes, and everyone waiting behind it returns + // that document instead of fetching again. A failing endpoint records retryNotBefore, so + // the backoff below short-circuits the queue on the way out. One fetch per burst, not one + // per caller. + Map before = cachedDocument; + fetchLock.lock(); try { + Map current = cachedDocument; + if (before != null && current != before) { + // Another thread's fetch landed while we waited. That is what we came for; a + // second round trip now would be the amplification the backoff exists to stop. + LOG.fine( + () -> + documentType + + " was refreshed by another thread while waiting; serving" + + " that document"); + return current; + } + long now = nowEpochSeconds(); if (!ignoreFailureBackoff && cachedDocument != null @@ -388,25 +400,23 @@ private long failureBackoffSeconds() { * *

A server expiry at or before the moment the document was cached is treated as no * preference rather than as an expiry, and the configured interval governs. It has to be: - * {@code CacheHeaderParser.parseExpiresAt} returns {@code 0L} for {@code Cache-Control: - * no-store} or {@code no-cache}, {@code now} for {@code max-age=0}, and a past epoch for a - * stale {@code Expires:} — and subtracting {@code cachedAtEpochSeconds} from any of those - * yields a negative TTL. For {@code no-store} that is about -1.7e9. + * {@code CacheHeaderParser.parseExpiresAt} returns {@code now} for {@code max-age=0} and a past + * epoch for a stale {@code Expires:} — and subtracting {@code cachedAtEpochSeconds} from either + * leaves nothing to honour. {@code Cache-Control: no-store} and {@code no-cache} used to arrive + * as {@code 0L}, which made the subtraction about -1.7e9; they now return {@code null} and no + * longer reach this guard, which still clamps them defensively. * *

A negative TTL makes {@code age >= effectiveTtl} true on every read, so {@link #get()} * takes the synchronous re-fetch branch on the caller's thread every single time, forever. The * failure backoff does not cover it, because that only arms when a fetch *throws*: an endpoint - * that answers {@code no-store} successfully clears the backoff and re-arms the expiry on the - * same call. + * that answered {@code no-store} successfully cleared the backoff and re-armed the expiry on + * the same call. * *

That was harmless while nothing on a verification path read this cache. It stopped being * harmless when metadata moved onto that path — verification now reads through here before * every key lookup, and that runs before signature verification, so an unauthenticated caller * would set the rate. This is the same failure the backoff was added to remove, reached by a * different door. - * - *

go-sdk clamps the equivalent case the same way: a zero expiry falls back to the configured - * default rather than being taken literally. */ private long effectiveTtlSeconds() { if (serverExpiresAtSeconds != null && serverExpiresAtSeconds > cachedAtEpochSeconds) { diff --git a/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java b/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java index 7d36d98..986fd1e 100644 --- a/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java +++ b/core/src/main/java/ai/authplane/sdk/core/fetching/JwksCache.java @@ -45,7 +45,7 @@ public JwksCache( * silent. Advancing the clock forward from the real present, which is what a deterministic TTL * test does, is fine: the offset only has to stay inside a TTL of wall time at the moment a * document is fetched. Threading the clock into the header parser would remove the constraint - * and is tracked in #34. + * and is tracked in AuthPlane/java-sdk#34. * * @param clock time source; pass {@link Clock#systemUTC()} unless driving TTL expiry * deterministically diff --git a/core/src/main/java/ai/authplane/sdk/core/oauth/TokenResponseParser.java b/core/src/main/java/ai/authplane/sdk/core/oauth/TokenResponseParser.java index 3b0d405..9216220 100644 --- a/core/src/main/java/ai/authplane/sdk/core/oauth/TokenResponseParser.java +++ b/core/src/main/java/ai/authplane/sdk/core/oauth/TokenResponseParser.java @@ -6,7 +6,9 @@ import com.nimbusds.jose.util.JSONObjectUtils; import ai.authplane.sdk.core.TokenResponse; +import ai.authplane.sdk.core.errors.AccessDeniedException; import ai.authplane.sdk.core.errors.ConsentRequiredException; +import ai.authplane.sdk.core.errors.InvalidTargetException; import ai.authplane.sdk.core.errors.TokenExchangeException; import ai.authplane.sdk.core.fetching.RawPostResponse; @@ -43,6 +45,12 @@ static TokenResponse parse( String causeDetail = causeObj instanceof String c && !c.isBlank() ? c : desc; throw new ConsentRequiredException(desc, error, serviceId, causeDetail, consentUrl); } + if ("access_denied".equals(error)) { + throw new AccessDeniedException(desc); + } + if ("invalid_target".equals(error)) { + throw new InvalidTargetException(desc); + } throw new TokenExchangeException(desc, error); } 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 2d8e4a9..c7517b6 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 @@ -186,10 +186,10 @@ public static String wellKnownUrl(String resourceUri) { // the host and "the path and/or query components, if any"). Raw form, so the encoding is // exactly what the operator configured. An empty query (a bare trailing '?', for which // getRawQuery() returns "") is treated as absent: RFC 3986 would allow reading it as - // present-but-empty, but on *this* sub-case — an empty query — the family agrees on the - // query-less URL, and parity wins over that reading. It is only the empty-query reading - // that is settled: for a non-empty query the implementations still differ, which is - // tracked in #32 rather than asserted here. + // present-but-empty, but a bare '?' names no parameter, so both readings derive a URL that + // addresses the same document and the query-less form is the one that survives ordinary + // normalisation. Only the empty-query case is decided here; a non-empty query is tracked + // in AuthPlane/java-sdk#32 rather than asserted. String query = uri.getRawQuery(); return query == null || query.isEmpty() ? url : url + "?" + query; } @@ -378,9 +378,12 @@ private static boolean isHexDigit(char c) { * null://api.example.com/mcp} and every DPoP-bound request fails with a mismatch that names * nothing an operator can act on. * - *

Only the scheme is required here. The identifier may still be any absolute URI RFC 8707 §2 - * permits — {@code urn:example:api} constructs; whether it can derive a PRM URL is the - * derivation gate's question, answered when a derivation is actually asked for. + *

The scheme is only half of the requirement. {@link #requireAuthority(String)} runs + * immediately after and requires the identifier to name a host, for the same reason and against + * the same sink: an identifier with no authority splices the literal text {@code "null"} into + * the {@code htu} exactly as a missing scheme does. The two stay separate gates so that each + * message names the half the identifier actually fails — an operator who wrote {@code + * urn:example:api} is not helped by being told the scheme is missing. * *

Works on the raw string, like the sibling gates: a scheme is present exactly when a {@code * :} appears before any {@code /}, {@code ?} or {@code #} and the text before it matches the @@ -388,8 +391,8 @@ private static boolean isHexDigit(char c) { * *

Called from the same construction boundaries as the sibling gates: {@link * Builder#build()}, the {@code AuthplaneResource} constructor, and {@code - * AuthplaneClient.resource(...)}. The derivation-time {@code requireDerivable} stays as the - * backstop for the public derivation helpers. + * AuthplaneClient.resource(...)}. The derivation-time {@link #requireDerivable(URI)} stays as + * the backstop for the public derivation helpers. * * @param resourceUri the resource identifier, as configured by the operator * @throws IllegalArgumentException if the identifier does not begin with a URI scheme @@ -412,6 +415,119 @@ public static void requireScheme(String resourceUri) { + " https://api.example.com/mcp)."); } + /** + * Requires the identifier to name a host — the other half of the absolute-hierarchical-URI + * requirement, gated at construction beside {@link #requireScheme(String)}. + * + *

An identifier with no authority clears every other gate and is then unusable at the sink + * that matters most. {@code AuthplaneResource.normalizeRequestUrl} builds the DPoP {@code htu} + * binding target as {@code scheme + "://" + rawAuthority + requestPath}, and {@link + * URI#getRawAuthority()} is {@code null} for an identifier that has no authority — so {@code + * urn:example:api} binds every request to an origin of literally {@code urn://null} (measured + * through the constructor, not inferred). No client proof can match that, so every DPoP-bound + * request against such a resource fails an {@code htu} mismatch naming a host that does not + * exist and that nothing in the operator's configuration mentions. That is verbatim the failure + * mode {@link #requireScheme(String)} exists to close — a missing component read as the literal + * text {@code "null"} — and the authority is its other half. An identifier that is accepted and + * then cannot carry a DPoP-bound request has not been accepted in any useful sense. + * + *

The requirement is not RFC 8707 §2's, and citing it here would be citing the wrong axis: + * §2 governs the {@code resource} parameter of a token request and permits any absolute URI, + * which is a different thing from the identifier a resource server is configured with. The + * applicable clause is RFC 9728 §3, which forms the metadata URL by inserting the well-known + * string "between the host component and the path and/or query components" — there has to be a + * host component to insert after. The counter-argument, that an opaque identifier publishes no + * PRM document and so §3 never binds it, is real but does not survive the {@code htu} sink + * above: the DPoP binding is derived from the identifier whether or not a document is ever + * served. + * + *

Works on the raw string like the sibling gates, reusing the same {@code authorityBounds} + * the userinfo gate reads so that the two can never disagree about where the authority is. An + * empty authority ({@code https:///mcp}) and an authority carrying only a port ({@code + * https://:8443/mcp}) name no host and are rejected: the first derives the same literal {@code + * "null"} the opaque case does, the second an origin no client addresses. A host with a port + * ({@code https://api.example.com:8443/mcp}), an IPv6 literal ({@code https://[::1]:8443/mcp}) + * and a plain host all pass. + * + *

A scheme-relative reference ({@code //api.example.com/mcp}) does name a host and passes + * this gate; {@link #requireScheme(String)}, which runs immediately before it at every call + * site, is what rejects that shape. Each gate answers exactly one question, so the message an + * operator gets names the half that is actually missing. + * + *

Called from the three construction boundaries the sibling gates are called from: {@link + * Builder#build()}, the {@code AuthplaneResource} constructor, and {@code + * AuthplaneClient.resource(...)}. {@link #wellKnownUrl(String)} deliberately does not call it: + * {@link #requireDerivable(URI)} already answers the same question there, and answers it in the + * wording a derivation caller needs. + * + * @param resourceUri the resource identifier, as configured by the operator + * @throws IllegalArgumentException if the identifier has no authority component, or its + * authority names no host + */ + public static void requireAuthority(String resourceUri) { + Objects.requireNonNull(resourceUri, "resourceUri must not be null"); + // Cut the fragment first, like the sibling gates: a '/' or '@' after a '#' belongs to the + // fragment, not the authority. requireNoFragment has already rejected any '#' at every call + // site, but this method is public and answers its own question. + int fragmentStart = resourceUri.indexOf('#'); + String beforeFragment = + fragmentStart < 0 ? resourceUri : resourceUri.substring(0, fragmentStart); + int[] authority = authorityBounds(beforeFragment); + + // Name the requirement this identifier actually fails, for the reason requireDerivable + // names its own: "urn:example:api" and "https://:8443/mcp" fail different halves, and a + // message covering both points the operator at the wrong one. + String defect; + if (authority == null) { + defect = "it has no authority component (no \"//\" follows the scheme)"; + } else if (!namesHost(beforeFragment, authority[0], authority[1])) { + defect = "its authority names no host"; + } else { + return; + } + throw new IllegalArgumentException( + "Resource identifier \"" + + elideSecrets(resourceUri) + + "\" (fragment and any userinfo elided) does not name a host: " + + defect + + ". Both sinks that reassemble the identifier are built from its" + + " authority. The DPoP htu binding target splices the scheme, the" + + " authority and the request path, so an absent authority reads as the" + + " literal text \"null\" — \"urn:example:api\" binds every request to" + + " \"urn://null\", which no client proof can match — and RFC 9728 §3" + + " derives the Protected Resource Metadata URL by inserting the well-known" + + " string between the host component and the path, which needs a host to" + + " insert after. Configure the absolute URL clients address this resource" + + " by (e.g. https://api.example.com/mcp)."); + } + + /** + * Whether the authority delimited by {@code [start, end)} names a non-empty host. + * + *

The host is what remains of the authority once the userinfo and the port are removed (RFC + * 3986 §3.2): everything after the last {@code @} within the authority, up to the {@code :} + * that opens the port. An IPv6 literal is bracketed (§3.2.2) and its own colons sit inside the + * brackets, so the port separator is looked for after the closing {@code ]} rather than from + * the start — otherwise {@code [::1]:8443} would be read as an empty host. + * + *

Userinfo is skipped rather than rejected here: {@link #requireNoUserinfo(String)} owns + * that question, and this helper must still give the right answer for {@code + * https://svc:pw@/mcp}, whichever gate the caller happens to reach first. + */ + private static boolean namesHost(String beforeFragment, int start, int end) { + int userInfoEnd = beforeFragment.lastIndexOf('@', end - 1); + int hostStart = userInfoEnd < start ? start : userInfoEnd + 1; + int hostEnd = end; + if (hostStart < end && beforeFragment.charAt(hostStart) == '[') { + int close = beforeFragment.indexOf(']', hostStart); + hostEnd = close < 0 || close >= end ? end : close + 1; + } else { + int portColon = beforeFragment.indexOf(':', hostStart); + hostEnd = portColon < 0 || portColon >= end ? end : portColon; + } + return hostEnd > hostStart; + } + /** * Rejects a resource identifier whose authority carries a userinfo component. * @@ -438,15 +554,16 @@ public static void requireScheme(String resourceUri) { * *

Called from the same construction boundaries as the sibling gates — {@link * Builder#build()}, the {@code AuthplaneResource} constructor, and {@code - * AuthplaneClient.resource(...)} — after {@link #requireScheme(String)}, so an identifier that - * is also scheme-relative is reported for the missing scheme, the defect an operator fixes - * first. + * AuthplaneClient.resource(...)} — and last of the five, after {@link #requireScheme(String)} + * and {@link #requireAuthority(String)}, so an identifier that is also scheme-relative or + * hostless is reported for that, the defect an operator fixes first. * - *

The four gates run in the same *set* everywhere but not in the same *order*: the three - * construction sites run fragment, query, scheme, userinfo, while {@link #wellKnownUrl(String)} - * runs fragment, scheme, userinfo, query. So an identifier that violates two of them can be - * reported for a different component depending on the entrypoint. Both reject either way; only - * the message differs. Unifying the four behind one private gate is tracked in #33. + *

The gates run in the same *set* everywhere but not in the same *order*: the three + * construction sites run fragment, query, scheme, authority, userinfo, while {@link + * #wellKnownUrl(String)} runs fragment, scheme, userinfo, query and leaves the authority to + * {@link #requireDerivable(URI)}. So an identifier that violates two of them can be reported + * for a different component depending on the entrypoint. Both reject either way; only the + * message differs. Unifying them behind one private gate is tracked in AuthPlane/java-sdk#33. * * @param resourceUri the resource identifier, as configured by the operator * @throws IllegalArgumentException if the identifier's authority carries a userinfo component @@ -648,11 +765,13 @@ private static String elideSecrets(String resourceUri) { /** * 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. + *

RFC 8707 §2 permits a resource indicator that is any absolute URI, which is the reading + * under which {@code urn:example:api} used to reach this helper at all. 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. {@link #requireAuthority(String)} now + * refuses such an identifier at construction, so this is what remains for the callers that + * never construct anything. * *

The scheme is gated for the same reason, and needs its own test: a scheme-relative * reference such as {@code //api.example.com/mcp} is neither opaque nor authority-less, so it @@ -660,6 +779,24 @@ private static String elideSecrets(String resourceUri) { * null://api.example.com/.well-known/oauth-protected-resource/mcp} into that same challenge * parameter. RFC 8707 §2 requires the resource indicator to be an absolute URI, and RFC 3986 * §4.3 defines one as always carrying a scheme, so no legitimate identifier is turned away. + * + *

What is left of its job now that {@link #requireScheme(String)} and {@link + * #requireAuthority(String)} gate the same three components at construction: this is no longer + * reachable from a constructed resource — {@code AuthplaneResource.prmUrl()} cannot arrive here + * with a defective identifier, because no such identifier constructs — but {@link + * #wellKnownPath(URI)} is public and is the one entry point in this class that takes an already + * parsed {@link URI} rather than a raw string, so for those callers this is the only gate there + * is. It is also what keeps {@link #wellKnownUrl(String)} from needing an authority gate of its + * own: the question is the same, and the wording a derivation caller needs is not the wording a + * constructor argument needs. Deleting it on the grounds that construction now covers the SDK's + * own paths would leave an external caller deriving {@code urn://null/.well-known/...} and + * handing that to the {@code resource_metadata} parameter of a 401 challenge — precisely the + * output this check was written to prevent. + * + *

The accepted narrowing is that none of its three branches is reachable from inside this + * SDK any more: every internal caller now arrives with an identifier the construction gates + * have already cleared. They stay because this helper serves a public entry point and has to + * answer for itself. */ private static void requireDerivable(URI resourceUri) { // Name the requirement this identifier actually fails — a generic both-requirements @@ -681,9 +818,10 @@ private static void requireDerivable(URI resourceUri) { + "\" (any userinfo elided): " + defect + ". PRM derivation requires a hierarchical resource identifier with a" - + " scheme and 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."); + + " scheme and an authority (e.g. https://api.example.com/mcp). A resource" + + " identifier configured through this SDK is held to the same requirement" + + " at construction, so this message reaches only a caller of the public" + + " derivation helpers."); } // ----------------------------------------------------------------------- @@ -781,6 +919,7 @@ public ProtectedResourceMetadata build() { requireNoFragment(resource); requireValidQuery(resource); requireScheme(resource); + requireAuthority(resource); requireNoUserinfo(resource); return new ProtectedResourceMetadata( diff --git a/core/src/test/java/ai/authplane/sdk/core/ASCredentialsTest.java b/core/src/test/java/ai/authplane/sdk/core/ASCredentialsTest.java index bbd2481..16597a7 100644 --- a/core/src/test/java/ai/authplane/sdk/core/ASCredentialsTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/ASCredentialsTest.java @@ -48,10 +48,19 @@ void constructor_nullClientSecret_throwsNullPointerException() { } @Test - void constructor_emptyClientSecret_succeeds() { - // Empty secret is allowed — some AS allow empty secrets - ASCredentials creds = new ASCredentials("my-client", ""); - assertThat(creds.clientSecret()).isEmpty(); + void constructor_emptyClientSecret_throwsIllegalArgumentException() { + // A public (secret-less) client cannot introspect: authserver >= 0.1.2 answers + // active=false to unauthenticated introspection, so reject at construction. + assertThatThrownBy(() -> new ASCredentials("my-client", "")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("clientSecret"); + } + + @Test + void constructor_blankClientSecret_throwsIllegalArgumentException() { + assertThatThrownBy(() -> new ASCredentials("my-client", " ")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("clientSecret"); } @Test 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 17c4e79..a409b0a 100644 --- a/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/AuthplaneClientTest.java @@ -410,6 +410,61 @@ void resourceConstructor_userinfoInResource_throwsIAE() throws Exception { client.close(); } + @Test + void resource_identifierThatNamesNoHost_throwsIAE() throws Exception { + // RFC 9728 §3 forms the metadata URL by inserting the well-known string after the host + // component, and the DPoP htu binding target is built from the same authority. An + // identifier with none used to construct cleanly and then bind every DPoP request to the + // literal origin "urn://null" — an htu mismatch naming a host that does not exist. The + // permissive reading came from RFC 8707 §2, which governs the resource *parameter* of a + // token request, a different axis from the identifier a resource server is configured with. + AuthplaneClient client = buildClient(); + for (String identifier : + new String[] {"urn:example:api", "https:///mcp", "https://:8443/mcp"}) { + assertThatThrownBy(() -> client.resource(identifier, TestFixtures.SCOPES)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("does not name a host"); + } + client.close(); + } + + @Test + void resourceConstructor_identifierThatNamesNoHost_throwsIAE() throws Exception { + // Same authoritative-line reasoning as the fragment, scheme and userinfo cases above: + // every other hostless identifier enters through client.resource(...), which throws at its + // own gate first, so without this test the constructor's gate is the line no test pins. + AuthplaneClient client = buildClient(); + assertThatThrownBy( + () -> + new AuthplaneResource( + client, + "urn:example:api", + TestFixtures.SCOPES, + ResourceOptions.defaults())) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("does not name a host"); + client.close(); + } + + @Test + void resource_identifiersThatNameAHost_stillConstruct() throws Exception { + // The gate turns away identifiers that name no host, not everything that is not a bare + // https host: a development host on a port, an IPv6 literal, a percent-escape in the + // registered name and a query all still construct and are published verbatim. + AuthplaneClient client = buildClient(); + for (String identifier : + new String[] { + "http://localhost:8080/mcp", + "https://[::1]:8443/mcp", + "https://a%2Db.example.com/mcp", + "https://api.example.com/mcp?tenant=acme", + }) { + assertThat(client.resource(identifier, TestFixtures.SCOPES).prmResponse()) + .containsEntry("resource", identifier); + } + client.close(); + } + @Test void resource_hostWithPort_isAccepted() throws Exception { // A ':' in the authority is a port delimiter far more often than a userinfo one, so the diff --git a/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java b/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java index 3373a69..0dbfba1 100644 --- a/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/AuthplaneResourceTest.java @@ -332,6 +332,89 @@ void prmPath_resourceWithQuery_staysPathKeyed() throws Exception { assertThat(resource.prmPath()).isEqualTo("/.well-known/oauth-protected-resource/mcp"); } + @Test + void resourceMetadataUrl_noOverride_isTheDerivedPrmUrl() throws Exception { + // The default topology: this server hosts the document, and the challenge advertises the + // URL it is served at. Byte-identical to prmUrl(), which the adapters used to read. + resource = createResource("https://mcp.example.com/mcp"); + assertThat(resource.resourceMetadataUrl()).isEqualTo(resource.prmUrl()); + assertThat(resource.resourceMetadataUrl()) + .isEqualTo("https://mcp.example.com/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void resourceMetadataUrl_override_replacesTheDerivedUrl() throws Exception { + // The AS-hosted topology: authserver >= 0.2.0 publishes the document for every registered + // resource, and this server only points at it. prmUrl() keeps deriving the resource-hosted + // URL — the override governs what is advertised, not where a served document would live. + client = AuthplaneClient.builder(baseUrl).devMode(true).build().get(); + resource = + client.resource( + "https://mcp.example.com/mcp", + TestFixtures.SCOPES, + ResourceOptions.builder() + .resourceMetadataUrl( + "https://auth.example.com/.well-known/oauth-protected-resource/mcp") + .build()); + + assertThat(resource.resourceMetadataUrl()) + .isEqualTo("https://auth.example.com/.well-known/oauth-protected-resource/mcp"); + assertThat(resource.prmUrl()) + .isEqualTo("https://mcp.example.com/.well-known/oauth-protected-resource/mcp"); + assertThat(resource.prmPath()).isEqualTo("/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void resourceMetadataUrl_httpOverrideOnHttpsResource_isAccepted() throws Exception { + // No comparison against the resource identifier's own scheme: the derived PRM URL this + // value replaces is not scheme-narrowed either, and a narrower gate refuses the in-cluster + // and docker-compose topologies dev mode exists to serve. One acceptance envelope for this + // property across the SDKs matters more than the marginal hardening, since the same + // deployment config has to start everywhere. + client = AuthplaneClient.builder(baseUrl).devMode(true).build().get(); + resource = + client.resource( + "https://mcp.example.com/mcp", + TestFixtures.SCOPES, + ResourceOptions.builder() + .resourceMetadataUrl( + "http://auth.example.com/.well-known/oauth-protected-resource/mcp") + .build()); + + assertThat(resource.resourceMetadataUrl()) + .isEqualTo("http://auth.example.com/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void resourceMetadataUrl_rawNonAsciiInQuery_throwsAtConfiguration() { + // Same gate the identifier's query gets. URI.create does not stand in for it: it rejects + // only space, ", \, |, ^, {, }, < and >, so a raw non-ASCII octet would otherwise be + // spliced into the WWW-Authenticate header. + assertThatThrownBy( + () -> + ResourceOptions.builder() + .resourceMetadataUrl("https://auth.example.com/prm?x=café") + .build()) + .isInstanceOf(IllegalArgumentException.class); + } + + @Test + void resourceMetadataUrl_httpOverrideOnHttpResource_isAccepted() throws Exception { + // The local topology the demos use: an http resource pointing at an http AS document. + client = AuthplaneClient.builder(baseUrl).devMode(true).build().get(); + resource = + client.resource( + "http://localhost:8080/mcp", + TestFixtures.SCOPES, + ResourceOptions.builder() + .resourceMetadataUrl( + "http://localhost:9000/.well-known/oauth-protected-resource/mcp") + .build()); + + assertThat(resource.resourceMetadataUrl()) + .isEqualTo("http://localhost:9000/.well-known/oauth-protected-resource/mcp"); + } + @Test void normalizeRequestUrl_substitutesResourceHost_keepsRequestPath() throws Exception { resource = createResource("https://api.example.com/mcp"); diff --git a/core/src/test/java/ai/authplane/sdk/core/CircuitPolicyTest.java b/core/src/test/java/ai/authplane/sdk/core/CircuitPolicyTest.java index 6a8ad0e..e71d078 100644 --- a/core/src/test/java/ai/authplane/sdk/core/CircuitPolicyTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/CircuitPolicyTest.java @@ -6,6 +6,8 @@ import org.junit.jupiter.api.Test; +import ai.authplane.sdk.core.errors.AccessDeniedException; +import ai.authplane.sdk.core.errors.InvalidTargetException; import ai.authplane.sdk.core.errors.TokenExchangeException; import ai.authplane.sdk.core.fetching.ssrf.SsrfException; @@ -23,6 +25,8 @@ void oauthNoCircuitErrors_doNotTrip() { "invalid_grant", "invalid_scope", "invalid_request", + "invalid_target", + "access_denied", "consent_required", "interaction_required", "invalid_dpop_proof", @@ -34,6 +38,16 @@ void oauthNoCircuitErrors_doNotTrip() { } } + @Test + void accessDeniedAndInvalidTarget_typedExceptions_doNotTrip() { + // authserver 0.2.0 answers these when the exchanging client is not allowlisted on the + // Resource (403) or `resource` does not match a granted resource (400): not an outage. + assertThat(CircuitPolicy.shouldTrip(new AccessDeniedException("not allowlisted"))) + .isFalse(); + assertThat(CircuitPolicy.shouldTrip(new InvalidTargetException("resource mismatch"))) + .isFalse(); + } + @Test void invalidClientAndUnauthorizedTrip() { assertThat(CircuitPolicy.shouldTrip(new TokenExchangeException("bad", "invalid_client"))) diff --git a/core/src/test/java/ai/authplane/sdk/core/ResourceOptionsTest.java b/core/src/test/java/ai/authplane/sdk/core/ResourceOptionsTest.java index a813592..1c91d8b 100644 --- a/core/src/test/java/ai/authplane/sdk/core/ResourceOptionsTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/ResourceOptionsTest.java @@ -93,6 +93,95 @@ void builder_customThenBuiltin_throwsIllegalState() { .hasMessageContaining("custom RevocationChecker"); } + // ----------------------------------------------------------------------- + // resourceMetadataUrl — the advertised PRM URL override + // ----------------------------------------------------------------------- + + @Test + void defaults_resourceMetadataUrlIsNull() { + // null is what makes AuthplaneResource.resourceMetadataUrl() fall back to the derived, + // resource-hosted URL — the default topology. + assertThat(ResourceOptions.defaults().resourceMetadataUrl()).isNull(); + } + + @Test + void builder_resourceMetadataUrl_customized() { + ResourceOptions opts = + ResourceOptions.builder() + .resourceMetadataUrl( + "https://auth.example.com/.well-known/oauth-protected-resource/mcp") + .build(); + assertThat(opts.resourceMetadataUrl()) + .isEqualTo("https://auth.example.com/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void builder_resourceMetadataUrl_relativeReference_throwsIllegalArgument() { + // The value is spliced into a header that reaches unauthenticated clients; a relative + // reference names no document they can fetch. + assertThatThrownBy( + () -> + ResourceOptions.builder() + .resourceMetadataUrl( + "/.well-known/oauth-protected-resource/mcp")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("it has no scheme"); + } + + @Test + void builder_resourceMetadataUrl_nonHttpScheme_throwsIllegalArgument() { + assertThatThrownBy(() -> ResourceOptions.builder().resourceMetadataUrl("urn:example:prm")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("not http or https"); + } + + @Test + void builder_resourceMetadataUrl_noHost_throwsIllegalArgument() { + assertThatThrownBy( + () -> + ResourceOptions.builder() + .resourceMetadataUrl( + "https:///.well-known/oauth-protected-resource")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("it names no host"); + } + + @Test + void builder_resourceMetadataUrl_userinfo_throwsIllegalArgumentAndElidesTheCredential() { + // URI.getHost() is "auth.example.com" for this shape, so the host gate alone lets it + // through — and the value is then advertised in every 401 and 403 to unauthenticated + // callers, which is exactly what the identifier's own userinfo gate exists to stop. + assertThatThrownBy( + () -> + ResourceOptions.builder() + .resourceMetadataUrl( + "https://svc:s3cr3t@auth.example.com/.well-known/oauth-protected-resource/mcp")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("resourceMetadataUrl") + .hasMessageContaining("userinfo") + .hasMessageNotContaining("s3cr3t"); + } + + @Test + void builder_resourceMetadataUrl_fragment_throwsIllegalArgument() { + // A fragment is never sent to the server, so it names a document the client fetches + // without it — the advertised URL and the one retrieved disagree. + assertThatThrownBy( + () -> + ResourceOptions.builder() + .resourceMetadataUrl( + "https://auth.example.com/.well-known/oauth-protected-resource/mcp#frag")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("resourceMetadataUrl") + .hasMessageContaining("fragment"); + } + + @Test + void builder_resourceMetadataUrl_null_throwsNpe() { + assertThatThrownBy(() -> ResourceOptions.builder().resourceMetadataUrl(null)) + .isInstanceOf(NullPointerException.class); + } + // ----------------------------------------------------------------------- // allowedAlgorithms is immutable // ----------------------------------------------------------------------- diff --git a/core/src/test/java/ai/authplane/sdk/core/RevocationTest.java b/core/src/test/java/ai/authplane/sdk/core/RevocationTest.java index dceaacb..608efb0 100644 --- a/core/src/test/java/ai/authplane/sdk/core/RevocationTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/RevocationTest.java @@ -7,9 +7,14 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import java.util.ArrayList; import java.util.List; import java.util.Map; import java.util.concurrent.ExecutionException; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.BeforeAll; @@ -96,6 +101,69 @@ private String localIssuerToken() { return TestFixtures.token().rsaKey(rsaKeys).issuer(baseUrl).build(); } + /** Stubs metadata (issuer=baseUrl) that also advertises an introspection endpoint. */ + private void stubMetadataWithIntrospection() { + String metadataBody = + TestFixtures.serializeMap( + Map.of( + "issuer", + baseUrl, + "jwks_uri", + baseUrl + "/jwks", + "introspection_endpoint", + baseUrl + "/introspect")); + wireMock.stubFor( + get(urlEqualTo("/.well-known/oauth-authorization-server")) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(metadataBody))); + } + + private void stubIntrospection(String body) { + wireMock.stubFor( + post(urlEqualTo("/introspect")) + .willReturn( + aResponse() + .withStatus(200) + .withHeader("Content-Type", "application/json") + .withBody(body))); + } + + /** Runs {@code body} while capturing WARNING records emitted by the built-in checker. */ + private static List captureCheckerWarnings(ThrowingRunnable body) throws Exception { + Logger logger = Logger.getLogger(IntrospectionChecker.class.getName()); + List records = new ArrayList<>(); + Handler handler = + new Handler() { + @Override + public void publish(LogRecord record) { + if (record.getLevel().intValue() >= Level.WARNING.intValue()) { + records.add(record); + } + } + + @Override + public void flush() {} + + @Override + public void close() {} + }; + logger.addHandler(handler); + try { + body.run(); + } finally { + logger.removeHandler(handler); + } + return records; + } + + @FunctionalInterface + private interface ThrowingRunnable { + void run() throws Exception; + } + // ----------------------------------------------------------------------- // Revocation disabled (null) // ----------------------------------------------------------------------- @@ -200,6 +268,50 @@ void verify_customChecker_throwsException_failClosed_rejectsToken() throws Excep .isInstanceOf(TokenRevokedException.class); } + /** + * The interrupt arm also restores the flag the catch consumed. That half is not asserted here: + * verify() runs on the common ForkJoinPool, whose worker clears the flag as the task completes, + * so it is no longer readable from this thread by the time get() returns. + */ + @Test + void verify_customChecker_interrupted_failOpenByDefault() throws Exception { + RevocationChecker interrupted = + (token, jti) -> { + throw new InterruptedException("executor shutting down"); + }; + AuthplaneClient client = buildClient(); + AuthplaneResource verifier = + buildVerifier( + client, ResourceOptions.builder().revocationChecker(interrupted).build()); + + VerifiedClaims claims = verifier.verify(localIssuerToken()).get().claims(); + + assertThat(claims.jti()).isEqualTo(TestFixtures.JTI); + } + + @Test + void verify_customChecker_interrupted_failClosed_rejectsAsInterruptedNotRevoked() + throws Exception { + RevocationChecker interrupted = + (token, jti) -> { + throw new InterruptedException("executor shutting down"); + }; + AuthplaneClient client = buildClient(); + AuthplaneResource verifier = + buildVerifier( + client, + ResourceOptions.builder() + .revocationChecker(interrupted) + .failClosed() + .build()); + + assertThatThrownBy(() -> verifier.verify(localIssuerToken()).get()) + .isInstanceOf(ExecutionException.class) + .cause() + .isInstanceOf(TokenRevokedException.class) + .hasMessageContaining("revocation check was interrupted"); + } + // ----------------------------------------------------------------------- // Built-in introspection (default) // ----------------------------------------------------------------------- @@ -291,6 +403,105 @@ void verify_builtinIntrospection_activeFalse_throwsTokenRevoked() throws Excepti .isInstanceOf(TokenRevokedException.class); } + @Test + void builtinIntrospection_withoutAuthProvider_warnsAtConstruction() throws Exception { + // authserver >= 0.1.2 answers active=false to unauthenticated introspection, so a checker + // wired without credentials rejects every token; say so when it is built, not per token. + stubMetadataWithIntrospection(); + AuthplaneClient client = AuthplaneClient.builder(baseUrl).devMode(true).build().get(); + + List warnings = + captureCheckerWarnings( + () -> + buildVerifier( + client, + ResourceOptions.builder() + .useBuiltinRevocationChecker() + .build())); + + assertThat(warnings).hasSize(1); + assertThat(warnings.get(0).getMessage()) + .contains("without an AuthProvider") + .contains("active=false") + .contains("runtime-client"); + } + + @Test + void builtinIntrospection_withAuthProvider_noConstructionWarning() throws Exception { + stubMetadataWithIntrospection(); + AuthplaneClient client = + AuthplaneClient.builder(baseUrl) + .devMode(true) + .authProvider(new ASCredentials("my-rs", "s3cret")) + .build() + .get(); + + List warnings = + captureCheckerWarnings( + () -> + buildVerifier( + client, + ResourceOptions.builder() + .useBuiltinRevocationChecker() + .build())); + + assertThat(warnings).isEmpty(); + } + + @Test + void builtinIntrospection_activeFalseAfterLocalVerify_logsOwnershipWarningOnce() + throws Exception { + stubMetadataWithIntrospection(); + stubIntrospection("{\"active\":false}"); + AuthplaneClient client = + AuthplaneClient.builder(baseUrl) + .devMode(true) + .authProvider(new ASCredentials("my-rs", "s3cret")) + .build() + .get(); + AuthplaneResource verifier = + buildVerifier( + client, ResourceOptions.builder().useBuiltinRevocationChecker().build()); + + List warnings = + captureCheckerWarnings( + () -> { + for (int i = 0; i < 2; i++) { + assertThatThrownBy(() -> verifier.verify(localIssuerToken()).get()) + .isInstanceOf(ExecutionException.class) + .cause() + .isInstanceOf(TokenRevokedException.class); + } + }); + + // Two rejected tokens, one warning: the guidance is logged once per checker. + assertThat(warnings).hasSize(1); + assertThat(warnings.get(0).getMessage()) + .contains("jti='" + TestFixtures.JTI + "'") + .contains("passed local JWT verification") + .contains("runtime-client add --client-id --slug "); + } + + @Test + void builtinIntrospection_activeTrue_noOwnershipWarning() throws Exception { + stubMetadataWithIntrospection(); + stubIntrospection("{\"active\":true}"); + AuthplaneClient client = + AuthplaneClient.builder(baseUrl) + .devMode(true) + .authProvider(new ASCredentials("my-rs", "s3cret")) + .build() + .get(); + AuthplaneResource verifier = + buildVerifier( + client, ResourceOptions.builder().useBuiltinRevocationChecker().build()); + + List warnings = + captureCheckerWarnings(() -> verifier.verify(localIssuerToken()).get()); + + assertThat(warnings).isEmpty(); + } + @Test void verify_defaultRevocation_noRevocationCheck() throws Exception { // Default (no revocation options) = no revocation checking. diff --git a/core/src/test/java/ai/authplane/sdk/core/VerifiedClaimsTest.java b/core/src/test/java/ai/authplane/sdk/core/VerifiedClaimsTest.java index 2cf01eb..20bb37f 100644 --- a/core/src/test/java/ai/authplane/sdk/core/VerifiedClaimsTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/VerifiedClaimsTest.java @@ -116,17 +116,20 @@ void act_nonMap_returnsNull() { } @Test + @SuppressWarnings("removal") void mayAct_present_returnedAsImmutableCopy() { VerifiedClaims c = claims(Map.of("may_act", Map.of("sub", "actor"))); assertThat(c.mayAct()).containsEntry("sub", "actor"); } @Test + @SuppressWarnings("removal") void mayAct_absent_returnsNull() { assertThat(claims(Map.of()).mayAct()).isNull(); } @Test + @SuppressWarnings("removal") void mayAct_nonMap_returnsNull() { assertThat(claims(Map.of("may_act", List.of("not-a-map"))).mayAct()).isNull(); } diff --git a/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java b/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java index 859aea6..c39fb6e 100644 --- a/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/errors/ErrorsTest.java @@ -9,6 +9,7 @@ import ai.authplane.sdk.core.dpop.DPoPNotSupportedException; import ai.authplane.sdk.core.dpop.DPoPProofMissingException; import ai.authplane.sdk.core.dpop.MultipleDpopProofsException; +import ai.authplane.sdk.core.errors.WwwAuthenticate.ChallengeOptions; /** * Tests for the exception hierarchy. @@ -201,15 +202,18 @@ void tokenRevokedException_messageConstructor() { @Test void wwwAuthenticate_escapesDoubleQuotesInErrorDescription() { + // The fixed description carries no quote to escape, so the message path is + // exercised through the verbose overload — the one way a caller-influenced + // string can still reach error_description. var ex = new InvalidClaimsException("bad \"kid\" value"); - String header = WwwAuthenticate.of(ex); + String header = WwwAuthenticate.of(ex, ChallengeOptions.empty(), true); assertThat(header).contains("error_description=\"bad \\\"kid\\\" value\""); } @Test void wwwAuthenticate_escapesBackslashesInErrorDescription() { var ex = new InvalidClaimsException("path\\to\\file"); - String header = WwwAuthenticate.of(ex); + String header = WwwAuthenticate.of(ex, ChallengeOptions.empty(), true); assertThat(header).contains("error_description=\"path\\\\to\\\\file\""); } @@ -236,7 +240,7 @@ void wwwAuthenticate_stripsCrLfFromErrorDescription() { // CR/LF cannot appear inside a quoted-string (RFC 9110 §5.6.4); leaving them in would // let an attacker inject a follow-on header line. var ex = new InvalidClaimsException("line1\r\nSet-Cookie: pwned=1"); - String header = WwwAuthenticate.of(ex); + String header = WwwAuthenticate.of(ex, ChallengeOptions.empty(), true); assertThat(header).doesNotContain("\r"); assertThat(header).doesNotContain("\n"); assertThat(header).contains("error_description=\"line1Set-Cookie: pwned=1\""); @@ -457,4 +461,16 @@ void httpStatus_unknownAuthplaneException_returns500() { AuthplaneException custom = new AuthplaneException(MSG) {}; assertThat(HttpStatus.of(custom)).isEqualTo(500); } + + @Test + void descriptionFor_fallsBackForACodeWithNoRow() { + assertThat(WwwAuthenticate.descriptionFor("invalid_token")) + .isEqualTo("The access token is missing or not valid for this resource"); + assertThat(WwwAuthenticate.descriptionFor("insufficient_scope")) + .isEqualTo("The access token does not carry the scope this operation requires"); + assertThat(WwwAuthenticate.descriptionFor("invalid_dpop_proof")) + .isEqualTo("The DPoP proof is missing or not valid for this request"); + assertThat(WwwAuthenticate.descriptionFor("something_new")) + .isEqualTo(WwwAuthenticate.FALLBACK_ERROR_DESCRIPTION); + } } diff --git a/core/src/test/java/ai/authplane/sdk/core/errors/FailureResponseTest.java b/core/src/test/java/ai/authplane/sdk/core/errors/FailureResponseTest.java index 2706417..8f24fb8 100644 --- a/core/src/test/java/ai/authplane/sdk/core/errors/FailureResponseTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/errors/FailureResponseTest.java @@ -24,7 +24,12 @@ void invalidToken_is401BearerInvalidToken() { assertThat(c.wwwAuthenticate()).contains("error=\"invalid_token\""); assertThat(c.wwwAuthenticate()).contains("resource_metadata=\"https://r/.well-known/x\""); assertThat(c.jsonBody()).contains("\"error\":\"invalid_token\""); - assertThat(c.jsonBody()).contains("\"error_description\":\"expired\""); + // The body carries the same fixed sentence the challenge does; the message + // ("expired") reaches neither half. + assertThat(c.jsonBody()) + .contains( + "\"error_description\":\"The access token is missing or not valid for this resource\""); + assertThat(c.jsonBody()).doesNotContain("expired"); } @Test @@ -51,4 +56,70 @@ void dpopProofError_usesDpopSchemeAndCode() { assertThat(c.wwwAuthenticate()).contains("error=\"invalid_dpop_proof\""); assertThat(c.jsonBody()).contains("\"error\":\"invalid_dpop_proof\""); } + + @Test + void body_neverCarriesTheExceptionMessage() { + // The body reaches a caller who by definition has not authenticated, and + // the SDK's messages name the failing detail — here the exact audience the + // resource expects, which is the value a caller needs in order to go + // request a token for it. + FailureResponse.Challenge c = + FailureResponse.of( + new InvalidClaimsException( + "aud mismatch: expected https://api.example.com/mcp"), + ChallengeOptions.empty()); + + assertThat(c.jsonBody()) + .contains( + "\"error_description\":\"The access token is missing or not valid for this resource\""); + assertThat(c.jsonBody()).doesNotContain("api.example.com"); + assertThat(c.wwwAuthenticate()).doesNotContain("api.example.com"); + } + + @Test + void body_andChallenge_carryTheSameCodeAndDescription() { + // One table, two surfaces: a client reads whichever half it finds, so they + // must not drift. + FailureResponse.Challenge c = + FailureResponse.of( + new InsufficientScopeException("admin", List.of("read")), + ChallengeOptions.empty()); + + assertThat(c.wwwAuthenticate()).contains("error=\"insufficient_scope\""); + assertThat(c.jsonBody()).contains("\"error\":\"insufficient_scope\""); + assertThat(c.wwwAuthenticate()) + .contains( + "error_description=\"The access token does not carry the scope this operation requires\""); + assertThat(c.jsonBody()) + .contains( + "\"error_description\":\"The access token does not carry the scope this operation requires\""); + } + + @Test + void verboseDescription_withANullMessage_keepsBothHalvesOnTheFixedSentence() { + // The verbose escape hatch reads error.getMessage(), which is nullable — a + // TokenExchangeException wrapping a cause that has none reaches here. The body guarded it + // and the challenge did not, so the header emitted error_description="" beside a body + // carrying the fixed sentence: the one case the invariant above does not reach. + FailureResponse.Challenge c = + FailureResponse.of( + new TokenExchangeException(null, null), ChallengeOptions.empty(), true); + + String expected = WwwAuthenticate.descriptionFor("invalid_token"); + assertThat(c.wwwAuthenticate()).contains("error_description=\"" + expected + "\""); + assertThat(c.jsonBody()).contains("\"error_description\":\"" + expected + "\""); + } + + @Test + void verboseDescription_restoresTheMessageOnBothHalves() { + // The escape hatch opens both surfaces together: one that opened only the + // header would be a way to believe the message was suppressed while the + // body still shipped it. + FailureResponse.Challenge c = + FailureResponse.of( + new TokenExpiredException("expired"), ChallengeOptions.empty(), true); + + assertThat(c.wwwAuthenticate()).contains("error_description=\"expired\""); + assertThat(c.jsonBody()).contains("\"error_description\":\"expired\""); + } } 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 1c8a30e..fc59851 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 @@ -14,6 +14,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiConsumer; import org.junit.jupiter.api.Test; @@ -105,7 +106,7 @@ void onChangeCallback_calledWhenDocumentChanges() throws Exception { } @Test - void forceRefresh_alwaysFetches() throws Exception { + void forceRefresh_fetchesWhenNotBackingOff() throws Exception { AtomicInteger fetchCount = new AtomicInteger(); cache = cacheWith(countingFetcher(DOC_V1, fetchCount), 300); cache.fetch(); @@ -168,11 +169,12 @@ void constructor_rejectsANullClock() { /** * A server expiry that is not in the future is no expiry at all. * - *

`Cache-Control: no-store` and `no-cache` parse to `0L`, `max-age=0` to `now`, and a stale - * `Expires:` to a past epoch. Subtracting the cache timestamp from any of those gives a - * negative TTL, which makes the document permanently expired: every read takes the synchronous - * re-fetch branch, on the caller's thread. The failure backoff cannot help, because a - * `no-store` endpoint that *answers* clears it and re-arms the expiry on the same call. + *

`max-age=0` parses to `now` and a stale `Expires:` to a past epoch. Taking either as an + * expiry gives a non-positive TTL, which makes the document permanently expired: every read + * takes the synchronous re-fetch branch, on the caller's thread. The failure backoff cannot + * help, because an endpoint that *answers* clears it and re-arms the expiry on the same call. + * `no-store` and `no-cache` used to arrive here as `0L`; they now parse to `null` and no longer + * reach the clamp, which still covers them. * *

This matters now that verification reads through the metadata cache on every key lookup — * and does so before signature verification, so an unauthenticated caller would set the fetch @@ -180,7 +182,8 @@ void constructor_rejectsANullClock() { */ @Test void get_serverExpiryNotInTheFuture_fallsBackToTheConfiguredInterval() throws Exception { - // 0L is what no-store and no-cache parse to; -1 stands for a stale Expires: header. + // 0L is what no-store and no-cache used to parse to; -1 stands for a stale Expires: + // header. Both stay as defensive fixtures: the clamp must hold for any non-future value. for (long serverExpiry : new long[] {0L, -1L}) { AtomicInteger fetchCount = new AtomicInteger(); TestClock clock = new TestClock(); @@ -296,6 +299,118 @@ void advanceSeconds(long seconds) { } } + @Test + void forceRefresh_respectsTheFailureBackoff_theIgnoringVariantDoesNot() throws Exception { + // The pair these two methods' semantics turn on. forceRefresh() is reachable from a + // request path, so a failing endpoint must not cost a fetch per call; + // forceRefreshIgnoringFailureBackoff() is for the caller that wants the attempt made and + // is prepared to wait for the timeout. + AtomicInteger fetchCount = new AtomicInteger(); + TestClock clock = new TestClock(); + DocumentFetcher fetcher = + url -> { + int n = fetchCount.incrementAndGet(); + if (n == 2) { + return CompletableFuture.failedFuture( + new RuntimeException("jwks endpoint down")); + } + return CompletableFuture.completedFuture( + new FetchResult(n == 1 ? DOC_V1 : DOC_V2, null)); + }; + cache = cacheWith(fetcher, 100, clock); + cache.fetch(); // fetch 1 — DOC_V1 + + clock.advanceSeconds(101); // expired, so get() refreshes synchronously and that fetch fails + assertThat(cache.get()).as("stale is served when the refresh fails").isEqualTo(DOC_V1); + assertThat(fetchCount.get()).as("the failed refresh armed the backoff").isEqualTo(2); + + assertThat(cache.forceRefresh()).isEqualTo(DOC_V1); + assertThat(fetchCount.get()) + .as("forceRefresh() does not fetch while the backoff is armed") + .isEqualTo(2); + + assertThat(cache.forceRefreshIgnoringFailureBackoff()).isEqualTo(DOC_V2); + assertThat(fetchCount.get()).as("the ignoring variant makes the attempt").isEqualTo(3); + } + + // Bounded for the same reason as the get() test below: a regression here blocks, and an + // unbounded hang surfaces as a build timeout instead of a named failure. + @Test + @Timeout(10) + void forceRefresh_whileARefreshIsInFlight_waitsForTheDocumentThatLands() throws Exception { + // forceRefresh() deliberately blocks where get() returns early, and the asymmetry is the + // point. get()'s caller wants *a* document, so the published copy answers it. This + // caller wants a *newer* one: JwksCache.getKeyByKid(kid, true) reaches forceRefresh() + // precisely because the held document does not carry the kid. Serving that same document + // back guaranteed the lookup found nothing, so every concurrent kid-miss caller saw a + // spurious invalid_token for the length of every healthy rotation's fetch window. + CountDownLatch fetchStarted = new CountDownLatch(1); + CountDownLatch releaseFetch = new CountDownLatch(1); + AtomicInteger fetchCount = new AtomicInteger(); + TestClock clock = new TestClock(); + DocumentFetcher fetcher = + url -> + CompletableFuture.supplyAsync( + () -> { + if (fetchCount.incrementAndGet() > 1) { + fetchStarted.countDown(); + try { + releaseFetch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new CompletionException(e); + } + return new FetchResult(DOC_V2, null); + } + return new FetchResult(DOC_V1, null); + }); + cache = cacheWith(fetcher, 100, clock); + cache.fetch(); + + clock.advanceSeconds(101); // expired, so the refresher below takes the synchronous branch + + Thread refresher = + new Thread( + () -> { + try { + cache.get(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + }); + refresher.start(); + assertThat(fetchStarted.await(5, TimeUnit.SECONDS)) + .as("the refresh reached the fetcher and is holding fetchLock") + .isTrue(); + + AtomicReference> forced = new AtomicReference<>(); + Thread forcer = + new Thread( + () -> { + try { + forced.set(cache.forceRefresh()); + } catch (Exception e) { + throw new IllegalStateException(e); + } + }); + forcer.start(); + + // releaseFetch has not been counted down, so a forceRefresh that served the in-flight + // copy would already have returned. Still running means it is waiting on fetchLock. + forcer.join(500); + assertThat(forcer.isAlive()).as("forceRefresh waits for the fetch in flight").isTrue(); + + releaseFetch.countDown(); + forcer.join(5_000); + refresher.join(5_000); + assertThat(forcer.isAlive()).isFalse(); + + assertThat(forced.get()) + .as("the waiter is served the document that landed, not the one it already held") + .isEqualTo(DOC_V2); + assertThat(fetchCount.get()).as("waiting did not cost a second fetch").isEqualTo(2); + } + // A regression here blocks rather than returning the wrong value, and in CI a hang is not // the same as a failure: it burns the job's wall clock and surfaces as a build timeout // instead of a named test. The bound turns it back into a failure that says what broke. diff --git a/core/src/test/java/ai/authplane/sdk/core/oauth/TokenExchangeTest.java b/core/src/test/java/ai/authplane/sdk/core/oauth/TokenExchangeTest.java index 9b66346..7fddbcd 100644 --- a/core/src/test/java/ai/authplane/sdk/core/oauth/TokenExchangeTest.java +++ b/core/src/test/java/ai/authplane/sdk/core/oauth/TokenExchangeTest.java @@ -25,7 +25,9 @@ import ai.authplane.sdk.core.ASCredentials; import ai.authplane.sdk.core.TokenExchangeOptions; import ai.authplane.sdk.core.TokenResponse; +import ai.authplane.sdk.core.errors.AccessDeniedException; import ai.authplane.sdk.core.errors.ConsentRequiredException; +import ai.authplane.sdk.core.errors.InvalidTargetException; import ai.authplane.sdk.core.errors.TokenExchangeException; import ai.authplane.sdk.core.fetching.FetchSettings; import ai.authplane.sdk.core.fetching.HttpTransport; @@ -216,6 +218,54 @@ void exchange_interactionRequired_mapsToConsentRequiredException() throws Except }); } + @Test + void exchange_accessDenied_mapsToAccessDeniedException() throws Exception { + // authserver 0.2.0: cross-client exchange by a client that is not in the Resource's + // policy.exchange.allowed_client_ids. + wireMock.stubFor( + post(urlEqualTo("/token")) + .willReturn( + aResponse() + .withStatus(403) + .withHeader("Content-Type", "application/json") + .withBody( + "{\"error\":\"access_denied\"," + + "\"error_description\":\"client not allowed to exchange for this resource\"}"))); + + assertThatThrownBy( + () -> TokenExchange.exchange(tokenUrl, defaultOptions(), null, transport)) + .isInstanceOf(AccessDeniedException.class) + .satisfies( + ex -> { + AccessDeniedException ade = (AccessDeniedException) ex; + assertThat(ade.oauthError()).isEqualTo("access_denied"); + assertThat(ade.getMessage()) + .isEqualTo("client not allowed to exchange for this resource"); + }); + } + + @Test + void exchange_invalidTarget_mapsToInvalidTargetException() throws Exception { + // RFC 8707 §2.2: `resource` does not match a granted resource byte for byte. + wireMock.stubFor( + post(urlEqualTo("/token")) + .willReturn( + aResponse() + .withStatus(400) + .withHeader("Content-Type", "application/json") + .withBody("{\"error\":\"invalid_target\"}"))); + + assertThatThrownBy( + () -> TokenExchange.exchange(tokenUrl, defaultOptions(), null, transport)) + .isInstanceOf(InvalidTargetException.class) + .satisfies( + ex -> { + InvalidTargetException ite = (InvalidTargetException) ex; + assertThat(ite.oauthError()).isEqualTo("invalid_target"); + assertThat(ite.getMessage()).isEqualTo("invalid_target"); + }); + } + // ----------------------------------------------------------------------- // Missing access_token // ----------------------------------------------------------------------- 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 b06363b..f569bf3 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 @@ -281,24 +281,32 @@ void requireNoFragment_acceptsAFragmentFreeIdentifier() { } @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"); + void urnStyleResource_isRejectedAtConstruction() { + // RFC 8707 §2 permits any absolute URI as the resource *parameter* of a token request, and + // an opaque identifier used to construct here on that reading. It does not survive: the + // identifier is also the origin of the DPoP htu binding target, which reads a null + // authority as the literal text "null", so "urn:example:api" bound every request to + // "urn://null" — accepted at construction and unusable at the sink. The gate now closes at + // construction, where RFC 9728 §3 applies: the metadata URL is formed by inserting the + // well-known string after the host component, so there has to be a host. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("urn:example:api") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("does not name a host") + .hasMessageContaining("no authority component"); } @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. + // Construction rejects an opaque identifier (above), so this pins what the derivation + // helpers still answer on their own: they are public and reachable with a string no + // constructor saw. 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( @@ -355,8 +363,9 @@ void requireScheme_rejectsSchemelessIdentifiers() { @Test void requireScheme_acceptsAnyAbsoluteUri() { - // Scheme only — not scheme+host. An opaque absolute URI constructs (stored verbatim); - // whether it can derive a PRM URL is the derivation gate's question. + // This gate answers one question and only that one: is there a scheme. An opaque absolute + // URI clears it and is then rejected by requireAuthority, which runs immediately after at + // every construction site — so each message names the half that is actually missing. ProtectedResourceMetadata.requireScheme("https://api.example.com/mcp"); ProtectedResourceMetadata.requireScheme("urn:example:api"); ProtectedResourceMetadata.requireScheme("custom+v1.2-x://host/path"); @@ -410,6 +419,92 @@ void builder_reportsTheFragmentBeforeTheMissingScheme() { .satisfies(e -> assertThat(e.getMessage()).doesNotContain("secret")); } + @Test + void requireAuthority_rejectsIdentifiersThatNameNoHost() { + // The construction gate for the other half of an absolute hierarchical identifier. Every + // shape here reaches AuthplaneResource.normalizeRequestUrl with a null or hostless + // authority, which splices the literal text "null" (or a bare port) into the DPoP htu + // binding target — measured: "urn:example:api" produced the origin "urn://null". + for (String identifier : + new String[] { + "urn:example:api", // opaque: no authority at all + "mailto:ops@example.com", // opaque, and the '@' is not userinfo + "https:example.com/mcp", // hierarchical-looking, but no "//" opens an authority + "https:///mcp", // empty authority + "https://:8443/mcp", // a port and no host + "https://@:8443/mcp", // an '@' and a port, but nothing between them + }) { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireAuthority(identifier)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("does not name a host"); + } + } + + @Test + void requireAuthority_acceptsIdentifiersThatNameAHost() { + // The gate must not simply reject everything that is not a bare https host: a port, an + // IPv6 literal (whose own colons sit inside the brackets, so the port separator is looked + // for after the ']'), a percent-escape in the registered name and a query all name a host. + ProtectedResourceMetadata.requireAuthority("https://api.example.com/mcp"); + ProtectedResourceMetadata.requireAuthority("https://api.example.com:8443/mcp"); + ProtectedResourceMetadata.requireAuthority("http://localhost:8080/mcp"); + ProtectedResourceMetadata.requireAuthority("https://[::1]:8443/mcp"); + ProtectedResourceMetadata.requireAuthority("https://[::1]/mcp"); + ProtectedResourceMetadata.requireAuthority("https://a%2Db.example.com/mcp"); + ProtectedResourceMetadata.requireAuthority("https://api.example.com/mcp?tenant=acme"); + ProtectedResourceMetadata.requireAuthority("https://api.example.com"); + + // Scheme-relative: it does name a host, so this gate is not the one that rejects it. + // requireScheme, which runs immediately before at every call site, is. + ProtectedResourceMetadata.requireAuthority("//api.example.com/mcp"); + } + + @Test + void requireAuthority_errorMessage_elidesUserinfo() { + // The message renders the identifier, and a hostless authority can still carry a + // credential ("https://svc:pw@/mcp" is what a template with an empty host variable + // produces). Same discipline as every sibling gate: the userinfo never reaches a log. + assertThatThrownBy( + () -> ProtectedResourceMetadata.requireAuthority("https://svc:s3cr3t@/mcp")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("does not name a host") + .hasMessageContaining("***@/mcp") + .satisfies(e -> assertThat(e.getMessage()).doesNotContain("s3cr3t")); + } + + @Test + void requireAuthority_null_throwsNamedNpe() { + assertThatThrownBy(() -> ProtectedResourceMetadata.requireAuthority(null)) + .isInstanceOf(NullPointerException.class) + .hasMessageContaining("resourceUri"); + } + + @Test + void builder_rejectsAnIdentifierWithAnEmptyAuthority() { + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("https:///mcp") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("does not name a host"); + } + + @Test + void builder_reportsTheMissingSchemeBeforeTheMissingHost() { + // Gate order: a scheme-relative reference names a host, so it is reported for the scheme; + // an identifier missing both is reported for the scheme, which an operator fixes first. + assertThatThrownBy( + () -> + ProtectedResourceMetadata.builder() + .resource("mcp") + .authorizationServer("https://auth.example.com") + .build()) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("has no scheme"); + } + @Test void requireNoUserinfo_rejectsUserinfoBearingIdentifiers() { // RFC 9110 §4.2.4: userinfo is deprecated and a recipient is to reject a URI carrying diff --git a/mcp/docs/user-guide.md b/mcp/docs/user-guide.md index d5ba162..5def7e3 100644 --- a/mcp/docs/user-guide.md +++ b/mcp/docs/user-guide.md @@ -93,6 +93,7 @@ Every builder method on `AuthplaneMcpSetup.Builder`: | `tokenCacheConfig(TokenCacheConfig)` | `TokenCacheConfig.defaults()` | Token cache tuning: TTL buffer (default `30`s), fallback TTL (default `3600`s), max entries (default `10000`, LRU) | | `outboundDPoP(OutboundDPoPOptions)` | `null` | Enables outbound DPoP proofs on AS calls | | `inboundDPoP(InboundDPoPOptions)` | `null` | Enables inbound DPoP proof validation | +| `resourceMetadataUrl(String)` | derived | Advertises this URL as the PRM document's location instead of the derived, resource-hosted one (see §7) | Both refresh intervals are driven by traffic rather than by a background timer: the first token verification past the interval pays for the refetch. That is what keeps an MCP server — which never calls the token, introspection or revocation endpoints — following a rotated `jwks_uri`: the metadata read that discovers the new URI rebinds JWKS fetching before the token is verified. A metadata endpoint that is unreachable does not fail verification; the last known good document keeps being served. @@ -161,6 +162,33 @@ setup.prmPath(); // e.g. "/.well-known/oauth-protected-resource/mcp" The response includes the authorization server issuer, supported scopes, supported bearer methods (`header`), and the resource identifier. +### Where the PRM document lives + +Two topologies, one option: + +| Topology | Who serves the document | What points at it | +|---|---|---| +| Resource-hosted (default) | This server, via the `PrmServlet` registered at `prmPath()` | The derived `/.well-known/oauth-protected-resource[/path]` | +| AS-hosted | The authorization server, which publishes one document per registered resource (authserver 0.2.0 and later serves `/.well-known/oauth-protected-resource/{ref}`, `ref` being the RFC 9728 §3.1 path suffix of the resource URI, or its slug) | `resourceMetadataUrl(...)` on the builder | + +```java +AuthplaneMcpSetup setup = AuthplaneMcpSetup.builder() + .issuer("https://auth.example.com") + .resource("https://mcp.example.com/mcp") + .scopes(List.of("tools/query")) + .resourceMetadataUrl("https://auth.example.com/.well-known/oauth-protected-resource/mcp") + .build() + .get(); + +setup.resource().resourceMetadataUrl(); // the AS-hosted URL — what challenges advertise +``` + +Reach for the AS-hosted topology when this server cannot serve well-known paths — a platform that owns them, or a gateway that routes only the MCP path. `prmPath()` and `prmServlet()` are unaffected; skip `registerServlets(...)`'s PRM mapping (or wire the transport servlet yourself) if you do not want to serve a second copy. + +Either way RFC 9728 §3.3 binds the document to the identifier: the `resource` member the client reads must equal, byte for byte, the identifier it derived the metadata request from. The resource registered at the authorization server, the `resource(...)` configured here and the URL clients actually call therefore have to be the same string. + +**Transport-tier limitation.** The MCP Java SDK renders this adapter's transport-tier 401/403 through `ServerTransportSecurityException(int statusCode, String message)`, which carries a status and a message and no headers — the SDK passes them to `HttpServletResponse.sendError(...)`. Those responses therefore carry no `WWW-Authenticate` header at all, and so no `resource_metadata` parameter, on either topology (this predates the option and is not changed by it). Clients discover the document from the well-known path, a front proxy adds the header, or the Spring Security path in `authplane-spring` emits the full RFC 6750 / RFC 9728 challenge — including the configured URL. + ## 8. Token revocation checking By default, tokens are validated offline (signature + claims only). Two opt-in modes are available: @@ -180,6 +208,14 @@ AuthplaneMcpSetup setup = AuthplaneMcpSetup.builder() The introspection endpoint is discovered from AS metadata. If the endpoint returns `active=false`, the token is rejected. **Fails open** on transport errors. +The `ASCredentials` client must be confidential (a public client cannot introspect at all) and must be either the client the token was issued to or a runtime-client of the Resource named in `aud`; authserver ≥ 0.1.2 answers `{"active": false}` to everyone else, which rejects every token as revoked. Register the MCP server on its Resource with: + +```bash +authserver admin resource runtime-client add --client-id --slug +``` + +The checker warns at construction when no `authProvider` is set, and once when `active=false` comes back for a token that already passed local JWT verification. + ### Custom checker ```java @@ -238,6 +274,26 @@ Feature A (token exchange, in the core SDK) surfaces consent as a typed `Consent Always wrap any tool handler that triggers token exchange — otherwise the `ConsentRequiredException` bubbles as a generic error and the client never learns about the consent URL. +### Other exchange errors + +Two sibling subtypes of `TokenExchangeException` are not consent problems, so the wrapper does not turn them into elicitation and re-prompting the user will not help: + +- `AccessDeniedException` (`access_denied`, HTTP 403): on a cross-client exchange, the operator has not allowlisted this MCP server's client on the target Resource. Fix the Resource policy (below). +- `InvalidTargetException` (`invalid_target`, HTTP 400, RFC 8707 §2.2): the `resource` string does not match a granted resource exactly — a trailing slash is enough. Send the identifier byte for byte as granted. + +Neither counts toward the circuit breaker. + +### Operator step: allowlist the exchanging client + +For each MCP server that exchanges for a downstream resource it does not itself act as, allowlist its client ID on that Resource: + +```http +PATCH /admin/resources/{id} +{"policy": {"exchange": {"allowed_client_ids": [""]}}} +``` + +A client exchanging a token issued to itself, fronted exchanges, and Broker resources need nothing. + ### Using the helper directly If you already have a `Throwable` in hand and want to convert it manually: 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 0a109e0..9feac57 100644 --- a/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapter.java +++ b/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapter.java @@ -18,6 +18,7 @@ import ai.authplane.sdk.core.dpop.VerificationRequestContext; import ai.authplane.sdk.core.errors.AuthplaneException; import ai.authplane.sdk.core.errors.InsufficientScopeException; +import ai.authplane.sdk.core.errors.WwwAuthenticate; import ai.authplane.sdk.core.http.HttpHeaders; import io.modelcontextprotocol.common.McpTransportContext; import io.modelcontextprotocol.server.McpTransportContextExtractor; @@ -154,7 +155,7 @@ public void validateHeaders(Map> headers) try { VerificationRequestContext.assertSingleDpopHeader(HttpHeaders.values(headers, "dpop")); } catch (MultipleDpopProofsException e) { - throw new ServerTransportSecurityException(401, e.getMessage()); + throw new ServerTransportSecurityException(401, safeDescription(e)); } try { @@ -276,12 +277,26 @@ private static Throwable unwrapCompletion(Throwable t) { * the correct HTTP status. */ private static ServerTransportSecurityException mapToSecurityException(Throwable t) { - if (t instanceof InsufficientScopeException) { - return new ServerTransportSecurityException(403, t.getMessage()); + if (t instanceof InsufficientScopeException ise) { + return new ServerTransportSecurityException(403, safeDescription(ise)); } - if (t instanceof AuthplaneException) { - return new ServerTransportSecurityException(401, t.getMessage()); + if (t instanceof AuthplaneException ae) { + return new ServerTransportSecurityException(401, safeDescription(ae)); } return new ServerTransportSecurityException(401, "Token verification failed"); } + + /** + * The fixed, caller-safe sentence for an exception, rather than its own message. + * + *

The MCP SDK's transport renders this message into the response it sends the caller, so + * whatever is put here reaches someone who by definition has not authenticated — the same seam + * {@code WwwAuthenticate} closes for the challenge this adapter does not build. The SDK's + * messages name the unknown {@code kid}, the claim that did not validate, and on an audience + * mismatch the exact {@code aud} the resource expects. The original exception is still thrown + * with its own message available to the server's logs. + */ + private static String safeDescription(AuthplaneException error) { + return WwwAuthenticate.descriptionFor(WwwAuthenticate.errorCodeFor(error)); + } } diff --git a/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpSetup.java b/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpSetup.java index 6dc159a..1061e2e 100644 --- a/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpSetup.java +++ b/mcp/src/main/java/ai/authplane/sdk/mcp/AuthplaneMcpSetup.java @@ -191,6 +191,7 @@ public static final class Builder { private boolean useBuiltinRevocationChecker = false; private RevocationChecker revocationChecker; private InboundDPoPOptions inboundDPoP; + private String resourceMetadataUrl; private Builder() {} @@ -301,6 +302,19 @@ public Builder inboundDPoP(InboundDPoPOptions options) { return this; } + /** + * Advertises {@code url} as the RFC 9728 document's location instead of the resource-hosted + * one derived from {@link #resource(String)}, for a deployment where the authorization + * server publishes the document (authserver serves one per registered resource) and this + * server cannot serve the well-known path itself. Reaches every challenge built from {@link + * AuthplaneResource#resourceMetadataUrl()}; see the user guide for the transport-tier + * limitation on this adapter's own 401/403 responses. + */ + public Builder resourceMetadataUrl(String url) { + this.resourceMetadataUrl = Objects.requireNonNull(url, "url must not be null"); + return this; + } + /** * Validates configuration, creates the client, verifier, adapter, and PRM servlet. * @@ -344,6 +358,8 @@ public CompletableFuture build() { if (useBuiltinRevocationChecker) optBuilder.useBuiltinRevocationChecker(); if (inboundDPoP != null) optBuilder.inboundDPoP(inboundDPoP); + if (resourceMetadataUrl != null) + optBuilder.resourceMetadataUrl(resourceMetadataUrl); AuthplaneResource authplaneResource = client.resource(this.resource, scopes, optBuilder.build()); diff --git a/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapterTest.java b/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapterTest.java index 5ae710a..ca0860f 100644 --- a/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapterTest.java +++ b/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpAdapterTest.java @@ -389,7 +389,13 @@ void validateHeaders_multipleDpopHeaders_throws401() { ServerTransportSecurityException se = (ServerTransportSecurityException) e; assertThat(se.getStatusCode()).isEqualTo(401); - assertThat(se.getMessage()).contains("Multiple DPoP"); + // The transport renders this message to the caller, so it is the + // fixed per-code sentence, not core's "Multiple DPoP …". + assertThat(se.getMessage()) + .isEqualTo( + "The DPoP proof is missing or not valid for this" + + " request"); + assertThat(se.getMessage()).doesNotContain("Multiple DPoP"); }); } diff --git a/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpSetupTest.java b/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpSetupTest.java index 10f4563..40a0985 100644 --- a/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpSetupTest.java +++ b/mcp/src/test/java/ai/authplane/sdk/mcp/AuthplaneMcpSetupTest.java @@ -4,6 +4,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.get; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.mock; @@ -164,6 +165,62 @@ void builder_optionalMethods_returnSelf() { assertThat(b.jwksRefreshSeconds(600)).isSameAs(b); assertThat(b.metadataRefreshSeconds(7200)).isSameAs(b); assertThat(b.revocationChecker(RevocationChecker.noOp())).isSameAs(b); + assertThat(b.resourceMetadataUrl("http://localhost:9000/prm")).isSameAs(b); + } + + // ----------------------------------------------------------------------- + // Advertised PRM URL (AS-hosted topology) + // ----------------------------------------------------------------------- + + @Test + void build_noResourceMetadataUrl_advertisesTheDerivedUrl() throws Exception { + AuthplaneMcpSetup setup = + AuthplaneMcpSetup.builder() + .issuer(baseUrl) + .resource(baseUrl + "/mcp") + .scopes(List.of("tools/query")) + .devMode(true) + .build() + .get(); + + assertThat(setup.resource().resourceMetadataUrl()) + .isEqualTo(setup.resource().prmUrl()) + .isEqualTo(baseUrl + "/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void build_resourceMetadataUrl_reachesTheResource() throws Exception { + // The AS hosts the document; the servlet this setup can still register is unaffected, so + // prmPath() keeps naming the resource-hosted route. + String asHosted = baseUrl + "/.well-known/oauth-protected-resource/mcp"; + AuthplaneMcpSetup setup = + AuthplaneMcpSetup.builder() + .issuer(baseUrl) + .resource("http://mcp.internal:8080/mcp") + .scopes(List.of("tools/query")) + .devMode(true) + .resourceMetadataUrl(asHosted) + .build() + .get(); + + assertThat(setup.resource().resourceMetadataUrl()).isEqualTo(asHosted); + assertThat(setup.prmPath()).isEqualTo("/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void build_invalidResourceMetadataUrl_failsTheBuild() { + assertThatThrownBy( + () -> + AuthplaneMcpSetup.builder() + .issuer(baseUrl) + .resource(baseUrl + "/mcp") + .scopes(List.of("tools/query")) + .devMode(true) + .resourceMetadataUrl("not-a-url") + .build() + .get()) + .hasRootCauseInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("resourceMetadataUrl"); } @Test diff --git a/scripts/backport-fixes.sh b/scripts/backport-fixes.sh index 5e82a47..ffc0a25 100755 --- a/scripts/backport-fixes.sh +++ b/scripts/backport-fixes.sh @@ -22,8 +22,12 @@ Options: --from Source ref on origin: a branch (e.g. release/v0.6.0, hotfix/v0.5.1) or a tag (e.g. v0.6.0). Do not include 'origin/'. Required. Use the tag after the release - workflow has deleted the source branch. - --to Target branch on origin. Required. + workflow has deleted the source branch. A branch wins + if a branch and a tag share the name. One ref name, + not a pattern — globs like 'release/*' are rejected. + --to Target branch on origin. Required. Branch only: + backport-fixes.yml opens a PR with --base, which + needs a branch that exists on the remote. --branch Name for the local backport branch (default: `backport/vX.Y.Z` derived from --from when it matches release/vX.Y.Z, hotfix/vX.Y.Z, or vX.Y.Z; @@ -35,9 +39,11 @@ Options: -h, --help Show this help. Behavior: - 1. Fetches origin (both branch and tag refs). - 2. Lists commits on the source ref that aren't already on origin/, - and commits that are already there (skipped). + 1. Asks origin what and name (git ls-remote), then fetches + those two refs — not the whole remote. + 2. Lists commits on the resolved source ref — origin/ for a + branch, refs/tags/ for a tag — that aren't already on + origin/, and commits that are already there (skipped). 3. Creates the backport branch off origin/. 4. Runs `git cherry-pick -x` with the candidates, oldest-first. 5. On conflict: stops. Resolve, then `git cherry-pick --continue`. @@ -93,6 +99,31 @@ if [[ "$FROM" == "$TO" ]]; then exit 2 fi +# Each of --from / --to names exactly one ref, and nothing downstream enforces +# it. `git ls-remote` matches its argument as a glob and `*` is legal in a +# refspec, so `--from 'release/*'` answers "the ref is there", fetches +# wildcard-expanded — writing refs/remotes/origin/release/v1.0.0 — and leaves +# the source ref naming a pattern. `git cherry` then dies with `fatal: unknown +# commit`, the `|| true` on it swallows that, and the run ends "Nothing to +# backport." at exit 0: a tooling failure handed to the operator as a fact +# about the refs. +# +# `git check-ref-format` is git's own rule set for a ref name. It rejects the +# glob metacharacters and also `~`, `^`, `:`, `..` and friends — none of which +# name a single ref either. Checked here, before the first ls-remote, so a +# rejected argument cannot leave the local ref store changed. The rules are the +# same under refs/heads and refs/tags, so one check covers both arms. +require_single_ref_name() { + local flag="$1" name="$2" + if ! git check-ref-format "refs/heads/$name" 2>/dev/null; then + echo "error: $flag '$name' is not the name of a single ref" >&2 + echo " No globs ('*', '?', '['), no '~', '^', ':' or '..'." >&2 + exit 2 + fi +} +require_single_ref_name --from "$FROM" +require_single_ref_name --to "$TO" + # Must be in a git repo if ! git rev-parse --git-dir >/dev/null 2>&1; then echo "error: not inside a git repository" >&2 @@ -113,36 +144,99 @@ if ! git diff --quiet || ! git diff --cached --quiet; then exit 1 fi -echo "Fetching origin..." -# Fetch the target branch and try to fetch FROM as both a branch and a tag. -# Branches go to refs/remotes/origin/; tags go to refs/tags/. -# The two explicit refspecs make either accepted; whichever doesn't exist -# is silently ignored. -git fetch origin "$TO" --no-tags -git fetch origin --no-tags \ - "+refs/heads/$FROM:refs/remotes/origin/$FROM" 2>/dev/null \ - || true -git fetch origin --no-tags \ - "+refs/tags/$FROM:refs/tags/$FROM" 2>/dev/null \ - || true - -# Resolve FROM to a ref that exists locally after fetch. -if git rev-parse --verify "refs/remotes/origin/$FROM" >/dev/null 2>&1; then - from_ref="refs/remotes/origin/$FROM" - from_pretty="origin/$FROM" -elif git rev-parse --verify "refs/tags/$FROM" >/dev/null 2>&1; then - from_ref="refs/tags/$FROM" - from_pretty="$FROM (tag)" -else - echo "error: $FROM not found on origin (tried both branches and tags)" >&2 +echo "Resolving --from / --to against origin..." + +# Ask origin what a name is before fetching it, instead of attempting a fetch +# and reading its failure as absence. `git ls-remote --exit-code` answers 0 (the +# ref is there), 2 (the remote answered and has no such ref) or 128 (the remote +# was unreachable, or refused us). Only 2 means "not found"; reporting 128 as a +# missing ref sends the operator after a ref that is perfectly fine. +# +# Both refspecs below keep their leading `+`. Without the force, a source branch +# that was force-pushed — routine during release prep, e.g. an amended release +# commit — stops fast-forwarding and the fetch is rejected. +# +# The fetches run without -q on purpose: -q suppresses the per-ref status table, +# which is where `! [rejected]` is written, so a fetch that fails after +# ls-remote said the ref was there would exit with no explanation at all. +remote_ref_exists() { + local rc=0 + git ls-remote --exit-code origin "$1" >/dev/null || rc=$? + case "$rc" in + 0) return 0 ;; + 2) return 1 ;; + # git has already described the failure on stderr; adding a guess about the + # ref on top of it would only mislead. + *) exit 1 ;; + esac +} + +# These assign to from_ref / from_pretty rather than echoing their result. +# Called as `$(...)`, the body would run in a subshell, where the `exit 1` above +# exits only that subshell and the caller carries on with an empty ref. +from_ref="" +from_pretty="" + +# --from accepts a branch or a tag: after a release the source branch is gone, +# and the tag is the only ref naming those commits. Branches go to +# refs/remotes/origin/, tags to refs/tags/. A branch wins if a +# branch and a tag share the name. +fetch_source_ref() { + local name="$1" + if remote_ref_exists "refs/heads/$name"; then + git fetch origin --no-tags "+refs/heads/$name:refs/remotes/origin/$name" || exit 1 + from_ref="refs/remotes/origin/$name" + from_pretty="origin/$name" + elif remote_ref_exists "refs/tags/$name"; then + # A re-cut tag (deleted on origin and re-pushed at a new commit) lands here. + # `+` overwrites the stale local tag. Without it the fetch does not quietly + # keep the old tag — it is rejected outright, `! [rejected] v1.0.0 -> v1.0.0 + # (would clobber existing tag)`, and `|| exit 1` aborts the run. Loud, but + # for a tag origin has moved on purpose. t_recut_tag pins this arm. + git fetch origin --no-tags "+refs/tags/$name:refs/tags/$name" || exit 1 + from_ref="refs/tags/$name" + from_pretty="$name (tag)" + else + return 1 + fi +} + +# --to is branch-only, deliberately. A tag would resolve and `git checkout -b` +# would even work, but backport-fixes.yml opens a PR with `--base "$TO"`, which +# needs a branch that exists on the remote. +fetch_target_ref() { + local name="$1" + if remote_ref_exists "refs/heads/$name"; then + git fetch origin --no-tags "+refs/heads/$name:refs/remotes/origin/$name" || exit 1 + else + return 1 + fi +} + +if ! fetch_source_ref "$FROM"; then + echo "error: $FROM not found on origin as a branch or a tag" >&2 exit 1 fi -if ! git rev-parse --verify "refs/remotes/origin/$TO" >/dev/null 2>&1; then - echo "error: origin/$TO not found on remote" >&2 +if ! fetch_target_ref "$TO"; then + echo "error: $TO not found on origin as a branch (--to must be a branch)" >&2 exit 1 fi +# ls-remote said both refs were there and both fetches reported success, so both +# must now name a commit locally. Anything else — a tag object pointing at a +# blob, a ref that landed as something other than a commit — reaches `git +# cherry` below, which fails, and the `|| true` on it turns the failure into an +# empty candidate list and "Nothing to backport." at exit 0. Stop here instead: +# the run has already established these two refs resolve, so a ref that does not +# is a fault, not an answer. +for resolved in "$from_ref" "refs/remotes/origin/$TO"; do + if ! git rev-parse --verify --quiet "$resolved^{commit}" >/dev/null; then + echo "error: $resolved does not name a commit after fetching from origin" >&2 + exit 1 + fi +done + # `git cherry -v ` prints one line per commit: # + -> not on upstream (candidate for backport) # - -> already on upstream via patch-ID match diff --git a/scripts/backport-fixes.test.sh b/scripts/backport-fixes.test.sh new file mode 100755 index 0000000..17c69ef --- /dev/null +++ b/scripts/backport-fixes.test.sh @@ -0,0 +1,413 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Tests for backport-fixes.sh --from/--to ref resolution. +# +# The script had no test, and the failure it hid is not one a reader would +# guess: resolution used to run a fetch per candidate refspec with +# `2>/dev/null || true` and then look at what landed locally, so "the fetch +# failed" and "origin has no such ref" arrived as the same answer. Any other +# fetch failure — an unreachable remote, a refused auth, a ref that cannot be +# written locally — was reported as a missing ref, sending the operator after a +# ref that is present and current. +# +# Each case builds a throwaway origin + clone in a temp dir, so nothing here +# touches the real repository or the network. +# +# Run: scripts/backport-fixes.test.sh + +SCRIPT="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/backport-fixes.sh" +failures=0 + +pass() { printf ' ok %s\n' "$1"; } +fail() { printf ' FAIL %s\n %s\n' "$1" "$2"; failures=$((failures + 1)); } + +# Builds: origin with `main`, a v1.0.0 tag, and one commit after the tag that is +# only reachable from the tag's branch — the shape of a fix landed on a release +# branch at step 3 of the release flow. +make_fixture() { + local root="$1" + # -b main explicitly: the default branch name comes from init.defaultBranch, + # which differs between a developer machine and a CI runner. Without it the + # fixture builds `master` somewhere and every checkout of `main` fails. + git init -q -b main "$root/origin" + git -C "$root/origin" config user.email t@example.com + git -C "$root/origin" config user.name "Test" + echo base > "$root/origin/f.txt" + git -C "$root/origin" add -A + git -C "$root/origin" commit -qm "base" + + # Clone before the tag exists. A clone made afterwards fetches every tag, + # which leaves refs/tags/v1.0.0 populated locally and hides whether the + # script's own fetch materialises it. The real scenario is a maintainer who + # last fetched before the release. + git clone -q "$root/origin" "$root/clone" + + git -C "$root/origin" checkout -q -b release/v1.0.0 + echo fix > "$root/origin/f.txt" + git -C "$root/origin" commit -qam "fix: something landed on the release branch" + # Annotated, matching release.yml's `git tag -a`. A lightweight tag resolves + # the same way here, but the fixture should produce what the flow it models + # produces. + git -C "$root/origin" tag -a v1.0.0 -m "v1.0.0" + git -C "$root/origin" checkout -q main + git -C "$root/clone" config user.email t@example.com + git -C "$root/clone" config user.name "Test" +} + +# --- a branch as --from keeps working ----------------------------------------- +t_branch() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "landed on the release branch"; then + pass "a branch as --from cherry-picks its commits" + else + fail "a branch as --from cherry-picks its commits" "branch created but the commit is missing" + fi + else + fail "a branch as --from cherry-picks its commits" "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- a tag as --from ---------------------------------------------------------- +# After a release, release.yml has deleted release/vX.Y.Z, so the tag is the +# only ref naming those commits — the form the release guide and +# backport-fixes.yml both tell you to pass, and therefore the form least likely +# to be exercised before it is needed. +t_tag() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "landed on the release branch"; then + pass "a tag as --from cherry-picks its commits" + else + fail "a tag as --from cherry-picks its commits" "branch created but the commit is missing" + fi + else + fail "a tag as --from cherry-picks its commits" "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- an unknown ref fails, and leaves nothing behind --------------------------- +# It fails at the resolver, which is what the assertion below pins: `git +# ls-remote --exit-code` exits 2 for a name the remote does not have, both arms +# of fetch_source_ref return non-zero, and the script prints its own message. +# What matters is the contract: non-zero, and no branch created. +t_unknown() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from does-not-exist --to main 2>&1)"; then + fail "an unknown --from fails at the resolver, creating no branch" "script exited zero" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "an unknown --from fails at the resolver, creating no branch" \ + "it created a backport branch anyway" + elif ! grep -q "not found on origin as a branch or a tag" <<<"$out"; then + fail "an unknown --from fails at the resolver, creating no branch" \ + "reached the fetch, not the resolver: ${out##*$'\n'}" + else + pass "an unknown --from fails at the resolver, creating no branch" + fi +} + +# --- a glob is not a ref name -------------------------------------------------- +# `git ls-remote` matches its argument as a glob and `*` is legal in a refspec, +# so an unvalidated `--from 'release/*'` answered "the ref is there", fetched +# wildcard-expanded, and handed `git cherry` a pattern instead of a commit. The +# `|| true` on that turned `fatal: unknown commit` into an empty candidate list +# and the run reported "Nothing to backport." at exit 0 — a tooling failure +# delivered as a fact about the refs, the class this suite exists for. +# +# Asserts the absence of the script's own wrong claim rather than the presence +# of any particular message, plus the two side effects that make it worse than +# a bad exit code: no backport branch, and no write to the local ref store. The +# second one is the reason this is caught in argument validation and not after +# the fetch — a rejected argument must not leave the repository changed. +t_glob_from() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from 'release/*' --to main 2>&1)"; then + fail "a glob as --from is rejected, fetching nothing" "script exited zero" + elif grep -q "Nothing to backport" <<<"$out"; then + fail "a glob as --from is rejected, fetching nothing" \ + "the failure was reported as an empty backport" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "a glob as --from is rejected, fetching nothing" "it created a backport branch anyway" + elif git -C "$root/clone" show-ref --verify --quiet refs/remotes/origin/release/v1.0.0; then + fail "a glob as --from is rejected, fetching nothing" \ + "the wildcard refspec still wrote refs/remotes/origin/release/v1.0.0" + else + pass "a glob as --from is rejected, fetching nothing" + fi +} + +# Same defect on the other argument: `--to 'mai*'` fetched refs/heads/mai* into +# refs/remotes/origin/mai*, then compared against a ref of that name that does +# not exist, and again ended at "Nothing to backport." with exit 0. +t_glob_to() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to 'mai*' 2>&1)"; then + fail "a glob as --to is rejected, fetching nothing" "script exited zero" + elif grep -q "Nothing to backport" <<<"$out"; then + fail "a glob as --to is rejected, fetching nothing" \ + "the failure was reported as an empty backport" + elif git -C "$root/clone" show-ref --verify --quiet refs/remotes/origin/release/v1.0.0; then + fail "a glob as --to is rejected, fetching nothing" \ + "--from was fetched before --to was validated" + else + pass "a glob as --to is rejected, fetching nothing" + fi +} + +# --- --to is branch-only ------------------------------------------------------ +# A tag resolves and `git checkout -b` would even work, but backport-fixes.yml +# opens a PR with `--base "$TO"`, which needs a branch on the remote. Rejecting +# it here beats failing after the cherry-picks have run, and the message has to +# name the reason — "not found on remote" for a tag the remote demonstrably has +# is the same category of misdirection this suite exists for. +t_to_rejects_a_tag() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from main --to v1.0.0 2>&1)"; then + fail "--to rejects a tag" "script exited zero" + elif grep -q "must be a branch" <<<"$out"; then + pass "--to rejects a tag, naming the reason" + else + fail "--to rejects a tag" "unexpected message: ${out##*$'\n'}" + fi +} + +# --- a force-pushed source branch still backports ------------------------------ +# What the `+` on the refspecs is for. Without it the fetch is a non-fast-forward +# rejection, and the resolver would report that as "not found on origin". +# Amending a release commit during release prep is routine. +t_force_pushed_source() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + # Seed the remote-tracking ref at the pre-amend commit: the state of a + # maintainer who last fetched before the force-push. Without this the clone + # has no origin/release/v1.0.0 at all and any fetch is trivially a + # fast-forward, which is how a missing `+` would go unnoticed. + git -C "$root/clone" fetch -q origin \ + '+refs/heads/release/v1.0.0:refs/remotes/origin/release/v1.0.0' + + git -C "$root/origin" checkout -q release/v1.0.0 + echo amended > "$root/origin/f.txt" + git -C "$root/origin" commit -q --amend -am "fix: something landed on the release branch (amended)" + git -C "$root/origin" checkout -q main + + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "(amended)"; then + pass "a force-pushed source branch backports the rewritten commit" + else + fail "a force-pushed source branch backports the rewritten commit" \ + "it backported the pre-amend commit" + fi + else + fail "a force-pushed source branch backports the rewritten commit" \ + "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- a branch wins when a branch and a tag share the name ---------------------- +# The arm order in fetch_source_ref decides this and --help now states it, so it +# needs a case: a repo that tags v1.0.0 and later cuts a branch of the same name +# would otherwise silently change which commits get backported. +t_branch_beats_tag() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + # A branch literally named v1.0.0, carrying a commit the tag does not. + git -C "$root/origin" checkout -q -b v1.0.0 main + echo from-branch > "$root/origin/f.txt" + git -C "$root/origin" commit -qam "fix: reached through the branch" + git -C "$root/origin" checkout -q main + + local out + if out="$(cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to main 2>&1)"; then + if git -C "$root/clone" log --oneline main..HEAD | grep -q "reached through the branch"; then + pass "a branch wins over a tag of the same name" + else + fail "a branch wins over a tag of the same name" "it resolved the tag instead" + fi + else + fail "a branch wins over a tag of the same name" "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- a re-cut tag backports the new commit ------------------------------------- +# What the `+` on the *tag* refspec is for. t_force_pushed_source covers the +# branch arm only; without this case the tag arm's `+` can be deleted and the +# suite still passes. Re-cutting a tag — deleting it on origin and re-pushing it +# at a new commit — is what happens when a release is pulled and redone, and it +# is the one time the local tag and origin's disagree. +# +# Without the `+` the fetch is rejected (`would clobber existing tag`) and +# `|| exit 1` aborts, so this fails on the exit code rather than on a wrong +# backport. +t_recut_tag() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + # The maintainer fetched at release time, so the clone holds v1.0.0 at the + # original commit. Without this the local tag is simply absent, any fetch of + # it is trivially new, and a missing `+` would go unnoticed — the same trap + # t_force_pushed_source seeds around on the branch arm. + git -C "$root/clone" fetch -q origin '+refs/tags/v1.0.0:refs/tags/v1.0.0' + + git -C "$root/origin" checkout -q release/v1.0.0 + echo recut > "$root/origin/f.txt" + git -C "$root/origin" commit -qam "fix: shipped in the re-cut release" + git -C "$root/origin" tag -d v1.0.0 >/dev/null + git -C "$root/origin" tag -a v1.0.0 -m "v1.0.0" + git -C "$root/origin" checkout -q main + + local out subjects + if out="$(cd "$root/clone" && "$SCRIPT" --from v1.0.0 --to main 2>&1)"; then + # Not `git log | grep -q`: this is the first case whose log is more than one + # line with the match on the first of them, so grep -q exits before git has + # finished writing, git takes SIGPIPE, and `set -o pipefail` reports the + # pipeline as failed. Collect first, match second. + subjects="$(git -C "$root/clone" log --format=%s main..HEAD)" + if grep -q "re-cut release" <<<"$subjects"; then + pass "a re-cut tag backports the commit it now names" + else + fail "a re-cut tag backports the commit it now names" \ + "it backported the superseded tag's commits" + fi + else + fail "a re-cut tag backports the commit it now names" \ + "script exited non-zero: ${out##*$'\n'}" + fi +} + +# --- the fetches stay verbose -------------------------------------------------- +# A source-level assertion, deliberately. `-q` suppresses the per-ref status +# table, which is where `! [rejected]` is written — and with the `+` in place +# nothing can produce a rejection, so no fixture reaches the case while the +# script is otherwise correct. Measured on a stale local tag against a re-cut +# origin tag, with the `+` removed: +# +# without -q: ! [rejected] v1.0.0 -> v1.0.0 (would clobber existing tag) rc 1 +# with -q: (nothing on either stream) rc 1 +# +# `|| exit 1` then ends the run with no explanation at all, which is what the +# comment above the resolver says must not happen. The lock-error cases below do +# not catch it either: their `error:` lines come from the ref backend rather than +# the status table and survive -q. +t_fetches_stay_verbose() { + local offenders + offenders="$(grep -n 'git fetch' "$SCRIPT" | grep -E -- '(-q|--quiet)' || true)" + if [[ -n "$offenders" ]]; then + fail "the fetches run without -q" "${offenders//$'\n'/; }" + else + pass "the fetches run without -q" + fi +} + +# --- a failed fetch is not a missing ref --------------------------------------- +# The case the resolver was rewritten for. origin has release/v1.0.0 and will +# happily say so, but the fetch cannot write refs/remotes/origin/release/v1.0.0 +# because refs/remotes/origin/release already exists as a ref — the state of a +# clone that once tracked a branch called `release`. git fails the fetch with a +# lock error naming the conflict. +# +# The old resolver ran that fetch under `2>/dev/null || true`, found nothing +# under refs/remotes and nothing under refs/tags, and printed "not found on +# origin (tried both branches and tags)" about a branch origin has. +# +# It asserts the absence of the script's own message rather than the presence of +# git's, because the wording of the lock error is git's to change and a test +# pinning it would fail on a git upgrade for unrelated reasons. But absence +# alone would also sign off on a script that failed in silence, and "an +# actionable message" is the whole point — so it also asserts that *something* +# reached stderr, keyed on git's `error:` / `fatal:` prefix. That prefix is a +# convention across every git command, not a wording: git changing it would +# break far more than this suite. +t_failed_fetch_is_not_a_missing_ref() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + + local base + base="$(git -C "$root/clone" rev-parse main)" + git -C "$root/clone" update-ref refs/remotes/origin/release "$base" + + local out rc=0 + out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>"$root/stderr")" || rc=$? + local err; err="$(cat "$root/stderr")" + if [[ "$rc" -eq 0 ]]; then + fail "a failed fetch is not reported as a missing ref" "script exited zero" + elif grep -q "not found on origin" <<<"$out$err"; then + fail "a failed fetch is not reported as a missing ref" \ + "the fetch failure was reported as a missing ref" + elif ! grep -qE '^(error|fatal):' <<<"$err"; then + fail "a failed fetch is not reported as a missing ref" \ + "it failed without writing an explanation to stderr" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "a failed fetch is not reported as a missing ref" "it created a backport branch anyway" + else + pass "a failed fetch is not reported as a missing ref" + fi +} + +# --- an unreachable remote is not a missing ref -------------------------------- +# `git ls-remote --exit-code` answers 2 for "asked, and the remote has no such +# ref" and 128 for "could not ask" — unreachable, or refused. The 128 arm is the +# one that separates them, and without a case a regression in it is invisible: +# the suite passes while a network failure is reported as a missing ref. +# +# Same assertion shape as above, and for the same reasons: `fatal: Could not read +# from remote repository.` is git's wording to change, so what is pinned is that +# the script adds no claim about the ref on top of it — and that git's own +# explanation did reach the operator. +t_unreachable_remote() { + local root; root="$(mktemp -d)"; trap 'rm -rf "$root"' RETURN + make_fixture "$root" + git -C "$root/clone" remote set-url origin /nonexistent + + local out rc=0 + out="$(cd "$root/clone" && "$SCRIPT" --from release/v1.0.0 --to main 2>"$root/stderr")" || rc=$? + local err; err="$(cat "$root/stderr")" + if [[ "$rc" -eq 0 ]]; then + fail "an unreachable remote is not reported as a missing ref" "script exited zero" + elif grep -q "not found on origin" <<<"$out$err"; then + fail "an unreachable remote is not reported as a missing ref" \ + "the network failure was reported as a missing ref" + elif ! grep -qE '^(error|fatal):' <<<"$err"; then + fail "an unreachable remote is not reported as a missing ref" \ + "it failed without writing an explanation to stderr" + elif [[ -n "$(git -C "$root/clone" branch --list 'backport/*')" ]]; then + fail "an unreachable remote is not reported as a missing ref" "it created a backport branch anyway" + else + pass "an unreachable remote is not reported as a missing ref" + fi +} + +echo "backport-fixes.sh — --from/--to ref resolution" +t_branch +t_tag +t_unknown +t_glob_from +t_glob_to +t_to_rejects_a_tag +t_force_pushed_source +t_branch_beats_tag +t_recut_tag +t_fetches_stay_verbose +t_failed_fetch_is_not_a_missing_ref +t_unreachable_remote + +if [[ "$failures" -gt 0 ]]; then + echo "$failures failing" + exit 1 +fi +echo "all passing" diff --git a/scripts/manual-e2e-setup.sh b/scripts/manual-e2e-setup.sh index 93f5a69..8b4f83b 100755 --- a/scripts/manual-e2e-setup.sh +++ b/scripts/manual-e2e-setup.sh @@ -4,6 +4,7 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" REPO_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" AUTHSERVER_DIR="${AUTHSERVER_DIR:-$REPO_ROOT/../authserver}" +AUTHSERVER_REF="${AUTHSERVER_REF:-}" usage() { cat <<'EOF' @@ -12,6 +13,8 @@ Usage: Environment (optional): AUTHSERVER_DIR Path to local authserver repo (default: ../authserver) + AUTHSERVER_REF Git ref of authserver to check out before building, e.g. v0.2.0 + (default: leave the checkout as is) EOF } @@ -25,13 +28,20 @@ if [ ! -d "${AUTHSERVER_DIR}" ]; then exit 1 fi -echo "==> Starting authserver demo server (client_credentials enabled)" +if [ -n "${AUTHSERVER_REF}" ]; then + echo "==> Checking out authserver ${AUTHSERVER_REF}" + git -C "${AUTHSERVER_DIR}" fetch --tags --quiet + git -C "${AUTHSERVER_DIR}" checkout --quiet "${AUTHSERVER_REF}" + rm -f "${AUTHSERVER_DIR}/bin/authserver" +fi + +echo "==> Starting authserver demo server" ( cd "${AUTHSERVER_DIR}" if [ ! -x "bin/authserver" ]; then go build -o bin/authserver ./cmd/authserver fi - AUTHPLANE_CLIENT_CREDENTIALS_ENABLED=true ./demo/mcp-demo-server-start.sh + ./demo/mcp-demo-server-start.sh ) echo "" diff --git a/spring/demo/README.md b/spring/demo/README.md index c72874a..e50c20d 100644 --- a/spring/demo/README.md +++ b/spring/demo/README.md @@ -183,7 +183,7 @@ HTTP/1.1 401 Unauthorized WWW-Authenticate: Bearer resource_metadata="http://localhost:8080/.well-known/oauth-protected-resource/mcp" Content-Type: application/json -{"error":"invalid_token","error_description":"Bearer token is missing or invalid"} +{"error":"invalid_token","error_description":"The access token is missing or not valid for this resource"} ``` ### Insufficient scope (403) diff --git a/spring/docs/user-guide.md b/spring/docs/user-guide.md index a66f8ad..cfa072d 100644 --- a/spring/docs/user-guide.md +++ b/spring/docs/user-guide.md @@ -172,6 +172,7 @@ Both paths use the same `application.properties` keys: | `authplane.jwks-refresh-seconds` | `300` | JWKS cache TTL | | `authplane.metadata-refresh-seconds` | `3600` | AS metadata cache TTL | | `authplane.introspection.enabled` | `false` | Enable built-in RFC 7662 token introspection | +| `authplane.resource-metadata-url` | derived | URL advertised in the `resource_metadata` challenge parameter; unset means the resource-hosted document this config serves (see [PRM](#protected-resource-metadata-prm)) | | `authplane.timeout-seconds` | `0` (SDK default: 10s) | HTTP request timeout | | `authplane.circuit-breaker-threshold` | `0` (SDK default: 5) | Failures before the circuit breaker opens | | `authplane.circuit-breaker-cooldown-seconds` | `0` (SDK default: 30s) | Cooldown before the circuit breaker transitions to half-open | @@ -350,6 +351,27 @@ No additional configuration is needed; PRM is served automatically. **Path A note**: The config bypasses Spring Security's built-in PRM filter (which produces an incomplete document) and serves the correct RFC 9728 document via a Spring MVC `RouterFunction`. +### Where the PRM document lives + +Two topologies, one property: + +| Topology | Who serves the document | What points at it | +|---|---|---| +| Resource-hosted (default) | This application, at `/.well-known/oauth-protected-resource[/path]` | The derived URL, advertised automatically | +| AS-hosted | The authorization server, which publishes one document per registered resource (authserver 0.2.0 and later serves `/.well-known/oauth-protected-resource/{ref}`, `ref` being the RFC 9728 §3.1 path suffix of the resource URI, or its slug) | `authplane.resource-metadata-url` | + +```properties +authplane.issuer=https://auth.company.com +authplane.resource=https://mcp.company.com/mcp +authplane.resource-metadata-url=https://auth.company.com/.well-known/oauth-protected-resource/mcp +``` + +Every challenge Path A renders carries the configured URL — the 401 for a missing, malformed or invalid token, the DPoP challenges, and the 403 `insufficient_scope`. A value that is not an absolute `http(s)` URL naming a host fails at context startup, and `http` is refused outright for an `https` resource. Setting the property does not unregister the PRM endpoint: the application still serves its own document unless you exclude that bean, which is harmless as long as both documents carry the same `resource` member. + +Reach for the AS-hosted topology when the application cannot serve well-known paths — a platform or gateway that owns them. RFC 9728 §3.3 binds either one to the identifier: the `resource` member the client reads must equal, byte for byte, the identifier it derived the metadata request from, so the resource registered at the authorization server, `authplane.resource` and the URL clients actually call all have to be the same string. + +**Path B note**: the MCP transport hooks reject through `ServerTransportSecurityException(int statusCode, String message)`, which carries no headers, so those 401/403 responses have no `WWW-Authenticate` header and no `resource_metadata` parameter on either topology — see [Known Limitations](#known-limitations-transport-path). + ## Token Revocation Checking By default, tokens are validated offline (signature + claims only). You can enable revocation checking to catch tokens that have been revoked before they expire. @@ -372,7 +394,8 @@ authplane.resource=https://mcp.company.com/mcp authplane.introspection.enabled=true ``` -Authenticated introspection requires client credentials. The SDK reads these from an +Introspection requires client credentials: authserver ≥ 0.1.2 answers `{"active": false}` to an +unauthenticated call, which would reject every token as revoked. The SDK reads them from an `AuthProvider` bean (not from properties): ```java @@ -389,7 +412,8 @@ public AuthProvider authProvider() { - The introspection endpoint is automatically discovered from AS metadata. - If the endpoint returns `active=false`, the token is rejected. - **Fails open**: if the introspection endpoint is unavailable, the token is accepted (offline validation still applies). -- The `AuthProvider` bean enables authenticated introspection (recommended for production). +- The `AuthProvider` bean is required in practice: the client must be confidential (a public client cannot introspect at all) and must be either the client the token was issued to or a runtime-client of the Resource named in `aud`. Register it with `authserver admin resource runtime-client add --client-id --slug `. +- Without the bean the checker logs a warning at startup; it also warns once when `active=false` comes back for a token that already passed local JWT verification. ### Custom Revocation Checker @@ -462,6 +486,26 @@ class MyTools { The token exchange client inherits SSRF settings and credentials from the original configuration. +### Exchange errors + +Besides `ConsentRequiredException` (see below), `client.exchange(...)` can fail with two sibling subtypes of `TokenExchangeException` that re-prompting the user will not fix: + +- `ai.authplane.sdk.core.errors.AccessDeniedException` (`access_denied`, HTTP 403): on a cross-client exchange, the operator has not allowlisted this client on the target Resource. Fix the Resource policy (next step). Import it by its full name: the simple name collides with Spring Security's `org.springframework.security.access.AccessDeniedException`, which this adapter also throws, and catching the wrong one compiles and catches nothing. +- `InvalidTargetException` (`invalid_target`, HTTP 400, RFC 8707 §2.2): the `resource` string does not match a granted resource exactly — a trailing slash is enough. Send the identifier byte for byte as granted. + +None of the three trips the circuit breaker. + +### Operator step: allowlist the exchanging client + +For each MCP server that exchanges for a downstream resource it does not itself act as, allowlist its client ID on that Resource: + +```http +PATCH /admin/resources/{id} +{"policy": {"exchange": {"allowed_client_ids": [""]}}} +``` + +A client exchanging a token issued to itself, fronted exchanges, and Broker resources need nothing. + ## URL Elicitation (No Equivalent) The `authplane-mcp` adapter ships a `UrlElicitationSupport.wrapToolWithUrlElicitation(...)` helper that translates `consent_required` / `interaction_required` token-exchange errors into MCP JSON-RPC URL elicitation responses (error code `-32042`). **There is no Spring-adapter equivalent.** @@ -499,6 +543,8 @@ The MCP Java SDK splits transport-level auth into two hooks with different capab 1. **`validateHeaders(Map>)`** — receives only headers. Can return proper HTTP status codes via `ServerTransportSecurityException` (401/403). 2. **`extract(ServerRequest)`** — receives the full request (method, URL, headers). Exceptions bubble as unhandled 500s; there is no mechanism to return a structured HTTP error. +The rejection `validateHeaders` can produce is `ServerTransportSecurityException(int statusCode, String message)`, which carries a status and a message and nothing else: the transport-tier 401/403 responses have no `WWW-Authenticate` header, and therefore no `resource_metadata` parameter, whether the PRM document is resource-hosted or AS-hosted. Path A is where the full RFC 6750 / RFC 9728 challenge is emitted. + This produces two observable consequences: ### Duplicate verification / double introspection diff --git a/spring/pom.xml b/spring/pom.xml index 881d456..06c8bba 100644 --- a/spring/pom.xml +++ b/spring/pom.xml @@ -146,8 +146,8 @@ org.springframework 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 c62adc5..215d8ff 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 @@ -26,6 +26,7 @@ import ai.authplane.sdk.core.dpop.VerificationRequestContext; import ai.authplane.sdk.core.errors.AuthplaneException; import ai.authplane.sdk.core.errors.InsufficientScopeException; +import ai.authplane.sdk.core.errors.WwwAuthenticate; import ai.authplane.sdk.core.fetching.FetchSettings; import ai.authplane.sdk.core.http.HttpHeaders; import io.modelcontextprotocol.common.McpTransportContext; @@ -159,7 +160,7 @@ public void validateHeaders(Map> headers) try { VerificationRequestContext.assertSingleDpopHeader(HttpHeaders.values(headers, "dpop")); } catch (MultipleDpopProofsException e) { - throw new ServerTransportSecurityException(401, e.getMessage()); + throw new ServerTransportSecurityException(401, safeDescription(e)); } try { @@ -268,15 +269,29 @@ private static Throwable unwrapCompletion(Throwable t) { * the correct HTTP status. */ private static ServerTransportSecurityException mapToSecurityException(Throwable t) { - if (t instanceof InsufficientScopeException) { - return new ServerTransportSecurityException(403, t.getMessage()); + if (t instanceof InsufficientScopeException ise) { + return new ServerTransportSecurityException(403, safeDescription(ise)); } - if (t instanceof AuthplaneException) { - return new ServerTransportSecurityException(401, t.getMessage()); + if (t instanceof AuthplaneException ae) { + return new ServerTransportSecurityException(401, safeDescription(ae)); } return new ServerTransportSecurityException(401, "Token verification failed"); } + /** + * The fixed, caller-safe sentence for an exception, rather than its own message. + * + *

The MCP SDK's transport renders this message into the response it sends the caller, so + * whatever is put here reaches someone who by definition has not authenticated — the same seam + * {@code WwwAuthenticate} closes for the challenge this adapter does not build. The SDK's + * messages name the unknown {@code kid}, the claim that did not validate, and on an audience + * mismatch the exact {@code aud} the resource expects. The original exception is still thrown + * with its own message available to the server's logs. + */ + private static String safeDescription(AuthplaneException error) { + return WwwAuthenticate.descriptionFor(WwwAuthenticate.errorCodeFor(error)); + } + // ----------------------------------------------------------------------- // Builder // ----------------------------------------------------------------------- diff --git a/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfig.java b/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfig.java index 28720d6..1c46bac 100644 --- a/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfig.java +++ b/spring/src/main/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfig.java @@ -58,6 +58,9 @@ * authplane.jwks-refresh-seconds = 300 # Background JWKS refresh interval * authplane.metadata-refresh-seconds = 3600 # RFC 8414 metadata refresh interval * authplane.introspection.enabled = false # Enable built-in RFC 7662 token introspection + * authplane.resource-metadata-url = # PRM URL to advertise (default: derived, resource-hosted). + * # No effect on this config's own transport-tier 401/403, + * # which carry no WWW-Authenticate at all — see the guide. * authplane.timeout-seconds = 0 # HTTP timeout (0 = use SDK default of 10s) * authplane.circuit-breaker-threshold = 0 # Failures before circuit opens (0 = SDK default of 5) * authplane.circuit-breaker-cooldown-seconds = 0 # Cooldown before half-open (0 = SDK default of 30s) @@ -215,6 +218,7 @@ public AuthplaneResource authplaneResource( @Value("${authplane.allowed-algorithms:RS256,ES256}") List allowedAlgorithms, @Value("${authplane.clock-skew-seconds:30}") int clockSkewSeconds, @Value("${authplane.introspection.enabled:false}") boolean introspectionEnabled, + @Value("${authplane.resource-metadata-url:}") String resourceMetadataUrl, ObjectProvider revocationCheckerProvider, ObjectProvider inboundDPoPProvider) { @@ -237,6 +241,13 @@ public AuthplaneResource authplaneResource( optBuilder.inboundDPoP(inboundDPoP); } + // Unset (the default) leaves the challenge advertising the resource-hosted document this + // config serves itself; set it to the AS-hosted copy when this server cannot serve the + // well-known path. A malformed value fails here, at context startup. + if (!resourceMetadataUrl.isBlank()) { + optBuilder.resourceMetadataUrl(resourceMetadataUrl); + } + return client.resource(resource, scopes, optBuilder.build()); } diff --git a/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPoint.java b/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPoint.java index c675d41..d54b0e5 100644 --- a/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPoint.java +++ b/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPoint.java @@ -32,7 +32,8 @@ public final class AuthplaneAuthenticationEntryPoint implements AuthenticationEn private final AuthplaneResource resource; /** - * @param resource the resource being protected (supplies the {@code resource_metadata} URL) + * @param resource the resource being protected (supplies the {@code resource_metadata} URL, + * derived or configured via {@code authplane.resource-metadata-url}) */ public AuthplaneAuthenticationEntryPoint(AuthplaneResource resource) { this.resource = Objects.requireNonNull(resource, "resource must not be null"); @@ -50,9 +51,14 @@ public void commence( ? ae : new TokenMissingException("Bearer token is missing or invalid"); + // resourceMetadataUrl(), not prmUrl(): the resource decides which RFC 9728 topology it is + // in — document served here, or served by the AS and only pointed at — and every challenge + // this entry point renders (401, and 403 for insufficient_scope) reads that one decision. FailureResponse.Challenge challenge = FailureResponse.of( - error, ChallengeOptions.empty().withResourceMetadataUrl(resource.prmUrl())); + error, + ChallengeOptions.empty() + .withResourceMetadataUrl(resource.resourceMetadataUrl())); response.setStatus(challenge.status()); response.setHeader("WWW-Authenticate", challenge.wwwAuthenticate()); diff --git a/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProvider.java b/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProvider.java index 48a1d99..d5a9303 100644 --- a/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProvider.java +++ b/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProvider.java @@ -34,8 +34,6 @@ */ public final class AuthplaneAuthenticationProvider implements AuthenticationProvider { - private static final String GENERIC_DESCRIPTION = "Token validation failed"; - private final AuthplaneResource resource; /** @@ -80,18 +78,22 @@ public boolean supports(Class authentication) { * (which only catches {@link AuthenticationException}) as an unhandled 500. */ static OAuth2AuthenticationException toOAuth2Exception(Throwable cause) { - // Only SDK-owned exceptions get their message reflected back to the client. Anything else - // (transport NPE, downstream lib failure, …) may carry sensitive details in getMessage(), - // so we surface the generic description and let server-side logging hold the detail. + // No exception's message is reflected back to the client, SDK-owned or not. The SDK's own + // messages name the unknown kid, the claim that did not validate, or the aud the resource + // expects; the description is the fixed sentence for the code instead, and server-side + // logging holds the detail. The SDK's default wiring routes to + // AuthplaneAuthenticationEntryPoint, which discards both halves of this exception, but an + // application wiring this provider under Spring's own oauth2ResourceServer gets + // BearerTokenAuthenticationEntryPoint, which renders the OAuth2Error description straight + // into error_description — and with server.error.include-message=always the exception + // message reaches the error body too. String errorCode; - String description; if (cause instanceof AuthplaneException ae) { errorCode = WwwAuthenticate.errorCodeFor(ae); - description = ae.getMessage() != null ? ae.getMessage() : GENERIC_DESCRIPTION; } else { errorCode = "invalid_token"; - description = GENERIC_DESCRIPTION; } + String description = WwwAuthenticate.descriptionFor(errorCode); return new OAuth2AuthenticationException( new OAuth2Error(errorCode, description, null), description, cause); } diff --git a/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfig.java b/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfig.java index e90444e..33c9a12 100644 --- a/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfig.java +++ b/spring/src/main/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfig.java @@ -60,6 +60,7 @@ * authplane.jwks-refresh-seconds = 300 # Background JWKS refresh interval * authplane.metadata-refresh-seconds = 3600 # RFC 8414 metadata refresh interval * authplane.introspection.enabled = false # Enable built-in RFC 7662 token introspection + * authplane.resource-metadata-url = # PRM URL to advertise (default: derived, resource-hosted) * authplane.timeout-seconds = 0 # HTTP timeout (0 = use SDK default of 10s) * authplane.circuit-breaker-threshold = 0 # Failures before circuit opens (0 = SDK default of 5) * authplane.circuit-breaker-cooldown-seconds = 0 # Cooldown before half-open (0 = SDK default of 30s) @@ -239,6 +240,7 @@ public AuthplaneResource authplaneResource( @Value("${authplane.allowed-algorithms:RS256,ES256}") List allowedAlgorithms, @Value("${authplane.clock-skew-seconds:30}") int clockSkewSeconds, @Value("${authplane.introspection.enabled:false}") boolean introspectionEnabled, + @Value("${authplane.resource-metadata-url:}") String resourceMetadataUrl, ObjectProvider revocationCheckerProvider, ObjectProvider inboundDPoPProvider) { @@ -261,6 +263,13 @@ public AuthplaneResource authplaneResource( optBuilder.inboundDPoP(inboundDPoP); } + // Unset (the default) leaves the challenge advertising the resource-hosted document this + // config serves itself; set it to the AS-hosted copy when this server cannot serve the + // well-known path. A malformed value fails here, at context startup. + if (!resourceMetadataUrl.isBlank()) { + optBuilder.resourceMetadataUrl(resourceMetadataUrl); + } + return client.resource(resource, scopes, optBuilder.build()); } diff --git a/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapterTest.java b/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapterTest.java index d528eb1..0f9d34e 100644 --- a/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapterTest.java +++ b/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerAdapterTest.java @@ -320,7 +320,15 @@ void validateHeaders_multipleDpopHeaders_throws401() { ServerTransportSecurityException se = (ServerTransportSecurityException) e; assertThat(se.getStatusCode()).isEqualTo(401); - assertThat(se.getMessage()).contains("Multiple DPoP"); + // The transport renders this message to the caller, so it is the + // fixed per-code sentence, not core's "Multiple DPoP …". The + // extract() test below still pins the raw message, which surfaces + // to the host application rather than to the wire. + assertThat(se.getMessage()) + .isEqualTo( + "The DPoP proof is missing or not valid for this" + + " request"); + assertThat(se.getMessage()).doesNotContain("Multiple DPoP"); }); } diff --git a/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfigTest.java b/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfigTest.java index 0f12579..9604d76 100644 --- a/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfigTest.java +++ b/spring/src/test/java/ai/authplane/sdk/spring/mcp/AuthplaneMcpServerConfigTest.java @@ -102,6 +102,29 @@ void authplaneVerifier_basicBuild_succeeds() throws Exception { assertThat(v.prmResponse()).containsEntry("resource", baseUrl + "/mcp"); } + @Test + void resourceMetadataUrlProperty_isAdvertisedInsteadOfTheDerivedUrl() throws Exception { + // This config carries its own copy of the three-line wiring, which is exactly why it needs + // its own assertion: nothing else catches the branch being dropped or inverted in one file. + AuthplaneClient client = buildClient(0); + AuthplaneResource v = + buildVerifier( + client, + false, + "https://auth.example.com/.well-known/oauth-protected-resource/mcp"); + + assertThat(v.resourceMetadataUrl()) + .isEqualTo("https://auth.example.com/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void resourceMetadataUrlProperty_blank_derivesTheResourceHostedUrl() throws Exception { + AuthplaneClient client = buildClient(0); + AuthplaneResource v = buildVerifier(client, false, ""); + + assertThat(v.resourceMetadataUrl()).isEqualTo(v.prmUrl()); + } + // ----------------------------------------------------------------------- // Credentials (AuthProvider bean) // ----------------------------------------------------------------------- @@ -307,6 +330,12 @@ private AuthplaneClient buildClient(int timeoutSeconds) throws Exception { /** Calls authplaneVerifier() with the given client and introspection flag. */ private AuthplaneResource buildVerifier(AuthplaneClient client, boolean introspectionEnabled) { + return buildVerifier(client, introspectionEnabled, ""); + } + + /** Calls authplaneVerifier() with an explicit authplane.resource-metadata-url. */ + private AuthplaneResource buildVerifier( + AuthplaneClient client, boolean introspectionEnabled, String resourceMetadataUrl) { return config.authplaneResource( client, baseUrl + "/mcp", // resource @@ -314,6 +343,7 @@ private AuthplaneResource buildVerifier(AuthplaneClient client, boolean introspe List.of("RS256"), 30, // clockSkewSeconds introspectionEnabled, + resourceMetadataUrl, revocationCheckerProvider, inboundDPoPProvider); } diff --git a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPointTest.java b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPointTest.java index 805da23..9d7acf5 100644 --- a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPointTest.java +++ b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationEntryPointTest.java @@ -8,6 +8,7 @@ import java.io.PrintWriter; import java.io.StringWriter; +import java.util.List; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -19,6 +20,7 @@ import ai.authplane.sdk.core.AuthplaneResource; import ai.authplane.sdk.core.dpop.MultipleDpopProofsException; +import ai.authplane.sdk.core.errors.InsufficientScopeException; class AuthplaneAuthenticationEntryPointTest { @@ -26,8 +28,12 @@ class AuthplaneAuthenticationEntryPointTest { "https://api.example.com/.well-known/oauth-protected-resource/mcp"; private AuthplaneAuthenticationEntryPoint entryPoint() { + return entryPoint(PRM_URL); + } + + private AuthplaneAuthenticationEntryPoint entryPoint(String advertisedPrmUrl) { AuthplaneResource resource = mock(AuthplaneResource.class); - when(resource.prmUrl()).thenReturn(PRM_URL); + when(resource.resourceMetadataUrl()).thenReturn(advertisedPrmUrl); return new AuthplaneAuthenticationEntryPoint(resource); } @@ -71,4 +77,50 @@ void authplaneCause_rendersThatErrorAndScheme() throws Exception { assertThat(header.getValue()).startsWith("DPoP ").contains("error=\"invalid_dpop_proof\""); assertThat(body.toString()).contains("\"error\":\"invalid_dpop_proof\""); } + + // ----------------------------------------------------------------------- + // resource_metadata comes from the resource's advertised URL (AS-hosted topology) + // ----------------------------------------------------------------------- + + @Test + void configuredResourceMetadataUrl_isAdvertisedOn401() throws Exception { + // The resource was configured with an override (authplane.resource-metadata-url), so the + // challenge points at the AS-hosted document instead of the derived resource-hosted one. + String asHosted = "https://auth.example.com/.well-known/oauth-protected-resource/mcp"; + HttpServletResponse res = mock(HttpServletResponse.class); + wire(res); + + entryPoint(asHosted).commence(mock(HttpServletRequest.class), res, null); + + verify(res).setStatus(401); + ArgumentCaptor header = ArgumentCaptor.forClass(String.class); + verify(res).setHeader(eq("WWW-Authenticate"), header.capture()); + assertThat(header.getValue()).contains("resource_metadata=\"" + asHosted + "\""); + } + + @Test + void insufficientScope_writes403ChallengeCarryingTheAdvertisedUrl() throws Exception { + // The 403 challenge is rendered here too (Spring routes the authentication failure to the + // entry point, and FailureResponse maps InsufficientScopeException to 403), so it has to + // carry the same advertised URL as the 401. + String asHosted = "https://auth.example.com/.well-known/oauth-protected-resource/mcp"; + HttpServletResponse res = mock(HttpServletResponse.class); + StringWriter body = wire(res); + OAuth2AuthenticationException ex = + new OAuth2AuthenticationException( + new OAuth2Error("insufficient_scope"), + "forbidden", + new InsufficientScopeException("tools/write", List.of("tools/read"))); + + entryPoint(asHosted).commence(mock(HttpServletRequest.class), res, ex); + + verify(res).setStatus(403); + ArgumentCaptor header = ArgumentCaptor.forClass(String.class); + verify(res).setHeader(eq("WWW-Authenticate"), header.capture()); + assertThat(header.getValue()) + .startsWith("Bearer ") + .contains("error=\"insufficient_scope\"") + .contains("resource_metadata=\"" + asHosted + "\""); + assertThat(body.toString()).contains("\"error\":\"insufficient_scope\""); + } } diff --git a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProviderTest.java b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProviderTest.java index e8b1d33..821589f 100644 --- a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProviderTest.java +++ b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneAuthenticationProviderTest.java @@ -122,7 +122,8 @@ void authenticate_completionExceptionWithAuthplaneCause_throwsOAuth2Exception() assertThatThrownBy(() -> provider.authenticate(request("bad-token"))) .isInstanceOf(OAuth2AuthenticationException.class) - .hasMessageContaining("issuer mismatch"); + .hasMessageContaining("The access token is missing or not valid for this resource") + .hasMessageNotContaining("issuer mismatch"); } @Test @@ -154,7 +155,7 @@ void authenticate_errorWithNullMessage_usesDefaultDescription() { assertThatThrownBy(() -> provider.authenticate(request("bad-token"))) .isInstanceOf(OAuth2AuthenticationException.class) - .hasMessageContaining("Token validation failed"); + .hasMessageContaining("The access token is missing or not valid for this resource"); } @Test @@ -173,7 +174,8 @@ void authenticate_nonAuthplaneCause_doesNotLeakMessage() { assertThat(thrown).isNotNull(); assertThat(thrown.getMessage()).doesNotContain("secret"); - assertThat(thrown.getError().getDescription()).isEqualTo("Token validation failed"); + assertThat(thrown.getError().getDescription()) + .isEqualTo("The access token is missing or not valid for this resource"); assertThat(thrown.getError().getErrorCode()).isEqualTo("invalid_token"); } @@ -184,7 +186,8 @@ void authenticate_directAuthplaneException_throwsOAuth2Exception() { assertThatThrownBy(() -> provider.authenticate(request("direct-fail"))) .isInstanceOf(OAuth2AuthenticationException.class) - .hasMessageContaining("synchronous failure"); + .hasMessageContaining("The access token is missing or not valid for this resource") + .hasMessageNotContaining("synchronous failure"); } @Test @@ -198,6 +201,7 @@ void authenticate_dpopBindingMismatch_throwsOAuth2Exception() { assertThatThrownBy(() -> provider.authenticate(request("bound-token"))) .isInstanceOf(OAuth2AuthenticationException.class) - .hasMessageContaining("binding mismatch"); + .hasMessageContaining("The access token is missing or not valid for this resource") + .hasMessageNotContaining("binding mismatch"); } } diff --git a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java index fbdafd6..824841f 100644 --- a/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java +++ b/spring/src/test/java/ai/authplane/sdk/spring/security/AuthplaneSecurityConfigTest.java @@ -231,6 +231,7 @@ void authenticationEntryPoint_queryBearingResource_advertisesTheQueryInTheChalle List.of("RS256"), 30, false, + "", // resourceMetadataUrl (unset → derived, resource-hosted) revocationCheckerProvider, inboundDPoPProvider); @@ -246,6 +247,82 @@ void authenticationEntryPoint_queryBearingResource_advertisesTheQueryInTheChalle + "/.well-known/oauth-protected-resource/mcp?tenant=a\""); } + @Test + void resourceMetadataUrlProperty_isAdvertisedInsteadOfTheDerivedUrl() throws Exception { + // authplane.resource-metadata-url configured: the whole path from the property to the + // header, through the real resource and the real entry point. The AS hosts the document + // (authserver >= 0.2.0 serves one per registered resource); this server only points at it. + String asHosted = baseUrl + "/.well-known/oauth-protected-resource/mcp"; + AuthplaneResource v = + config.authplaneResource( + buildClient(0), + baseUrl + "/mcp", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + asHosted, + revocationCheckerProvider, + inboundDPoPProvider); + + MockHttpServletResponse response = new MockHttpServletResponse(); + new AuthplaneAuthenticationEntryPoint(v) + .commence(new MockHttpServletRequest("GET", "/mcp"), response, null); + + assertThat(response.getStatus()).isEqualTo(401); + assertThat(response.getHeader("WWW-Authenticate")) + .contains("resource_metadata=\"" + asHosted + "\""); + // The document still has a resource-hosted address; only what is advertised changed. + assertThat(v.prmPath()).isEqualTo("/.well-known/oauth-protected-resource/mcp"); + } + + @Test + void resourceMetadataUrlProperty_unset_keepsTheDerivedChallengeUnchanged() throws Exception { + // The default: no property, no behaviour change — the challenge advertises the document + // this config serves itself. + AuthplaneResource v = + config.authplaneResource( + buildClient(0), + baseUrl + "/mcp", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + "", + revocationCheckerProvider, + inboundDPoPProvider); + + MockHttpServletResponse response = new MockHttpServletResponse(); + new AuthplaneAuthenticationEntryPoint(v) + .commence(new MockHttpServletRequest("GET", "/mcp"), response, null); + + assertThat(response.getHeader("WWW-Authenticate")) + .contains( + "resource_metadata=\"" + + baseUrl + + "/.well-known/oauth-protected-resource/mcp\""); + } + + @Test + void resourceMetadataUrlProperty_relativeValue_failsAtContextStartup() { + // A relative value would be advertised verbatim to unauthenticated clients, which cannot + // resolve it. Like the invalid query above, it fails where the operator wrote it. + assertThatThrownBy( + () -> + config.authplaneResource( + buildClient(0), + baseUrl + "/mcp", + List.of("tools/add"), + List.of("RS256"), + 30, + false, + "/.well-known/oauth-protected-resource/mcp", + revocationCheckerProvider, + inboundDPoPProvider)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("resourceMetadataUrl"); + } + @Test void authplaneResource_queryOutsideTheRfc3986Grammar_failsAtContextStartup() { // A query octet the §3.4 grammar does not admit used to construct cleanly and then throw @@ -260,6 +337,8 @@ void authplaneResource_queryOutsideTheRfc3986Grammar_failsAtContextStartup() { List.of("RS256"), 30, false, + "", // resourceMetadataUrl (unset → derived, + // resource-hosted) revocationCheckerProvider, inboundDPoPProvider)) .isInstanceOf(IllegalArgumentException.class) @@ -282,6 +361,8 @@ void authplaneResource_schemeRelativeIdentifier_failsAtContextStartup() { List.of("RS256"), 30, false, + "", // resourceMetadataUrl (unset → derived, + // resource-hosted) revocationCheckerProvider, inboundDPoPProvider)) .isInstanceOf(IllegalArgumentException.class) @@ -439,6 +520,7 @@ void authplaneSecurityFilterChain_resourceWithoutPath_throwsIllegalState() throw List.of("RS256"), 30, false, + "", // resourceMetadataUrl (unset → derived, resource-hosted) revocationCheckerProvider, inboundDPoPProvider); @@ -465,6 +547,8 @@ void authplaneResource_fragmentInProperty_throwsIllegalArgument() throws Excepti List.of("RS256"), 30, false, + "", // resourceMetadataUrl (unset → derived, + // resource-hosted) revocationCheckerProvider, inboundDPoPProvider)) .isInstanceOf(IllegalArgumentException.class) @@ -502,6 +586,7 @@ private AuthplaneResource buildVerifier(AuthplaneClient client, boolean introspe List.of("RS256"), 30, // clockSkewSeconds introspectionEnabled, + "", // resourceMetadataUrl (unset → derived, resource-hosted) revocationCheckerProvider, inboundDPoPProvider); }