test(integration): add production refresh-path IT coverage against Keycloak - #672
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @cuioss-oliver, you've used your own review budget of 250,000 diff characters for the last 7 days.
You can request another review in 2 days by commenting @sourcery-ai review. Upgrade to get a review now.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cuioss/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds module descriptions, generation data, and type metadata to project architecture files. It also adds Keycloak-backed integration coverage for production refresh flows, client authentication, token rotation, lifecycle handling, error paths, concurrency, reuse detection, and DPoP binding. ChangesArchitecture metadata
Refresh integration coverage
Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: ⚪ Minimal · up to This PR adds integration coverage and test-fixture changes without changing production behavior; the reported refresh exception was not reproduced, and the integration suite passed, so no actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Reviewer Guide 🔍(Review updated until commit d3d6a3a)
|
|
@coderabbitai review |
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 05f3688d-1b32-43d9-9596-1a78cc31cc13
📒 Files selected for processing (27)
.plan/project-architecture/_project.json.plan/project-architecture/benchmark-core/enriched.json.plan/project-architecture/benchmark-integration-wrk/enriched.json.plan/project-architecture/benchmarking-common/enriched.json.plan/project-architecture/benchmarking/enriched.json.plan/project-architecture/documentation/enriched.json.plan/project-architecture/e-2-e-playwright-maven/enriched.json.plan/project-architecture/e-2-e-playwright-npm/enriched.json.plan/project-architecture/token-sheriff-bom/enriched.json.plan/project-architecture/token-sheriff-client-quarkus-deployment/enriched.json.plan/project-architecture/token-sheriff-client-quarkus/enriched.json.plan/project-architecture/token-sheriff-client/enriched.json.plan/project-architecture/token-sheriff-parent/enriched.json.plan/project-architecture/token-sheriff-quarkus-integration-tests/enriched.json.plan/project-architecture/token-sheriff-quarkus-parent/enriched.json.plan/project-architecture/token-sheriff-validation-quarkus-deployment-maven/enriched.json.plan/project-architecture/token-sheriff-validation-quarkus-deployment-npm/enriched.json.plan/project-architecture/token-sheriff-validation-quarkus/enriched.json.plan/project-architecture/token-sheriff-validation/enriched.jsontoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/main/docker/keycloak/integration-realm.jsontoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/TestRealm.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/FullFlowE2EIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshEngineSupport.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshLifecycleSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshProductionPathSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/security/DpopProofHelper.java
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…ssor Adds refresh-fast-client (access.token.lifespan=35) to the integration realm so a freshly issued access token becomes proactively refresh-eligible within seconds, plus createFastRefreshRealm() and an expiresInSeconds component on TestRealm.TokenResponse. Co-Authored-By: Claude <noreply@anthropic.com>
Adds RefreshEngineSupport, which assembles the production client engine (ClientConfiguration, TokenEndpointClient, validation bridges over a real TokenValidator) against the container with a chain-validating SSLContext, and RefreshProductionPathSpecIT, which exercises rotation and DPoP cnf.jkt continuity through RefreshFlow.refresh rather than a hand-rolled HttpRequest. DpopProofHelper gains a KeyPair constructor so one test-owned key spans both legs. Co-Authored-By: Claude <noreply@anthropic.com>
…eycloak RefreshLifecycleSpecIT drives the session-scoped refresh path over the production engine: the proactive-refresh window on the fast-expiry client, rotation applied to the stored bundle with the ID token carried forward, client-side refresh-token reuse detection with an observed RFC 7009 revocation, and the bearer-downgrade fail-close for a sender-constrained bundle. Co-Authored-By: Claude <noreply@anthropic.com>
Output of `verify -Ppre-commit` over the new refresh-path integration tests: - collapse the expanded JUnit Assertions static imports to a star import in RefreshLifecycleSpecIT and RefreshProductionPathSpecIT, and expand `java.util.*` to explicit imports in FullFlowE2EIT (one threshold rule, applied in both directions) - re-indent the TokenResponse record parameter list in TestRealm - inject the InvalidExceptionUsageRecipe TODO marker above the RuntimeException catch in RefreshProductionPathSpecIT Committing keeps the tree converged so the next `-Ppre-commit` run is a no-op. Co-Authored-By: Claude <noreply@anthropic.com>
…n-coverage Re-align the TokenResponse record parameter continuation in TestRealm, which the pre-commit OpenRewrite pass had left with a stray left-margin indent. Co-Authored-By: Claude <noreply@anthropic.com>
…stRealm The finalize simplify sweep re-indented the TokenResponse record parameter continuation, but `verify -Ppre-commit` deterministically re-applies its own 4-space form. The OpenRewrite ruleset is authoritative here, so the simplify edit was a false positive against tool-enforced formatting. Restoring the rewrite output converges the tree and stops the two passes fighting. Co-Authored-By: Claude <noreply@anthropic.com>
Extends the production refresh-path coverage from a sampled set to the full sensibly-testable variant matrix. Every variant below was reachability-probed against the live Keycloak container before its test was written, so none of them can pass vacuously. Case 2a (RefreshFlow, the surface API Sheriff's BFF shares): - expiry-driven entry — the access token genuinely expires before RefreshFlow is called, rather than being redeemed eagerly at acquisition. This was the most material residual: the reported failure surfaced during ordinary use - authorization-server refusals — unknown refresh token and revoked refresh token both surface as typed failures raised from a production frame, never as an opaque transport error and never as a RotationResult - client_secret_post and private_key_jwt refresh legs, plus ClientAuthenticationSelector routing against the realm's genuinely advertised token_endpoint_auth_methods_supported. Only client_secret_basic had ever run - replay of a superseded token through the bare 2a surface. This realm ACCEPTS it (revokeRefreshToken / refreshTokenMaxReuse are absent), so the test asserts that permissive behaviour and documents it as a realm-configuration property rather than pretending the engine rejects replay Case 2b (TokenLifecycleManager, session-scoped): - CLIENT-18 positive path — a real DPoP-bound session refreshed through RefreshFlow + SenderConstraint and applied via the six-arg applyRefresh(..., confirmedBinding, ...), asserting the stored bundle retains its cnf.jkt. The requirement says refresh MUST preserve the constraint; nothing previously exercised that - the bearer-downgrade fail-close test now starts from a genuinely DPoP-bound session instead of a fabricated thumbprint stapled to a bearer token, and is renamed to describe the real mechanism: the coordinator hard-codes a null refreshed binding, so it is not detecting a server-side downgrade - single-flight collapsing, with the overlap constructed rather than hoped for - revocation-endpoint failure on the reuse path — the swallow branch still leaves the store cleared and the reuse signal intact Fixture: adds private-key-jwt-client to the existing integration realm plus its test-classpath key. No new realm, no application.properties change. Note for future readers: a private_key_jwt assertion must be audienced at the realm's INTERNAL token endpoint, not the loopback URL actually connected to; Keycloak matches aud against its own issuer-derived endpoint. Measured: verify -Pintegration-tests, 119 tests, zero failures, 144s. Co-Authored-By: Claude <noreply@anthropic.com>
CodeRabbit — stale architecture descriptions. `_project.json` described token-sheriff-client, -client-quarkus and -client-quarkus-deployment as empty skeletons with no functional content. All three demonstrably have content: the client engine carries AuthorizationCodeFlow, RefreshFlow, TokenLifecycleManager and the DPoP surface; the runtime extension carries TokenSheriffClientProducer, ClientExceptionMapper and the config package; the deployment module carries TokenSheriffClientProcessor with feature and additional-bean registration. Fixed at the source rather than the symptom: the descriptions in `_project.json` are populated by `architecture discover` FROM each module's `enriched.json`, so editing `_project.json` alone would be reverted by the next refresh. The three enriched descriptors were corrected via `architecture enrich`, their stale key_packages entries replaced with path-keyed ones, and `discover --force` re-run. The same staleness in the client-quarkus package-info Javadoc is corrected here too — it would have been the next review comment. Sourcery — committed RSA private key. The private_key_jwt fixture shipped a real BEGIN PRIVATE KEY block into a public repository. Removed rather than annotated: the keypair is now generated per run and its public half registered on the realm client through the Keycloak Admin REST API (KeycloakAdminSupport), and the hardcoded jwks.string is gone from integration-realm.json. No placeholder key survives, so a failed registration surfaces as invalid_client rather than silently passing against stale material. Every admin call asserts its status and the write is read back and compared. Note the key remains in this branch's history at the commit that added it. Rewriting that would not un-publish it — it was pushed to a public repo — and it is a throwaway test key that protected nothing and is now referenced by no code. Rebased onto origin/main (Quarkus 3.38.3 -> 3.39.0, two eslint bumps). Measured on the new base: verify -Pintegration-tests, 119 tests, zero failures, 154s. The private_key_jwt specs pass with no committed key, which is what verifies the runtime registration. Co-Authored-By: Claude <noreply@anthropic.com>
64baba9 to
b53914d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
token-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshClientAuthSpecIT.java (1)
171-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the advertised methods from the live discovery document.
The comment states the realm advertises these methods, and the class Javadoc states this spec pins the selector routing "against the realm's genuinely advertised
token_endpoint_auth_methods_supported". No code reads that set.RefreshEngineSupport.providerMetadata()populates onlyissuer,tokenEndpoint,revocationEndpoint, andjwksUri, so this literal is the only source of the advertised methods.The drift this permits: if the realm stops advertising
private_key_jwt, this test still passes, because the selector reads the literal the test supplies. The selection assertion then verifiesClientAuthenticationSelectorin isolation instead of the realm contract.Read
token_endpoint_auth_methods_supportedfrom the realm's discovery endpoint and assign it tometadata.tokenEndpointAuthMethodsSupported. Assert that the fetched set containsclient_secret_basicandprivate_key_jwtbefore running the selection. The selector already skipstls_client_authby design, so a fetched set that includes it stays valid.As per path instructions: "Treat a hardcoded list that must mirror a set defined elsewhere — build goals, enum constants, registered handlers, dispatch tables — as a defect unless it is derived from that source at build or run time. Name the authoritative definition and the drift it permits."
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: cuioss/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5af0f52c-c3b2-47d7-a2fe-a5a1ff1a98db
📒 Files selected for processing (14)
.plan/project-architecture/_project.json.plan/project-architecture/token-sheriff-client-quarkus-deployment/enriched.json.plan/project-architecture/token-sheriff-client-quarkus/enriched.json.plan/project-architecture/token-sheriff-client/enriched.jsontoken-sheriff-quarkus-parent/token-sheriff-client-quarkus/src/main/java/de/cuioss/sheriff/token/client/quarkus/package-info.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/main/docker/keycloak/integration-realm.jsontoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/KeycloakAdminSupport.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshClientAuthSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshConstraintLifecycleSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshEngineSupport.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshErrorPathSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshLifecycleSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshProductionPathSpecIT.javatoken-sheriff-quarkus-parent/token-sheriff-quarkus-integration-tests/src/test/java/de/cuioss/sheriff/token/integration/client/RefreshSingleFlightSpecIT.java
🚧 Files skipped from review as they are similar to previous changes (2)
- .plan/project-architecture/_project.json
- .plan/project-architecture/token-sheriff-client-quarkus-deployment/enriched.json
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
…n-coverage Two within-footprint duplications introduced by the variant-coverage pass: - productionFrame / PRODUCTION_PACKAGE: a near-identical stack-scan helper had been copied into RefreshErrorPathSpecIT and RefreshProductionPathSpecIT, differing only in return shape. Collapsed into RefreshEngineSupport.productionFrame() returning Optional<String>; both call sites updated and the stale @link in RefreshProductionPathSpecIT retargeted. - generateRsaKeyPair(): identical RSA-2048 generation duplicated in RefreshConstraintLifecycleSpecIT and RefreshProductionPathSpecIT. Collapsed into RefreshEngineSupport.generateRsaKeyPair(); unused imports removed. Deliberately left alone: DpopProofHelper's own keypair generation (different package, and its sibling duplicate is outside this plan's footprint); RefreshClientAuthSpecIT's inline generation (propagates a checked NoSuchAlgorithmException per its documented contract, so it is a different variant rather than a copy); and the acquireBundle / newSessionId duplications, where collapsing would either drag JUnit assertions into the pure-engine support class or add a file for two call sites. Co-Authored-By: Claude <noreply@anthropic.com>
The simplify pass moved generateRsaKeyPair and productionFrame into RefreshEngineSupport, pushing its java.security imports over the threshold at which the OpenRewrite ruleset collapses them to a star import. Committing the rewrite output keeps the tree converged. Co-Authored-By: Claude <noreply@anthropic.com>
Triage dispositionsIn reply to comment_id:
|
|
Tip For best results, initiate chat on the files or code changes.
The infrastructure and review-summary comments need no action. The private-key finding is stale because commit I withdraw the discovery-metadata finding. The documented internal/external base-URL split makes the static, pre-verified authentication-method list appropriate for this focused client-authentication test. ✏️ Learnings added
You are interacting with an AI system. |
Wrap the executor-consuming region of shouldCollapseConcurrentRefreshesOntoOneRedemption in a try/finally so callers.shutdownNow() runs on every exit path, including a Future.get timeout, an execution exception, or an assertion failure. Previously the fixed two-thread pool leaked whenever either get(...) call or an assertion threw. The inner releaseFirstCaller finally and the WAITING-state handover gate are unchanged, and no assertion was weakened. Co-Authored-By: Claude <noreply@anthropic.com>
|
/review |
…onPathSpecIT pr-agent's review of c748662 caught an unresolved OpenRewrite marker committed at the RuntimeException catch in drive(). AGENTS.md is explicit: "Never commit code with markers present." The marker was injected by the pre-commit profile and shipped under this plan's blanket commit-the-rewrites decision, which conflated rewrite OUTPUT (keep) with a rewrite REQUEST TO ACT (resolve). Resolved with the class-level suppression AGENTS.md prescribes rather than by narrowing the catch. drive() catches RuntimeException deliberately: it is the elimination test's failure funnel, and the whole point is that the engine's failure mode is not known in advance. Narrowing it would let an unanticipated engine exception escape as an opaque error instead of an AssertionError naming the production frame -- the one thing this spec exists to report. The suppression carries that rationale inline. Verified the rewrite pass accepts the suppression rather than re-injecting the marker: verify -Ppre-commit green, marker absent, tree clean. Co-Authored-By: Claude <noreply@anthropic.com>
|
/review |
1 participant
Summary
Adds the first integration coverage of TokenSheriff's production refresh path against a real
Keycloak container. This plan is a cross-repository elimination test for a refresh-time exception
reported against API Sheriff's BFF: does the exception reproduce inside TokenSheriff's client
engine, or is TokenSheriff's shared surface eliminated as the fault?
verify -Pintegration-testsran green — 109 tests, zero failures, against a live Keycloak26.4.0 container. The reported exception did not reproduce.
Two refresh surfaces are covered and reported separately — their verdicts must not be merged:
RefreshFlow.refresh(metadata, refreshToken)— the ONLY surface API Sheriff's BFFshares with this engine. Result:
not_reproduced, under an enumerated, bounded set of exercisedconditions (plain rotation, ID-token consistency, DPoP sender-constraint continuity, full E2E
lifecycle, wire-level control). This is a bounded elimination, not a clean bill of health —
the un-exercised conditions (expiry-driven entry, replay, non-
client_secret_basicauth,discovery-resolved metadata, and more) remain open.
TokenLifecycleManager.refresh(...)— session-scoped layer that API Sheriff's BFF doesnot use at all (confirmed absent from that repository at its cleanup-pass sha). Result:
not_reproducedfor plain-bearer sessions. Carries no cross-repository consequence.A notable engine finding surfaced during 2b verification:
TokenLifecycleManager.doRefreshpassesa hard-coded
nullConstraintBindingintoapplyRefresh, andStoredToken.refreshedthrowsIllegalStateExceptionwhen a constrained bundle receives a non-matching binding — so aDPoP-bound session cannot refresh through
TokenLifecycleManagerat all, by design. This isintentional, documented behaviour (not a latent bug), but its observable shape — an exception
raised at refresh time on the ordinary-use path — is close enough to the reported symptom to record
as the leading in-repo candidate for any other consumer of this layer. It cannot be the reported
fault itself, because API Sheriff never touches
TokenLifecycleManager.A bounded, fully-enumerated verdict — naming exactly which variants were exercised and which were
not, and naming no repository as owner of the reported defect — is recorded at
work/refresh-elimination-verdict.mdin this plan.Hand-off outstanding. Carrying this verdict into API-Sheriff's
.plan/local/orchestrator/deployment-configurability/ledger is an operator action — thisplan's write boundary forbids touching that repository, and nothing automates the hand-off.
Intent
Changes
refresh-fast-client(access.token.lifespan: 35) insidethe existing
integrationrealm (integration-realm.json), chosen over a separate realm becausethe replay/reuse coverage this plan adds is client-side, driven by
RefreshTokenFamily, and doesnot depend on
revokeRefreshToken/refreshTokenMaxReuse.RefreshProductionPathSpecIT— drives the productionRefreshFlow.refreshsurface (case2a): plain rotation, ID-token §12.2 consistency, DPoP sender-constraint continuity.
RefreshLifecycleSpecIT— drives the productionTokenLifecycleManager.refreshsurface(case 2b): expiry-driven proactive-refresh window, rotation applied to session state, reuse
detection with RFC 7009 revocation, and the bearer-downgrade fail-close for a sender-constrained
bundle.
RefreshEngineSupportandDpopProofHelperas shared test-fixture support for the two newIT classes.
RefreshSpecIT's rotation assertion from a substring check to a genuineassertNotEquals, and removesFullFlowE2EIT's hand-rolled refresh leg so it now exercises theproduction
RefreshFlowfor its refresh step.TestRealmfor the new fixture client.refresh (19
.plan/project-architecture/**/enriched.jsonfiles), and a restore of the rewriteindentation.
Test Plan
verify -Pintegration-tests -pl token-sheriff-quarkus-parent/token-sheriff-quarkus-integration-testspassed — 109 tests, zero failures, against a live Keycloak 26.4.0 containerRelated Issues
None.
Generated by plan-finalize skill
Intent
Problem. A refresh-time exception was reported against API Sheriff's BFF, but TokenSheriff's
production refresh path had never been exercised against a real authorization server — only via
MockWebServer unit tests and hand-rolled HTTP legs in existing ITs. There was no way to say whether
the exception originates inside TokenSheriff's client engine or in API Sheriff's own BFF wiring.
Approach. Stand up a fast-expiry Keycloak fixture and drive the two production refresh surfaces
separately and explicitly through real Keycloak-backed integration tests:
RefreshFlow.refresh(2a — the only surface API Sheriff's BFF actually shares with this engine) and
TokenLifecycleManager.refresh(2b — session-scoped, valuable for this repo but API Sheriff nevertouches it). Keeping the two cases apart is deliberate: a merged verdict would overstate what was
eliminated for API Sheriff. Existing tests with weak assertions (
RefreshSpecIT's substring rotationcheck,
FullFlowE2EIT's hand-rolled refresh leg) were corrected so at least one path genuinelyexercises the production
RefreshFlow. The enumerated bound of exactly which conditions were andwere not exercised is recorded as a standalone verdict document rather than folded into test
comments, so a later reader does not have to reverse-engineer scope from the test bodies.
Non-goals. No fix is staged for the by-design
[Intent truncated — 1392 of 1848 characters shown; full outline in the plan workspace]
Summary by CodeRabbit
New Features
Documentation